Merge pull request #2076 from tvdeyen/fix-page-issues

Fix page versioning issues

app/models/alchemy/element.rb

@@ -152,7 +152,7 @@ def new(attributes = {})
       #   @copy.public? # => false
       #
       def copy(source_element, differences = {})
-        DuplicateElement.new(source_element).call(differences)
+        Alchemy::DuplicateElement.new(source_element).call(differences)
       end
 
       def all_from_clipboard(clipboard)

app/models/alchemy/page.rb

@@ -126,7 +126,8 @@ 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 }
+    before_create -> { versions.build },
+      if: -> { versions.none? }
 
     before_save :set_language_code,
       if: -> { language.present? }
@@ -223,11 +224,13 @@ def language_root_for(language_id)
       # @return [Alchemy::Page]
       #
       def copy(source, differences = {})
-        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
+        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
       end
 
@@ -467,7 +470,7 @@ def public_on=(time)
         self.public_version = nil
       elsif public_version
         public_version.public_on = time
-      else
+      elsif time.present?
         versions.build(public_on: time)
       end
     end

spec/models/alchemy/page_spec.rb

@@ -90,6 +90,18 @@ module Alchemy
             page.save!
           }.to change { page.versions.length }.by(1)
         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
+          end
+
+          it "builds no version" do
+            expect { page.save! }.to_not change { page.versions.length }
+          end
+        end
       end
 
       context "before_save" do
@@ -352,8 +364,9 @@ class AnotherUrlPathClass; end
         expect(subject.name).to eq("#{page.name} (Copy)")
       end
 
-      it "the copy should have a draft version" do
-        expect(subject.draft_version).to_not be_nil
+      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
@@ -428,6 +441,19 @@ class AnotherUrlPathClass; end
           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
@@ -1303,6 +1329,15 @@ class AnotherUrlPathClass; end
           expect(page.versions.last).to be
           expect(page.versions.last.public_on).to be_within(1.second).of(time)
         end
+
+        context "and the time is empty string" do
+          let(:time) { "" }
+          let!(:page) { create(:alchemy_page) }
+
+          it "does not build a new version" do
+            expect { subject }.to_not change(page.versions, :length)
+          end
+        end
       end
     end