diff --git a/app/controllers/spree/admin/variants_controller.rb b/app/controllers/spree/admin/variants_controller.rb index 49abb361a2..d6e7a43cf7 100644 --- a/app/controllers/spree/admin/variants_controller.rb +++ b/app/controllers/spree/admin/variants_controller.rb @@ -86,8 +86,6 @@ module Spree def assign_default_attributes @object.attributes = @object.product.master. attributes.except('id', 'created_at', 'deleted_at', 'sku', 'is_master') - # Shallow Clone of the default price to populate the price field. - @object.default_price = @object.product.master.default_price.clone end def collection diff --git a/app/models/spree/product.rb b/app/models/spree/product.rb index 723553a946..3d858a1272 100755 --- a/app/models/spree/product.rb +++ b/app/models/spree/product.rb @@ -75,10 +75,13 @@ module Spree ) } - delegate_belongs_to :master, :images, :sku, :price, :currency, :display_amount, :display_price, - :price_in, :amount_in, :unit_value, :unit_description + delegate_belongs_to :master, :images, :sku, :unit_value, :unit_description delegate :images_attributes=, :display_as=, to: :master + # Transient attribute used temporarily when creating a new product, + # this value is persisted on the product's variant + attr_accessor :price + after_create :set_master_variant_defaults after_save :save_master @@ -89,7 +92,6 @@ module Spree validates :name, presence: true validates :permalink, presence: true - validates :price, presence: true, if: proc { Spree::Config[:require_master_price] } validates :shipping_category, presence: true validates :supplier, presence: true @@ -338,13 +340,7 @@ module Spree # Here we rescue errors when saving master variants (without the need for a # validates_associated on master) and we get more specific data about the errors def save_master - if master && ( - master.changed? || master.new_record? || ( - master.default_price && ( - master.default_price.changed? || master.default_price.new_record? - ) - ) - ) + if master && (master.changed? || master.new_record?) master.save! end @@ -398,6 +394,7 @@ module Spree variant = master.dup variant.product = self variant.is_master = false + variant.price = price variants << variant end diff --git a/app/models/spree/variant.rb b/app/models/spree/variant.rb index 1b3625ba01..739680634f 100644 --- a/app/models/spree/variant.rb +++ b/app/models/spree/variant.rb @@ -53,10 +53,10 @@ module Spree localize_number :price, :weight - validate :check_price + validate :check_currency validates :price, numericality: { greater_than_or_equal_to: 0 }, presence: true, - if: proc { Spree::Config[:require_master_price] } + if: proc { !is_master } validates :unit_value, presence: true, if: ->(variant) { %w(weight volume).include?(variant.product&.variant_unit) @@ -198,15 +198,7 @@ module Spree private - # 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&.master - raise 'Must supply price for variant or master.price for product.' if self == product.master - - self.price = product.master.price - end - + def check_currency return unless currency.nil? self.currency = Spree::Config[:currency] diff --git a/spec/controllers/api/v0/products_controller_spec.rb b/spec/controllers/api/v0/products_controller_spec.rb index e4c8f1a9f7..170f662211 100644 --- a/spec/controllers/api/v0/products_controller_spec.rb +++ b/spec/controllers/api/v0/products_controller_spec.rb @@ -14,10 +14,7 @@ describe Api::V0::ProductsController, type: :controller do } let(:product_other_supplier) { create(:product, supplier: supplier2) } let(:product_with_image) { create(:product_with_image, supplier: supplier) } - let(:attributes) { - ["id", "name", "supplier", "price", "on_hand", "available_on", "permalink_live"] - } - let(:all_attributes) { ["id", "name", "price", "available_on", "variants"] } + let(:all_attributes) { ["id", "name", "available_on", "variants"] } let(:variants_attributes) { ["id", "options_text", "unit_value", "unit_description", "unit_to_display", "on_demand", "display_as", "display_name", "name_to_display", "sku", "on_hand", "price"] @@ -37,7 +34,7 @@ describe Api::V0::ProductsController, type: :controller do it "gets a single product" do product.master.images.create!(attachment: image("thinking-cat.jpg")) - product.variants.create!(unit_value: "1", unit_description: "thing") + product.variants.create!(unit_value: "1", unit_description: "thing", price: 1) product.variants.first.images.create!(attachment: image("thinking-cat.jpg")) product.set_property("spree", "rocks") api_get :show, id: product.to_param @@ -115,7 +112,7 @@ describe Api::V0::ProductsController, type: :controller do it "can create a new product" do api_post :create, product: { name: "The Other Product", - price: 19.99, + price: 123.45, shipping_category_id: create(:shipping_category).id, supplier_id: supplier.id, primary_taxon_id: FactoryBot.create(:taxon).id, @@ -125,6 +122,7 @@ describe Api::V0::ProductsController, type: :controller do expect(all_attributes.all?{ |attr| json_response.keys.include? attr }).to eq(true) expect(response.status).to eq(201) + expect(Spree::Product.last.variants.first.price).to eq 123.45 end it "cannot create a new product with invalid attributes" do @@ -133,7 +131,7 @@ describe Api::V0::ProductsController, type: :controller do expect(response.status).to eq(422) expect(json_response["error"]).to eq("Invalid resource. Please fix errors and try again.") errors = json_response["errors"] - expect(errors.keys).to match_array(["name", "price", "primary_taxon", "shipping_category", + expect(errors.keys).to match_array(["name", "primary_taxon", "shipping_category", "supplier", "variant_unit"]) end @@ -212,8 +210,8 @@ describe Api::V0::ProductsController, type: :controller do end # price info: it does not consider price changes; it considers the price set upon product creation - it '(does not) clone price which was updated' do - product.update_attribute(:price, 2.22) + it '(does not) clone master price which was updated' do + product.master.update_attribute(:price, 2.22) spree_post :clone, product_id: product.id, format: :json expect(json_response['price']).not_to eq(2.22) end diff --git a/spec/controllers/api/v0/variants_controller_spec.rb b/spec/controllers/api/v0/variants_controller_spec.rb index b8a3242871..f98c870ced 100644 --- a/spec/controllers/api/v0/variants_controller_spec.rb +++ b/spec/controllers/api/v0/variants_controller_spec.rb @@ -143,7 +143,8 @@ describe Api::V0::VariantsController, type: :controller do it "can create a new variant" do original_number_of_variants = variant.product.variants.count - api_post :create, variant: { sku: "12345", unit_value: "1", unit_description: "L" }, + api_post :create, variant: { sku: "12345", unit_value: "1", + unit_description: "L", price: "1" }, product_id: variant.product.to_param expect(attributes.all?{ |attr| json_response.include? attr.to_s }).to eq(true) diff --git a/spec/controllers/spree/admin/variants_controller_spec.rb b/spec/controllers/spree/admin/variants_controller_spec.rb index 09f34d14c8..fa59adab89 100644 --- a/spec/controllers/spree/admin/variants_controller_spec.rb +++ b/spec/controllers/spree/admin/variants_controller_spec.rb @@ -11,7 +11,7 @@ module Spree describe "deleted variants" do let(:product) { create(:product, name: 'Product A') } let(:deleted_variant) do - deleted_variant = product.variants.create(unit_value: "2") + deleted_variant = product.variants.create(unit_value: "2", price: 1) deleted_variant.delete deleted_variant end diff --git a/spec/models/spree/product_spec.rb b/spec/models/spree/product_spec.rb index c3edb52816..1bb88f1b09 100644 --- a/spec/models/spree/product_spec.rb +++ b/spec/models/spree/product_spec.rb @@ -45,46 +45,6 @@ module Spree end end - context "#price" do - # Regression test for Spree #1173 - it 'strips non-price characters' do - product.price = "$10" - expect(product.price).to eq 10.0 - end - end - - context "#display_price" do - before { product.price = 10.55 } - - context "with display_currency set to true" do - before { Spree::Config[:display_currency] = true } - - it "shows the currency" do - expect(product.display_price.to_s).to eq "$10.55 #{Spree::Config[:currency]}" - end - end - - context "with display_currency set to false" do - before { Spree::Config[:display_currency] = false } - - it "does not include the currency" do - expect(product.display_price.to_s).to eq "$10.55" - end - end - - context "with currency set to JPY" do - before do - product.master.default_price.currency = 'JPY' - product.master.default_price.save! - Spree::Config[:currency] = 'JPY' - end - - it "displays the currency in yen" do - expect(product.display_price.to_s).to eq "¥11" - end - end - end - describe 'Variants sorting' do context 'without master variant' do it 'sorts variants by position' do @@ -414,7 +374,7 @@ module Spree it "copies the properties on master variant to the first standard variant" do expect(product.variants.reload.length).to eq 1 standard_variant = product.variants.reload.first - expect(standard_variant.price).to eq product.master.price + expect(standard_variant.price).to eq 4.27 end it "only duplicates master with after_save when no standard variants exist" do diff --git a/spec/system/admin/bulk_product_update_spec.rb b/spec/system/admin/bulk_product_update_spec.rb index 4310652010..3348315cb5 100644 --- a/spec/system/admin/bulk_product_update_spec.rb +++ b/spec/system/admin/bulk_product_update_spec.rb @@ -146,15 +146,14 @@ describe ' end it "displays a price input (for each variant) for each product" do - p1 = FactoryBot.create(:product, price: 2.0) - v1 = FactoryBot.create(:variant, product: p1, is_master: false, price: 12.75) - v2 = FactoryBot.create(:variant, product: p1, is_master: false, price: 2.50) + p1 = create(:product, price: 2.0) + v1 = create(:variant, product: p1, is_master: false, price: 12.75) + v2 = create(:variant, product: p1, is_master: false, price: 2.50) visit spree.admin_products_path expect(page).to have_selector "a.view-variants", count: 1 all("a.view-variants").each(&:click) - expect(page).to have_field "price", with: "2.0", visible: false expect(page).to have_field "variant_price", with: "12.75" expect(page).to have_field "variant_price", with: "2.5" end @@ -401,10 +400,10 @@ describe ' end it "updating a product with variants" do - s1 = FactoryBot.create(:supplier_enterprise) - s2 = FactoryBot.create(:supplier_enterprise) - p = FactoryBot.create(:product, supplier: s1, available_on: Date.current, variant_unit: 'volume', variant_unit_scale: 0.001, - price: 3.0, unit_value: 0.25, unit_description: '(bottle)' ) + s1 = create(:supplier_enterprise) + s2 = create(:supplier_enterprise) + p = create(:product, supplier: s1, available_on: Date.current, variant_unit: 'volume', variant_unit_scale: 0.001, + price: 3.0, unit_value: 0.25, unit_description: '(bottle)' ) v = p.variants.first v.update_attribute(:sku, "VARIANTSKU") v.update_attribute(:on_demand, false) diff --git a/spec/system/admin/products_spec.rb b/spec/system/admin/products_spec.rb index 44121e5bd1..12fe2810b4 100644 --- a/spec/system/admin/products_spec.rb +++ b/spec/system/admin/products_spec.rb @@ -107,7 +107,7 @@ describe ' expect(product.unit_description).to eq("") expect(product.variant_unit_name).to eq("") expect(product.primary_taxon_id).to eq(taxon.id) - expect(product.price.to_s).to eq('19.99') + expect(product.variants.first.price.to_s).to eq('19.99') expect(product.on_hand).to eq(5) expect(product.tax_category_id).to eq(tax_category.id) expect(product.shipping_category).to eq(shipping_category)