Merge pull request #2066 from mamhoff/faster-element-duplication

Faster element duplication

app/models/alchemy/content/factory.rb

@@ -7,7 +7,7 @@ module Content::Factory
     extend ActiveSupport::Concern
 
     module ClassMethods
-      SKIPPED_ATTRIBUTES_ON_COPY = %w(position created_at updated_at creator_id updater_id id)
+      SKIPPED_ATTRIBUTES_ON_COPY = %w(position created_at updated_at creator_id updater_id element_id id)
 
       # Builds a new content as descriped in the elements.yml file.
       #
@@ -54,7 +54,7 @@ def create(attributes = {})
       #
       def copy(source, differences = {})
         Content.new(
-          source.attributes.
+          source.attributes.with_indifferent_access.
             except(*SKIPPED_ATTRIBUTES_ON_COPY).
             merge(differences.with_indifferent_access)
         ).tap do |new_content|

app/models/alchemy/element.rb

@@ -45,17 +45,6 @@ class Element < BaseRecord
       "deprecated",
     ].freeze
 
-    SKIPPED_ATTRIBUTES_ON_COPY = [
-      "cached_tag_list",
-      "created_at",
-      "creator_id",
-      "id",
-      "folded",
-      "position",
-      "updated_at",
-      "updater_id",
-    ].freeze
-
     # All Elements that share the same page version and parent element and are fixed or not are considered a list.
     #
     # If parent_element_id is nil (typical case for a simple page),
@@ -163,26 +152,7 @@ def new(attributes = {})
       #   @copy.public? # => false
       #
       def copy(source_element, differences = {})
-        attributes = source_element.attributes.with_indifferent_access
-          .except(*SKIPPED_ATTRIBUTES_ON_COPY)
-          .merge(differences)
-          .merge({
-            autogenerate_contents: false,
-            autogenerate_nested_elements: false,
-            tag_list: source_element.tag_list,
-          })
-
-        new_element = create!(attributes)
-
-        if source_element.contents.any?
-          source_element.copy_contents_to(new_element)
-        end
-
-        if source_element.nested_elements.any?
-          source_element.copy_nested_elements_to(new_element)
-        end
-
-        new_element
+        DuplicateElement.new(source_element).call(differences)
       end
 
       def all_from_clipboard(clipboard)
@@ -313,16 +283,6 @@ def nestable_elements
       definition.fetch("nestable_elements", [])
     end
 
-    # Copy all nested elements from current element to given target element.
-    def copy_nested_elements_to(target_element)
-      nested_elements.map do |nested_element|
-        Element.copy(nested_element, {
-          parent_element_id: target_element.id,
-          page_version_id: target_element.page_version_id,
-        })
-      end
-    end
-
     private
 
     def generate_nested_elements

app/models/alchemy/elements_repository.rb

@@ -111,6 +111,10 @@ def limit(limit)
       self.class.new elements[0..(limit.to_i - 1)]
     end
 
+    def children_of(parent)
+      self.class.new(select { |e| e.parent_element_id == parent.id })
+    end
+
     def each(&blk)
       elements.each(&blk)
     end

app/models/alchemy/page/publisher.rb

@@ -19,9 +19,16 @@ def publish!(public_on:)
           version = public_version(public_on)
           DeleteElements.new(version.elements).call
 
-          # We must not use .find_each here to not mess up the order of elements
-          page.draft_version.elements.not_nested.available.each do |element|
-            Element.copy(element, page_version_id: version.id)
+          repository = page.draft_version.element_repository
+          ActiveRecord::Base.no_touching do
+            Element.acts_as_list_no_update do
+              repository.visible.not_nested.each.with_index(1) do |element, position|
+                Alchemy::DuplicateElement.new(element, repository: repository).call(
+                  page_version_id: version.id,
+                  position: position
+                )
+              end
+            end
           end
         end
       end

app/models/alchemy/page_version.rb

@@ -45,6 +45,10 @@ def still_public_for?(time = Time.current)
       public_until.nil? || public_until >= time
     end
 
+    def element_repository
+      ElementsRepository.new(elements.includes({ contents: :essence }, :tags))
+    end
+
     private
 
     def delete_elements

app/services/alchemy/duplicate_element.rb

@@ -0,0 +1,53 @@
+# frozen_string_literal: true
+
+module Alchemy
+  class DuplicateElement
+    SKIPPED_ATTRIBUTES_ON_COPY = [
+      "cached_tag_list",
+      "created_at",
+      "creator_id",
+      "position",
+      "id",
+      "folded",
+      "updated_at",
+      "updater_id",
+    ].freeze
+
+    attr_reader :source_element, :repository
+
+    def initialize(source_element, repository: source_element.page_version.element_repository)
+      @source_element = source_element
+      @repository = repository
+    end
+
+    def call(differences = {})
+      attributes = source_element.attributes.with_indifferent_access
+        .except(*SKIPPED_ATTRIBUTES_ON_COPY)
+        .merge(differences)
+        .merge(
+          autogenerate_contents: false,
+          autogenerate_nested_elements: false,
+          tags: source_element.tags,
+        )
+
+      new_element = Element.create(attributes)
+
+      source_element.contents.map do |content|
+        Content.copy(content, element: new_element)
+      end
+
+      nested_elements = repository.children_of(source_element)
+      Element.acts_as_list_no_update do
+        nested_elements.each.with_index(1) do |nested_element, position|
+          self.class.new(nested_element, repository: repository).call(
+            parent_element: new_element,
+            page_version: new_element.page_version,
+            position: position
+          )
+        end
+      end
+
+      new_element
+    end
+  end
+end

