From 3f2b3b88eb7d494734a473e6eb7f65f36c95bc78 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Mon, 5 Sep 2022 10:59:15 +1000 Subject: [PATCH 1/5] Re-use in-memory test objects And reduce code by referring to objects instead of just ids. --- .../consumer/split_checkout_tax_not_incl_spec.rb | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/spec/system/consumer/split_checkout_tax_not_incl_spec.rb b/spec/system/consumer/split_checkout_tax_not_incl_spec.rb index 1d6188e585..fe1c970e1a 100644 --- a/spec/system/consumer/split_checkout_tax_not_incl_spec.rb +++ b/spec/system/consumer/split_checkout_tax_not_incl_spec.rb @@ -10,26 +10,26 @@ describe "As a consumer, I want to see adjustment breakdown" do include AuthenticationHelper include WebHelper - let!(:address_within_zone) { create(:address, state_id: Spree::State.first.id) } - let!(:address_outside_zone) { create(:address, state_id: Spree::State.second.id) } + let!(:address_within_zone) { create(:address, state: Spree::State.first) } + let!(:address_outside_zone) { create(:address, state: Spree::State.second) } let!(:user_within_zone) { - create(:user, bill_address_id: address_within_zone.id, - ship_address_id: address_within_zone.id) + create(:user, bill_address: address_within_zone, + ship_address: address_within_zone) } let!(:user_outside_zone) { - create(:user, bill_address_id: address_outside_zone.id, - ship_address_id: address_outside_zone.id) + create(:user, bill_address: address_outside_zone, + ship_address: address_outside_zone) } let!(:zone) { create(:zone_with_state_member, name: 'Victoria', default_tax: false) } let!(:tax_category) { create(:tax_category, name: "Veggies", is_default: "f") } let!(:tax_rate) { create(:tax_rate, name: "Tax rate - included or not", amount: 0.13, - zone_id: zone.id, tax_category_id: tax_category.id, included_in_price: false) + zone: zone, tax_category: tax_category, included_in_price: false) } let(:distributor) { create(:distributor_enterprise, charges_sales_tax: true) } let(:supplier) { create(:supplier_enterprise) } let!(:product_with_tax) { - create(:simple_product, supplier: supplier, price: 10, tax_category_id: tax_category.id) + create(:simple_product, supplier: supplier, price: 10, tax_category: tax_category) } let!(:variant_with_tax) { product_with_tax.variants.first } let!(:order_cycle) { From cc5125f49e2e90db80215a40412b16d636647f78 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Mon, 5 Sep 2022 11:40:47 +1000 Subject: [PATCH 2/5] Apply related db assertions in tax spec Somehow the helper methods were testing two different orders even though each spec is using only one order, the one which is currently being checked out by the user. So I brought the code closer into the example to easier see what is related and tested. I also changed the assertions for the pending spec but can't really verify them yet until I have completely working code. --- .../split_checkout_tax_not_incl_spec.rb | 32 ++++++++----------- 1 file changed, 13 insertions(+), 19 deletions(-) diff --git a/spec/system/consumer/split_checkout_tax_not_incl_spec.rb b/spec/system/consumer/split_checkout_tax_not_incl_spec.rb index fe1c970e1a..a3f7e37989 100644 --- a/spec/system/consumer/split_checkout_tax_not_incl_spec.rb +++ b/spec/system/consumer/split_checkout_tax_not_incl_spec.rb @@ -107,7 +107,8 @@ describe "As a consumer, I want to see adjustment breakdown" do click_on "Place order now" # DB checks - assert_db_tax_added + order_within_zone.reload + expect(order_within_zone.additional_tax_total).to eq(1.3) # UI checks expect(page).to have_selector('#order_total', text: with_currency(11.30)) @@ -134,7 +135,8 @@ describe "As a consumer, I want to see adjustment breakdown" do click_on "Complete order" # DB checks - assert_db_tax_added + order_within_zone.reload + expect(order_within_zone.additional_tax_total).to eq(1.3) # UI checks expect(page).to have_content("Confirmed") @@ -164,7 +166,9 @@ describe "As a consumer, I want to see adjustment breakdown" do click_on "Place order now" # DB checks - assert_db_no_tax_added + order_outside_zone.reload + expect(order_outside_zone.included_tax_total).to eq(0.0) + expect(order_outside_zone.additional_tax_total).to eq(0.0) # UI checks expect(page).to have_content("Confirmed") @@ -192,7 +196,9 @@ describe "As a consumer, I want to see adjustment breakdown" do click_on "Complete order" # DB checks - assert_db_no_tax_added + order_outside_zone.reload + expect(order_outside_zone.included_tax_total).to eq(0.0) + expect(order_outside_zone.additional_tax_total).to eq(0.0) # UI checks expect(page).to have_content("Confirmed") @@ -236,7 +242,9 @@ describe "As a consumer, I want to see adjustment breakdown" do click_on "Complete order" # DB checks - assert_db_tax_added + order_outside_zone.reload + expect(order_outside_zone.included_tax_total).to eq(0.0) + expect(order_outside_zone.additional_tax_total).to eq(1.3) # UI checks - Order confirmation page should reflect changes expect(page).to have_content("Confirmed") @@ -247,18 +255,4 @@ describe "As a consumer, I want to see adjustment breakdown" do end end end - - private - - def assert_db_tax_added - order_within_zone.reload - expect(order_outside_zone.included_tax_total).to eq(0.0) - expect(order_within_zone.additional_tax_total).to eq(1.3) - end - - def assert_db_no_tax_added - order_outside_zone.reload - expect(order_outside_zone.included_tax_total).to eq(0.0) - expect(order_outside_zone.additional_tax_total).to eq(0.0) - end end From eaca953106e027507b5547c6f41a6ceb683cfe41 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Mon, 5 Sep 2022 13:36:33 +1000 Subject: [PATCH 3/5] Create only required test data for speed It reduced the execution time by 5% on my machine (2-3 seconds). --- .../split_checkout_tax_not_incl_spec.rb | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/spec/system/consumer/split_checkout_tax_not_incl_spec.rb b/spec/system/consumer/split_checkout_tax_not_incl_spec.rb index a3f7e37989..da450a765d 100644 --- a/spec/system/consumer/split_checkout_tax_not_incl_spec.rb +++ b/spec/system/consumer/split_checkout_tax_not_incl_spec.rb @@ -12,11 +12,11 @@ describe "As a consumer, I want to see adjustment breakdown" do let!(:address_within_zone) { create(:address, state: Spree::State.first) } let!(:address_outside_zone) { create(:address, state: Spree::State.second) } - let!(:user_within_zone) { + let(:user_within_zone) { create(:user, bill_address: address_within_zone, ship_address: address_within_zone) } - let!(:user_outside_zone) { + let(:user_outside_zone) { create(:user, bill_address: address_outside_zone, ship_address: address_outside_zone) } @@ -28,11 +28,11 @@ describe "As a consumer, I want to see adjustment breakdown" do } let(:distributor) { create(:distributor_enterprise, charges_sales_tax: true) } let(:supplier) { create(:supplier_enterprise) } - let!(:product_with_tax) { + let(:product_with_tax) { create(:simple_product, supplier: supplier, price: 10, tax_category: tax_category) } - let!(:variant_with_tax) { product_with_tax.variants.first } - let!(:order_cycle) { + let(:variant_with_tax) { product_with_tax.variants.first } + let(:order_cycle) { create(:simple_order_cycle, suppliers: [supplier], distributors: [distributor], coordinator: distributor, variants: [variant_with_tax]) } @@ -46,12 +46,12 @@ describe "As a consumer, I want to see adjustment breakdown" do name: "Payment without Fee", description: "Payment without fee", calculator: Calculator::FlatRate.new(preferred_amount: 0.00)) } - let!(:order_within_zone) { + let(:order_within_zone) { create(:order, order_cycle: order_cycle, distributor: distributor, user: user_within_zone, bill_address: address_within_zone, ship_address: address_within_zone, state: "cart", line_items: [create(:line_item, variant: variant_with_tax)]) } - let!(:order_outside_zone) { + let(:order_outside_zone) { create(:order, order_cycle: order_cycle, distributor: distributor, user: user_outside_zone, bill_address: address_outside_zone, ship_address: address_outside_zone, state: "cart", line_items: [create(:line_item, variant: variant_with_tax)]) @@ -118,6 +118,10 @@ describe "As a consumer, I want to see adjustment breakdown" do context "on split-checkout" do before do + # WIP: Create order before split checkout is enabled. + # Otherwise the spec fails which may be a hint to issue 9153. + order_within_zone + allow(Flipper).to receive(:enabled?).with(:split_checkout).and_return(true) allow(Flipper).to receive(:enabled?).with(:split_checkout, anything).and_return(true) From 6c7a08952735b30df83390f636e3d4f12f2a8d90 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Mon, 5 Sep 2022 15:14:08 +1000 Subject: [PATCH 4/5] Recalculate tax on summary step --- app/controllers/split_checkout_controller.rb | 1 + spec/system/consumer/split_checkout_tax_not_incl_spec.rb | 4 ---- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb index 79a1acbcb6..1b772ab633 100644 --- a/app/controllers/split_checkout_controller.rb +++ b/app/controllers/split_checkout_controller.rb @@ -22,6 +22,7 @@ class SplitCheckoutController < ::BaseController def edit redirect_to_step_based_on_order unless params[:step] check_step if params[:step] + @order.create_tax_charge! if params[:step] == "summary" end def update diff --git a/spec/system/consumer/split_checkout_tax_not_incl_spec.rb b/spec/system/consumer/split_checkout_tax_not_incl_spec.rb index da450a765d..6b117cd338 100644 --- a/spec/system/consumer/split_checkout_tax_not_incl_spec.rb +++ b/spec/system/consumer/split_checkout_tax_not_incl_spec.rb @@ -118,10 +118,6 @@ describe "As a consumer, I want to see adjustment breakdown" do context "on split-checkout" do before do - # WIP: Create order before split checkout is enabled. - # Otherwise the spec fails which may be a hint to issue 9153. - order_within_zone - allow(Flipper).to receive(:enabled?).with(:split_checkout).and_return(true) allow(Flipper).to receive(:enabled?).with(:split_checkout, anything).and_return(true) From 1d0451036515cba0ce8dc653c20e7a77cf8ad03c Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Mon, 5 Sep 2022 15:28:09 +1000 Subject: [PATCH 5/5] Update totals on summary step --- app/controllers/split_checkout_controller.rb | 7 ++++++- spec/system/consumer/split_checkout_tax_not_incl_spec.rb | 1 - 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb index 1b772ab633..26e6c3bcae 100644 --- a/app/controllers/split_checkout_controller.rb +++ b/app/controllers/split_checkout_controller.rb @@ -22,7 +22,7 @@ class SplitCheckoutController < ::BaseController def edit redirect_to_step_based_on_order unless params[:step] check_step if params[:step] - @order.create_tax_charge! if params[:step] == "summary" + recalculate_tax if params[:step] == "summary" end def update @@ -156,4 +156,9 @@ class SplitCheckoutController < ::BaseController redirect_to checkout_step_path(:payment) if params[:step] == "summary" end end + + def recalculate_tax + @order.create_tax_charge! + @order.update_order! + end end diff --git a/spec/system/consumer/split_checkout_tax_not_incl_spec.rb b/spec/system/consumer/split_checkout_tax_not_incl_spec.rb index 6b117cd338..1d0d24ec08 100644 --- a/spec/system/consumer/split_checkout_tax_not_incl_spec.rb +++ b/spec/system/consumer/split_checkout_tax_not_incl_spec.rb @@ -223,7 +223,6 @@ describe "As a consumer, I want to see adjustment breakdown" do end it "should re-calculate the tax accordingly" do - pending("#9153") select "Victoria", from: "order_bill_address_attributes_state_id" # it should not be necessary to save as new default bill address