diff --git a/app/models/alchemy/page.rb b/app/models/alchemy/page.rb index 4d76c9c576..6d9e540e78 100644 --- a/app/models/alchemy/page.rb +++ b/app/models/alchemy/page.rb @@ -7,12 +7,12 @@ # id :integer not null, primary key # name :string # urlname :string -# title :string +# title :string (deprecated - use draft_version.title) # language_code :string # language_root :boolean # page_layout :string -# meta_keywords :text -# meta_description :text +# meta_keywords :text (deprecated - use draft_version.meta_keywords) +# meta_description :text (deprecated - use draft_version.meta_description) # lft :integer # rgt :integer # parent_id :integer @@ -66,11 +66,12 @@ class Page < BaseRecord depth urlname cached_tag_list + title + meta_description + meta_keywords ] PERMITTED_ATTRIBUTES = [ - :meta_description, - :meta_keywords, :name, :page_layout, :public_on, @@ -81,10 +82,12 @@ class Page < BaseRecord :searchable, :sitemap, :tag_list, - :title, :urlname, :layoutpage, - :menu_id + :menu_id, + { + draft_version_attributes: [:id] + PageVersion::METADATA_ATTRIBUTES.map(&:to_sym) + } ] acts_as_nested_set(dependent: :destroy, scope: [:layoutpage, :language_id]) @@ -120,6 +123,8 @@ class Page < BaseRecord has_one :draft_version, -> { drafts }, class_name: "Alchemy::PageVersion" has_one :public_version, -> { published }, class_name: "Alchemy::PageVersion", autosave: -> { persisted? } + accepts_nested_attributes_for :draft_version + has_many :page_ingredients, class_name: "Alchemy::Ingredients::Page", foreign_key: :related_object_id, dependent: :nullify before_validation :set_language, @@ -129,8 +134,7 @@ class Page < BaseRecord validates_format_of :page_layout, with: /\A[a-z0-9_-]+\z/, unless: -> { page_layout.blank? } validates_presence_of :parent, unless: -> { layoutpage? || language_root? } - before_create -> { versions.build }, - if: -> { versions.none? } + after_initialize :ensure_draft_version before_save :set_language_code, if: -> { language.present? } @@ -179,7 +183,7 @@ def url_path_class=(klass) end def searchable_alchemy_resource_attributes - %w[name urlname title] + %w[name urlname] end # @return the language root page for given language id. @@ -213,8 +217,7 @@ def copy_and_paste(source, new_parent, new_name) .call(changed_attributes: { parent: new_parent, language: new_parent&.language, - name: new_name, - title: new_name + name: new_name }) if source.children.any? source.copy_children_to(page) @@ -458,6 +461,54 @@ def public_until attribute_fixed?(:public_until) ? fixed_attributes[:public_until] : public_version&.public_until end + # Returns the title from the public version, falling back to draft version + # + # If it's a fixed attribute then the fixed value is returned instead + # + def title + return fixed_attributes[:title] if attribute_fixed?(:title) + + public_version&.title || draft_version&.title + end + + # Returns the meta_description from the public version, falling back to draft version + # + # If it's a fixed attribute then the fixed value is returned instead + # + def meta_description + return fixed_attributes[:meta_description] if attribute_fixed?(:meta_description) + + public_version&.meta_description || draft_version&.meta_description + end + + # Returns the meta_keywords from the public version, falling back to draft version + # + # If it's a fixed attribute then the fixed value is returned instead + # + def meta_keywords + return fixed_attributes[:meta_keywords] if attribute_fixed?(:meta_keywords) + + public_version&.meta_keywords || draft_version&.meta_keywords + end + + # @deprecated Use draft_version.title= instead + def title=(value) + draft_version&.title = value + end + deprecate "title=": :"page.draft_version.title=", deprecator: Alchemy::Deprecation + + # @deprecated Use draft_version.meta_description= instead + def meta_description=(value) + draft_version&.meta_description = value + end + deprecate "meta_description=": :"page.draft_version.meta_description=", deprecator: Alchemy::Deprecation + + # @deprecated Use draft_version.meta_keywords= instead + def meta_keywords=(value) + draft_version&.meta_keywords = value + end + deprecate "meta_keywords=": :"page.draft_version.meta_keywords=", deprecator: Alchemy::Deprecation + # Returns the name of the creator of this page. # # If no creator could be found or associated user model @@ -499,9 +550,18 @@ def menus private + def ensure_draft_version + self.draft_version ||= versions.build + end + def set_fixed_attributes fixed_attributes.all.each do |attribute, value| - send(:"#{attribute}=", value) + attribute_name = attribute.to_s + if PageVersion::METADATA_ATTRIBUTES.include?(attribute_name) + draft_version&.send(:"#{attribute}=", value) + else + send(:"#{attribute}=", value) + end end end diff --git a/app/models/alchemy/page/page_naming.rb b/app/models/alchemy/page/page_naming.rb index badedb5c93..0b2d9bc823 100644 --- a/app/models/alchemy/page/page_naming.rb +++ b/app/models/alchemy/page/page_naming.rb @@ -19,9 +19,6 @@ module PageNaming uniqueness: {scope: [:language_id, :layoutpage], if: -> { urlname.present? }, case_sensitive: false}, exclusion: {in: RESERVED_URLNAMES} - before_save :set_title, - if: -> { title.blank? } - after_update :update_descendants_urlnames, if: :saved_change_to_urlname? @@ -69,10 +66,6 @@ def set_urlname self[:urlname] = nested_url_name end - def set_title - self[:title] = name - end - # Returns the full nested urlname. # def nested_url_name diff --git a/app/models/alchemy/page/publisher.rb b/app/models/alchemy/page/publisher.rb index f81f5bdc7b..554aa02b21 100644 --- a/app/models/alchemy/page/publisher.rb +++ b/app/models/alchemy/page/publisher.rb @@ -24,6 +24,8 @@ def publish!(public_on:) version = public_version(public_on) DeleteElements.new(version.elements).call + copy_metadata(public_version: version) + repository = page.draft_version.element_repository ActiveRecord::Base.no_touching do Element.acts_as_list_no_update do @@ -51,6 +53,16 @@ def publish!(public_on:) def public_version(public_on) page.public_version || page.versions.create!(public_on: public_on) end + + # Copy metadata from draft_version to public_version. + def copy_metadata(public_version:) + draft = page.draft_version + return unless draft + + PageVersion::METADATA_ATTRIBUTES.each do |attr| + public_version.send(:"#{attr}=", draft.send(attr)) + end + end end end end diff --git a/app/models/alchemy/page_version.rb b/app/models/alchemy/page_version.rb index 2c84c63d28..4694a0fedc 100644 --- a/app/models/alchemy/page_version.rb +++ b/app/models/alchemy/page_version.rb @@ -2,6 +2,13 @@ module Alchemy class PageVersion < BaseRecord + # Metadata attributes that are versioned (moved from Page) + METADATA_ATTRIBUTES = %w[ + title + meta_description + meta_keywords + ].freeze + belongs_to :page, class_name: "Alchemy::Page", inverse_of: :versions, touch: true has_many :elements, -> { order(:position) }, @@ -11,6 +18,8 @@ class PageVersion < BaseRecord scope :drafts, -> { where(public_on: nil).order(updated_at: :desc) } scope :published, -> { where.not(public_on: nil).order(public_on: :desc) } + before_create :set_title_from_page + def self.public_on(time = Time.current) where("#{table_name}.public_on <= :time AND " \ "(#{table_name}.public_until IS NULL " \ @@ -54,5 +63,11 @@ def element_repository def delete_elements DeleteElements.new(elements).call end + + def set_title_from_page + return if title.present? + + self.title = page&.name + end end end diff --git a/app/services/alchemy/copy_page.rb b/app/services/alchemy/copy_page.rb index 992813f995..a313890e9b 100644 --- a/app/services/alchemy/copy_page.rb +++ b/app/services/alchemy/copy_page.rb @@ -28,8 +28,14 @@ class CopyPage depth urlname cached_tag_list + title + meta_description + meta_keywords ] + # Metadata to copy via nested attributes (title is derived from page.name) + METADATA_ATTRIBUTES_TO_COPY = (Alchemy::PageVersion::METADATA_ATTRIBUTES - %w[title]).freeze + attr_reader :page # @param page [Alchemy::Page] @@ -70,6 +76,7 @@ def attributes_from_source_for_copy(differences = {}) .merge(DEFAULT_ATTRIBUTES_FOR_COPY) .merge(differences) desired_attributes["name"] = best_name_for_copy(source_attributes, desired_attributes) + desired_attributes["draft_version_attributes"] = draft_version_attributes_for_copy desired_attributes.except(*SKIPPED_ATTRIBUTES_ON_COPY) end @@ -94,5 +101,15 @@ def best_name_for_copy(source_attributes, desired_attributes) desired_name end end + + # Builds nested attributes for draft_version metadata (except title). + # Title is handled by PageVersion#set_title_from_page callback based on page.name. + def draft_version_attributes_for_copy + return {} unless page.draft_version + + METADATA_ATTRIBUTES_TO_COPY.each_with_object({}) do |attr, hash| + hash[attr] = page.draft_version.send(attr) + end + end end end diff --git a/app/views/alchemy/admin/pages/_form.html.erb b/app/views/alchemy/admin/pages/_form.html.erb index 49321d32d0..ea8740d557 100644 --- a/app/views/alchemy/admin/pages/_form.html.erb +++ b/app/views/alchemy/admin/pages/_form.html.erb @@ -19,9 +19,14 @@ <%= f.input :name, autofocus: true %> <%= f.input :urlname, as: 'string', input_html: {value: @page.slug}, label: Alchemy::Page.human_attribute_name(:slug) %> - - <%= f.input :title %> - + + <%= f.fields_for :draft_version, @page.draft_version do |v| %> + + <%= v.input :title, input_html: { + disabled: @page.attribute_fixed?(:title) + } %> + + <% end %> <% if Alchemy.config.show_page_searchable_checkbox %>
@@ -40,13 +45,20 @@
- - <%= f.input :meta_description, as: 'text' %> - + <%= f.fields_for :draft_version, @page.draft_version do |v| %> + + <%= v.input :meta_description, as: 'text', input_html: { + disabled: @page.attribute_fixed?(:meta_description) + } %> + - <%= f.input :meta_keywords, - as: 'text', - hint: Alchemy.t('pages.update.comma_seperated') %> + <%= v.input :meta_keywords, + as: 'text', + hint: Alchemy.t('pages.update.comma_seperated'), + input_html: { + disabled: @page.attribute_fixed?(:meta_keywords) + } %> + <% end %> <%= render Alchemy::Admin::TagsAutocomplete.new do %> <%= f.input :tag_list, input_html: { value: f.object.tag_list.join(",") } %> diff --git a/db/migrate/20260102121232_add_metadata_to_page_versions.rb b/db/migrate/20260102121232_add_metadata_to_page_versions.rb new file mode 100644 index 0000000000..29a4d838ad --- /dev/null +++ b/db/migrate/20260102121232_add_metadata_to_page_versions.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class AddMetadataToPageVersions < ActiveRecord::Migration[7.1] + def change + add_column :alchemy_page_versions, :title, :string + add_column :alchemy_page_versions, :meta_description, :text + add_column :alchemy_page_versions, :meta_keywords, :text + end +end diff --git a/lib/alchemy/tasks/usage.rb b/lib/alchemy/tasks/usage.rb index 6607fd1290..898149748c 100644 --- a/lib/alchemy/tasks/usage.rb +++ b/lib/alchemy/tasks/usage.rb @@ -18,11 +18,11 @@ def elements_count_by_name end def pages_count_by_type - res = Alchemy::Page.all - .select("page_layout, COUNT(*) AS count") + res = Alchemy::Page .group(:page_layout) - .order("count DESC, page_layout ASC") - .map { |p| {"page_layout" => p.page_layout, "count" => p.count} } + .order("count_all DESC, page_layout ASC") + .count + .map { |layout, count| {"page_layout" => layout, "count" => count} } Alchemy::PageDefinition.all.reject { |page_layout| res.map { |p| p["page_layout"] }.include?(page_layout.name) }.sort_by(&:name).each do |page_layout| res << {"page_layout" => page_layout.name, "count" => 0} end diff --git a/lib/alchemy/test_support/factories/page_version_factory.rb b/lib/alchemy/test_support/factories/page_version_factory.rb index 156e446890..31176289db 100644 --- a/lib/alchemy/test_support/factories/page_version_factory.rb +++ b/lib/alchemy/test_support/factories/page_version_factory.rb @@ -3,6 +3,9 @@ FactoryBot.define do factory :alchemy_page_version, class: "Alchemy::PageVersion" do association :page, factory: :alchemy_page + title { nil } + meta_description { nil } + meta_keywords { nil } trait :published do public_on { Time.current } diff --git a/lib/alchemy/upgrader.rb b/lib/alchemy/upgrader.rb index b5db370b67..ca955c1aab 100644 --- a/lib/alchemy/upgrader.rb +++ b/lib/alchemy/upgrader.rb @@ -12,11 +12,19 @@ class Upgrader Dir["#{File.dirname(__FILE__)}/upgrader/*.rb"].sort.each { require(_1) } VERSION_MODULE_MAP = { - "8.0" => "Alchemy::Upgrader::EightZero" + "8.0" => "Alchemy::Upgrader::EightZero", + "8.1" => "Alchemy::Upgrader::EightOne" } source_root Alchemy::Engine.root.join("lib/generators/alchemy/install") + # Returns a memoized upgrader instance for the given version. + # This ensures todos are accumulated across rake tasks. + def self.[](version) + @instances ||= {} + @instances[version.to_s] ||= new(version) + end + def initialize(version) super() self.class.include VERSION_MODULE_MAP[version.to_s].constantize diff --git a/lib/alchemy/upgrader/eight_one.rb b/lib/alchemy/upgrader/eight_one.rb new file mode 100644 index 0000000000..036e6a644a --- /dev/null +++ b/lib/alchemy/upgrader/eight_one.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +module Alchemy + class Upgrader + module EightOne + def migrate_page_metadata + desc "Migrating page metadata to page versions" + + connection = ActiveRecord::Base.connection + pages_table = Alchemy::Page.table_name + versions_table = Alchemy::PageVersion.table_name + + unmigrated_count = connection.select_value(<<-SQL.strip_heredoc).to_i + SELECT COUNT(*) FROM #{versions_table} + WHERE title IS NULL + AND meta_description IS NULL + AND meta_keywords IS NULL + SQL + + if unmigrated_count == 0 + log "All page versions have meta data.", :skip + return + end + + sql = if connection.adapter_name.downcase.match?(/mysql|trilogy/) + <<-SQL.strip_heredoc + UPDATE #{versions_table} pv + INNER JOIN #{pages_table} p ON pv.page_id = p.id + SET pv.title = p.title, + pv.meta_description = p.meta_description, + pv.meta_keywords = p.meta_keywords + WHERE pv.title IS NULL + AND pv.meta_description IS NULL + AND pv.meta_keywords IS NULL + SQL + else + <<-SQL.strip_heredoc + UPDATE #{versions_table} + SET title = p.title, + meta_description = p.meta_description, + meta_keywords = p.meta_keywords + FROM #{pages_table} p + WHERE #{versions_table}.page_id = p.id + AND #{versions_table}.title IS NULL + AND #{versions_table}.meta_description IS NULL + AND #{versions_table}.meta_keywords IS NULL + SQL + end + + migrated_count = connection.update(sql) + + log "Migrated metadata for #{migrated_count} page versions." + end + end + end +end diff --git a/lib/tasks/alchemy/upgrade.rake b/lib/tasks/alchemy/upgrade.rake index 9e2ba05229..64f05e39f6 100644 --- a/lib/tasks/alchemy/upgrade.rake +++ b/lib/tasks/alchemy/upgrade.rake @@ -3,16 +3,15 @@ require "alchemy/upgrader" require "alchemy/version" -Upgrader = Alchemy::Upgrader.new("8.0") - namespace :alchemy do desc "Upgrades your app to AlchemyCMS v#{Alchemy::VERSION}." task upgrade: [ "alchemy:upgrade:prepare", - "alchemy:upgrade:8.0:run" + "alchemy:upgrade:8.0:run", + "alchemy:upgrade:8.1:run" ] do - Upgrader.run_migrations - Upgrader.display_todos + Alchemy::Upgrader["8.1"].run_migrations + Alchemy::Upgrader["8.1"].display_todos end namespace :upgrade do @@ -29,7 +28,7 @@ namespace :alchemy do desc "Alchemy Upgrader: Update configuration file." task config: [:environment] do - Upgrader.update_config + Alchemy::Upgrader["8.0"].update_config end namespace "8.0" do @@ -38,7 +37,18 @@ namespace :alchemy do ] task :mention_alchemy_config_initializer do - Upgrader.mention_alchemy_config_initializer + Alchemy::Upgrader["8.0"].mention_alchemy_config_initializer + end + end + + namespace "8.1" do + task "run" => [ + "alchemy:upgrade:8.1:migrate_page_metadata" + ] + + desc "Migrate page metadata to page versions" + task migrate_page_metadata: [:environment] do + Alchemy::Upgrader["8.1"].migrate_page_metadata end end end diff --git a/spec/dummy/db/migrate/20260102111901_add_metadata_to_page_versions.alchemy.rb b/spec/dummy/db/migrate/20260102111901_add_metadata_to_page_versions.alchemy.rb new file mode 100644 index 0000000000..21ce644d4f --- /dev/null +++ b/spec/dummy/db/migrate/20260102111901_add_metadata_to_page_versions.alchemy.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +# This migration comes from alchemy (originally 20260102121232) +class AddMetadataToPageVersions < ActiveRecord::Migration[7.1] + def change + add_column :alchemy_page_versions, :title, :string + add_column :alchemy_page_versions, :meta_description, :text + add_column :alchemy_page_versions, :meta_keywords, :text + end +end diff --git a/spec/dummy/db/schema.rb b/spec/dummy/db/schema.rb index a6dbba0834..119e239d1d 100644 --- a/spec/dummy/db/schema.rb +++ b/spec/dummy/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.0].define(version: 2025_09_05_140502) do +ActiveRecord::Schema[8.0].define(version: 2026_01_02_111901) do create_table "active_storage_attachments", force: :cascade do |t| t.string "name", null: false t.string "record_type", null: false @@ -175,6 +175,9 @@ t.datetime "public_until", precision: nil t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.string "title" + t.text "meta_description" + t.text "meta_keywords" t.index ["page_id"], name: "index_alchemy_page_versions_on_page_id" t.index ["public_on", "public_until"], name: "index_alchemy_page_versions_on_public_on_and_public_until" end diff --git a/spec/features/admin/page_editing_feature_spec.rb b/spec/features/admin/page_editing_feature_spec.rb index 23119aa6aa..32bc5e1f30 100644 --- a/spec/features/admin/page_editing_feature_spec.rb +++ b/spec/features/admin/page_editing_feature_spec.rb @@ -83,7 +83,7 @@ it "should show all relevant input fields" do visit alchemy.configure_admin_page_path(a_page) expect(page).to have_selector("input#page_urlname") - expect(page).to have_selector("input#page_title") + expect(page).to have_selector("input#page_draft_version_attributes_title") expect(page).to have_selector("input#page_robot_index") expect(page).to have_selector("input#page_robot_follow") end @@ -269,7 +269,12 @@ it "is not possible to edit the attribute", :aggregate_failures do visit alchemy.configure_admin_page_path(readonly_page) readonly_page.fixed_attributes.all.each do |attribute, _v| - expect(page).to have_selector("#page_#{attribute}[disabled]") + # Version metadata attributes have different ID due to nested form + if Alchemy::PageVersion::METADATA_ATTRIBUTES.include?(attribute.to_s) + expect(page).to have_selector("#page_draft_version_attributes_#{attribute}[disabled]") + else + expect(page).to have_selector("#page_#{attribute}[disabled]") + end end end end diff --git a/spec/helpers/alchemy/pages_helper_spec.rb b/spec/helpers/alchemy/pages_helper_spec.rb index 2ad20e332c..53201ed8a9 100644 --- a/spec/helpers/alchemy/pages_helper_spec.rb +++ b/spec/helpers/alchemy/pages_helper_spec.rb @@ -168,14 +168,14 @@ module Alchemy end it "should render a breadcrumb of restricted pages only" do - page.update_columns(restricted: true, urlname: "a-restricted-public-page", name: "A restricted Public Page", title: "A restricted Public Page") + page.update_columns(restricted: true, urlname: "a-restricted-public-page", name: "A restricted Public Page") is_expected.to have_selector("*[contains(\"#{page.name}\")]") is_expected.to_not have_selector("*[contains(\"#{parent.name}\")]") end end it "should not include unpublished pages" do - page.update_columns(urlname: "a-unpublic-page", name: "A Unpublic Page", title: "A Unpublic Page") + page.update_columns(urlname: "a-unpublic-page", name: "A Unpublic Page") page.public_version.destroy is_expected.to_not match(/A Unpublic Page/) end @@ -186,7 +186,7 @@ module Alchemy end it "should render a breadcrumb without this page" do - page.update_columns(urlname: "not-me", name: "Not Me", title: "Not Me") + page.update_columns(urlname: "not-me", name: "Not Me") is_expected.not_to match(/Not Me/) end end @@ -197,7 +197,7 @@ module Alchemy end it "should render a breadcrumb without these pages." do - page.update_columns(urlname: "not-me", name: "Not Me", title: "Not Me") + page.update_columns(urlname: "not-me", name: "Not Me") is_expected.not_to match(/Not Me/) end end @@ -306,14 +306,14 @@ module Alchemy subject { helper.page_title } context "when current page has a title set" do - before { public_page.title = "My Public Page" } + before { public_page.public_version.title = "My Public Page" } it { is_expected.to eq "My Public Page" } end context "when current page has no title set" do before do - language_root.title = "Title from language root" + language_root.draft_version.title = "Title from language root" end context "when current page is the language root page" do @@ -326,7 +326,7 @@ module Alchemy context "when response status is not 200" do let(:response) { double("response", status: 404) } - before { public_page.title = "My Public Page" } + before { public_page.public_version.title = "My Public Page" } it "should return the status code" do is_expected.to eq "404" @@ -341,18 +341,18 @@ module Alchemy subject { helper.meta_description } context "when current page has a meta description set" do - before { public_page.meta_description = "description of my public page" } + before { public_page.public_version.meta_description = "description of my public page" } it { is_expected.to eq "description of my public page" } end context "when current page has no meta description set" do before do - language_root.meta_description = "description from language root" + language_root.draft_version.meta_description = "description from language root" allow(Language).to receive_messages(current_root_page: language_root) end context "when #meta_description is an empty string" do - before { public_page.meta_description = "" } + before { public_page.public_version.meta_description = "" } it "returns the meta description of its language root page" do is_expected.to eq "description from language root" @@ -360,7 +360,7 @@ module Alchemy end context "when #meta_description is nil" do - before { public_page.meta_description = nil } + before { public_page.public_version.meta_description = nil } it "returns the meta description of its language root page" do is_expected.to eq "description from language root" @@ -373,18 +373,18 @@ module Alchemy subject { helper.meta_keywords } context "when current page has meta keywords set" do - before { public_page.meta_keywords = "keywords, from public page" } + before { public_page.public_version.update!(meta_keywords: "keywords, from public page") } it { is_expected.to eq "keywords, from public page" } end context "when current page has no meta keywords set" do before do - language_root.meta_keywords = "keywords, from language root" + language_root.draft_version.update!(meta_keywords: "keywords, from language root") allow(Language).to receive_messages(current_root_page: language_root) end context "when #meta_keywords is an empty string" do - before { public_page.meta_keywords = "" } + before { public_page.public_version.update!(meta_keywords: "") } it "returns the keywords of its language root page" do is_expected.to eq "keywords, from language root" @@ -392,7 +392,7 @@ module Alchemy end context "when #meta_keywords is nil" do - before { public_page.meta_keywords = nil } + before { public_page.public_version.update!(meta_keywords: nil) } it "returns the keywords of its language root page" do is_expected.to eq "keywords, from language root" diff --git a/spec/lib/alchemy/upgrader/eight_one_spec.rb b/spec/lib/alchemy/upgrader/eight_one_spec.rb new file mode 100644 index 0000000000..0891a96803 --- /dev/null +++ b/spec/lib/alchemy/upgrader/eight_one_spec.rb @@ -0,0 +1,87 @@ +# frozen_string_literal: true + +require "rails_helper" +require "alchemy/upgrader" + +RSpec.describe Alchemy::Upgrader::EightOne do + let(:upgrader) { Alchemy::Upgrader["8.1"] } + + describe "#migrate_page_metadata" do + context "when all page versions have metadata" do + let!(:page) do + create(:alchemy_page).tap do |p| + p.draft_version.update_columns( + title: "Existing Title", + meta_description: "Existing description", + meta_keywords: "existing, keywords" + ) + end + end + + it "skips migration" do + expect { upgrader.migrate_page_metadata }.not_to raise_error + end + end + + context "with page versions without metadata" do + let!(:page) do + create(:alchemy_page).tap do |p| + p.update_columns( + title: "Page Title", + meta_description: "Page description", + meta_keywords: "page, keywords" + ) + p.draft_version.update_columns( + title: nil, + meta_description: nil, + meta_keywords: nil + ) + end + end + + it "copies metadata from page to all versions" do + upgrader.migrate_page_metadata + + page.versions.each do |version| + version.reload + expect(version.title).to eq("Page Title") + expect(version.meta_description).to eq("Page description") + expect(version.meta_keywords).to eq("page, keywords") + end + end + + it "is idempotent" do + upgrader.migrate_page_metadata + page.draft_version.update_columns(title: "Modified Title") + + upgrader.migrate_page_metadata + + expect(page.draft_version.reload.title).to eq("Modified Title") + end + + context "with published page" do + let!(:page) do + create(:alchemy_page, :public).tap do |p| + p.update_columns( + title: "Published Title", + meta_description: "Published description", + meta_keywords: "published, keywords" + ) + p.versions.update_all( + title: nil, + meta_description: nil, + meta_keywords: nil + ) + end + end + + it "copies metadata to both draft and public versions" do + upgrader.migrate_page_metadata + + expect(page.draft_version.reload.title).to eq("Published Title") + expect(page.public_version.reload.title).to eq("Published Title") + end + end + end + end +end diff --git a/spec/libraries/shell_spec.rb b/spec/libraries/shell_spec.rb index 639c008dcb..c60d13a06d 100644 --- a/spec/libraries/shell_spec.rb +++ b/spec/libraries/shell_spec.rb @@ -10,7 +10,10 @@ class MyToDoList end describe Shell do - before { allow(MyToDoList).to receive(:puts) } + before do + MyToDoList.todos.clear + allow(MyToDoList).to receive(:puts) + end describe ".todo" do it "should add given string as a todo by delegating to .add_todo" do diff --git a/spec/models/alchemy/page/publisher_spec.rb b/spec/models/alchemy/page/publisher_spec.rb index b5248dacd9..7f37115808 100644 --- a/spec/models/alchemy/page/publisher_spec.rb +++ b/spec/models/alchemy/page/publisher_spec.rb @@ -54,6 +54,24 @@ end end + context "with draft version metadata" do + before do + page.draft_version.update!( + title: "Draft Title", + meta_description: "Draft description", + meta_keywords: "draft, keywords" + ) + end + + it "copies metadata from draft_version to public_version" do + publish + page.reload + expect(page.public_version.title).to eq("Draft Title") + expect(page.public_version.meta_description).to eq("Draft description") + expect(page.public_version.meta_keywords).to eq("draft, keywords") + end + end + context "with published version existing" do let(:yesterday) { Date.yesterday.to_time } let!(:public_version) do diff --git a/spec/models/alchemy/page_spec.rb b/spec/models/alchemy/page_spec.rb index 8f58c8e4c0..c57f7a6faf 100644 --- a/spec/models/alchemy/page_spec.rb +++ b/spec/models/alchemy/page_spec.rb @@ -100,28 +100,41 @@ module Alchemy create(:alchemy_page, name: "My Testpage", language: language, parent: language_root) end - context "before_create" do + context "after_initialize" do let(:page) do build(:alchemy_page, language: language, parent: language_root) end it "builds a version" do - expect { - page.save! - }.to change { page.versions.length }.by(1) + expect(page.draft_version).to be_present end context "if there is already a version" do let(:page) do - build(:alchemy_page, language: language, parent: language_root).tap do |page| - page.versions.build - end + build(:alchemy_page, language: language, parent: language_root) end - it "builds no version" do + it "builds no additional version" do expect { page.save! }.to_not change { page.versions.length } end end + + context "if draft_version is set via nested attributes" do + let(:page) do + build(:alchemy_page, language: language, parent: language_root, + draft_version_attributes: {meta_description: "Test"}) + end + + it "has exactly one version" do + page.save! + expect(page.versions.length).to eq(1) + end + + it "sets it on the draft version" do + page.save! + expect(page.draft_version.meta_description).to eq("Test") + end + end end context "before_save" do @@ -133,7 +146,7 @@ module Alchemy end it "should not automatically set the title if it changed its value" do - page.title = "I like SEO" + page.draft_version.title = "I like SEO" page.save page.reload expect(page.title).to eq("I like SEO") @@ -692,7 +705,6 @@ module Alchemy "page_layout", "updated_at", "urlname", - "title", "name" ) end @@ -1231,14 +1243,14 @@ module Alchemy expect { subject }.to change { new_parent.children.count }.from(0).to 2 source_page.children.each do |child| - expect(new_parent.children.where(title: child.title).count).to eq 1 + expect(new_parent.children.where(name: child.name).count).to eq 1 end - source_page_grandchildren = source_page.children.find_by_title("child with children").children - new_parent_grandchildren = new_parent.children.find_by_title("child with children").children + source_page_grandchildren = source_page.children.find_by(name: "child with children").children + new_parent_grandchildren = new_parent.children.find_by(name: "child with children").children source_page_grandchildren.each do |grandchild| - expect(new_parent_grandchildren.where(title: grandchild.title).count).to eq 1 + expect(new_parent_grandchildren.where(name: grandchild.name).count).to eq 1 end end @@ -1249,14 +1261,14 @@ module Alchemy expect { subject }.to change { new_parent.children.count }.from(0).to 2 source_page.children.each do |child| - expect(new_parent.children.where(title: child.title).count).to eq 1 + expect(new_parent.children.where(name: child.name).count).to eq 1 end - source_page_grandchildren = source_page.children.find_by_title("child with children").children - new_parent_grandchildren = new_parent.children.find_by_title("child with children").children + source_page_grandchildren = source_page.children.find_by(name: "child with children").children + new_parent_grandchildren = new_parent.children.find_by(name: "child with children").children source_page_grandchildren.each do |grandchild| - expect(new_parent_grandchildren.where(title: grandchild.title).count).to eq 1 + expect(new_parent_grandchildren.where(name: grandchild.name).count).to eq 1 end end end @@ -2093,6 +2105,121 @@ module Alchemy end end + describe "metadata setters", :silence_deprecations do + let(:page) { create(:alchemy_page) } + + before do + allow(page.draft_version).to receive(:title=) + allow(page.draft_version).to receive(:meta_description=) + allow(page.draft_version).to receive(:meta_keywords=) + end + + describe "#title=" do + it "sets title on draft version" do + page.title = "New Title" + expect(page.draft_version).to have_received(:title=).with("New Title") + end + end + + describe "#meta_description=" do + it "sets meta_descriptionon draft version" do + page.meta_description = "New Description" + expect(page.draft_version).to have_received(:meta_description=).with("New Description") + end + end + + describe "#meta_keywords=" do + it "sets meta_keywords on draft version" do + page.meta_keywords = "draft, keywords" + expect(page.draft_version).to have_received(:meta_keywords=).with("draft, keywords") + end + end + end + + describe "metadata getters" do + context "with fixed attributes" do + let(:page) { create(:alchemy_page, page_layout: "readonly") } + + describe "#title" do + it "returns fixed value instead of version value" do + expect(page.title).to eq(false) + end + end + + describe "#meta_description" do + it "returns fixed value instead of version value" do + expect(page.meta_description).to be_nil + end + end + + describe "#meta_keywords" do + it "returns fixed value instead of version value" do + expect(page.meta_keywords).to be_nil + end + end + end + + context "with public version" do + let(:page) { create(:alchemy_page, :public) } + + before do + page.public_version.update!( + title: "Public Title", + meta_description: "Public Description", + meta_keywords: "public, keywords" + ) + end + + describe "#title" do + it "returns the public version value" do + expect(page.title).to eq("Public Title") + end + end + + describe "#meta_description" do + it "returns fixed value instead of version value" do + expect(page.meta_description).to eq("Public Description") + end + end + + describe "#meta_keywords" do + it "returns fixed value instead of version value" do + expect(page.meta_keywords).to eq("public, keywords") + end + end + end + + context "with only draft version" do + let(:page) { create(:alchemy_page) } + + before do + page.draft_version.update!( + title: "Draft Title", + meta_description: "Draft Description", + meta_keywords: "draft, keywords" + ) + end + + describe "#title" do + it "returns the draft version value" do + expect(page.title).to eq("Draft Title") + end + end + + describe "#meta_description" do + it "returns the draft version value" do + expect(page.meta_description).to eq("Draft Description") + end + end + + describe "#meta_keywords" do + it "returns the draft version value" do + expect(page.meta_keywords).to eq("draft, keywords") + end + end + end + end + describe "#fixed_attributes" do let(:page) { Alchemy::Page.new } diff --git a/spec/models/alchemy/page_version_spec.rb b/spec/models/alchemy/page_version_spec.rb index 346b72b69c..ed25dfa674 100644 --- a/spec/models/alchemy/page_version_spec.rb +++ b/spec/models/alchemy/page_version_spec.rb @@ -6,6 +6,10 @@ it { is_expected.to belong_to(:page) } it { is_expected.to have_many(:elements) } + it { is_expected.to have_db_column(:title).of_type(:string) } + it { is_expected.to have_db_column(:meta_description).of_type(:text) } + it { is_expected.to have_db_column(:meta_keywords).of_type(:text) } + let(:page) { create(:alchemy_page) } describe ".drafts" do @@ -79,6 +83,29 @@ end end + describe "#set_title_from_page" do + let(:page) { create(:alchemy_page, name: "My Page Name") } + + context "when title is blank" do + it "sets title from page name" do + page_version = page.versions.create!(title: nil) + expect(page_version.title).to eq("My Page Name") + end + + it "sets title from page name when title is empty string" do + page_version = page.versions.create!(title: "") + expect(page_version.title).to eq("My Page Name") + end + end + + context "when title is already set" do + it "does not override the title" do + page_version = page.versions.create!(title: "Custom Title") + expect(page_version.title).to eq("Custom Title") + end + end + end + describe "#public?" do subject { page_version.public? } diff --git a/spec/services/alchemy/copy_page_spec.rb b/spec/services/alchemy/copy_page_spec.rb index dc2ed0a3ca..fe17196829 100644 --- a/spec/services/alchemy/copy_page_spec.rb +++ b/spec/services/alchemy/copy_page_spec.rb @@ -58,6 +58,25 @@ end end + context "page with metadata" do + before do + page.draft_version.update!( + title: "Source Title", + meta_description: "Source description", + meta_keywords: "source, keywords" + ) + end + + it "copies meta_description and meta_keywords to the new page's draft version" do + expect(subject.draft_version.meta_description).to eq("Source description") + expect(subject.draft_version.meta_keywords).to eq("source, keywords") + end + + it "derives title from the new page name" do + expect(subject.draft_version.title).to eq("#{page.name} (Copy)") + end + end + context "page with fixed elements" do before { create(:alchemy_element, :fixed, page: page, page_version: page.draft_version) } @@ -88,6 +107,10 @@ it "should take this name" do expect(subject.name).to eq("Different name") end + + it "sets the draft version title to the new name" do + expect(subject.draft_version.title).to eq("Different name") + end end context "with exceptions during copy" do diff --git a/spec/views/pages/meta_data_spec.rb b/spec/views/pages/meta_data_spec.rb index 1d4f94a448..4fe86cae33 100644 --- a/spec/views/pages/meta_data_spec.rb +++ b/spec/views/pages/meta_data_spec.rb @@ -6,7 +6,11 @@ module Alchemy describe "alchemy/pages/_meta_data" do let!(:language) { create(:alchemy_language, code: :en) } let(:root_page) { Page.new } - let(:page) { Page.new(language_code: "en", title: "Road Runner", urlname: "roadrunner") } + let(:page) do + build(:alchemy_page, language_code: "en", urlname: "roadrunner").tap do |p| + p.draft_version&.title = "Road Runner" + end + end let(:title_prefix) { "" } let(:title_suffix) { "" } let(:title_separator) { "" } @@ -31,7 +35,7 @@ module Alchemy before { allow(Language).to receive(:current_root_page).and_return(root_page) } context "but the language root page has meta keywords" do - before { root_page.meta_keywords = "keywords, language, root" } + before { allow(root_page).to receive(:meta_keywords).and_return("keywords, language, root") } it "renders its keywords in the correct meta tag" do is_expected.to match(/meta name="keywords" content="keywords, language, root" lang="en"/) @@ -59,7 +63,7 @@ module Alchemy before { allow(Language).to receive(:current_root_page).and_return(root_page) } context "but the language root page has a meta description" do - before { root_page.meta_description = "description from language root" } + before { allow(root_page).to receive(:meta_description).and_return("description from language root") } it "renders its description in the correct meta tag" do is_expected.to match(/meta name="description" content="description from language root"/)