lib/alchemy/essence.rb

@@ -61,7 +61,7 @@ def acts_as_essence(options = {})
           delegate :restricted?, to: :page,    allow_nil: true
           delegate :public?,     to: :element, allow_nil: true
 
-          after_save :touch_element
+          after_update :touch_element
 
           def acts_as_essence_class
             #{name}

spec/models/alchemy/attachment_spec.rb

@@ -36,13 +36,12 @@ module Alchemy
         let(:attachment) { create(:alchemy_attachment) }
 
         let(:content) do
-          create(:alchemy_content, :essence_file).tap do |content|
-            content.element.update_column(:updated_at, 3.hours.ago)
-          end
+          create(:alchemy_content, :essence_file)
         end
 
         before do
           content.essence.update(attachment: attachment)
+          content.element.update_column(:updated_at, 3.hours.ago)
         end
 
         it "touches elements" do

spec/models/alchemy/elements_repository_spec.rb

@@ -243,4 +243,10 @@
 
     it_behaves_like "being chainable"
   end
+
+  describe "#children_of" do
+    subject { repo.children_of(visible_element) }
+
+    it { is_expected.to match_array([nested_element]) }
+  end
 end

spec/models/alchemy/page_version_spec.rb

@@ -137,5 +137,15 @@
         expect(Alchemy::EssenceRichtext.count).to be_zero
       end
     end
+
+    describe "#element_repository" do
+      let(:page_version) { create(:alchemy_page_version, :with_elements) }
+      subject { page_version.element_repository }
+
+      it "is an element repository containing the pages elements" do
+        expect(Alchemy::ElementsRepository).to receive(:new).with(page_version.elements).and_call_original
+        expect(subject).to be_a(Alchemy::ElementsRepository)
+      end
+    end
   end
 end

spec/models/alchemy/picture_spec.rb

@@ -512,13 +512,12 @@ module Alchemy
         let(:picture) { create(:alchemy_picture) }
 
         let(:content) do
-          create(:alchemy_content, :essence_picture).tap do |content|
-            content.element.update_column(:updated_at, 3.hours.ago)
-          end
+          create(:alchemy_content, :essence_picture)
         end
 
         before do
           content.essence.update(picture: picture)
+          content.element.update_column(:updated_at, 3.hours.ago)
         end
 
         it "touches elements" do

spec/services/alchemy/duplicate_element_spec.rb

@@ -0,0 +1,74 @@
+# frozen_string_literal: true
+
+require "rails_helper"
+
+RSpec.describe Alchemy::DuplicateElement do
+  let(:element) do
+    create(:alchemy_element, :with_contents, tag_list: "red, yellow")
+  end
+  let(:differences) { {} }
+  subject { described_class.new(element).call(differences) }
+
+  it "should not create contents from scratch" do
+    expect(subject.contents.count).to eq(element.contents.count)
+  end
+
+  context "with differences" do
+    let(:new_page_version) { create(:alchemy_page_version) }
+    let(:differences) { { page_version_id: new_page_version.id } }
+
+    it "should create a new record with all attributes of source except given differences" do
+      expect(subject.page_version_id).to eq(new_page_version.id)
+    end
+  end
+
+  it "should make copies of all contents of source" do
+    expect(subject.contents).not_to be_empty
+    expect(subject.contents.pluck(:id)).not_to eq(element.contents.pluck(:id))
+  end
+
+  it "the copy should include source element tags" do
+    expect(subject.tag_list).to eq(element.tag_list)
+  end
+
+  context "with nested elements" do
+    let(:element) do
+      create(:alchemy_element, :with_contents, :with_nestable_elements, {
+        tag_list: "red, yellow",
+        page: create(:alchemy_page),
+      })
+    end
+
+    before do
+      element.nested_elements << create(:alchemy_element, name: "slide")
+    end
+
+    it "should copy nested elements" do
+      expect(subject.nested_elements).to_not be_empty
+    end
+
+    context "copy to new page version" do
+      let(:new_page_version) { create(:alchemy_page_version) }
+      let(:differences) { { page_version_id: new_page_version.id } }
+
+      it "should set page_version id to new page_version's id" do
+        subject.nested_elements.each do |nested_element|
+          expect(nested_element.page_version_id).to eq(new_page_version.id)
+        end
+      end
+    end
+
+    context "copy to new page version" do
+      let(:public_version) do
+        element.page.versions.create!(public_on: Time.current)
+      end
+      let(:differences) { { page_version_id: public_version.id } }
+
+      it "sets page_version id" do
+        subject.nested_elements.each do |nested_element|
+          expect(nested_element.page_version_id).to eq(public_version.id)
+        end
+      end
+    end
+  end
+end