Merge pull request #2521 from mamhoff/do-not-add-copy-unless-necessary

Copying pages: Only add "(Copy)" if necessary

app/models/alchemy/page.rb

@@ -233,23 +233,17 @@ def language_root_for(language_id)
       # @return [Alchemy::Page]
       #
       def copy(source, differences = {})
-        transaction do
-          page = Alchemy::Page.new(attributes_from_source_for_copy(source, differences))
-          page.tag_list = source.tag_list
-          if page.save!
-            copy_elements(source, page)
-            page
-          end
-        end
+        Alchemy::CopyPage.new(page: source).call(changed_attributes: differences)
       end
 
       def copy_and_paste(source, new_parent, new_name)
-        page = copy(source, {
-          parent: new_parent,
-          language: new_parent&.language,
-          name: new_name,
-          title: new_name
-        })
+        page = Alchemy::CopyPage.new(page: source)
+          .call(changed_attributes: {
+            parent: new_parent,
+            language: new_parent&.language,
+            name: new_name,
+            title: new_name
+          })
         if source.children.any?
           source.copy_children_to(page)
         end
@@ -280,40 +274,6 @@ def link_target_options
         end
         options
       end
-
-      private
-
-      # Aggregates the attributes from given source for copy of page.
-      #
-      # @param [Alchemy::Page]
-      #   The source page
-      # @param [Hash]
-      #   A optional hash with attributes that take precedence over the source attributes
-      #
-      def attributes_from_source_for_copy(source, differences = {})
-        source.attributes.stringify_keys!
-        differences.stringify_keys!
-        attributes = source.attributes.merge(differences)
-        attributes.merge!(DEFAULT_ATTRIBUTES_FOR_COPY)
-        attributes["name"] = new_name_for_copy(differences["name"], source.name)
-        attributes.except(*SKIPPED_ATTRIBUTES_ON_COPY)
-      end
-
-      # Returns a new name for copy of page.
-      #
-      # If the differences hash includes a new name this is taken.
-      # Otherwise +source.name+
-      #
-      # @param [String]
-      #   The differences hash that contains a new name
-      # @param [String]
-      #   The name of the source
-      #
-      def new_name_for_copy(custom_name, source_name)
-        return custom_name if custom_name.present?
-
-        "#{source_name} (#{Alchemy.t("Copy")})"
-      end
     end
 
     # Instance methods
@@ -447,7 +407,7 @@ def copy_children_to(new_parent)
       children.each do |child|
         next if child == new_parent
 
