Merge pull request solidusio#641 from jhawthorn/product_master_defaul…

…t_price_autosave

Use autosave for Product#master and Variant#default_price

api/spec/controllers/spree/api/products_controller_spec.rb

@@ -334,8 +334,7 @@ module Spree
           expect(response.status).to eq(422)
           expect(json_response["error"]).to eq("Invalid resource. Please fix errors and try again.")
           errors = json_response["errors"]
-          errors.delete("slug") # Don't care about this one.
-          expect(errors.keys).to match_array(["name", "price", "shipping_category_id"])
+          expect(errors.keys).to include("name", "price", "shipping_category_id")
         end
       end
 

core/app/models/concerns/spree/default_price.rb

@@ -6,7 +6,9 @@ module DefaultPrice
       has_one :default_price,
         -> { where currency: Spree::Config[:currency], is_default: true },
         class_name: 'Spree::Price',
-        dependent: :destroy
+        inverse_of: :variant,
+        dependent: :destroy,
+        autosave: true
 
       def find_or_build_default_price
         default_price || build_default_price

core/app/models/spree/price.rb

@@ -1,7 +1,7 @@
 module Spree
   class Price < Spree::Base
     acts_as_paranoid
-    belongs_to :variant, -> { with_deleted }, class_name: 'Spree::Variant', inverse_of: :prices, touch: true
+    belongs_to :variant, -> { with_deleted }, class_name: 'Spree::Variant', touch: true
 
     validate :check_price
     validates :amount, numericality: { greater_than_or_equal_to: 0 }, allow_nil: true

core/app/models/spree/product.rb

@@ -34,7 +34,8 @@ class Product < Spree::Base
     has_one :master,
       -> { where(is_master: true).with_deleted },
       inverse_of: :product,
-      class_name: 'Spree::Variant'
+      class_name: 'Spree::Variant',
+      autosave: true
 
     has_many :variants,
       -> { where(is_master: false).order(:position) },
@@ -77,9 +78,7 @@ def find_or_build_master
 
     after_initialize :ensure_master
 
-    after_save :save_master
-    after_save :run_touch_callbacks, if: :anything_changed?
-    after_save :reset_nested_changes
+    after_save :run_touch_callbacks, if: :changed?
     after_touch :touch_taxons
 
     before_validation :normalize_slug, on: :update
@@ -323,30 +322,9 @@ def punch_slug
       update_column :slug, "#{Time.current.to_i}_#{slug}" # punch slug with date prefix to allow reuse of original
     end
 
-    def anything_changed?
-      changed? || @nested_changes
-    end
-
-    def reset_nested_changes
-      @nested_changes = false
-    end
-
-    # there's a weird quirk with the delegate stuff that does not automatically save the delegate object
-    # when saving so we force a save using a hook
-    # Fix for issue https://github.com/spree/spree/issues/5306
-    def save_master
-      if master && (master.changed? || master.new_record? || (master.default_price && (master.default_price.changed? || master.default_price.new_record?)))
-        master.save!
-        @nested_changes = true
-      end
-    end
-
-    # If the master cannot be saved, the Product object will get its errors
-    # and will be destroyed
+    # If the master is invalid, the Product object will be assigned its errors
     def validate_master
-      # We call master.default_price here to ensure price is initialized.
-      # Required to avoid Variant#check_price validation failing on create.
-      unless master.default_price && master.valid?
+      unless master.valid?
         master.errors.each do |att, error|
           self.errors.add(att, error)
         end

core/app/models/spree/variant.rb

@@ -338,9 +338,12 @@ def set_master_out_of_stock
       # Ensures a new variant takes the product master price when price is not supplied
       def check_price
         if price.nil? && Spree::Config[:require_master_price]
-          raise 'No master variant found to infer price' unless (product && product.master)
-          raise 'Must supply price for variant or master.price for product.' if self == product.master
-          self.price = product.master.price
+          if is_master?
+            errors.add :price, 'Must supply price for variant or master.price for product.'
+          else
+            raise 'No master variant found to infer price' unless (product && product.master)
+            self.price = product.master.price
+          end
         end
         if currency.nil?
           self.currency = Spree::Config[:currency]

core/spec/models/spree/product_spec.rb

@@ -38,16 +38,58 @@ class Extension < Spree::Base
       end
     end
 
-    context "master variant" do
+    describe "#save" do
+      before { product.update_columns(updated_at: 1.day.ago) }
+      subject { product.save! }
+
+      shared_examples "a change occurred" do
+        it "should change updated_at" do
+          expect { subject }.to change{ product.updated_at }
+        end
+
+        it "should touch taxons" do
+          taxon = create(:taxon, products: [product])
+          taxon.update_columns(updated_at: 1.day.ago)
+          product.taxons.reload
+          expect { subject }.to change{ taxon.reload.updated_at }
+        end
+      end
+
+      shared_examples "no change occurred" do
+        it "should not change updated_at" do
+          expect { subject }.not_to change{ product.updated_at }
+        end
+
+        it "should not touch taxons" do
+          taxon = create(:taxon, products: [product])
+          taxon.update_columns(updated_at: 1.day.ago)
+          product.taxons.reload
+          expect { subject }.not_to change{ taxon.reload.updated_at }
+        end
+      end
+
+      context "when nothing has changed" do
+        it_behaves_like "no change occurred"
+      end
+
+      context "when the product itself was changed" do
+        before do
+          product.name = "Perri-air"
+        end
+
+        it_behaves_like "a change occurred"
+      end
 
       context "when master variant changed" do
         before do
           product.master.sku = "Something changed"
         end
 
+        it_behaves_like "a change occurred"
+
         it "saves the master" do
-          expect(product.master).to receive(:save!)
-          product.save
+          product.save!
+          expect(product.reload.master.sku).to eq "Something changed"
         end
       end
 
@@ -59,21 +101,11 @@ class Extension < Spree::Base
           product.master.default_price.price = 12
         end
 
-        it "saves the master" do
-          expect(product.master).to receive(:save!)
-          product.save
-        end
-
-        it "saves the default price" do
-          expect(product.master.default_price).to receive(:save)
-          product.save
-        end
-      end
+        it_behaves_like "a change occurred"
 
-      context "when master variant and price haven't changed" do
-        it "does not save the master" do
-          expect(product.master).not_to receive(:save!)
-          product.save
+        it "saves the default_price" do
+          product.save!
+          expect(product.reload.master.default_price.price).to eq 12
         end
       end
     end