raggedstaff/openfoodnetwork
1
0
Fork 0
mirror of https://github.com/openfoodfoundation/openfoodnetwork synced 2026-02-07 22:46:06 +00:00
Code Issues Packages Projects Releases Wiki Activity

Stop using master variant as a potential store for prices

Browse Source 
(cherry picked from commit 1b06c20197)
This commit is contained in:
Matt-Yorkley
2023-05-21 16:04:21 +01:00
committed by David Cook
parent da3202460c
commit 79a2d1228d
9 changed files with 29 additions and 84 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
app/controllers/spree/admin/variants_controller.rb
View File

@@ -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

17
app/models/spree/product.rb
View File

@@ -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

14
app/models/spree/variant.rb
View File

@@ -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]

16
spec/controllers/api/v0/products_controller_spec.rb
View File

@@ -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

3
spec/controllers/api/v0/variants_controller_spec.rb
View File

@@ -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)

2
spec/controllers/spree/admin/variants_controller_spec.rb
View File

@@ -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

42
spec/models/spree/product_spec.rb
View File

@@ -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

15
spec/system/admin/bulk_product_update_spec.rb
View File

@@ -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)

2
spec/system/admin/products_spec.rb
View File

@@ -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)