From acb5e9e170700bf3d15c81aebe40168dbe986ccb Mon Sep 17 00:00:00 2001 From: David Cook Date: Thu, 27 Jul 2023 16:02:21 +1000 Subject: [PATCH 1/5] Remove unnecessary extra page load --- spec/system/consumer/split_checkout_spec.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/spec/system/consumer/split_checkout_spec.rb b/spec/system/consumer/split_checkout_spec.rb index f2c563bf52a..384586689b0 100644 --- a/spec/system/consumer/split_checkout_spec.rb +++ b/spec/system/consumer/split_checkout_spec.rb @@ -724,15 +724,16 @@ create(:voucher, code: 'some_code', enterprise: distributor, amount: 15) end - before do - visit checkout_step_path(:payment) - end - it "shows voucher input" do + visit checkout_step_path(:payment) expect(page).to have_content "Apply voucher" end describe "adding voucher to the order" do + before do + visit checkout_step_path(:payment) + end + shared_examples "adding voucher to the order" do before do fill_in "Enter voucher code", with: "some_code" @@ -779,7 +780,6 @@ describe "removing voucher from order" do before do voucher.create_adjustment(voucher.code, order) - # Reload the page so we pickup the voucher visit checkout_step_path(:payment) end From 2e8153e7b78ee0b13a0dd994202b4178d40baf9c Mon Sep 17 00:00:00 2001 From: David Cook Date: Thu, 27 Jul 2023 16:04:25 +1000 Subject: [PATCH 2/5] Pending spec: vouchers should not require payment --- spec/system/consumer/split_checkout_spec.rb | 39 +++++++++++++++++++-- 1 file changed, 37 insertions(+), 2 deletions(-) diff --git a/spec/system/consumer/split_checkout_spec.rb b/spec/system/consumer/split_checkout_spec.rb index 384586689b0..5460aad7ce9 100644 --- a/spec/system/consumer/split_checkout_spec.rb +++ b/spec/system/consumer/split_checkout_spec.rb @@ -726,6 +726,7 @@ it "shows voucher input" do visit checkout_step_path(:payment) + expect(page).to have_field "Enter voucher code" expect(page).to have_content "Apply voucher" end @@ -765,6 +766,17 @@ "you may not be able to spend the remaining value." ) end + + it "proceeds without requiring payment" do + fill_in "Enter voucher code", with: "some_code" + click_button("Apply") + + pending + expect(page).to have_content "No payment required" + click_button "Next - Order summary" + # Expect to be on the Order Summary page + expect(page).to have_content "Delivery details" + end end context "voucher doesn't exist" do @@ -781,19 +793,42 @@ before do voucher.create_adjustment(voucher.code, order) visit checkout_step_path(:payment) - end - it "removes voucher" do accept_confirm "Are you sure you want to remove the voucher?" do click_on "Remove code" end + end + it "removes voucher" do within '#voucher-section' do expect(page).to have_button("Apply", disabled: true) + expect(page).to have_field "Enter voucher code" # Currently no confirmation msg end expect(order.voucher_adjustments.length).to eq(0) end + + it "can re-enter a voucher" do + # Re-enter a voucher code + fill_in "Enter voucher code", with: "some_code" + click_button("Apply") + + expect(page).to have_content("$15.00 Voucher") + expect(order.reload.voucher_adjustments.length).to eq(1) + pending + expect(page).to have_content "No payment required" + + click_button "Next - Order summary" + # Expect to be on the Order Summary page + expect(page).to have_content "Delivery details" + end + + it "can proceed with payment" do + choose "Payment with Fee" + click_button "Next - Order summary" + # Expect to be on the Order Summary page + expect(page).to have_content "Delivery details" + end end end end From 563ec452517dd505bbb6b3444d1ff78a0a85a5db Mon Sep 17 00:00:00 2001 From: David Cook Date: Thu, 27 Jul 2023 16:35:43 +1000 Subject: [PATCH 3/5] Check if record actually saved I'm not sure if it will ever make a difference, but I think this makes more sense. --- app/controllers/voucher_adjustments_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/voucher_adjustments_controller.rb b/app/controllers/voucher_adjustments_controller.rb index 9b6a2bac26d..329ca05712c 100644 --- a/app/controllers/voucher_adjustments_controller.rb +++ b/app/controllers/voucher_adjustments_controller.rb @@ -38,7 +38,7 @@ def add_voucher adjustment = voucher.create_adjustment(voucher.code, @order) - unless adjustment.valid? + unless adjustment.persisted? @order.errors.add(:voucher_code, I18n.t('split_checkout.errors.add_voucher_error')) adjustment.errors.each { |error| @order.errors.import(error) } return false From 5e40e70926dd076ac89783885c7c6eee53d97f7e Mon Sep 17 00:00:00 2001 From: David Cook Date: Thu, 27 Jul 2023 16:56:08 +1000 Subject: [PATCH 4/5] Always update order totals when creating voucher adjustment Maybe there was a reason we don't do this, but it seems like the best way to solve the problem to me. I'd like Gaetan to review this. --- app/models/voucher.rb | 13 +++++++++---- spec/models/voucher_spec.rb | 4 ++++ spec/system/consumer/split_checkout_spec.rb | 3 +-- 3 files changed, 14 insertions(+), 6 deletions(-) diff --git a/app/models/voucher.rb b/app/models/voucher.rb index 1551c6807b7..f7a9e933d7c 100644 --- a/app/models/voucher.rb +++ b/app/models/voucher.rb @@ -21,11 +21,9 @@ def display_value # but vouchers have complicated calculation so we can't easily use Spree::Calculator. We keep # the same method to stay as consistent as possible. # - # Creates a new voucher adjustment for the given order with an amount of 0 - # The amount will be calculated via VoucherAdjustmentsService#update def create_adjustment(label, order) adjustment_attributes = { - amount: 0, + amount: 0, # Amount is updated by VoucherAdjustmentsService below originator: self, order: order, label: label, @@ -34,7 +32,14 @@ def create_adjustment(label, order) tax_category: nil } - order.adjustments.create(adjustment_attributes) + adjustment = order.adjustments.new(adjustment_attributes) + + if adjustment.save + VoucherAdjustmentsService.new(order).update + order.update_totals_and_states + end + + adjustment end # We limit adjustment to the maximum amount needed to cover the order, ie if the voucher diff --git a/spec/models/voucher_spec.rb b/spec/models/voucher_spec.rb index 0c62a814968..fd1d3f52fe1 100644 --- a/spec/models/voucher_spec.rb +++ b/spec/models/voucher_spec.rb @@ -56,5 +56,9 @@ it 'sets the adjustment as open' do expect(adjustment.state).to eq("open") end + + it 'updates order totals' do + expect{ subject }.to change(order, :total).by(-25) + end end end diff --git a/spec/system/consumer/split_checkout_spec.rb b/spec/system/consumer/split_checkout_spec.rb index 5460aad7ce9..df58f9c8a22 100644 --- a/spec/system/consumer/split_checkout_spec.rb +++ b/spec/system/consumer/split_checkout_spec.rb @@ -771,7 +771,6 @@ fill_in "Enter voucher code", with: "some_code" click_button("Apply") - pending expect(page).to have_content "No payment required" click_button "Next - Order summary" # Expect to be on the Order Summary page @@ -815,7 +814,6 @@ expect(page).to have_content("$15.00 Voucher") expect(order.reload.voucher_adjustments.length).to eq(1) - pending expect(page).to have_content "No payment required" click_button "Next - Order summary" @@ -824,6 +822,7 @@ end it "can proceed with payment" do + pending choose "Payment with Fee" click_button "Next - Order summary" # Expect to be on the Order Summary page From 782b37da4af4586c67079b25f8700b7f64202a29 Mon Sep 17 00:00:00 2001 From: David Cook Date: Thu, 27 Jul 2023 17:08:51 +1000 Subject: [PATCH 5/5] Always update order after removing voucher I'm not sure if we need to refresh other adjustments, but it seems worth being comprehensive. Alternatively we could probably call just order.update_totals. --- app/controllers/voucher_adjustments_controller.rb | 1 + spec/system/consumer/split_checkout_spec.rb | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/voucher_adjustments_controller.rb b/app/controllers/voucher_adjustments_controller.rb index 329ca05712c..f56b64b52c5 100644 --- a/app/controllers/voucher_adjustments_controller.rb +++ b/app/controllers/voucher_adjustments_controller.rb @@ -13,6 +13,7 @@ def create def destroy @order.voucher_adjustments.find_by(id: params[:id])&.destroy + @order.update_order! # adjustments, totals and states update_payment_section end diff --git a/spec/system/consumer/split_checkout_spec.rb b/spec/system/consumer/split_checkout_spec.rb index df58f9c8a22..52af54ec146 100644 --- a/spec/system/consumer/split_checkout_spec.rb +++ b/spec/system/consumer/split_checkout_spec.rb @@ -822,7 +822,6 @@ end it "can proceed with payment" do - pending choose "Payment with Fee" click_button "Next - Order summary" # Expect to be on the Order Summary page