-        new_child = Page.copy(child, {
+        new_child = Alchemy::CopyPage.new(page: child).call(changed_attributes: {
           parent_id: new_parent.id,
           language_id: new_parent.language_id,
           language_code: new_parent.language_code

app/services/alchemy/copy_page.rb

@@ -0,0 +1,98 @@
+# frozen_string_literal: true
+
+module Alchemy
+  # Creates a copy of given source page.
+  #
+  # Also copies all elements included in source page.
+  #
+  # === Note:
+  #
+  # It prevents the element auto generator from running.
+  class CopyPage
+    DEFAULT_ATTRIBUTES_FOR_COPY = {
+      autogenerate_elements: false,
+      public_on: nil,
+      public_until: nil,
+      locked_at: nil,
+      locked_by: nil
+    }
+
+    SKIPPED_ATTRIBUTES_ON_COPY = %w[
+      id
+      updated_at
+      created_at
+      creator_id
+      updater_id
+      lft
+      rgt
+      depth
+      urlname
+      cached_tag_list
+    ]
+
+    attr_reader :page
+
+    # @param page [Alchemy::Page]
+    #   The source page the copy is taken from
+    def initialize(page:)
+      @page = page
+    end
+
+    # @param changed_attributes [Hash]
+    #   A optional hash with attributes that take precedence over the source attributes
+    #
+    # @return [Alchemy::Page]
+    #
+    def call(changed_attributes:)
+      Alchemy::Page.transaction do
+        new_page = Alchemy::Page.new(attributes_from_source_for_copy(changed_attributes))
+        new_page.tag_list = page.tag_list
+        if new_page.save!
+          Alchemy::Page.copy_elements(page, new_page)
+        end
+        new_page
+      end
+    end
+
+    private
+
+    # Aggregates the attributes from given source for copy of page.
+    #
+    # @param [Alchemy::Page]
+    #   The source page
+    # @param [Hash]
+    #   A optional hash with attributes that take precedence over the source attributes
+    #
+    def attributes_from_source_for_copy(differences = {})
+      source_attributes = page.attributes.stringify_keys
+      differences.stringify_keys!
+      desired_attributes = source_attributes
+        .merge(DEFAULT_ATTRIBUTES_FOR_COPY)
+        .merge(differences)
+      desired_attributes["name"] = best_name_for_copy(source_attributes, desired_attributes)
+      desired_attributes.except(*SKIPPED_ATTRIBUTES_ON_COPY)
+    end
+
+    # Returns a new name for copy of page.
+    #
+    # If the differences hash includes a new name this is taken.
+    # Otherwise +source.name+
+    #
+    # @param [Hash]
+    #   The differences hash that contains a new name
+    # @param [Hash]
+    #   The name of the source
+    #
+    def best_name_for_copy(source_attributes, desired_attributes)
+      desired_name = desired_attributes["name"].presence || source_attributes["name"]
+
+      new_parent_id = desired_attributes["parent"]&.id || desired_attributes["parent_id"]
+
+      if Alchemy::Page.where(parent_id: new_parent_id, name: desired_name).exists?
+        "#{desired_name} (#{Alchemy.t("Copy")})"
+      else
+        desired_name
+      end
+    end
+  end
+end

spec/models/alchemy/page_spec.rb

@@ -388,97 +388,6 @@ module Alchemy
       it "the copy should have added (copy) to name" do
         expect(subject.name).to eq("#{page.name} (Copy)")
       end
-
-      it "the copy should have one draft version" do
-        expect(subject.versions.length).to eq(1)
-        expect(subject.draft_version).to be
-      end
-
-      context "a public page" do
-        let(:page) { create(:alchemy_page, :public, name: "Source", public_until: Time.current) }
-
-        it "the copy should not be public" do
-          expect(subject.public_on).to be(nil)
-          expect(subject.public_until).to be(nil)
-        end
-      end
-
-      context "a locked page" do
-        let(:page) do
-          create(:alchemy_page, :public, :locked, name: "Source")
-        end
-
-        it "the copy should not be locked" do
-          expect(subject.locked?).to be(false)
-          expect(subject.locked_by).to be(nil)
-        end
-      end
-
-      context "page with tags" do
-        before do
-          page.tag_list = "red, yellow"
-          page.save!
-        end
-
-        it "the copy should have source tag_list" do
-          expect(subject.tag_list).not_to be_empty
-          expect(subject.tag_list).to match_array(page.tag_list)
-        end
-      end
-
-      context "page with elements" do
-        before { create(:alchemy_element, page: page, page_version: page.draft_version) }
-
-        it "the copy should have source elements on its draft version" do
-          expect(subject.draft_version.elements).not_to be_empty
-          expect(subject.draft_version.elements.count).to eq(page.draft_version.elements.count)
-        end
-      end
-
-      context "page with fixed elements" do
-        before { create(:alchemy_element, :fixed, page: page, page_version: page.draft_version) }
-
-        it "the copy should have source fixed elements on its draft version" do
-          expect(subject.draft_version.elements.fixed).not_to be_empty
-          expect(subject.draft_version.elements.fixed.count).to eq(page.draft_version.elements.fixed.count)
-        end
-      end
-
-      context "page with autogenerate elements" do
-        before do
-          page = create(:alchemy_page)
-          allow(page).to receive(:definition).and_return({
-            "name" => "standard",
-            "elements" => ["headline"],
-            "autogenerate" => ["headline"]
-          })
-        end
-
-        it "the copy should not autogenerate elements" do
-          expect(subject.draft_version.elements).to be_empty
-        end
-      end
-
-      context "with different page name given" do
-        subject { Page.copy(page, {name: "Different name"}) }
-
-        it "should take this name" do
-          expect(subject.name).to eq("Different name")
-        end
-      end
-
-      context "with exceptions during copy" do
-        before do
-          expect(Page).to receive(:copy_elements) { raise "boom" }
-        end
-
-        it "rolls back all changes" do
-          page
-          expect {
-            expect { Page.copy(page, {name: "Different name"}) }.to raise_error("boom")
-          }.to_not change(Alchemy::Page, :count)
-        end
-      end
     end
 
     describe ".create" do
@@ -1194,19 +1103,6 @@ module Alchemy
       end
     end
 
-    def copy_children_to(new_parent)
-      children.each do |child|
-        next if child == new_parent
-
-        new_child = Page.copy(child, {
-          language_id: new_parent.language_id,
-          language_code: new_parent.language_code
-        })
-        new_child.move_to_child_of(new_parent)
-        child.copy_children_to(new_child) unless child.children.blank?
-      end
-    end
-
     describe "#definition" do
       context "if the page layout could not be found in the definition file" do
         let(:page) { build_stubbed(:alchemy_page, page_layout: "notexisting") }
@@ -1251,49 +1147,35 @@ def copy_children_to(new_parent)
     end
 
     describe "#copy_and_paste" do
-      let(:source) { build_stubbed(:alchemy_page) }
-      let(:new_parent) { build_stubbed(:alchemy_page) }
+      let(:source) { create(:alchemy_page) }
+      let(:new_parent) { create(:alchemy_page) }
       let(:page_name) { "Pagename (pasted)" }
-      let(:copied_page) { mock_model("Page") }
 
       subject { Page.copy_and_paste(source, new_parent, page_name) }
 
       it "should copy the source page with the given name to the new parent" do
-        expect(Page).to receive(:copy).with(source, {
-          parent: new_parent,
-          language: new_parent.language,
-          name: page_name,
-          title: page_name
-        })
-        subject
+        expect(subject.name).to eq(page_name)
       end
 
       it "should return the copied page" do
-        allow(Page).to receive(:copy).and_return(copied_page)
-        expect(subject).to be_a(copied_page.class)
+        expect(subject).to be_a(Alchemy::Page)
       end
 
       context "if source page has children" do
+        let(:child_page) { create(:alchemy_page) }
+        let(:source) { create(:alchemy_page, children: [child_page]) }
+
         it "should also copy and paste the children" do
-          allow(Page).to receive(:copy).and_return(copied_page)
-          allow(source).to receive(:children).and_return([mock_model("Page")])
-          expect(source).to receive(:copy_children_to).with(copied_page)
-          subject
+          expect(subject.children.length).to eq(1)
         end
       end
 
       context "if the source page has no parent (global page)" do
-        let(:source) { build_stubbed(:alchemy_page, layoutpage: true, parent_id: nil) }
+        let(:source) { create(:alchemy_page, layoutpage: true, parent_id: nil) }
         let(:new_parent) { nil }
 
         it "copies the source page with the given name" do
-          expect(Page).to receive(:copy).with(source, {
-            parent: nil,
-            language: nil,
-            name: page_name,
-            title: page_name
-          })
-          subject
+          expect(subject.name).to eq(page_name)
         end
       end
     end