From 83af5d5df7f43fef67b1c6870064a84c09deb930 Mon Sep 17 00:00:00 2001 From: Tim Fischbach Date: Wed, 24 Jan 2024 07:31:50 +0100 Subject: [PATCH 01/10] Install Prosopite to detect N+1 queries REDMINE-20564 --- Gemfile | 3 +++ pageflow.gemspec | 3 +++ spec/support/config/prosopite.rb | 23 +++++++++++++++++++++++ 3 files changed, 29 insertions(+) create mode 100644 spec/support/config/prosopite.rb diff --git a/Gemfile b/Gemfile index 8fb76f989..a60622184 100644 --- a/Gemfile +++ b/Gemfile @@ -31,3 +31,6 @@ gem 'shakapacker', '~> 7.0' # Make tests fail on JS errors gem 'capybara-chromedriver-logger', git: 'https://github.com/codevise/capybara-chromedriver-logger', branch: 'fix-selenium-4-deprecation', require: false + +# See https://github.com/charkost/prosopite/pull/79 +gem 'prosopite', git: 'https://github.com/tf/prosopite', branch: 'location-backtrace-cleaner' diff --git a/pageflow.gemspec b/pageflow.gemspec index ddcc64364..fa864d0c5 100644 --- a/pageflow.gemspec +++ b/pageflow.gemspec @@ -171,6 +171,9 @@ Gem::Specification.new do |s| # Use assigns in controller specs s.add_development_dependency 'rails-controller-testing', '~> 1.0' + # Detect N+1 queries + s.add_development_dependency 'prosopite', '~> 1.4' + # Browser like integration testing s.add_development_dependency 'capybara', '~> 3.9' diff --git a/spec/support/config/prosopite.rb b/spec/support/config/prosopite.rb new file mode 100644 index 000000000..748f4373e --- /dev/null +++ b/spec/support/config/prosopite.rb @@ -0,0 +1,23 @@ +require 'prosopite' + +Prosopite.raise = true + +# Uses Rails.backtrace_cleaner by default which only leaves liney +# inside the dummy app +Prosopite.backtrace_cleaner = ActiveSupport::BacktraceCleaner.new + +# Only consider lines from within the gem to prevent false +# negatives caused by cached missing methods in gems like +# `builder` (see https://github.com/charkost/prosopite/pull/79) +Prosopite.location_backtrace_cleaner = ActiveSupport::BacktraceCleaner.new +Prosopite.location_backtrace_cleaner.add_silencer { |line| !/pageflow/.match?(line) } + +module NPlusOneQueriesTestHelper + def detect_n_plus_one_queries(&block) + Prosopite.scan(&block) + end +end + +RSpec.configure do |config| + config.include(NPlusOneQueriesTestHelper) +end From 7a513c73ac3cfeaea10b8e4e8ddf73196146fde1 Mon Sep 17 00:00:00 2001 From: Tim Fischbach Date: Fri, 19 Jan 2024 15:05:32 +0100 Subject: [PATCH 02/10] Add entry translation group model Allow grouping entries which are translations of the same content. REDMINE-20564 --- app/models/concerns/pageflow/translatable.rb | 43 +++++++ app/models/pageflow/entry.rb | 1 + .../pageflow/entry_translation_group.rb | 17 +++ ...create_pageflow_entry_translation_group.rb | 8 ++ spec/factories/entry_translation_groups.rb | 6 + .../concerns/pageflow/translatable_spec.rb | 113 ++++++++++++++++++ 6 files changed, 188 insertions(+) create mode 100644 app/models/concerns/pageflow/translatable.rb create mode 100644 app/models/pageflow/entry_translation_group.rb create mode 100644 db/migrate/20230419083307_create_pageflow_entry_translation_group.rb create mode 100644 spec/factories/entry_translation_groups.rb create mode 100644 spec/models/concerns/pageflow/translatable_spec.rb diff --git a/app/models/concerns/pageflow/translatable.rb b/app/models/concerns/pageflow/translatable.rb new file mode 100644 index 000000000..a1b3ec4e2 --- /dev/null +++ b/app/models/concerns/pageflow/translatable.rb @@ -0,0 +1,43 @@ +module Pageflow + # @api private + module Translatable + extend ActiveSupport::Concern + + included do + belongs_to :translation_group, + optional: true, + class_name: 'EntryTranslationGroup' + + has_many :translations, + ->(entry) { excluding(entry) }, + through: :translation_group, + source: :entries + + after_destroy do + translation_group.destroy if translation_group&.single_item_or_empty? + end + end + + def mark_as_translation_of(entry) + transaction do + unless translation_group + update!(translation_group: entry.translation_group || build_translation_group) + end + + if !entry.translation_group + entry.update!(translation_group:) + elsif entry.translation_group != translation_group + entry.translation_group.merge_into(translation_group) + end + end + end + + def remove_from_translation_group + if translation_group.entries.count <= 2 + translation_group.destroy + else + update!(translation_group: nil) + end + end + end +end diff --git a/app/models/pageflow/entry.rb b/app/models/pageflow/entry.rb index 0c0e727e5..50559bf85 100644 --- a/app/models/pageflow/entry.rb +++ b/app/models/pageflow/entry.rb @@ -7,6 +7,7 @@ class PasswordMissingError < StandardError include EntryPublicationStates include Permalinkable include SerializationBlacklist + include Translatable extend FriendlyId friendly_id :slug_candidates, :use => [:finders, :slugged] diff --git a/app/models/pageflow/entry_translation_group.rb b/app/models/pageflow/entry_translation_group.rb new file mode 100644 index 000000000..5b3a6ba4e --- /dev/null +++ b/app/models/pageflow/entry_translation_group.rb @@ -0,0 +1,17 @@ +module Pageflow + # @api private + class EntryTranslationGroup < ApplicationRecord + has_many :entries, + foreign_key: 'translation_group_id', + dependent: :nullify + + def merge_into(translation_group) + entries.update_all(translation_group_id: translation_group.id) + destroy + end + + def single_item_or_empty? + !entries.many? + end + end +end diff --git a/db/migrate/20230419083307_create_pageflow_entry_translation_group.rb b/db/migrate/20230419083307_create_pageflow_entry_translation_group.rb new file mode 100644 index 000000000..99778e383 --- /dev/null +++ b/db/migrate/20230419083307_create_pageflow_entry_translation_group.rb @@ -0,0 +1,8 @@ +class CreatePageflowEntryTranslationGroup < ActiveRecord::Migration[5.2] + def change + create_table :pageflow_entry_translation_groups do |t| + end + + add_reference(:pageflow_entries, :translation_group) + end +end diff --git a/spec/factories/entry_translation_groups.rb b/spec/factories/entry_translation_groups.rb new file mode 100644 index 000000000..c53ac9cab --- /dev/null +++ b/spec/factories/entry_translation_groups.rb @@ -0,0 +1,6 @@ +module Pageflow + FactoryBot.define do + factory :entry_translation_group, class: EntryTranslationGroup do + end + end +end diff --git a/spec/models/concerns/pageflow/translatable_spec.rb b/spec/models/concerns/pageflow/translatable_spec.rb new file mode 100644 index 000000000..bea20fb2a --- /dev/null +++ b/spec/models/concerns/pageflow/translatable_spec.rb @@ -0,0 +1,113 @@ +require 'spec_helper' + +module Pageflow + describe Translatable do + describe '#mark_as_translation_of' do + it 'adds entries to new translation group' do + entry = create(:entry) + other_entry = create(:entry) + + entry.mark_as_translation_of(other_entry) + + expect(entry.translations).to include(other_entry) + expect(other_entry.translations).to include(entry) + end + + it 'adds entry to existing translation group of other entry' do + entry = create(:entry) + fr_entry = create(:entry) + de_entry = create(:entry) + + fr_entry.mark_as_translation_of(entry) + de_entry.mark_as_translation_of(entry) + + expect(entry.translations).to include(fr_entry) + expect(entry.translations).to include(de_entry) + end + + it 'adds entry to own existing translation group' do + entry = create(:entry) + fr_entry = create(:entry) + de_entry = create(:entry) + + de_entry.mark_as_translation_of(entry) + entry.mark_as_translation_of(fr_entry) + + expect(entry.translations).to include(fr_entry) + expect(entry.translations).to include(de_entry) + end + + it 'merges translation groups' do + en_entry = create(:entry) + pl_entry = create(:entry) + fr_entry = create(:entry) + de_entry = create(:entry) + + en_entry.mark_as_translation_of(pl_entry) + fr_entry.mark_as_translation_of(de_entry) + fr_entry.mark_as_translation_of(pl_entry) + + expect(en_entry.reload.translations).to include(pl_entry) + expect(en_entry.reload.translations).to include(de_entry) + expect(fr_entry.reload.translations).to include(pl_entry) + expect(EntryTranslationGroup.count).to eq(1) + end + end + + describe '#remove_from_translation_group' do + it 'sets translation group to nil' do + translation_group = create(:entry_translation_group) + en_entry = create(:entry, translation_group:) + pl_entry = create(:entry, translation_group:) + fr_entry = create(:entry, translation_group:) + + en_entry.remove_from_translation_group + + expect(pl_entry.translations).to eq([fr_entry]) + end + + it 'destroys translation group with only one entry' do + translation_group = create(:entry_translation_group) + en_entry = create(:entry, translation_group:) + fr_entry = create(:entry, translation_group:) + + en_entry.remove_from_translation_group + + expect(fr_entry.translations).to eq([]) + expect(fr_entry.reload.translation_group_id).to eq(nil) + expect(EntryTranslationGroup.count).to eq(0) + end + end + + it 'destroying the penultimate entry in a translation group destroys the group' do + translation_group = create(:entry_translation_group) + en_entry = create(:entry, translation_group:) + fr_entry = create(:entry, translation_group:) + + en_entry.destroy + + expect(fr_entry.translations).to eq([]) + expect(fr_entry.reload.translation_group_id).to eq(nil) + expect(EntryTranslationGroup.count).to eq(0) + end + + it 'destroying an entry does not destroy translation group with multiple entries' do + translation_group = create(:entry_translation_group) + en_entry = create(:entry, translation_group:) + fr_entry = create(:entry, translation_group:) + pl_entry = create(:entry, translation_group:) + + en_entry.destroy + + expect(fr_entry.translations).to eq([pl_entry]) + end + + it 'allows destroying an entry without translation group' do + entry = create(:entry) + + expect { + entry.destroy + }.not_to raise_error + end + end +end From 2f22e81c261d49fd30a35ef7e05f7299fb670c2e Mon Sep 17 00:00:00 2001 From: Tim Fischbach Date: Fri, 19 Jan 2024 15:07:48 +0100 Subject: [PATCH 03/10] Add query object to find potential translations for entry REDMINE-20564 --- .../pageflow/potential_entry_translations.rb | 55 +++++++++++ .../potential_entry_translations_spec.rb | 96 +++++++++++++++++++ 2 files changed, 151 insertions(+) create mode 100644 app/models/pageflow/potential_entry_translations.rb create mode 100644 spec/models/pageflow/potential_entry_translations_spec.rb diff --git a/app/models/pageflow/potential_entry_translations.rb b/app/models/pageflow/potential_entry_translations.rb new file mode 100644 index 000000000..c86703ff5 --- /dev/null +++ b/app/models/pageflow/potential_entry_translations.rb @@ -0,0 +1,55 @@ +module Pageflow + # @api private + class PotentialEntryTranslations + def initialize(entry) + @entry = entry + end + + def self.for(entry) + new(entry) + end + + def resolve + preload( + group_by_translation_group( + exclude_translations( + other_entries_of_account + ) + ) + ) + end + + private + + def other_entries_of_account + @entry.account.entries.where.not(id: @entry.id) + end + + def exclude_translations(scope) + return scope unless @entry.translation_group + + scope + .where.not(translation_group_id: @entry.translation_group) + .or(scope.where(translation_group: nil)) + end + + def group_by_translation_group(scope) + scope + # Use MIN(id) to choose an arbitrary entry to represent its + # translation group. MIN(translation_group_id) is needed since + # technically translation_group_id is not part of the GROUP BY + # clause. + .select(<<-SQL) + MIN(id) as id, + GROUP_CONCAT(title) as title, + MIN(translation_group_id) as translation_group_id + SQL + .group('IFNULL(translation_group_id, id)') + .order('title ASC') + end + + def preload(scope) + scope.includes(translation_group: :entries) + end + end +end diff --git a/spec/models/pageflow/potential_entry_translations_spec.rb b/spec/models/pageflow/potential_entry_translations_spec.rb new file mode 100644 index 000000000..91ab0db11 --- /dev/null +++ b/spec/models/pageflow/potential_entry_translations_spec.rb @@ -0,0 +1,96 @@ +require 'spec_helper' + +module Pageflow + describe PotentialEntryTranslations do + it 'allows filtering by title' do + entry_de = create( + :entry, + title: 'Some story DE' + ) + entry_en = create( + :entry, + title: 'Some story EN', + account: entry_de.account + ) + other_entry_en = create( + :entry, + title: 'Other story EN', + account: entry_de.account + ) + query = PotentialEntryTranslations.for(entry_de) + + result = query.resolve.ransack(title_cont: 'story EN').result + + expect(result).to eq([entry_en, other_entry_en]) + end + + it 'excludes entry itself' do + entry_de = create( + :entry, + title: 'Some story DE' + ) + entry_en = create( + :entry, + title: 'Some story EN', + account: entry_de.account + ) + query = PotentialEntryTranslations.for(entry_de) + + result = query.resolve.ransack(title_cont: 'story').result + + expect(result).to eq([entry_en]) + end + + it 'represents translation group with matching entry by single entry' do + entry_de = create( + :entry, + title: 'Some story DE' + ) + translation_group = create(:entry_translation_group) + entry_en = create( + :entry, + title: 'Some story EN', + account: entry_de.account, + translation_group: + ) + create( + :entry, + title: 'Some story FR', + account: entry_de.account, + translation_group: + ) + query = PotentialEntryTranslations.for(entry_de) + + result = query.resolve.ransack(title_cont: 'story').result + + expect(result).to eq([entry_en]) + expect(result[0].association(:translation_group)).to be_loaded + expect(result[0].translation_group.entries).to be_loaded + end + + it 'excludes entry representing current translation group' do + translation_group = create(:entry_translation_group) + entry_de = create( + :entry, + title: 'Some story DE', + translation_group: + ) + create( + :entry, + title: 'Some story EN', + account: entry_de.account, + translation_group: + ) + entry_fr = create( + :entry, + title: 'Some story FR', + account: entry_de.account + ) + query = PotentialEntryTranslations.for(entry_de) + + result = query.resolve.ransack(title_cont: 'story').result + + expect(result).to eq([entry_fr]) + end + end +end From c9cdb6de2ed0d6e1896606dff0f6887257d453fe Mon Sep 17 00:00:00 2001 From: Tim Fischbach Date: Fri, 19 Jan 2024 15:08:23 +0100 Subject: [PATCH 04/10] Fix admin theme for button_to forms --- app/assets/stylesheets/pageflow/admin/entries/index_table.scss | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/assets/stylesheets/pageflow/admin/entries/index_table.scss b/app/assets/stylesheets/pageflow/admin/entries/index_table.scss index 1b5d54a48..65827013f 100644 --- a/app/assets/stylesheets/pageflow/admin/entries/index_table.scss +++ b/app/assets/stylesheets/pageflow/admin/entries/index_table.scss @@ -1,4 +1,5 @@ -.index_table { +.index_table, +.embedded_index_table { .publication_state { width: 16px; padding: 5px; From e32e7634df5bf5583a0dbb8739a7bd44d6c892fa Mon Sep 17 00:00:00 2001 From: Tim Fischbach Date: Fri, 19 Jan 2024 16:33:50 +0100 Subject: [PATCH 05/10] Add admin to manage entry translations REDMINE-20564 --- .rubocop.yml | 2 + admins/pageflow/entry.rb | 1 + admins/pageflow/translations.rb | 66 ++++++++ .../admin/entry_translations_helper.rb | 13 ++ app/policies/pageflow/entry_policy.rb | 6 +- app/views/admin/translations/_form.html.erb | 31 ++++ .../pageflow/admin/entry_translations_tab.rb | 60 ++++++++ config/initializers/admin_resource_tabs.rb | 3 + config/locales/new/entry_translations.de.yml | 25 ++++ config/locales/new/entry_translations.en.yml | 25 ++++ lib/pageflow/ability_mixin.rb | 4 + .../admin/translations_controller_spec.rb | 141 ++++++++++++++++++ .../admin/entry_translations_helper_spec.rb | 28 ++++ spec/policies/pageflow/entry_policy_spec.rb | 13 ++ .../admin/entry_translations_tab_spec.rb | 57 +++++++ 15 files changed, 474 insertions(+), 1 deletion(-) create mode 100644 admins/pageflow/translations.rb create mode 100644 app/helpers/pageflow/admin/entry_translations_helper.rb create mode 100644 app/views/admin/translations/_form.html.erb create mode 100644 app/views/components/pageflow/admin/entry_translations_tab.rb create mode 100644 config/locales/new/entry_translations.de.yml create mode 100644 config/locales/new/entry_translations.en.yml create mode 100644 spec/controllers/admin/translations_controller_spec.rb create mode 100644 spec/helpers/pageflow/admin/entry_translations_helper_spec.rb create mode 100644 spec/views/components/pageflow/admin/entry_translations_tab_spec.rb diff --git a/.rubocop.yml b/.rubocop.yml index 071067c93..c93a64ea0 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -48,6 +48,7 @@ Metrics/BlockLength: # In specs methods using RSpec DSL often become long Metrics/MethodLength: Exclude: + - '**/app/views/components/**/*' - '**/spec/**/*' # Long spec files are ok @@ -61,6 +62,7 @@ Metricts/ParameterLists: # Do not require class documentation for specs and migrations Style/Documentation: Exclude: + - admins/**/* - '**/spec/**/*' - db/migrate/* diff --git a/admins/pageflow/entry.rb b/admins/pageflow/entry.rb index 5129c3d5f..eea4fb314 100644 --- a/admins/pageflow/entry.rb +++ b/admins/pageflow/entry.rb @@ -205,6 +205,7 @@ module Pageflow helper Admin::FormHelper helper Admin::MembershipsHelper helper Admin::RevisionsHelper + helper Admin::EntryTranslationsHelper helper_method :account_policy_scope helper_method :site_policy_scope diff --git a/admins/pageflow/translations.rb b/admins/pageflow/translations.rb new file mode 100644 index 000000000..bd82bbba1 --- /dev/null +++ b/admins/pageflow/translations.rb @@ -0,0 +1,66 @@ +module Pageflow + ActiveAdmin.register Entry, as: 'Translations' do + menu false + belongs_to :entry + + actions :new, :create, :destroy + + searchable_select_options(name: :potential_entry_translations, + scope: lambda do |_params| + authorize!(:manage_translations, parent) + PotentialEntryTranslations.for(parent).resolve + end, + text_attribute: :title, + display_text: lambda do |entry| + if entry.translation_group + entry.translation_group + .entries + .order('title ASC') + .map(&:title) + .join(' / ') + .presence + else + entry.title + end + end) + + form partial: 'form' + + controller do + helper Pageflow::Admin::FormHelper + + def index + redirect_to admin_entry_path(parent, tab: 'translations') + end + + def create + entry = Entry.find(params.require(:entry)[:id]) + + authorize!(:manage_translations, parent) + authorize!(:manage_translations, entry) + parent.mark_as_translation_of(entry) + + redirect_to(admin_entry_path(parent, tab: 'translations')) + end + + def destroy + entry = Entry.find(params[:id]) + + authorize!(:manage_translations, entry) + entry.remove_from_translation_group + + redirect_to(admin_entry_path(parent, tab: 'translations')) + end + + protected + + def authorized?(action, subject = nil) + if subject.is_a?(Entry) && subject.new_record? + super(:manage_translations, parent) + else + super + end + end + end + end +end diff --git a/app/helpers/pageflow/admin/entry_translations_helper.rb b/app/helpers/pageflow/admin/entry_translations_helper.rb new file mode 100644 index 000000000..8492a1f53 --- /dev/null +++ b/app/helpers/pageflow/admin/entry_translations_helper.rb @@ -0,0 +1,13 @@ +module Pageflow + module Admin + # @api private + module EntryTranslationsHelper + def entry_translation_display_locale(entry) + I18n.t( + 'pageflow.public._language', + locale: (entry.published_revision || entry.draft).locale + ) + end + end + end +end diff --git a/app/policies/pageflow/entry_policy.rb b/app/policies/pageflow/entry_policy.rb index 9594d7036..4885255b3 100644 --- a/app/policies/pageflow/entry_policy.rb +++ b/app/policies/pageflow/entry_policy.rb @@ -99,7 +99,11 @@ def create? end def duplicate? - publish? + query.has_at_least_role?(:publisher) + end + + def manage_translations? + query.has_at_least_account_role?(:publisher) end def manage? diff --git a/app/views/admin/translations/_form.html.erb b/app/views/admin/translations/_form.html.erb new file mode 100644 index 000000000..a95c21977 --- /dev/null +++ b/app/views/admin/translations/_form.html.erb @@ -0,0 +1,31 @@ +<%= admin_form_for resource, url: admin_entry_translations_path(parent) do |f| %> + <%= f.inputs do %> +
  • +

    + <%= t('pageflow.admin.entry_translations.new_hint_html') %> +

    +
    • + <%= f.input(:entry_id, + as: :searchable_select, + collection: [[parent.title, parent.id]], + label: t('pageflow.admin.entry_translations.entry_id'), + selected: parent.id, + input_html: { + value: parent.id, + disabled: true + }) %> + <%= f.input(:id, + as: :searchable_select, + label: t('pageflow.admin.entry_translations.id'), + ajax: { + resource: 'Translations', + collection_name: :potential_entry_translations, + path_params: {entry_id: parent.id} + }, + include_blank: false) %> + <% end %> + <%= f.actions do %> + <%= f.action(:submit, label: t('pageflow.admin.entry_translations.create_model')) %> + <%= f.action(:cancel, :wrapper_html => {:class => 'cancel'}) %> + <% end %> +<% end %> diff --git a/app/views/components/pageflow/admin/entry_translations_tab.rb b/app/views/components/pageflow/admin/entry_translations_tab.rb new file mode 100644 index 000000000..f8a57eede --- /dev/null +++ b/app/views/components/pageflow/admin/entry_translations_tab.rb @@ -0,0 +1,60 @@ +module Pageflow + module Admin + # @api private + class EntryTranslationsTab < ViewComponent + def build(entry) + embedded_index_table( + entry.translations.preload(:draft, :published_revision, :translation_group), + blank_slate_text: I18n.t('pageflow.admin.entry_translations.no_translations') + ) do + table_for_collection i18n: Entry do + column class: 'publication_state' do |translated_entry| + entry_publication_state_indicator(translated_entry) + end + column :title do |translated_entry| + entry_link_or_title(translated_entry) + end + column :locale do |translated_entry| + entry_translation_display_locale(translated_entry) + end + column do |translated_entry| + remove_link(entry, translated_entry) + end + end + end + + add_link(entry) + end + + private + + def entry_link_or_title(translated_entry) + if authorized?(:read, translated_entry) + link_to(translated_entry.title, + admin_entry_path(translated_entry, tab: 'translations')) + else + translated_entry.title + end + end + + def remove_link(entry, translated_entry) + return unless authorized?(:manage_translations, entry) + + link_to(I18n.t('pageflow.admin.entries.remove'), + admin_entry_translation_path(entry, translated_entry), + method: :delete, + data: { + confirm: I18n.t('pageflow.admin.entry_translations.delete_confirmation') + }) + end + + def add_link(entry) + return unless authorized?(:manage_translations, entry) + + text_node(link_to(t('pageflow.admin.entry_translations.add'), + new_admin_entry_translation_path(entry), + class: 'button')) + end + end + end +end diff --git a/config/initializers/admin_resource_tabs.rb b/config/initializers/admin_resource_tabs.rb index ca74a0770..0e4f9daaa 100644 --- a/config/initializers/admin_resource_tabs.rb +++ b/config/initializers/admin_resource_tabs.rb @@ -5,6 +5,9 @@ config.admin_resource_tabs.register(:entry, name: :revisions, component: Pageflow::Admin::RevisionsTab) + config.admin_resource_tabs.register(:entry, + name: :translations, + component: Pageflow::Admin::EntryTranslationsTab) config.admin_resource_tabs.register(:user, name: :accounts, diff --git a/config/locales/new/entry_translations.de.yml b/config/locales/new/entry_translations.de.yml new file mode 100644 index 000000000..05e90fd47 --- /dev/null +++ b/config/locales/new/entry_translations.de.yml @@ -0,0 +1,25 @@ +de: + pageflow: + admin: + resource_tabs: + translations: Übersetzungen + entry_translations: + add: Übersetzung zuordnen + entry_id: Beitrag + id: Übersetzung(en) + new_hint_html: |- + Verknüpfe Beiträge, die denselben Inhalt in verschiedenen + Sprachen darstellen. + create_model: Als Übersetzung zuordnen + no_translations: Keine Beiträge zugeordnet + delete_confirmation: |- + Beitrag wirklich aus der Liste der Übersetzungen entfernen? + activerecord: + models: + translation: + one: Übersetzung + other: Übersetzungen + active_admin: + resources: + translations: + new_model: Übersetzung zuordnen diff --git a/config/locales/new/entry_translations.en.yml b/config/locales/new/entry_translations.en.yml new file mode 100644 index 000000000..cbdfce873 --- /dev/null +++ b/config/locales/new/entry_translations.en.yml @@ -0,0 +1,25 @@ +en: + pageflow: + admin: + resource_tabs: + translations: Translations + entry_translations: + add: Link translation + entry_id: Entry + id: Translation(s) + new_hint_html: |- + Associate entries that represent the same content in + different languages. + create_model: Link as translation + no_translations: No linked entries + delete_confirmation: |- + Are you sure you want to remove the entry from the list of translations? + activerecord: + models: + translation: + one: Translation + other: Translations + active_admin: + resources: + translations: + new_model: Link translation diff --git a/lib/pageflow/ability_mixin.rb b/lib/pageflow/ability_mixin.rb index 0784f988e..30e7008fe 100644 --- a/lib/pageflow/ability_mixin.rb +++ b/lib/pageflow/ability_mixin.rb @@ -156,6 +156,10 @@ def pageflow_default_abilities(user) EntryPolicy.new(user, entry).duplicate? end + can :manage_translations, Entry do |entry| + EntryPolicy.new(user, entry).manage_translations? + end + can :edit_outline, Entry do |entry| EntryPolicy.new(user, entry).edit_outline? end diff --git a/spec/controllers/admin/translations_controller_spec.rb b/spec/controllers/admin/translations_controller_spec.rb new file mode 100644 index 000000000..7c18996a7 --- /dev/null +++ b/spec/controllers/admin/translations_controller_spec.rb @@ -0,0 +1,141 @@ +require 'spec_helper' + +describe Admin::TranslationsController do + render_views + + describe '#index' do + it 'redirects to entry page' do + admin = create(:user, :admin) + sign_in(admin, scope: :user) + entry = create(:entry) + + get(:index, + params: { + entry_id: entry + }) + + expect(response) + .to redirect_to(admin_entry_path(id: entry, tab: 'translations')) + end + end + + describe '#new' do + it 'requires publisher role on account' do + entry = create(:entry) + user = create(:user, :editor, on: entry.account) + + sign_in(user, scope: :user) + get(:new, params: {entry_id: entry}) + + expect(flash[:error]).to be_present + expect(response).to redirect_to(admin_root_path) + end + + it 'displays parent entry in disabled select' do + entry = create(:entry) + user = create(:user, :publisher, on: entry.account) + + sign_in(user, scope: :user) + get(:new, params: {entry_id: entry}) + + expect(response.status).to eq(200) + expect(response.body).to have_field('entry[entry_id]', disabled: true, with: entry.id) + end + end + + describe '#create' do + it 'lets account publisher mark as translation of other entry' do + entry = create(:entry) + translation = create(:entry, account: entry.account) + user = create(:user, :publisher, on: entry.account) + + sign_in(user, scope: :user) + post(:create, params: {entry_id: entry, entry: {id: translation}}) + + expect(response) + .to redirect_to(admin_entry_path(id: entry, tab: 'translations')) + expect(entry.reload.translations).to include(translation) + end + + it 'requires publisher role on account' do + entry = create(:entry) + translation = create(:entry, account: entry.account) + user = create(:user, :editor, on: entry.account) + + sign_in(user, scope: :user) + post(:create, params: {entry_id: entry, entry: {id: translation}}) + + expect(entry.reload.translations).to be_empty + expect(flash[:error]).to be_present + expect(response).to redirect_to(admin_root_path) + end + + it 'does not allow marking entry of other account as translation' do + entry = create(:entry) + user = create(:user, :publisher, on: entry.account) + entry_of_other_account = create(:entry) + + sign_in(user, scope: :user) + post(:create, params: {entry_id: entry, entry: {id: entry_of_other_account}}) + + expect(entry.reload.translations).to be_empty + expect(flash[:error]).to be_present + expect(response).to redirect_to(admin_root_path) + end + end + + describe '#destroy' do + it 'lets account publisher remove entry from translation group' do + entry = create(:entry) + translation = create(:entry, account: entry.account) + entry.mark_as_translation_of(translation) + user = create(:user, :publisher, on: entry.account) + + sign_in(user, scope: :user) + delete(:destroy, params: {entry_id: entry, id: translation}) + + expect(response) + .to redirect_to(admin_entry_path(id: entry, tab: 'translations')) + expect(entry.reload.translations).to be_empty + end + + it 'requires publisher role on account' do + entry = create(:entry) + translation = create(:entry, account: entry.account) + entry.mark_as_translation_of(translation) + user = create(:user, :editor, on: entry.account) + + sign_in(user, scope: :user) + delete(:destroy, params: {entry_id: entry, id: translation}) + + expect(entry.reload.translations).to eq([entry, translation]) + expect(flash[:error]).to be_present + expect(response).to redirect_to(admin_root_path) + end + end + + describe '#potential_entry_translations' do + it 'lists entries from same account' do + entry = create(:entry) + translation = create(:entry, account: entry.account) + user = create(:user, :publisher, on: entry.account) + + sign_in(user, scope: :user) + get(:potential_entry_translations_options, params: {entry_id: entry}, format: :json) + + expect(response.body).to include_json(results: [ + {text: translation.title, id: translation.id} + ]) + end + + it 'requires publisher role on account' do + entry = create(:entry) + user = create(:user, :editor, on: entry.account) + + sign_in(user, scope: :user) + get(:potential_entry_translations_options, params: {entry_id: entry}, format: :json) + + expect(response.status).to eq(401) + end + end +end diff --git a/spec/helpers/pageflow/admin/entry_translations_helper_spec.rb b/spec/helpers/pageflow/admin/entry_translations_helper_spec.rb new file mode 100644 index 000000000..f7345cd93 --- /dev/null +++ b/spec/helpers/pageflow/admin/entry_translations_helper_spec.rb @@ -0,0 +1,28 @@ +require 'spec_helper' + +module Pageflow + module Admin + describe EntryTranslationsHelper do + describe '#entry_translation_display_locale' do + it 'renders locale for unplublished entry' do + entry = create(:entry, draft_attributes: {locale: 'de'}) + + result = helper.entry_translation_display_locale(entry) + + expect(result).to eq('Deutsch') + end + + it 'prefers locale of published revision' do + entry = create(:entry, + :published, + draft_attributes: {locale: 'de'}, + published_revision_attributes: {locale: 'en'}) + + result = helper.entry_translation_display_locale(entry) + + expect(result).to eq('English') + end + end + end + end +end diff --git a/spec/policies/pageflow/entry_policy_spec.rb b/spec/policies/pageflow/entry_policy_spec.rb index 9548816be..d8a4926ba 100644 --- a/spec/policies/pageflow/entry_policy_spec.rb +++ b/spec/policies/pageflow/entry_policy_spec.rb @@ -57,6 +57,19 @@ module Pageflow to: :duplicate, topic: -> { create(:entry) } + it_behaves_like 'a membership-based permission that', + allows: :publisher, + but_forbids: :editor, + of_account: ->(topic) { topic.account }, + to: :manage_translations, + topic: -> { create(:entry) } + + it_behaves_like 'a membership-based permission that', + forbids: :manager, + of_entry: ->(topic) { topic }, + to: :manage_translations, + topic: -> { create(:entry) } + it_behaves_like 'a membership-based permission that', allows: :editor, but_forbids: :previewer, diff --git a/spec/views/components/pageflow/admin/entry_translations_tab_spec.rb b/spec/views/components/pageflow/admin/entry_translations_tab_spec.rb new file mode 100644 index 000000000..33bc21858 --- /dev/null +++ b/spec/views/components/pageflow/admin/entry_translations_tab_spec.rb @@ -0,0 +1,57 @@ +require 'spec_helper' + +module Pageflow + describe Admin::EntryTranslationsTab, type: :view_component do + before do + helper.extend(ActiveAdmin::ViewHelpers) + helper.extend(Pageflow::Admin::EntryTranslationsHelper) + end + + it 'renders table with translations of entry' do + de_entry = create(:entry, draft_attributes: {locale: 'de'}) + en_entry = create(:entry, title: 'My story EN', draft_attributes: {locale: 'en'}) + de_entry.mark_as_translation_of(en_entry) + + allow(helper).to receive(:authorized?).and_return(true) + + detect_n_plus_one_queries do + render(de_entry) + end + + expect(rendered).to have_selector('table td a', text: 'My story EN') + expect(rendered).to have_selector('table td', text: 'English') + expect(rendered).to have_selector('table td a', text: 'Remove') + expect(rendered).to have_selector('a', text: 'Link translation') + end + + it 'hides remove and add links if user cannot manage translations' do + de_entry = create(:entry, draft_attributes: {locale: 'de'}) + en_entry = create(:entry, title: 'My story EN', draft_attributes: {locale: 'en'}) + de_entry.mark_as_translation_of(en_entry) + + allow(helper).to receive(:authorized?).and_return(true) + allow(helper) + .to receive(:authorized?).with(:manage_translations, de_entry).and_return(false) + + render(de_entry) + + expect(rendered).not_to have_selector('table td a', text: 'Remove') + expect(rendered).not_to have_selector('a', text: 'Link translation') + end + + it 'does not link to entry the user cannot read' do + de_entry = create(:entry, draft_attributes: {locale: 'de'}) + en_entry = create(:entry, title: 'My story EN', draft_attributes: {locale: 'en'}) + de_entry.mark_as_translation_of(en_entry) + + allow(helper).to receive(:authorized?).and_return(true) + allow(helper) + .to receive(:authorized?).with(:read, en_entry).and_return(false) + + render(de_entry) + + expect(rendered).to have_selector('table td', text: 'My story EN') + expect(rendered).not_to have_selector('table td a', text: 'My story EN') + end + end +end From f789b375aae8722a3049a879cbe7816ec0254151 Mon Sep 17 00:00:00 2001 From: Tim Fischbach Date: Mon, 22 Jan 2024 11:45:15 +0100 Subject: [PATCH 06/10] Allow marking default translation in translation group REDMINE-20564 --- admins/pageflow/translations.rb | 9 +++ .../admin/entry_translations_helper.rb | 8 ++- app/models/concerns/pageflow/translatable.rb | 37 ++++++++--- .../pageflow/entry_translation_group.rb | 5 ++ .../pageflow/admin/entry_translations_tab.rb | 47 +++++++++++--- config/locales/new/entry_translations.de.yml | 11 +++- config/locales/new/entry_translations.en.yml | 11 +++- ...create_pageflow_entry_translation_group.rb | 1 + .../admin/translations_controller_spec.rb | 30 +++++++++ .../admin/entry_translations_helper_spec.rb | 14 ++++ .../concerns/pageflow/translatable_spec.rb | 65 ++++++++++++++++++- .../potential_entry_translations_spec.rb | 2 +- .../admin/entry_translations_tab_spec.rb | 44 +++++++++++++ 13 files changed, 260 insertions(+), 24 deletions(-) diff --git a/admins/pageflow/translations.rb b/admins/pageflow/translations.rb index bd82bbba1..0106411d6 100644 --- a/admins/pageflow/translations.rb +++ b/admins/pageflow/translations.rb @@ -26,6 +26,15 @@ module Pageflow form partial: 'form' + member_action :default, method: :put do + entry = Entry.find(params[:id]) + + authorize!(:manage_translations, entry) + entry.mark_as_default_translation + + redirect_to(admin_entry_path(parent, tab: 'translations')) + end + controller do helper Pageflow::Admin::FormHelper diff --git a/app/helpers/pageflow/admin/entry_translations_helper.rb b/app/helpers/pageflow/admin/entry_translations_helper.rb index 8492a1f53..cb619ce68 100644 --- a/app/helpers/pageflow/admin/entry_translations_helper.rb +++ b/app/helpers/pageflow/admin/entry_translations_helper.rb @@ -3,10 +3,16 @@ module Admin # @api private module EntryTranslationsHelper def entry_translation_display_locale(entry) - I18n.t( + display_locale = t( 'pageflow.public._language', locale: (entry.published_revision || entry.draft).locale ) + + if entry.default_translation? + t('pageflow.admin.entry_translations.default_translation', display_locale:) + else + display_locale + end end end end diff --git a/app/models/concerns/pageflow/translatable.rb b/app/models/concerns/pageflow/translatable.rb index a1b3ec4e2..adf2e4298 100644 --- a/app/models/concerns/pageflow/translatable.rb +++ b/app/models/concerns/pageflow/translatable.rb @@ -4,25 +4,26 @@ module Translatable extend ActiveSupport::Concern included do - belongs_to :translation_group, + belongs_to(:translation_group, optional: true, - class_name: 'EntryTranslationGroup' + class_name: 'EntryTranslationGroup') - has_many :translations, - ->(entry) { excluding(entry) }, + has_many(:translations, through: :translation_group, - source: :entries + source: :entries) after_destroy do - translation_group.destroy if translation_group&.single_item_or_empty? + if translation_group&.single_item_or_empty? + translation_group.destroy + elsif default_translation? + translation_group.update(default_translation: nil) + end end end def mark_as_translation_of(entry) transaction do - unless translation_group - update!(translation_group: entry.translation_group || build_translation_group) - end + ensure_translation_group(entry) if !entry.translation_group entry.update!(translation_group:) @@ -36,8 +37,26 @@ def remove_from_translation_group if translation_group.entries.count <= 2 translation_group.destroy else + translation_group.update(default_translation: nil) if default_translation? update!(translation_group: nil) end end + + def mark_as_default_translation + translation_group.update!(default_translation: self) + end + + def default_translation? + translation_group&.default_translation == self + end + + private + + def ensure_translation_group(other_entry) + return if translation_group + + update!(translation_group: other_entry.translation_group || + build_translation_group(default_translation: self)) + end end end diff --git a/app/models/pageflow/entry_translation_group.rb b/app/models/pageflow/entry_translation_group.rb index 5b3a6ba4e..2a31d785d 100644 --- a/app/models/pageflow/entry_translation_group.rb +++ b/app/models/pageflow/entry_translation_group.rb @@ -2,9 +2,14 @@ module Pageflow # @api private class EntryTranslationGroup < ApplicationRecord has_many :entries, + -> { order('title ASC') }, foreign_key: 'translation_group_id', dependent: :nullify + belongs_to :default_translation, + class_name: 'Entry', + optional: true + def merge_into(translation_group) entries.update_all(translation_group_id: translation_group.id) destroy diff --git a/app/views/components/pageflow/admin/entry_translations_tab.rb b/app/views/components/pageflow/admin/entry_translations_tab.rb index f8a57eede..47858e380 100644 --- a/app/views/components/pageflow/admin/entry_translations_tab.rb +++ b/app/views/components/pageflow/admin/entry_translations_tab.rb @@ -3,6 +3,8 @@ module Admin # @api private class EntryTranslationsTab < ViewComponent def build(entry) + default_translation_hint(entry) + embedded_index_table( entry.translations.preload(:draft, :published_revision, :translation_group), blank_slate_text: I18n.t('pageflow.admin.entry_translations.no_translations') @@ -12,11 +14,14 @@ def build(entry) entry_publication_state_indicator(translated_entry) end column :title do |translated_entry| - entry_link_or_title(translated_entry) + entry_link_or_title(entry, translated_entry) end column :locale do |translated_entry| entry_translation_display_locale(translated_entry) end + column do |translated_entry| + mark_as_default_link(entry, translated_entry) + end column do |translated_entry| remove_link(entry, translated_entry) end @@ -28,8 +33,16 @@ def build(entry) private - def entry_link_or_title(translated_entry) - if authorized?(:read, translated_entry) + def default_translation_hint(entry) + return if entry.translations.none? + + div class: 'side_hint' do + para text_node I18n.t('pageflow.admin.entry_translations.default_translation_hint') + end + end + + def entry_link_or_title(entry, translated_entry) + if authorized?(:read, translated_entry) && translated_entry != entry link_to(translated_entry.title, admin_entry_path(translated_entry, tab: 'translations')) else @@ -37,15 +50,31 @@ def entry_link_or_title(translated_entry) end end + def mark_as_default_link(entry, translated_entry) + return unless authorized?(:manage_translations, entry) + return if translated_entry.default_translation? + + link_to( + I18n.t('pageflow.admin.entry_translations.mark_as_default'), + default_admin_entry_translation_path(entry, translated_entry), + method: :put, + data: { + confirm: I18n.t('pageflow.admin.entry_translations.mark_as_default_confirmation') + } + ) + end + def remove_link(entry, translated_entry) return unless authorized?(:manage_translations, entry) - link_to(I18n.t('pageflow.admin.entries.remove'), - admin_entry_translation_path(entry, translated_entry), - method: :delete, - data: { - confirm: I18n.t('pageflow.admin.entry_translations.delete_confirmation') - }) + link_to( + I18n.t('pageflow.admin.entry_translations.remove'), + admin_entry_translation_path(entry, translated_entry), + method: :delete, + data: { + confirm: I18n.t('pageflow.admin.entry_translations.remove_confirmation') + } + ) end def add_link(entry) diff --git a/config/locales/new/entry_translations.de.yml b/config/locales/new/entry_translations.de.yml index 05e90fd47..994ffc6e4 100644 --- a/config/locales/new/entry_translations.de.yml +++ b/config/locales/new/entry_translations.de.yml @@ -12,8 +12,17 @@ de: Sprachen darstellen. create_model: Als Übersetzung zuordnen no_translations: Keine Beiträge zugeordnet - delete_confirmation: |- + remove: Entfernen + remove_confirmation: |- Beitrag wirklich aus der Liste der Übersetzungen entfernen? + mark_as_default: Als Standard markieren + mark_as_default_confirmation: |- + Beitrag wirklich as Standardübersetzung markieren? + default_translation: "%{display_locale} (Standard)" + default_translation_hint: |- + Suchmaschinen verwenden den als Standard markierten Beitrag, + falls keine Übersetzung verfügbar ist, die den + Spracheinstellungen des Besuchers entspricht. activerecord: models: translation: diff --git a/config/locales/new/entry_translations.en.yml b/config/locales/new/entry_translations.en.yml index cbdfce873..d4092bf77 100644 --- a/config/locales/new/entry_translations.en.yml +++ b/config/locales/new/entry_translations.en.yml @@ -12,8 +12,17 @@ en: different languages. create_model: Link as translation no_translations: No linked entries - delete_confirmation: |- + remove: Remove + remove_confirmation: |- Are you sure you want to remove the entry from the list of translations? + mark_as_default: Mark as default + mark_as_default_confirmation: |- + Really mark this translation as default? + default_translation: "%{display_locale} (Standard)" + default_translation_hint: |- + Search engines fall back to the default language if no + translation is available that matches the user's language + settings. activerecord: models: translation: diff --git a/db/migrate/20230419083307_create_pageflow_entry_translation_group.rb b/db/migrate/20230419083307_create_pageflow_entry_translation_group.rb index 99778e383..dee422389 100644 --- a/db/migrate/20230419083307_create_pageflow_entry_translation_group.rb +++ b/db/migrate/20230419083307_create_pageflow_entry_translation_group.rb @@ -1,6 +1,7 @@ class CreatePageflowEntryTranslationGroup < ActiveRecord::Migration[5.2] def change create_table :pageflow_entry_translation_groups do |t| + t.belongs_to :default_translation, index: false end add_reference(:pageflow_entries, :translation_group) diff --git a/spec/controllers/admin/translations_controller_spec.rb b/spec/controllers/admin/translations_controller_spec.rb index 7c18996a7..b60ae32e1 100644 --- a/spec/controllers/admin/translations_controller_spec.rb +++ b/spec/controllers/admin/translations_controller_spec.rb @@ -84,6 +84,36 @@ end end + describe '#default' do + it 'marks translation as default of group' do + entry = create(:entry) + translation = create(:entry, account: entry.account) + entry.mark_as_translation_of(translation) + user = create(:user, :publisher, on: entry.account) + + sign_in(user, scope: :user) + put(:default, params: {entry_id: entry, id: translation}) + + expect(response) + .to redirect_to(admin_entry_path(id: entry, tab: 'translations')) + expect(entry.translation_group.reload.default_translation).to eq(translation) + end + + it 'requires publisher role on account' do + entry = create(:entry) + translation = create(:entry, account: entry.account) + entry.mark_as_translation_of(translation) + user = create(:user, :editor, on: entry.account) + + sign_in(user, scope: :user) + put(:default, params: {entry_id: entry, id: translation}) + + expect(flash[:error]).to be_present + expect(response).to redirect_to(admin_root_path) + expect(entry.translation_group.reload.default_translation).to eq(entry) + end + end + describe '#destroy' do it 'lets account publisher remove entry from translation group' do entry = create(:entry) diff --git a/spec/helpers/pageflow/admin/entry_translations_helper_spec.rb b/spec/helpers/pageflow/admin/entry_translations_helper_spec.rb index f7345cd93..b733172ea 100644 --- a/spec/helpers/pageflow/admin/entry_translations_helper_spec.rb +++ b/spec/helpers/pageflow/admin/entry_translations_helper_spec.rb @@ -22,6 +22,20 @@ module Admin expect(result).to eq('English') end + + it 'marks default translation' do + entry = create(:entry, + :published, + draft_attributes: {locale: 'de'}, + published_revision_attributes: {locale: 'en'}) + translation = create(:entry) + entry.mark_as_translation_of(translation) + entry.mark_as_default_translation + + result = helper.entry_translation_display_locale(entry) + + expect(result).to eq('English (Standard)') + end end end end diff --git a/spec/models/concerns/pageflow/translatable_spec.rb b/spec/models/concerns/pageflow/translatable_spec.rb index bea20fb2a..159816452 100644 --- a/spec/models/concerns/pageflow/translatable_spec.rb +++ b/spec/models/concerns/pageflow/translatable_spec.rb @@ -13,6 +13,15 @@ module Pageflow expect(other_entry.translations).to include(entry) end + it 'makes first entry of group the default translation' do + entry = create(:entry) + other_entry = create(:entry) + + entry.mark_as_translation_of(other_entry) + + expect(entry.default_translation?).to eq(true) + end + it 'adds entry to existing translation group of other entry' do entry = create(:entry) fr_entry = create(:entry) @@ -63,7 +72,19 @@ module Pageflow en_entry.remove_from_translation_group - expect(pl_entry.translations).to eq([fr_entry]) + expect(pl_entry.translations).to eq([pl_entry, fr_entry]) + end + + it 'resets default translation' do + translation_group = create(:entry_translation_group) + entry = create(:entry, translation_group:) + create(:entry, translation_group:) + create(:entry, translation_group:) + + entry.mark_as_default_translation + entry.remove_from_translation_group + + expect(translation_group.default_translation).to be_nil end it 'destroys translation group with only one entry' do @@ -99,7 +120,19 @@ module Pageflow en_entry.destroy - expect(fr_entry.translations).to eq([pl_entry]) + expect(fr_entry.translations).to eq([fr_entry, pl_entry]) + end + + it 'destroying the default translations resets the association' do + translation_group = create(:entry_translation_group) + entry = create(:entry, translation_group:) + create(:entry, translation_group:) + create(:entry, translation_group:) + + entry.mark_as_default_translation + entry.destroy + + expect(translation_group.default_translation).to be_nil end it 'allows destroying an entry without translation group' do @@ -110,4 +143,32 @@ module Pageflow }.not_to raise_error end end + + describe '#mark_as_default_translation' do + it 'updates translation group' do + translation_group = create(:entry_translation_group) + entry = create(:entry, translation_group:) + + entry.mark_as_default_translation + + expect(translation_group.reload.default_translation).to eq(entry) + end + end + + describe '#default_translation?' do + it 'returns false by default' do + entry = create(:entry) + + expect(entry.default_translation?).to eq(false) + end + + it 'returns true once marked as default' do + translation_group = create(:entry_translation_group) + entry = create(:entry, translation_group:) + + entry.mark_as_default_translation + + expect(entry.default_translation?).to eq(true) + end + end end diff --git a/spec/models/pageflow/potential_entry_translations_spec.rb b/spec/models/pageflow/potential_entry_translations_spec.rb index 91ab0db11..7273cf82b 100644 --- a/spec/models/pageflow/potential_entry_translations_spec.rb +++ b/spec/models/pageflow/potential_entry_translations_spec.rb @@ -21,7 +21,7 @@ module Pageflow result = query.resolve.ransack(title_cont: 'story EN').result - expect(result).to eq([entry_en, other_entry_en]) + expect(result).to eq([other_entry_en, entry_en]) end it 'excludes entry itself' do diff --git a/spec/views/components/pageflow/admin/entry_translations_tab_spec.rb b/spec/views/components/pageflow/admin/entry_translations_tab_spec.rb index 33bc21858..905abff91 100644 --- a/spec/views/components/pageflow/admin/entry_translations_tab_spec.rb +++ b/spec/views/components/pageflow/admin/entry_translations_tab_spec.rb @@ -22,6 +22,49 @@ module Pageflow expect(rendered).to have_selector('table td', text: 'English') expect(rendered).to have_selector('table td a', text: 'Remove') expect(rendered).to have_selector('a', text: 'Link translation') + expect(rendered).to have_selector('a', text: 'Mark as default') + end + + it 'includes self in table without link' do + de_entry = create(:entry, title: 'My story DE', draft_attributes: {locale: 'de'}) + en_entry = create(:entry, title: 'My story EN', draft_attributes: {locale: 'en'}) + de_entry.mark_as_translation_of(en_entry) + + allow(helper).to receive(:authorized?).and_return(true) + + render(de_entry) + + expect(rendered).to have_selector('table td', text: 'My story DE') + expect(rendered).not_to have_selector('table td a', text: 'My story DE') + end + + it 'hides mark as default for default translation' do + de_entry = create(:entry, draft_attributes: {locale: 'de'}) + en_entry = create(:entry, title: 'My story EN', draft_attributes: {locale: 'en'}) + de_entry.mark_as_translation_of(en_entry) + en_entry.mark_as_default_translation + + allow(helper).to receive(:authorized?).and_return(true) + + render(de_entry) + + expect(rendered).to have_selector('a', text: 'Mark as default').once + end + + it 'renders table with translations of entry' do + de_entry = create(:entry, draft_attributes: {locale: 'de'}) + en_entry = create(:entry, title: 'My story EN', draft_attributes: {locale: 'en'}) + de_entry.mark_as_translation_of(en_entry) + + allow(helper).to receive(:authorized?).and_return(true) + + render(de_entry) + + expect(rendered).to have_selector('table td a', text: 'My story EN') + expect(rendered).to have_selector('table td', text: 'English') + expect(rendered).to have_selector('table td a', text: 'Remove') + expect(rendered).to have_selector('a', text: 'Link translation') + expect(rendered).to have_selector('a', text: 'Mark as default') end it 'hides remove and add links if user cannot manage translations' do @@ -36,6 +79,7 @@ module Pageflow render(de_entry) expect(rendered).not_to have_selector('table td a', text: 'Remove') + expect(rendered).not_to have_selector('table td a', text: 'Mark as default') expect(rendered).not_to have_selector('a', text: 'Link translation') end From 545c243f082aa53dedf116773379460a07f90d6b Mon Sep 17 00:00:00 2001 From: Tim Fischbach Date: Wed, 24 Jan 2024 08:01:13 +0100 Subject: [PATCH 07/10] Render hreflang alternate links in entry meta data REDMINE-20564 --- app/helpers/pageflow/hreflang_links_helper.rb | 34 +++++++++ app/models/concerns/pageflow/translatable.rb | 5 ++ .../pageflow/entry_translation_group.rb | 2 +- app/models/pageflow/published_entry.rb | 17 +++-- .../pageflow_paged/entries_controller.rb | 1 + .../pageflow_paged/entries/show.html.erb | 1 + .../pageflow_scrolled/entries_controller.rb | 1 + .../editor/seed_html_helper.rb | 1 + .../pageflow_scrolled/entries/show.html.erb | 1 + .../pageflow/hreflang_links_helper_spec.rb | 47 +++++++++++++ spec/models/pageflow/published_entry_spec.rb | 69 ++++++++++++++++++- 11 files changed, 172 insertions(+), 7 deletions(-) create mode 100644 app/helpers/pageflow/hreflang_links_helper.rb create mode 100644 spec/helpers/pageflow/hreflang_links_helper_spec.rb diff --git a/app/helpers/pageflow/hreflang_links_helper.rb b/app/helpers/pageflow/hreflang_links_helper.rb new file mode 100644 index 000000000..5b2700f39 --- /dev/null +++ b/app/helpers/pageflow/hreflang_links_helper.rb @@ -0,0 +1,34 @@ +module Pageflow + # Helpers to render alternate links to translations of an entry. + # + # @since edge + module HreflangLinksHelper + include SocialShareHelper + + # Render alternate links to all published entries that have been + # marked as translations of the given entry. + def hreflang_link_tags_for_entry(entry) + translations = + entry.translations(-> { preload(:site, :translation_group, permalink: :directory) }) + + safe_join( + translations.each_with_object([]) do |translation, links| + links << hreflang_link_tag(translation) + + if translation.default_translation? + links << hreflang_link_tag(translation, hreflang: 'x-default') + end + end + ) + end + + private + + def hreflang_link_tag(entry, hreflang: entry.locale) + tag('link', + rel: 'alternate', + hreflang:, + href: social_share_entry_url(entry)) + end + end +end diff --git a/app/models/concerns/pageflow/translatable.rb b/app/models/concerns/pageflow/translatable.rb index adf2e4298..c839c932c 100644 --- a/app/models/concerns/pageflow/translatable.rb +++ b/app/models/concerns/pageflow/translatable.rb @@ -12,6 +12,11 @@ module Translatable through: :translation_group, source: :entries) + has_many(:published_translations, + -> { published_without_password_protection }, + through: :translation_group, + source: :entries) + after_destroy do if translation_group&.single_item_or_empty? translation_group.destroy diff --git a/app/models/pageflow/entry_translation_group.rb b/app/models/pageflow/entry_translation_group.rb index 2a31d785d..73e969fa7 100644 --- a/app/models/pageflow/entry_translation_group.rb +++ b/app/models/pageflow/entry_translation_group.rb @@ -2,7 +2,7 @@ module Pageflow # @api private class EntryTranslationGroup < ApplicationRecord has_many :entries, - -> { order('title ASC') }, + -> { order(title: :asc) }, foreign_key: 'translation_group_id', dependent: :nullify diff --git a/app/models/pageflow/published_entry.rb b/app/models/pageflow/published_entry.rb index fa535da1f..98596443f 100644 --- a/app/models/pageflow/published_entry.rb +++ b/app/models/pageflow/published_entry.rb @@ -5,7 +5,9 @@ class PublishedEntry < EntryAtRevision attr_accessor :share_target - delegate(:authenticate, to: :entry) + delegate(:authenticate, + :default_translation?, + to: :entry) delegate(:password_protected?, to: :revision) @@ -18,6 +20,12 @@ def title revision.title.presence || entry.title end + def translations(scope = -> { self }) + self.class.wrap_all( + entry.published_translations.instance_exec(&scope) + ) + end + def stylesheet_model custom_revision? ? revision : entry end @@ -55,10 +63,9 @@ def self.find_by_permalink(directory: nil, slug:, scope:) ) end - def self.wrap_all(scope) - scope - .includes(:published_revision) - .map { |entry| new(entry) } + def self.wrap_all(entries) + entries = entries.includes(:published_revision) unless entries.loaded? + entries.map { |entry| new(entry) } end def cache_key diff --git a/entry_types/paged/app/controllers/pageflow_paged/entries_controller.rb b/entry_types/paged/app/controllers/pageflow_paged/entries_controller.rb index bc4d2e9e0..eb0197199 100644 --- a/entry_types/paged/app/controllers/pageflow_paged/entries_controller.rb +++ b/entry_types/paged/app/controllers/pageflow_paged/entries_controller.rb @@ -5,6 +5,7 @@ class EntriesController < PageflowPaged::ApplicationController include WithoutControllerNamespacePartialPathPrefix helper Pageflow::FeedsHelper + helper Pageflow::HreflangLinksHelper helper Pageflow::MetaTagsHelper helper Pageflow::StructuredDataHelper helper Pageflow::PublicI18nHelper diff --git a/entry_types/paged/app/views/pageflow_paged/entries/show.html.erb b/entry_types/paged/app/views/pageflow_paged/entries/show.html.erb index b9d05719c..596d3fb94 100644 --- a/entry_types/paged/app/views/pageflow_paged/entries/show.html.erb +++ b/entry_types/paged/app/views/pageflow_paged/entries/show.html.erb @@ -12,6 +12,7 @@ <%= tag :link, :rel => 'icon', :href => image_path("pageflow/themes/#{@entry.theme.directory_name}/favicon.ico"), :type => 'image/ico' %> <%= meta_tags_for_entry(@entry) %> + <%= hreflang_link_tags_for_entry(@entry) %> <%= feed_link_tags_for_entry(@entry) %> <% end %> <% end %> diff --git a/entry_types/scrolled/app/controllers/pageflow_scrolled/entries_controller.rb b/entry_types/scrolled/app/controllers/pageflow_scrolled/entries_controller.rb index e17b83a04..83a2474e3 100644 --- a/entry_types/scrolled/app/controllers/pageflow_scrolled/entries_controller.rb +++ b/entry_types/scrolled/app/controllers/pageflow_scrolled/entries_controller.rb @@ -5,6 +5,7 @@ class EntriesController < ActionController::Base helper Pageflow::EntriesHelper helper Pageflow::FeedsHelper + helper Pageflow::HreflangLinksHelper helper Pageflow::WidgetsHelper helper Pageflow::SocialShareHelper helper Pageflow::MetaTagsHelper diff --git a/entry_types/scrolled/app/helpers/pageflow_scrolled/editor/seed_html_helper.rb b/entry_types/scrolled/app/helpers/pageflow_scrolled/editor/seed_html_helper.rb index 2be6778f3..3e67e5817 100644 --- a/entry_types/scrolled/app/helpers/pageflow_scrolled/editor/seed_html_helper.rb +++ b/entry_types/scrolled/app/helpers/pageflow_scrolled/editor/seed_html_helper.rb @@ -20,6 +20,7 @@ def scrolled_editor_iframe_seed_html_script_tag(entry) skip_ssr: true, skip_structured_data: true, skip_feed_link_tags: true, + skip_hreflang_link_tags: true, seed_options: { skip_collections: true, include_unused_additional_seed_data: true, diff --git a/entry_types/scrolled/app/views/pageflow_scrolled/entries/show.html.erb b/entry_types/scrolled/app/views/pageflow_scrolled/entries/show.html.erb index 26886a3cc..b4c70a677 100644 --- a/entry_types/scrolled/app/views/pageflow_scrolled/entries/show.html.erb +++ b/entry_types/scrolled/app/views/pageflow_scrolled/entries/show.html.erb @@ -9,6 +9,7 @@ <%= social_share_meta_tags_for(entry) %> <%= meta_tags_for_entry(entry) %> + <%= hreflang_link_tags_for_entry(entry) unless local_assigns[:skip_hreflang_link_tags] %> <%= feed_link_tags_for_entry(entry) unless local_assigns[:skip_feed_link_tags] %> <%= scrolled_favicons_for_entry(entry, entry_mode: widget_scope) %> diff --git a/spec/helpers/pageflow/hreflang_links_helper_spec.rb b/spec/helpers/pageflow/hreflang_links_helper_spec.rb new file mode 100644 index 000000000..72202d8e3 --- /dev/null +++ b/spec/helpers/pageflow/hreflang_links_helper_spec.rb @@ -0,0 +1,47 @@ +require 'spec_helper' + +module Pageflow + describe HreflangLinksHelper do + describe '#hreflang_link_tags_for_entry' do + it 'is blank if entry does not have translations' do + entry = create(:published_entry) + + result = hreflang_link_tags_for_entry(entry) + + expect(result).to be_blank + end + + it 'renders hreflang hreflang links if entry has translations' do + en_entry = create(:published_entry, + title: 'entry-one', + permalink_attributes: { + slug: 'custom-slug', + directory_path: 'en/' + }, + revision_attributes: {locale: 'en'}) + de_entry = create(:published_entry, + title: 'entry-two', + permalink_attributes: { + slug: 'custom-slug', + directory_path: 'de/' + }, + revision_attributes: {locale: 'de'}) + en_entry.entry.mark_as_translation_of(de_entry.entry) + + result = detect_n_plus_one_queries do + hreflang_link_tags_for_entry(en_entry) + end + + expect(result) + .to have_selector('link[rel=alternate][hreflang=en][href="http://test.host/en/custom-slug"]', + visible: false) + expect(result) + .to have_selector('link[rel=alternate][hreflang=de][href="http://test.host/de/custom-slug"]', + visible: false) + expect(result) + .to have_selector('link[rel=alternate][hreflang=x-default][href="http://test.host/en/custom-slug"]', + visible: false) + end + end + end +end diff --git a/spec/models/pageflow/published_entry_spec.rb b/spec/models/pageflow/published_entry_spec.rb index 1f7de8619..7e92bf498 100644 --- a/spec/models/pageflow/published_entry_spec.rb +++ b/spec/models/pageflow/published_entry_spec.rb @@ -213,6 +213,71 @@ module Pageflow end end + describe '#translations' do + it 'returns published entries for published translations of entry' do + entry = create(:entry, :published) + translation = create(:entry, :published) + entry.mark_as_translation_of(translation) + published_entry = PublishedEntry.new(entry) + + result = published_entry.translations + + expect(result.length).to eq(2) + expect(result[0]).to be_kind_of(PublishedEntry) + expect(result[0].title).to eq(entry.title) + expect(result[1]).to be_kind_of(PublishedEntry) + expect(result[1].title).to eq(translation.title) + end + + it 'filters out non-published entries' do + entry = create(:entry, :published) + translation = create(:entry) + entry.mark_as_translation_of(translation) + published_entry = PublishedEntry.new(entry) + + result = published_entry.translations + + expect(result.length).to eq(1) + expect(result[0].title).to eq(entry.title) + end + + it 'filters out password protected entries' do + entry = create(:entry, :published) + translation = create(:entry, :published_with_password) + entry.mark_as_translation_of(translation) + published_entry = PublishedEntry.new(entry) + + result = published_entry.translations + + expect(result.length).to eq(1) + expect(result[0].title).to eq(entry.title) + end + + it 'allows modifying the entries scope' do + entry = create(:entry, :published) + translation = create(:entry, :published, permalink_attributes: {slug: 'some-slug'}) + entry.mark_as_translation_of(translation) + published_entry = PublishedEntry.new(entry) + + result = published_entry.translations(-> { preload(:permalink) }) + + expect(result[0].entry.association(:permalink).loaded?).to eq(true) + end + + it 'uses already preloaded published translations' do + entry = create(:entry, :published) + translation = create(:entry, :published, permalink_attributes: {slug: 'some-slug'}) + entry.mark_as_translation_of(translation) + published_entry = PublishedEntry.new( + Entry.preload(published_translations: :permalink).find(entry.id) + ) + + result = published_entry.translations + + expect(result[0].entry.association(:permalink).loaded?).to eq(true) + end + end + describe '.find' do it 'finds published entry' do entry = create(:entry, :published) @@ -377,7 +442,9 @@ module Pageflow published_revision_attributes: {title: 'Story Two'}) create(:entry) - result = PublishedEntry.wrap_all(Entry.published) + result = detect_n_plus_one_queries do + PublishedEntry.wrap_all(Entry.published) + end expect(result.map(&:title)).to eq(['Story One', 'Story Two']) end From 4c3b6fc06789a85726c93070c1ed89436b0a63e3 Mon Sep 17 00:00:00 2001 From: Tim Fischbach Date: Wed, 24 Jan 2024 08:04:08 +0100 Subject: [PATCH 08/10] Render hreflang links in sitemap.xml REDMINE-20564 --- app/models/pageflow/sitemaps.rb | 7 +- app/views/pageflow/sitemaps/index.xml.builder | 10 ++- .../pageflow/sitemaps_controller_spec.rb | 84 +++++++++++++++++++ 3 files changed, 99 insertions(+), 2 deletions(-) diff --git a/app/models/pageflow/sitemaps.rb b/app/models/pageflow/sitemaps.rb index 408464458..d3abaf39f 100644 --- a/app/models/pageflow/sitemaps.rb +++ b/app/models/pageflow/sitemaps.rb @@ -5,9 +5,14 @@ def self.entries_for(site:) PublishedEntry.wrap_all( site .entries + .preload(permalink: :directory, + published_translations: [ + :published_revision, + {permalink: :directory} + ]) .published_without_password_protection .published_without_noindex - .order('first_published_at DESC') + .order(first_published_at: :desc) ) end end diff --git a/app/views/pageflow/sitemaps/index.xml.builder b/app/views/pageflow/sitemaps/index.xml.builder index a3347c4a8..d6c7063d7 100644 --- a/app/views/pageflow/sitemaps/index.xml.builder +++ b/app/views/pageflow/sitemaps/index.xml.builder @@ -1,9 +1,17 @@ xml.instruct! -xml.urlset xmlns: 'http://www.sitemaps.org/schemas/sitemap/0.9' do +xml.urlset xmlns: 'http://www.sitemaps.org/schemas/sitemap/0.9', + 'xmlns:xhtml': 'http://www.w3.org/1999/xhtml' do @entries.each do |entry| xml.url do xml.loc(social_share_entry_url(entry)) xml.lastmod(entry.published_at.utc.xmlschema) + + entry.translations.each do |translation| + xml.tag!('xhtml:link', + rel: 'alternate', + hreflang: translation.locale, + href: social_share_entry_url(translation)) + end end end end diff --git a/spec/controllers/pageflow/sitemaps_controller_spec.rb b/spec/controllers/pageflow/sitemaps_controller_spec.rb index c5d9ce5d5..219560bec 100644 --- a/spec/controllers/pageflow/sitemaps_controller_spec.rb +++ b/spec/controllers/pageflow/sitemaps_controller_spec.rb @@ -65,6 +65,90 @@ module Pageflow expect(response.body).not_to have_xpath('//url') end + it 'renders links to entry translations' do + site = create(:site, cname: 'pageflow.example.com') + en_entry = create( + :entry, + :published, + title: 'Story One', + site:, + permalink_attributes: { + slug: 'story-one', + directory_path: 'en/' + }, + published_revision_attributes: { + locale: 'en' + } + ) + de_entry = create( + :entry, + :published, + title: 'Erster Beitrag', + site:, + permalink_attributes: { + slug: 'erster-beitrag', + directory_path: 'de/' + }, + published_revision_attributes: { + locale: 'de' + } + ) + en_entry.mark_as_translation_of(de_entry) + en_entry2 = create( + :entry, + :published, + title: 'Story two', + site:, + permalink_attributes: { + slug: 'story-two', + directory_path: 'en/' + }, + published_revision_attributes: { + locale: 'en' + } + ) + de_entry2 = create( + :entry, + :published, + title: 'Deux', + site:, + permalink_attributes: { + slug: 'deux', + directory_path: 'fr/' + }, + published_revision_attributes: { + locale: 'fr' + } + ) + en_entry2.mark_as_translation_of(de_entry2) + + request.env['HTTP_HOST'] = 'pageflow.example.com' + + detect_n_plus_one_queries do + get(:index, format: 'xml') + end + + expect(response.status).to eq(200) + expect(response.body) + .to include('xmlns:xhtml="http://www.w3.org/1999/xhtml') + expect(response.body) + .to have_xpath('//urlset/url[*[@rel="alternate" and @hreflang="de"' \ + ' and @href="http://pageflow.example.com/de/erster-beitrag"]]/loc', + text: 'http://pageflow.example.com/en/story-one') + expect(response.body) + .to have_xpath('//urlset/url[*[@rel="alternate" and @hreflang="en"' \ + ' and @href="http://pageflow.example.com/en/story-one"]]/loc', + text: 'http://pageflow.example.com/en/story-one') + expect(response.body) + .to have_xpath('//urlset/url[*[@rel="alternate" and @hreflang="de"' \ + ' and @href="http://pageflow.example.com/de/erster-beitrag"]]/loc', + text: 'http://pageflow.example.com/de/erster-beitrag') + expect(response.body) + .to have_xpath('//urlset/url[*[@rel="alternate" and @hreflang="en"' \ + ' and @href="http://pageflow.example.com/en/story-one"]]/loc', + text: 'http://pageflow.example.com/de/erster-beitrag') + end + it 'responds with 404 for unknown site' do site = create(:site, cname: 'pageflow.example.com') create(:entry, :published, site: site) From 37863380635dbb01572de95c9587094bec521a95 Mon Sep 17 00:00:00 2001 From: Tim Fischbach Date: Wed, 24 Jan 2024 09:53:19 +0100 Subject: [PATCH 09/10] Work around problem with preloading has many through association When loading entries for the sitemap we want to preload all translations. Preloading through the has may through relationship causes also non-published entries to be loaded as well. REDMINE-20564 --- app/models/concerns/pageflow/translatable.rb | 5 -- .../pageflow/entry_translation_group.rb | 5 ++ app/models/pageflow/published_entry.rb | 4 +- app/models/pageflow/sitemaps.rb | 10 ++-- .../pageflow/sitemaps_controller_spec.rb | 50 +++++++++++++++++++ spec/models/pageflow/published_entry_spec.rb | 14 +++++- 6 files changed, 77 insertions(+), 11 deletions(-) diff --git a/app/models/concerns/pageflow/translatable.rb b/app/models/concerns/pageflow/translatable.rb index c839c932c..adf2e4298 100644 --- a/app/models/concerns/pageflow/translatable.rb +++ b/app/models/concerns/pageflow/translatable.rb @@ -12,11 +12,6 @@ module Translatable through: :translation_group, source: :entries) - has_many(:published_translations, - -> { published_without_password_protection }, - through: :translation_group, - source: :entries) - after_destroy do if translation_group&.single_item_or_empty? translation_group.destroy diff --git a/app/models/pageflow/entry_translation_group.rb b/app/models/pageflow/entry_translation_group.rb index 73e969fa7..d1264c6fa 100644 --- a/app/models/pageflow/entry_translation_group.rb +++ b/app/models/pageflow/entry_translation_group.rb @@ -6,6 +6,11 @@ class EntryTranslationGroup < ApplicationRecord foreign_key: 'translation_group_id', dependent: :nullify + has_many :publicly_visible_entries, + -> { published_without_password_protection.published_without_noindex }, + foreign_key: 'translation_group_id', + class_name: 'Entry' + belongs_to :default_translation, class_name: 'Entry', optional: true diff --git a/app/models/pageflow/published_entry.rb b/app/models/pageflow/published_entry.rb index 98596443f..bbb92d682 100644 --- a/app/models/pageflow/published_entry.rb +++ b/app/models/pageflow/published_entry.rb @@ -21,8 +21,10 @@ def title end def translations(scope = -> { self }) + return [] unless entry.translation_group + self.class.wrap_all( - entry.published_translations.instance_exec(&scope) + entry.translation_group.publicly_visible_entries.instance_exec(&scope) ) end diff --git a/app/models/pageflow/sitemaps.rb b/app/models/pageflow/sitemaps.rb index d3abaf39f..6281324c0 100644 --- a/app/models/pageflow/sitemaps.rb +++ b/app/models/pageflow/sitemaps.rb @@ -6,10 +6,12 @@ def self.entries_for(site:) site .entries .preload(permalink: :directory, - published_translations: [ - :published_revision, - {permalink: :directory} - ]) + translation_group: { + publicly_visible_entries: [ + :published_revision, + {permalink: :directory} + ] + }) .published_without_password_protection .published_without_noindex .order(first_published_at: :desc) diff --git a/spec/controllers/pageflow/sitemaps_controller_spec.rb b/spec/controllers/pageflow/sitemaps_controller_spec.rb index 219560bec..347ce5fa6 100644 --- a/spec/controllers/pageflow/sitemaps_controller_spec.rb +++ b/spec/controllers/pageflow/sitemaps_controller_spec.rb @@ -149,6 +149,56 @@ module Pageflow text: 'http://pageflow.example.com/de/erster-beitrag') end + it 'does not render links to non-published entry translations' do + site = create(:site, cname: 'pageflow.example.com') + entry = create( + :entry, + :published, + site:, + published_revision_attributes: { + locale: 'en' + } + ) + entry.mark_as_translation_of( + create( + :entry, + :published_with_password, + site:, + published_revision_attributes: { + locale: 'de' + } + ) + ) + entry.mark_as_translation_of( + create( + :entry, + :published_with_noindex, + site:, + published_revision_attributes: { + locale: 'es' + } + ) + ) + entry.mark_as_translation_of( + create( + :entry, + site:, + draft_attributes: { + locale: 'fr' + } + ) + ) + + request.env['HTTP_HOST'] = 'pageflow.example.com' + + get(:index, format: 'xml') + + expect(response.status).to eq(200) + expect(response.body).not_to have_xpath('//*[@rel="alternate" and @hreflang="de"]') + expect(response.body).not_to have_xpath('//*[@rel="alternate" and @hreflang="es"]') + expect(response.body).not_to have_xpath('//*[@rel="alternate" and @hreflang="fr"]') + end + it 'responds with 404 for unknown site' do site = create(:site, cname: 'pageflow.example.com') create(:entry, :published, site: site) diff --git a/spec/models/pageflow/published_entry_spec.rb b/spec/models/pageflow/published_entry_spec.rb index 7e92bf498..2c67f0647 100644 --- a/spec/models/pageflow/published_entry_spec.rb +++ b/spec/models/pageflow/published_entry_spec.rb @@ -253,6 +253,18 @@ module Pageflow expect(result[0].title).to eq(entry.title) end + it 'filters out noindex entries' do + entry = create(:entry, :published) + translation = create(:entry, :published_with_noindex) + entry.mark_as_translation_of(translation) + published_entry = PublishedEntry.new(entry) + + result = published_entry.translations + + expect(result.length).to eq(1) + expect(result[0].title).to eq(entry.title) + end + it 'allows modifying the entries scope' do entry = create(:entry, :published) translation = create(:entry, :published, permalink_attributes: {slug: 'some-slug'}) @@ -269,7 +281,7 @@ module Pageflow translation = create(:entry, :published, permalink_attributes: {slug: 'some-slug'}) entry.mark_as_translation_of(translation) published_entry = PublishedEntry.new( - Entry.preload(published_translations: :permalink).find(entry.id) + Entry.preload(translation_group: {publicly_visible_entries: :permalink}).find(entry.id) ) result = published_entry.translations From 0ab629fefdfbcf8e43e7d193f439cf549e279da5 Mon Sep 17 00:00:00 2001 From: Tim Fischbach Date: Fri, 19 Jan 2024 14:05:11 +0100 Subject: [PATCH 10/10] Upgrade Rubocop Disable in Hound since it only supports Rubocop 1.22, which does not support Ruby 3.2. --- .hound.yml | 1 + .rubocop.yml | 10 +++------- pageflow.gemspec | 2 +- 3 files changed, 5 insertions(+), 8 deletions(-) diff --git a/.hound.yml b/.hound.yml index a598da023..f1d78bdd3 100644 --- a/.hound.yml +++ b/.hound.yml @@ -1,5 +1,6 @@ ruby: config_file: .rubocop.yml + enabled: false javascript: config_file: .jshintrc ignore_file: .jshintignore diff --git a/.rubocop.yml b/.rubocop.yml index c93a64ea0..7eb85b4bc 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -1,5 +1,5 @@ AllCops: - TargetRubyVersion: 2.5 + TargetRubyVersion: 3.2 # Use double quotes only for interpolation. Style/StringLiterals: @@ -14,7 +14,7 @@ Style/BlockDelimiters: EnforcedStyle: braces_for_chaining # The default of 80 characters is a little to narrow. -Metrics/LineLength: +Layout/LineLength: Max: 100 # Only place spaces inside blocks written with braces. @@ -56,7 +56,7 @@ Metrics/ModuleLength: Exclude: - '**/spec/**/*' -Metricts/ParameterLists: +Metrics/ParameterLists: CountKeywordArgs: false # Do not require class documentation for specs and migrations @@ -81,7 +81,3 @@ Style/ModuleFunction: # Allow method names like has_text? Naming/PredicateName: Enabled: false - -# Braces can have meaning in Ruby 3 -Style/BracesAroundHashParameters: - Enabled: false diff --git a/pageflow.gemspec b/pageflow.gemspec index fa864d0c5..0ba0803cb 100644 --- a/pageflow.gemspec +++ b/pageflow.gemspec @@ -211,7 +211,7 @@ Gem::Specification.new do |s| s.add_development_dependency 'ammeter', '~> 1.1' # Ruby code linter - s.add_development_dependency 'rubocop', '~> 0.54.0' + s.add_development_dependency 'rubocop', '~> 1.60' # Scss code linter s.add_development_dependency 'scss_lint', '~> 0.60.0'