From 54c3c73ed21d5a5e6b6dede72f47049d7b9e0dc2 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Tue, 24 Mar 2020 12:46:21 +0100 Subject: [PATCH 1/6] Fix duplicate key in hash --- spec/models/calculator/weight_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/models/calculator/weight_spec.rb b/spec/models/calculator/weight_spec.rb index 94f7d1b498..010abd1d0b 100644 --- a/spec/models/calculator/weight_spec.rb +++ b/spec/models/calculator/weight_spec.rb @@ -141,7 +141,7 @@ describe Calculator::Weight do end context "when the product uses item unit" do - let!(:product_attributes) { { variant_unit: "items", variant_unit_scale: nil, variant_unit: "pc", display_as: "pc" } } + let!(:product_attributes) { { variant_unit: "items", variant_unit_scale: nil, variant_unit_name: "pc", display_as: "pc" } } let!(:variant_attributes) { { unit_value: 3.0, weight: 2.5, display_as: "pc" } } it "is correct" do From 87ee4bbebc65dc7f7173deb999c8ae0b7e8778db Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Tue, 24 Mar 2020 13:24:24 +0100 Subject: [PATCH 2/6] Add spec for current problematic behaviour --- spec/models/calculator/weight_spec.rb | 28 +++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/spec/models/calculator/weight_spec.rb b/spec/models/calculator/weight_spec.rb index 010abd1d0b..cebaba31cd 100644 --- a/spec/models/calculator/weight_spec.rb +++ b/spec/models/calculator/weight_spec.rb @@ -151,4 +151,32 @@ describe Calculator::Weight do end end end + + context "when variant_unit is 'items' and unit_value is zero" do + let(:product) { + create(:product, variant_unit: 'items', variant_unit_scale: nil, variant_unit_name: "bunch") + } + let(:variant1) { create(:variant, product: product, unit_value: 0, weight: nil) } + let(:variant2) { create(:variant, product: product, unit_value: 0, weight: 10.0) } + let(:line_item1) { create(:line_item, variant: variant1, quantity: 1) } + let(:line_item2) { create(:line_item, variant: variant2, quantity: 1) } + + before { subject.set_preference(:per_kg, 5) } + + it "returns NaN if variant.weight is not present" do + expect(subject.compute(line_item1).nan?).to be_truthy + end + + xit "uses zero weight if variant.weight is not present" do + expect(subject.compute(line_item1)).to eq 0 + end + + it "returns NaN if variant.weight is present" do + expect(subject.compute(line_item2).nan?).to be_truthy + end + + xit "uses the variant weight if variant.weight is present" do + expect(subject.compute(line_item2)).to eq 50.0 + end + end end From ef0fb18fdac6e85c4af79dd9755f4e61643d1fab Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Tue, 24 Mar 2020 13:51:47 +0100 Subject: [PATCH 3/6] Fix calculations for weight when variant.unit_value is zero --- app/models/calculator/weight.rb | 2 ++ spec/models/calculator/weight_spec.rb | 12 ++---------- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/app/models/calculator/weight.rb b/app/models/calculator/weight.rb index cff4dba4fa..d5bf102c14 100644 --- a/app/models/calculator/weight.rb +++ b/app/models/calculator/weight.rb @@ -54,6 +54,8 @@ module Calculator # Customer ends up getting 350mL (line_item.final_weight_volume) of wine # that represent 2.8 (quantity_implied_in_final_weight_volume) glasses of wine def quantity_implied_in_final_weight_volume(line_item) + return line_item.quantity if line_item.variant.unit_value.zero? + (1.0 * line_item.final_weight_volume / line_item.variant.unit_value).round(3) end diff --git a/spec/models/calculator/weight_spec.rb b/spec/models/calculator/weight_spec.rb index cebaba31cd..8155456bf0 100644 --- a/spec/models/calculator/weight_spec.rb +++ b/spec/models/calculator/weight_spec.rb @@ -163,19 +163,11 @@ describe Calculator::Weight do before { subject.set_preference(:per_kg, 5) } - it "returns NaN if variant.weight is not present" do - expect(subject.compute(line_item1).nan?).to be_truthy - end - - xit "uses zero weight if variant.weight is not present" do + it "uses zero weight if variant.weight is not present" do expect(subject.compute(line_item1)).to eq 0 end - it "returns NaN if variant.weight is present" do - expect(subject.compute(line_item2).nan?).to be_truthy - end - - xit "uses the variant weight if variant.weight is present" do + it "uses the variant weight if variant.weight is present" do expect(subject.compute(line_item2)).to eq 50.0 end end From 9af4bb9757bea166015ad4f4345a1fc144310470 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Wed, 25 Mar 2020 11:22:40 +0000 Subject: [PATCH 4/6] Use create instead of build so that we test with callbacks --- spec/models/calculator/weight_spec.rb | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/spec/models/calculator/weight_spec.rb b/spec/models/calculator/weight_spec.rb index 8155456bf0..298293db92 100644 --- a/spec/models/calculator/weight_spec.rb +++ b/spec/models/calculator/weight_spec.rb @@ -19,8 +19,8 @@ describe Calculator::Weight do end describe "line item with variant_unit weight and variant unit_value" do - let(:variant) { build(:variant, unit_value: 10_000) } - let(:line_item) { build(:line_item, variant: variant, quantity: 2) } + let(:variant) { create(:variant, unit_value: 10_000) } + let(:line_item) { create(:line_item, variant: variant, quantity: 2) } before { subject.set_preference(:per_kg, 5) } @@ -46,11 +46,11 @@ describe Calculator::Weight do end it "computes shipping cost for an object with an order" do - variant1 = build(:variant, unit_value: 10_000) - variant2 = build(:variant, unit_value: 20_000) + variant1 = create(:variant, unit_value: 10_000) + variant2 = create(:variant, unit_value: 20_000) - line_item1 = build(:line_item, variant: variant1, quantity: 1) - line_item2 = build(:line_item, variant: variant2, quantity: 2) + line_item1 = create(:line_item, variant: variant1, quantity: 1) + line_item2 = create(:line_item, variant: variant2, quantity: 2) order = double(:order, line_items: [line_item1, line_item2]) object_with_order = double(:object_with_order, order: order) @@ -65,7 +65,7 @@ describe Calculator::Weight do let(:calculator) { described_class.new(preferred_per_kg: 6) } let(:line_item) do - build(:line_item, variant: variant, quantity: 2).tap do |object| + create(:line_item, variant: variant, quantity: 2).tap do |object| object.send(:calculate_final_weight_volume) end end From 24c8f38111bdf2a0d9b131d26dcc23a74a6d8cfd Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Wed, 25 Mar 2020 11:23:25 +0000 Subject: [PATCH 5/6] Restructure spec to avoid variable names with numbers --- spec/models/calculator/weight_spec.rb | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/spec/models/calculator/weight_spec.rb b/spec/models/calculator/weight_spec.rb index 298293db92..e469733b4e 100644 --- a/spec/models/calculator/weight_spec.rb +++ b/spec/models/calculator/weight_spec.rb @@ -156,19 +156,24 @@ describe Calculator::Weight do let(:product) { create(:product, variant_unit: 'items', variant_unit_scale: nil, variant_unit_name: "bunch") } - let(:variant1) { create(:variant, product: product, unit_value: 0, weight: nil) } - let(:variant2) { create(:variant, product: product, unit_value: 0, weight: 10.0) } - let(:line_item1) { create(:line_item, variant: variant1, quantity: 1) } - let(:line_item2) { create(:line_item, variant: variant2, quantity: 1) } + let(:line_item) { create(:line_item, variant: variant, quantity: 1) } before { subject.set_preference(:per_kg, 5) } - it "uses zero weight if variant.weight is not present" do - expect(subject.compute(line_item1)).to eq 0 + context "when variant.weight is present" do + let(:variant) { create(:variant, product: product, unit_value: 0, weight: 10.0) } + + it "uses the variant weight" do + expect(subject.compute(line_item)).to eq 50.0 + end end - it "uses the variant weight if variant.weight is present" do - expect(subject.compute(line_item2)).to eq 50.0 + context "when variant.weight is nil" do + let(:variant) { create(:variant, product: product, unit_value: 0, weight: nil) } + + it "uses zero weight" do + expect(subject.compute(line_item)).to eq 0 + end end end end From 4bcd665379e00dda0c3f9be49c5409fbd43a21aa Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 26 Mar 2020 17:13:29 +1100 Subject: [PATCH 6/6] Handle all line items without unit_value in weight calculation --- app/models/calculator/weight.rb | 2 +- spec/models/calculator/weight_spec.rb | 30 ++++++++++++++++++++++++--- 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/app/models/calculator/weight.rb b/app/models/calculator/weight.rb index d5bf102c14..009d6bbf83 100644 --- a/app/models/calculator/weight.rb +++ b/app/models/calculator/weight.rb @@ -54,7 +54,7 @@ module Calculator # Customer ends up getting 350mL (line_item.final_weight_volume) of wine # that represent 2.8 (quantity_implied_in_final_weight_volume) glasses of wine def quantity_implied_in_final_weight_volume(line_item) - return line_item.quantity if line_item.variant.unit_value.zero? + return line_item.quantity if line_item.variant.unit_value.to_f.zero? (1.0 * line_item.final_weight_volume / line_item.variant.unit_value).round(3) end diff --git a/spec/models/calculator/weight_spec.rb b/spec/models/calculator/weight_spec.rb index e469733b4e..1ca1759b40 100644 --- a/spec/models/calculator/weight_spec.rb +++ b/spec/models/calculator/weight_spec.rb @@ -152,7 +152,7 @@ describe Calculator::Weight do end end - context "when variant_unit is 'items' and unit_value is zero" do + context "when variant_unit is 'items'" do let(:product) { create(:product, variant_unit: 'items', variant_unit_scale: nil, variant_unit_name: "bunch") } @@ -160,7 +160,7 @@ describe Calculator::Weight do before { subject.set_preference(:per_kg, 5) } - context "when variant.weight is present" do + context "when unit_value is zero variant.weight is present" do let(:variant) { create(:variant, product: product, unit_value: 0, weight: 10.0) } it "uses the variant weight" do @@ -168,12 +168,36 @@ describe Calculator::Weight do end end - context "when variant.weight is nil" do + context "when unit_value is zero variant.weight is nil" do let(:variant) { create(:variant, product: product, unit_value: 0, weight: nil) } it "uses zero weight" do expect(subject.compute(line_item)).to eq 0 end end + + context "when unit_value is nil and variant.weight is present" do + let(:variant) { + create(:variant, product: product, unit_description: "bunches", unit_value: nil, weight: 10.0) + } + + it "uses the variant weight" do + line_item.final_weight_volume = 1 + + expect(subject.compute(line_item)).to eq 50.0 + end + end + + context "when unit_value is nil and variant.weight is nil" do + let(:variant) { + create(:variant, product: product, unit_description: "bunches", unit_value: nil, weight: nil) + } + + it "uses zero weight" do + line_item.final_weight_volume = 1 + + expect(subject.compute(line_item)).to eq 0 + end + end end end