From f7ee01b9f8f1d1539a14b0ea7d937ddabc1c16bd Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Wed, 8 Mar 2023 14:35:12 +1100 Subject: [PATCH 01/39] Add voucher input on checkout payment's step Voucher input is displayed only if the distributor has any voucher --- app/views/split_checkout/_payment.html.haml | 10 ++++++++ .../css/darkswarm/split-checkout.scss | 21 ++++++++++++++++ config/locales/en.yml | 4 ++++ spec/system/consumer/split_checkout_spec.rb | 24 +++++++++++++++++++ 4 files changed, 59 insertions(+) diff --git a/app/views/split_checkout/_payment.html.haml b/app/views/split_checkout/_payment.html.haml index 0e98fe6df4..6c743a33cd 100644 --- a/app/views/split_checkout/_payment.html.haml +++ b/app/views/split_checkout/_payment.html.haml @@ -1,5 +1,15 @@ .medium-6 %div.checkout-substep{"data-controller": "paymentmethod"} + - if @order.distributor.vouchers.present? + .checkout-title + = t("split_checkout.step2.voucher.apply_voucher") + + .checkout-input.voucher + .two-columns-inputs.voucher + = f.text_field :voucher_code, { placeholder: t("split_checkout.step2.voucher.placeholder"), class: "voucher" } + - # TODO: enable button when code entered + = f.submit t("split_checkout.step2.voucher.apply"), class: "button cancel voucher", disabled: true + %div.checkout-title = t("split_checkout.step2.payment_method.title") diff --git a/app/webpacker/css/darkswarm/split-checkout.scss b/app/webpacker/css/darkswarm/split-checkout.scss index 96c351e198..259a9bbfa4 100644 --- a/app/webpacker/css/darkswarm/split-checkout.scss +++ b/app/webpacker/css/darkswarm/split-checkout.scss @@ -404,6 +404,22 @@ gap: 1rem; justify-content: space-between; + &.voucher { + justify-content: normal; + + input { + width: 50%; + } + + .button { + &.cancel { + width: 30%; + border-radius: 0.5em; + padding:0; + } + } + } + > .checkout-input { flex: 1; } @@ -418,6 +434,11 @@ &:last-child > .checkout-input { margin-bottom: 1.5rem; } + + &.voucher { + flex-direction: row; + gap: 1rem; + } } } diff --git a/config/locales/en.yml b/config/locales/en.yml index c8a9ab96fc..993f2f8406 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -2055,6 +2055,10 @@ en: explaination: You can review and confirm your order in the next step which includes the final costs. submit: Next - Order summary cancel: Back to Your details + voucher: + apply_voucher: Apply voucher + apply: Apply + placeholder: Enter voucher code step3: delivery_details: title: Delivery details diff --git a/spec/system/consumer/split_checkout_spec.rb b/spec/system/consumer/split_checkout_spec.rb index e114184a0e..d274bafd7a 100644 --- a/spec/system/consumer/split_checkout_spec.rb +++ b/spec/system/consumer/split_checkout_spec.rb @@ -708,6 +708,30 @@ describe "As a consumer, I want to checkout my order" do end end + describe "vouchers" do + context "with no voucher available" do + before do + visit checkout_step_path(:payment) + end + + it "doesn't show voucher input" do + expect(page).not_to have_content "Apply voucher" + end + end + + context "with voucher available" do + let!(:voucher) { Voucher.create(code: 'some_code', enterprise: distributor) } + + before do + visit checkout_step_path(:payment) + end + + it "shows voucher input" do + expect(page).to have_content "Apply voucher" + end + end + end + describe "choosing" do shared_examples "different payment methods" do |pay_method| context "checking out with #{pay_method}", if: pay_method.eql?("Stripe SCA") == false do From 9b680e1a926e1e20a385e0612d9c11f8c86ce5db Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 14 Mar 2023 14:55:07 +1100 Subject: [PATCH 02/39] Add CalculatedAdjustments to Voucher Vouchers are only flat rate of 10 for now, so we add a FlatRate calculator in a before_validation callback --- app/models/voucher.rb | 13 +++++++++++++ spec/models/voucher_spec.rb | 17 +++++++++++++++-- 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/app/models/voucher.rb b/app/models/voucher.rb index 28c74ca7de..783c5c93aa 100644 --- a/app/models/voucher.rb +++ b/app/models/voucher.rb @@ -1,10 +1,16 @@ # frozen_string_literal: false class Voucher < ApplicationRecord + include CalculatedAdjustments + belongs_to :enterprise + has_many :adjustments, as: :originator, class_name: 'Spree::Adjustment' + validates :code, presence: true, uniqueness: { scope: :enterprise_id } + before_validation :add_calculator + def value 10 end @@ -12,4 +18,11 @@ class Voucher < ApplicationRecord def display_value Spree::Money.new(value) end + + private + + # For now voucher are only flat rate of 10 + def add_calculator + self.calculator = Calculator::FlatRate.new(preferred_amount: -value) + end end diff --git a/spec/models/voucher_spec.rb b/spec/models/voucher_spec.rb index 9d9590455f..ea798d1f62 100644 --- a/spec/models/voucher_spec.rb +++ b/spec/models/voucher_spec.rb @@ -3,16 +3,29 @@ require 'spec_helper' describe Voucher do + let(:enterprise) { build(:enterprise) } + describe 'associations' do it { is_expected.to belong_to(:enterprise) } + it { is_expected.to have_many(:adjustments) } end describe 'validations' do subject { Voucher.new(code: 'new_code', enterprise: enterprise) } - let(:enterprise) { build(:enterprise) } - it { is_expected.to validate_presence_of(:code) } it { is_expected.to validate_uniqueness_of(:code).scoped_to(:enterprise_id) } end + + describe 'after_save' do + subject { Voucher.create(code: 'new_code', enterprise: enterprise) } + + it 'adds a FlateRate calculator' do + expect(subject.calculator.instance_of?(Calculator::FlatRate)).to be(true) + end + + it 'has a preferred_amount of -10' do + expect(subject.calculator.preferred_amount.to_f).to eq(-10) + end + end end From 3ec58d879110866721d325cc628cbf50d3e8d328 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Wed, 15 Mar 2023 14:07:21 +1100 Subject: [PATCH 03/39] Add #vouchers, return an array of voucher adjustments --- app/models/spree/order.rb | 4 ++++ spec/models/spree/order_spec.rb | 17 +++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index 40dfbcb093..4a5ef3bc81 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -618,6 +618,10 @@ module Spree end end + def vouchers + adjustments.where(originator_type: 'Voucher') + end + private def fee_handler diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index 4ea1933d6c..7260ca431a 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -1430,4 +1430,21 @@ describe Spree::Order do end end end + + describe "#vouchers" do + let(:voucher) { Voucher.create(code: 'new_code', enterprise: order.distributor) } + + context "when no voucher adjustment" do + it 'returns an empty array' do + expect(order.vouchers).to eq([]) + end + end + + it "returns an array of voucher adjusment" do + order.save! + expected_adjustments = Array.new(2) { voucher.create_adjustment(voucher.code, order) } + + expect(order.vouchers).to eq(expected_adjustments) + end + end end From a9d9b33f7dbf4f116d3c1e51829205cfa32438dd Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 17 Apr 2023 14:27:34 +1000 Subject: [PATCH 04/39] First take at adding a voucher to an order Use voucher.create_adjustment to add an adjustment to the order Currently doesn't take into account tax part --- app/controllers/split_checkout_controller.rb | 25 ++++++++++++++++++++ app/views/split_checkout/_payment.html.haml | 9 ++++--- config/locales/en.yml | 1 + spec/system/consumer/split_checkout_spec.rb | 18 ++++++++++++++ 4 files changed, 50 insertions(+), 3 deletions(-) diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb index d65a661f32..63bf2db6a8 100644 --- a/app/controllers/split_checkout_controller.rb +++ b/app/controllers/split_checkout_controller.rb @@ -22,6 +22,9 @@ class SplitCheckoutController < ::BaseController before_action :hide_ofn_navigation, only: [:edit, :update] def edit + # TODO Calculate percent amount covered by the voucher for display purposes + @voucher_adjustment = @order.vouchers.first + redirect_to_step_based_on_order unless params[:step] check_step if params[:step] recalculate_tax if params[:step] == "summary" @@ -30,6 +33,8 @@ class SplitCheckoutController < ::BaseController end def update + return add_voucher if payment_step? and params[:order][:voucher_code] + if confirm_order || update_order return if performed? @@ -179,10 +184,30 @@ class SplitCheckoutController < ::BaseController selected_shipping_method.first.require_ship_address == false end + def add_voucher + # Fetch Voucher + voucher = Voucher.find_by(code: params[:order][:voucher_code], enterprise: @order.distributor) + + if voucher.nil? + @order.errors.add(:voucher, I18n.t('split_checkout.errors.voucher_not_found')) + return render_error + end + + # Create adjustment + # TODO add tax part of adjustement + voucher.create_adjustment(voucher.code, @order) + + redirect_to checkout_step_path(:payment) + end + def summary_step? params[:step] == "summary" end + def payment_step? + params[:step] == "payment" + end + def advance_order_state return if @order.complete? diff --git a/app/views/split_checkout/_payment.html.haml b/app/views/split_checkout/_payment.html.haml index 6c743a33cd..aeeccaa6ba 100644 --- a/app/views/split_checkout/_payment.html.haml +++ b/app/views/split_checkout/_payment.html.haml @@ -6,9 +6,12 @@ .checkout-input.voucher .two-columns-inputs.voucher - = f.text_field :voucher_code, { placeholder: t("split_checkout.step2.voucher.placeholder"), class: "voucher" } - - # TODO: enable button when code entered - = f.submit t("split_checkout.step2.voucher.apply"), class: "button cancel voucher", disabled: true + -if @voucher_adjustment.present? + = "Voucher used: #{@voucher_adjustment.label}" + - else + = f.text_field :voucher_code, { placeholder: t("split_checkout.step2.voucher.placeholder"), class: "voucher" } + - # TODO: enable button when code entered + = f.submit t("split_checkout.step2.voucher.apply"), class: "button cancel voucher", disabled: false %div.checkout-title = t("split_checkout.step2.payment_method.title") diff --git a/config/locales/en.yml b/config/locales/en.yml index 993f2f8406..bfdab857d2 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -2091,6 +2091,7 @@ en: select_a_shipping_method: Select a shipping method select_a_payment_method: Select a payment method no_shipping_methods_available: Checkout is not possible due to absence of shipping options. Please contact the shop owner. + voucher_not_found: Not found order_paid: PAID order_not_paid: NOT PAID order_total: Total order diff --git a/spec/system/consumer/split_checkout_spec.rb b/spec/system/consumer/split_checkout_spec.rb index d274bafd7a..9fd3f90568 100644 --- a/spec/system/consumer/split_checkout_spec.rb +++ b/spec/system/consumer/split_checkout_spec.rb @@ -729,6 +729,24 @@ describe "As a consumer, I want to checkout my order" do it "shows voucher input" do expect(page).to have_content "Apply voucher" end + + describe "adding voucher to the order" do + context "voucher doesn't exist" do + it "show an error" do + fill_in "Enter voucher code", with: "non_code" + click_button("Apply") + + expect(page).to have_content("Voucher Not found") + end + end + + it "adds a voucher to the order" do + fill_in "Enter voucher code", with: voucher.code + click_button("Apply") + + expect(page).to have_content("Voucher used: some_code") + end + end end end From b6213b25e9d77baa8798bf5db258ff1be41f3384 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 20 Mar 2023 13:45:42 +1100 Subject: [PATCH 05/39] add acts_as_paranoid to voucher This is so the relation `belongs_to :originator, -> { with_deleted }, polymorphic: true` setup in Spree::Adjustement works as expected. --- app/models/voucher.rb | 2 ++ db/migrate/20230315040748_add_deleted_at_to_vouchers.rb | 6 ++++++ db/schema.rb | 2 ++ 3 files changed, 10 insertions(+) create mode 100644 db/migrate/20230315040748_add_deleted_at_to_vouchers.rb diff --git a/app/models/voucher.rb b/app/models/voucher.rb index 783c5c93aa..533b7aece2 100644 --- a/app/models/voucher.rb +++ b/app/models/voucher.rb @@ -1,6 +1,8 @@ # frozen_string_literal: false class Voucher < ApplicationRecord + acts_as_paranoid + include CalculatedAdjustments belongs_to :enterprise diff --git a/db/migrate/20230315040748_add_deleted_at_to_vouchers.rb b/db/migrate/20230315040748_add_deleted_at_to_vouchers.rb new file mode 100644 index 0000000000..d9993f2b93 --- /dev/null +++ b/db/migrate/20230315040748_add_deleted_at_to_vouchers.rb @@ -0,0 +1,6 @@ +class AddDeletedAtToVouchers < ActiveRecord::Migration[6.1] + def change + add_column :vouchers, :deleted_at, :datetime + add_index :vouchers, :deleted_at + end +end diff --git a/db/schema.rb b/db/schema.rb index dbcb97b5a1..41b3d45046 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1196,7 +1196,9 @@ ActiveRecord::Schema[7.0].define(version: 2023_04_24_141213) do t.datetime "created_at", null: false t.datetime "updated_at", null: false t.bigint "enterprise_id" + t.datetime "deleted_at" t.index ["code", "enterprise_id"], name: "index_vouchers_on_code_and_enterprise_id", unique: true + t.index ["deleted_at"], name: "index_vouchers_on_deleted_at" t.index ["enterprise_id"], name: "index_vouchers_on_enterprise_id" end From e487ed05320dfe65005556b1c2ac6518c5efe9f0 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 20 Mar 2023 15:55:55 +1100 Subject: [PATCH 06/39] Add link to remove voucher when a voucher has been applied Improve styling to match the design --- app/controllers/split_checkout_controller.rb | 10 +++++-- app/views/split_checkout/_payment.html.haml | 7 +++-- .../css/darkswarm/split-checkout.scss | 26 +++++++++++++++++++ config/locales/en.yml | 3 +++ config/routes.rb | 4 ++- spec/system/consumer/split_checkout_spec.rb | 20 +++++++++++++- 6 files changed, 64 insertions(+), 6 deletions(-) diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb index 63bf2db6a8..10b3632740 100644 --- a/app/controllers/split_checkout_controller.rb +++ b/app/controllers/split_checkout_controller.rb @@ -22,7 +22,6 @@ class SplitCheckoutController < ::BaseController before_action :hide_ofn_navigation, only: [:edit, :update] def edit - # TODO Calculate percent amount covered by the voucher for display purposes @voucher_adjustment = @order.vouchers.first redirect_to_step_based_on_order unless params[:step] @@ -33,7 +32,7 @@ class SplitCheckoutController < ::BaseController end def update - return add_voucher if payment_step? and params[:order][:voucher_code] + return add_voucher if payment_step? && params[:order][:voucher_code] if confirm_order || update_order return if performed? @@ -51,6 +50,13 @@ class SplitCheckoutController < ::BaseController render cable_ready: cable_car.redirect_to(url: checkout_step_path(:payment)) end + def destroy + adjustment = Spree::Adjustment.find_by(id: params[:adjustment_id]) + adjustment.destroy + + redirect_to checkout_step_path(:payment) + end + private def render_error diff --git a/app/views/split_checkout/_payment.html.haml b/app/views/split_checkout/_payment.html.haml index aeeccaa6ba..df952e8895 100644 --- a/app/views/split_checkout/_payment.html.haml +++ b/app/views/split_checkout/_payment.html.haml @@ -4,10 +4,13 @@ .checkout-title = t("split_checkout.step2.voucher.apply_voucher") - .checkout-input.voucher + .checkout-input .two-columns-inputs.voucher -if @voucher_adjustment.present? - = "Voucher used: #{@voucher_adjustment.label}" + %span.button.voucher-added + %i.ofn-i_051-check-big + = "#{@voucher_adjustment.originator.display_value} #{t("split_checkout.step2.voucher.voucher")}" + = link_to t("split_checkout.step2.voucher.remove_code"), checkout_destroy_path(adjustment_id: @voucher_adjustment.id), method: "delete", data: { confirm: t("split_checkout.step2.voucher.confirm_delete") } - else = f.text_field :voucher_code, { placeholder: t("split_checkout.step2.voucher.placeholder"), class: "voucher" } - # TODO: enable button when code entered diff --git a/app/webpacker/css/darkswarm/split-checkout.scss b/app/webpacker/css/darkswarm/split-checkout.scss index 259a9bbfa4..49807dc610 100644 --- a/app/webpacker/css/darkswarm/split-checkout.scss +++ b/app/webpacker/css/darkswarm/split-checkout.scss @@ -406,19 +406,45 @@ &.voucher { justify-content: normal; + align-items: center; input { width: 50%; } + a { + color: inherit; + } + .button { &.cancel { width: 30%; border-radius: 0.5em; padding:0; + height: 2.5em; + background-color: $teal-400 } } } + + .voucher-added { + padding: 10px; + background-color: $teal-300; + color: $teal-500; + margin: 0; + cursor: default; + + i.ofn-i_051-check-big:before { + background-color: $teal-500; + color: $teal-300; + border-radius: 50%; + font-style: normal; + } + + i.ofn-i_051-check-big { + font-style: italic; + } + } > .checkout-input { flex: 1; diff --git a/config/locales/en.yml b/config/locales/en.yml index bfdab857d2..40a5c5c043 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -2056,9 +2056,12 @@ en: submit: Next - Order summary cancel: Back to Your details voucher: + voucher: Voucher apply_voucher: Apply voucher apply: Apply placeholder: Enter voucher code + remove_code: Remove code + confirm_delete: Are you sure you want to remove the voucher ? step3: delivery_details: title: Delivery details diff --git a/config/routes.rb b/config/routes.rb index 04a800bfd5..364c0425ed 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -74,7 +74,7 @@ Openfoodnetwork::Application.routes.draw do match "/checkout", via: :get, controller: "payment_gateways/stripe", action: "confirm" match "/orders/:order_number", via: :get, controller: "payment_gateways/stripe", action: "authorize" end - + namespace :payment_gateways do get "/paypal", to: "paypal#express", as: :paypal_express get "/paypal/confirm", to: "paypal#confirm", as: :confirm_paypal @@ -92,6 +92,8 @@ Openfoodnetwork::Application.routes.draw do put '/checkout/:step', to: 'split_checkout#update', as: :checkout_update end + delete '/checkout/payment', to: 'split_checkout#destroy', as: :checkout_destroy + # Redirects to the new checkout for any other 'step' (ie. /checkout/cart from the legacy checkout) get '/checkout/:other', to: redirect('/checkout') end diff --git a/spec/system/consumer/split_checkout_spec.rb b/spec/system/consumer/split_checkout_spec.rb index 9fd3f90568..f1b353aee9 100644 --- a/spec/system/consumer/split_checkout_spec.rb +++ b/spec/system/consumer/split_checkout_spec.rb @@ -744,7 +744,25 @@ describe "As a consumer, I want to checkout my order" do fill_in "Enter voucher code", with: voucher.code click_button("Apply") - expect(page).to have_content("Voucher used: some_code") + expect(page).to have_content("$10.00 Voucher") + end + end + + 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 + + it "removes voucher" do + accept_confirm "Are you sure you want to remove the voucher ?" do + click_on "Remove code" + end + + within '.voucher' do + expect(page).to have_button("Apply") + end end end end From 856386fcb013c199a5efe68d6e9a149d4e16f66c Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 20 Mar 2023 16:26:58 +1100 Subject: [PATCH 07/39] SplitCheckoutController refactor add_voucher Now all redirection/render_error happen in #update, it makes it easier to understand. --- app/controllers/split_checkout_controller.rb | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb index 10b3632740..e8ab1d6e7f 100644 --- a/app/controllers/split_checkout_controller.rb +++ b/app/controllers/split_checkout_controller.rb @@ -32,7 +32,11 @@ class SplitCheckoutController < ::BaseController end def update - return add_voucher if payment_step? && params[:order][:voucher_code] + if add_voucher + return redirect_to checkout_step_path(:payment) + elsif @order.errors.present? + return render_error + end if confirm_order || update_order return if performed? @@ -191,19 +195,21 @@ class SplitCheckoutController < ::BaseController end def add_voucher + return unless payment_step? && params[:order] && params[:order][:voucher_code] + # Fetch Voucher voucher = Voucher.find_by(code: params[:order][:voucher_code], enterprise: @order.distributor) if voucher.nil? @order.errors.add(:voucher, I18n.t('split_checkout.errors.voucher_not_found')) - return render_error + return false end # Create adjustment # TODO add tax part of adjustement voucher.create_adjustment(voucher.code, @order) - redirect_to checkout_step_path(:payment) + true end def summary_step? From 74a8730f045502cc77e103deaba893c2c06f0cf0 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 21 Mar 2023 11:52:50 +1100 Subject: [PATCH 08/39] Highlight voucher on order summary step --- app/views/split_checkout/_summary.html.haml | 8 ++++++-- app/webpacker/css/darkswarm/split-checkout.scss | 4 ++++ spec/system/consumer/split_checkout_spec.rb | 17 +++++++++++++++++ 3 files changed, 27 insertions(+), 2 deletions(-) diff --git a/app/views/split_checkout/_summary.html.haml b/app/views/split_checkout/_summary.html.haml index 692be1510d..e395bd7411 100644 --- a/app/views/split_checkout/_summary.html.haml +++ b/app/views/split_checkout/_summary.html.haml @@ -85,8 +85,12 @@ - checkout_adjustments_for(@order, exclude: [:line_item]).reverse_each do |adjustment| .summary-right-line - .summary-right-line-label= adjustment.label - .summary-right-line-value= adjustment.display_amount.to_html + -if adjustment.originator_type == 'Voucher' + .summary-right-line-label.voucher= adjustment.label + .summary-right-line-value.voucher= adjustment.display_amount.to_html + -else + .summary-right-line-label= adjustment.label + .summary-right-line-value= adjustment.display_amount.to_html - if @order.total_tax > 0 .summary-right-line diff --git a/app/webpacker/css/darkswarm/split-checkout.scss b/app/webpacker/css/darkswarm/split-checkout.scss index 49807dc610..3d9c05b929 100644 --- a/app/webpacker/css/darkswarm/split-checkout.scss +++ b/app/webpacker/css/darkswarm/split-checkout.scss @@ -332,6 +332,10 @@ padding-left: 20px; padding-right: 20px; border-right: 1px solid #DDD; + + .voucher { + color: $teal-500 + } } } } diff --git a/spec/system/consumer/split_checkout_spec.rb b/spec/system/consumer/split_checkout_spec.rb index f1b353aee9..6e0378bea1 100644 --- a/spec/system/consumer/split_checkout_spec.rb +++ b/spec/system/consumer/split_checkout_spec.rb @@ -1084,6 +1084,23 @@ describe "As a consumer, I want to checkout my order" do }.to change { order.reload.state }.from("confirmation").to("address") end end + + describe "vouchers" do + let(:voucher) { Voucher.create(code: 'some_code', enterprise: distributor) } + + before do + # Add voucher to the order + voucher.create_adjustment(voucher.code, order) + + visit checkout_step_path(:summary) + end + + it "shows the applied voucher" do + within ".summary-right" do + expect(page).to have_content "some_code" + end + end + end end context "with previous open orders" do From 74e0b0f6b5c6780b7286660689994315b8b55b0c Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 21 Mar 2023 12:27:57 +1100 Subject: [PATCH 09/39] Add voucher adjustment loading to CheckoutCallbacks This is needed so the voucher is loaded properly when the "checkout" partials are render via cable_car. --- app/controllers/concerns/checkout_callbacks.rb | 2 ++ app/controllers/split_checkout_controller.rb | 2 -- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/concerns/checkout_callbacks.rb b/app/controllers/concerns/checkout_callbacks.rb index 4b6608b947..7d0925adb9 100644 --- a/app/controllers/concerns/checkout_callbacks.rb +++ b/app/controllers/concerns/checkout_callbacks.rb @@ -30,6 +30,8 @@ module CheckoutCallbacks @order.manual_shipping_selection = true @order.checkout_processing = true + @voucher_adjustment = @order.vouchers.first + redirect_to(main_app.shop_path) && return if redirect_to_shop? redirect_to_cart_path && return unless valid_order_line_items? end diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb index e8ab1d6e7f..b00f42b980 100644 --- a/app/controllers/split_checkout_controller.rb +++ b/app/controllers/split_checkout_controller.rb @@ -22,8 +22,6 @@ class SplitCheckoutController < ::BaseController before_action :hide_ofn_navigation, only: [:edit, :update] def edit - @voucher_adjustment = @order.vouchers.first - redirect_to_step_based_on_order unless params[:step] check_step if params[:step] recalculate_tax if params[:step] == "summary" From 596c775af6e86a1d690080b7e63a757ec6c753ab Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 27 Mar 2023 14:38:40 +1100 Subject: [PATCH 10/39] Add controller spec to cover voucher in split checkout --- .../split_checkout_controller_spec.rb | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/spec/controllers/split_checkout_controller_spec.rb b/spec/controllers/split_checkout_controller_spec.rb index 7f3ad6b074..86750668d4 100644 --- a/spec/controllers/split_checkout_controller_spec.rb +++ b/spec/controllers/split_checkout_controller_spec.rb @@ -235,6 +235,65 @@ describe SplitCheckoutController, type: :controller do expect(order.payments.first.source.id).to eq saved_card.id end end + + describe "Vouchers" do + let(:voucher) { Voucher.create(code: 'some_code', enterprise: distributor) } + + describe "adding a voucher" do + let(:checkout_params) do + { + order: { + voucher_code: voucher.code + } + } + end + + it "adds a voucher to the order" do + put :update, params: params + + expect(response).to redirect_to checkout_step_path(:payment) + expect(order.reload.vouchers.length).to eq(1) + end + + context "when voucher covers more than order total" do + pending "adds the voucher" + + pending "shows a warning" + end + + context "when voucher doesn't exist" do + let(:checkout_params) do + { + order: { + voucher_code: "non_voucher" + } + } + end + + it "returns 422 and an error message" do + put :update, params: params + + expect(response.status).to eq 422 + expect(flash[:error]).to match "Voucher Not found" + end + end + + context "when adding fails" do + pending "returns 422 and an error message" + end + end + + describe "removing a voucher" do + it "removes the voucher" do + adjustment = voucher.create_adjustment(voucher.code, order) + + delete :destroy, params: { adjustment_id: adjustment.id } + + expect(response).to redirect_to checkout_step_path(:payment) + expect(order.reload.vouchers.length).to eq(0) + end + end + end end context "summary step" do From 3f609d3842bb33e006a353b604ff413a607d95ff Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 27 Mar 2023 15:12:27 +1100 Subject: [PATCH 11/39] Add error message if adding a voucher fails It's not really useful to the user but better than failing silently --- app/controllers/split_checkout_controller.rb | 9 ++++++--- config/locales/en.yml | 1 + spec/controllers/split_checkout_controller_spec.rb | 11 ++++++++++- 3 files changed, 17 insertions(+), 4 deletions(-) diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb index b00f42b980..5202673270 100644 --- a/app/controllers/split_checkout_controller.rb +++ b/app/controllers/split_checkout_controller.rb @@ -203,9 +203,12 @@ class SplitCheckoutController < ::BaseController return false end - # Create adjustment - # TODO add tax part of adjustement - voucher.create_adjustment(voucher.code, @order) + adjustment = voucher.create_adjustment(voucher.code, @order) + + if adjustment.nil? + @order.errors.add(:voucher, I18n.t('split_checkout.errors.add_voucher_error')) + return false + end true end diff --git a/config/locales/en.yml b/config/locales/en.yml index 40a5c5c043..b4913ecb9f 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -2095,6 +2095,7 @@ en: select_a_payment_method: Select a payment method no_shipping_methods_available: Checkout is not possible due to absence of shipping options. Please contact the shop owner. voucher_not_found: Not found + add_voucher_error: There was an error while adding the voucher order_paid: PAID order_not_paid: NOT PAID order_total: Total order diff --git a/spec/controllers/split_checkout_controller_spec.rb b/spec/controllers/split_checkout_controller_spec.rb index 86750668d4..e441e12854 100644 --- a/spec/controllers/split_checkout_controller_spec.rb +++ b/spec/controllers/split_checkout_controller_spec.rb @@ -279,7 +279,16 @@ describe SplitCheckoutController, type: :controller do end context "when adding fails" do - pending "returns 422 and an error message" + it "returns 422 and an error message" do + # Makes adding the voucher fails + allow(voucher).to receive(:compute_amount).and_return(0) + allow(Voucher).to receive(:find_by).and_return(voucher) + + put :update, params: params + + expect(response.status).to eq 422 + expect(flash[:error]).to match "There was an error while adding the voucher" + end end end From d157d91054b5117504a4d98b09f7fcf127a205bf Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 27 Mar 2023 16:15:35 +1100 Subject: [PATCH 12/39] When voucher cover more than order total, use only amount needed The checkout payment step will also show a warning. The amount used is stored in the adjustment created, so we will be able to track how much has been used down the line. --- app/models/voucher.rb | 12 ++++++ app/views/split_checkout/_payment.html.haml | 6 ++- config/locales/en.yml | 1 + .../split_checkout_controller_spec.rb | 6 --- spec/models/voucher_spec.rb | 19 ++++++++++ spec/system/consumer/split_checkout_spec.rb | 38 +++++++++++++++---- 6 files changed, 68 insertions(+), 14 deletions(-) diff --git a/app/models/voucher.rb b/app/models/voucher.rb index 533b7aece2..b87e59a94b 100644 --- a/app/models/voucher.rb +++ b/app/models/voucher.rb @@ -21,6 +21,18 @@ class Voucher < ApplicationRecord Spree::Money.new(value) end + # override the one from CalculatedAdjustments so we limit adjustment to the maximum amount + # needed to cover the order, ie if the voucher covers more than the order.total we only need + # to create a adjustment covering the order.total + # Doesn't work with taxes for now + def compute_amount(order) + amount = calculator.compute(order) + + return -order.total if amount.abs > order.total + + amount + end + private # For now voucher are only flat rate of 10 diff --git a/app/views/split_checkout/_payment.html.haml b/app/views/split_checkout/_payment.html.haml index df952e8895..d211686e8f 100644 --- a/app/views/split_checkout/_payment.html.haml +++ b/app/views/split_checkout/_payment.html.haml @@ -6,11 +6,15 @@ .checkout-input .two-columns-inputs.voucher - -if @voucher_adjustment.present? + - if @voucher_adjustment.present? %span.button.voucher-added %i.ofn-i_051-check-big = "#{@voucher_adjustment.originator.display_value} #{t("split_checkout.step2.voucher.voucher")}" = link_to t("split_checkout.step2.voucher.remove_code"), checkout_destroy_path(adjustment_id: @voucher_adjustment.id), method: "delete", data: { confirm: t("split_checkout.step2.voucher.confirm_delete") } + - if @voucher_adjustment.originator.value > @order.total + .checkout-input + %span.formError.standalone + = t("split_checkout.step2.voucher.warning_forfeit_remaining_amount") - else = f.text_field :voucher_code, { placeholder: t("split_checkout.step2.voucher.placeholder"), class: "voucher" } - # TODO: enable button when code entered diff --git a/config/locales/en.yml b/config/locales/en.yml index b4913ecb9f..0dcfdd7585 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -2062,6 +2062,7 @@ en: placeholder: Enter voucher code remove_code: Remove code confirm_delete: Are you sure you want to remove the voucher ? + warning_forfeit_remaining_amount: Your voucher value is more than your order. By using this voucher you are forfeiting the remaining value. step3: delivery_details: title: Delivery details diff --git a/spec/controllers/split_checkout_controller_spec.rb b/spec/controllers/split_checkout_controller_spec.rb index e441e12854..60c36e5ade 100644 --- a/spec/controllers/split_checkout_controller_spec.rb +++ b/spec/controllers/split_checkout_controller_spec.rb @@ -255,12 +255,6 @@ describe SplitCheckoutController, type: :controller do expect(order.reload.vouchers.length).to eq(1) end - context "when voucher covers more than order total" do - pending "adds the voucher" - - pending "shows a warning" - end - context "when voucher doesn't exist" do let(:checkout_params) do { diff --git a/spec/models/voucher_spec.rb b/spec/models/voucher_spec.rb index ea798d1f62..97675b0d0a 100644 --- a/spec/models/voucher_spec.rb +++ b/spec/models/voucher_spec.rb @@ -28,4 +28,23 @@ describe Voucher do expect(subject.calculator.preferred_amount.to_f).to eq(-10) end end + + describe '#compute_amount' do + subject { Voucher.create(code: 'new_code', enterprise: enterprise) } + + let(:order) { create(:order_with_totals) } + + it 'returns -10' do + expect(subject.compute_amount(order).to_f).to eq(-10) + end + + context 'when order total is smaller than 10' do + it 'returns minus the order total' do + order.total = 6 + order.save! + + expect(subject.compute_amount(order).to_f).to eq(-6) + end + end + end end diff --git a/spec/system/consumer/split_checkout_spec.rb b/spec/system/consumer/split_checkout_spec.rb index 6e0378bea1..91a85fe289 100644 --- a/spec/system/consumer/split_checkout_spec.rb +++ b/spec/system/consumer/split_checkout_spec.rb @@ -731,6 +731,37 @@ describe "As a consumer, I want to checkout my order" do end describe "adding voucher to the order" do + shared_examples "adding voucher to the order" do + before do + fill_in "Enter voucher code", with: voucher.code + click_button("Apply") + end + + it "adds a voucher to the order" do + expect(page).to have_content("$10.00 Voucher") + end + end + + it_behaves_like "adding voucher to the order" + + context "when voucher covers more then the order total" do + before do + order.total = 6 + order.save! + end + + it_behaves_like "adding voucher to the order" + + it "shows a warning message" do + fill_in "Enter voucher code", with: voucher.code + click_button("Apply") + + expect(page).to have_content( + "Your voucher value is more than your order. By using this voucher you are forfeiting the remaining value." + ) + end + end + context "voucher doesn't exist" do it "show an error" do fill_in "Enter voucher code", with: "non_code" @@ -739,13 +770,6 @@ describe "As a consumer, I want to checkout my order" do expect(page).to have_content("Voucher Not found") end end - - it "adds a voucher to the order" do - fill_in "Enter voucher code", with: voucher.code - click_button("Apply") - - expect(page).to have_content("$10.00 Voucher") - end end describe "removing voucher from order" do From 9789911523b607183cc445db8ca05fc57063e78a Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 28 Mar 2023 11:41:26 +1100 Subject: [PATCH 13/39] Rework how voucher are applied to an order and handle tax calculation At the time when we can apply a voucher to an order (payment step) we don't have any data on possible taxes and payment fees. These are calculated when we move to the summary step. So voucher are applied in two step: - create an "open" voucher adjustment on the order when a customer enters a voucher code. - when we move to the summary step, recalulate the amount and possible tax for the voucher adjustment, once taxes and payment fees have been included in the order. And then close the original and update order to reflect the changes --- app/controllers/split_checkout_controller.rb | 9 ++ app/models/voucher.rb | 90 ++++++++++- spec/models/voucher_spec.rb | 144 ++++++++++++++++++ .../consumer/split_checkout_tax_incl_spec.rb | 38 +++++ .../split_checkout_tax_not_incl_spec.rb | 41 +++++ 5 files changed, 321 insertions(+), 1 deletion(-) diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb index 5202673270..a125ca6e20 100644 --- a/app/controllers/split_checkout_controller.rb +++ b/app/controllers/split_checkout_controller.rb @@ -290,5 +290,14 @@ class SplitCheckoutController < ::BaseController def recalculate_tax @order.create_tax_charge! @order.update_order! + + apply_voucher if @order.vouchers.present? + end + + def apply_voucher + Voucher.adjust!(@order) + + # update order to take into account the voucher we applied + @order.update_order! end end diff --git a/app/models/voucher.rb b/app/models/voucher.rb index b87e59a94b..99707fd5e0 100644 --- a/app/models/voucher.rb +++ b/app/models/voucher.rb @@ -13,6 +13,73 @@ class Voucher < ApplicationRecord before_validation :add_calculator + def self.adjust!(order) + return if order.nil? + + # Find open Voucher Adjustment + return if order.vouchers.empty? + + # We only support one voucher per order right now, we could just loop on vouchers + adjustment = order.vouchers.first + + # Recalculate value + amount = adjustment.originator.compute_amount(order) + + if order.additional_tax_total.positive? + handle_tax_excluded_from_price(order, amount) + else + handle_tax_included_in_price(order, amount) + end + + # Move to closed state + adjustment.close + end + + def self.handle_tax_excluded_from_price(order, amount) + voucher_rate = amount / order.total + + # TODO: might need to use VoucherTax has originator (sub class of Voucher) + # Adding the voucher tax part + tax_amount = voucher_rate * order.additional_tax_total + + adjustment = order.vouchers.first + adjustment_attributes = { + amount: tax_amount, + originator: adjustment.originator, + order: order, + label: "Tax #{adjustment.label}", + mandatory: false, + state: 'closed', + tax_category: nil, + included_tax: 0 + } + order.adjustments.create(adjustment_attributes) + + # Update the adjustment amount + amount = voucher_rate * (order.total - order.additional_tax_total) + + adjustment.update_columns( + amount: amount, + updated_at: Time.zone.now + ) + end + + def self.handle_tax_included_in_price(order, amount) + voucher_rate = amount / order.total + included_tax = voucher_rate * order.included_tax_total + + # Update Adjustment + adjustment = order.vouchers.first + + return unless amount != adjustment.amount || included_tax != 0 + + adjustment.update_columns( + amount: amount, + included_tax: included_tax, + updated_at: Time.zone.now + ) + end + def value 10 end @@ -21,9 +88,30 @@ class Voucher < ApplicationRecord Spree::Money.new(value) end + # override the one from CalculatedAdjustments + # Create an "open" adjustment which will be updated later once tax and other fees have + # been applied to the order + def create_adjustment(label, order, mandatory = false, _state = "open", tax_category = nil) + amount = compute_amount(order) + + return if amount.zero? && !mandatory + + adjustment_attributes = { + amount: amount, + originator: self, + order: order, + label: label, + mandatory: mandatory, + state: "open", + tax_category: tax_category + } + + order.adjustments.create(adjustment_attributes) + end + # override the one from CalculatedAdjustments so we limit adjustment to the maximum amount # needed to cover the order, ie if the voucher covers more than the order.total we only need - # to create a adjustment covering the order.total + # to create an adjustment covering the order.total # Doesn't work with taxes for now def compute_amount(order) amount = calculator.compute(order) diff --git a/spec/models/voucher_spec.rb b/spec/models/voucher_spec.rb index 97675b0d0a..fed7b91cd1 100644 --- a/spec/models/voucher_spec.rb +++ b/spec/models/voucher_spec.rb @@ -29,6 +29,131 @@ describe Voucher do end end + describe '.adjust!' do + let(:voucher) { Voucher.create(code: 'new_code', enterprise: enterprise) } + + context 'when voucher covers the order total' do + subject { order.vouchers.first } + + let(:order) { create(:order_with_totals) } + + it 'updates the adjustment amount to -order.total' do + voucher.create_adjustment(voucher.code, order) + + order.total = 6 + order.save! + + Voucher.adjust!(order) + + expect(subject.amount.to_f).to eq(-6.0) + end + end + + context 'with price included in order price' do + subject { order.vouchers.first } + + let(:order) do + create( + :order_with_taxes, + distributor: enterprise, + ship_address: create(:address), + product_price: 110, + tax_rate_amount: 0.10, + included_in_price: true, + tax_rate_name: "Tax 1" + ) + end + + before do + # create adjustment before tax are set + voucher.create_adjustment(voucher.code, order) + + # Update taxes + order.create_tax_charge! + order.update_shipping_fees! + order.update_order! + + Voucher.adjust!(order) + end + + it 'updates the adjustment included_tax' do + # voucher_rate = amount / order.total + # -10 / 150 = -0.066666667 + # included_tax = voucher_rate * order.included_tax_total + # -0.66666666 * 10 = -0.67 + expect(subject.included_tax.to_f).to eq(-0.67) + end + + it 'moves the adjustment state to closed' do + expect(subject.state).to eq('closed') + end + end + + context 'with price not included in order price' do + let(:order) do + create( + :order_with_taxes, + distributor: enterprise, + ship_address: create(:address), + product_price: 110, + tax_rate_amount: 0.10, + included_in_price: false, + tax_rate_name: "Tax 1" + ) + end + + before do + # create adjustment before tax are set + voucher.create_adjustment(voucher.code, order) + + # Update taxes + order.create_tax_charge! + order.update_shipping_fees! + order.update_order! + + Voucher.adjust!(order) + end + + it 'includes amount withou tax' do + adjustment = order.vouchers.first + # voucher_rate = amount / order.total + # -10 / 161 = -0.062111801 + # amount = voucher_rate * (order.total - order.additional_tax_total) + # -0.062111801 * (161 -11) = -9.32 + expect(adjustment.amount.to_f).to eq(-9.32) + end + + it 'creates a tax adjustment' do + # voucher_rate = amount / order.total + # -10 / 161 = -0.062111801 + # amount = voucher_rate * order.additional_tax_total + # -0.0585 * 11 = -0.68 + tax_adjustment = order.vouchers.second + expect(tax_adjustment.amount.to_f).to eq(-0.68) + expect(tax_adjustment.label).to match("Tax") + end + + it 'moves the adjustment state to closed' do + adjustment = order.vouchers.first + expect(adjustment.state).to eq('closed') + end + end + + context 'when no order given' do + it "doesn't blow up" do + expect { Voucher.adjust!(nil) }.to_not raise_error + end + end + + context 'when no voucher used on the given order' do + let(:order) { create(:order_with_line_items, line_items_count: 1, distributor: enterprise) } + + it "doesn't blow up" do + expect { Voucher.adjust!(order) }.to_not raise_error + end + end + end + describe '#compute_amount' do subject { Voucher.create(code: 'new_code', enterprise: enterprise) } @@ -47,4 +172,23 @@ describe Voucher do end end end + + describe '#create_adjustment' do + subject(:adjustment) { voucher.create_adjustment(voucher.code, order) } + + let(:voucher) { Voucher.create(code: 'new_code', enterprise: enterprise) } + let(:order) { create(:order_with_line_items, line_items_count: 1, distributor: enterprise) } + + it 'includes the full voucher amount' do + expect(adjustment.amount.to_f).to eq(-10.0) + end + + it 'has no included_tax' do + expect(adjustment.included_tax.to_f).to eq(0.0) + end + + it 'sets the adjustment as open' do + expect(adjustment.state).to eq("open") + end + end end diff --git a/spec/system/consumer/split_checkout_tax_incl_spec.rb b/spec/system/consumer/split_checkout_tax_incl_spec.rb index 146104e824..916e1db80f 100644 --- a/spec/system/consumer/split_checkout_tax_incl_spec.rb +++ b/spec/system/consumer/split_checkout_tax_incl_spec.rb @@ -134,6 +134,44 @@ describe "As a consumer, I want to see adjustment breakdown" do # DB checks assert_db_tax_incl end + + context "when using a voucher" do + let!(:voucher) { Voucher.create(code: 'some_code', enterprise: distributor) } + + it "will include a tax included amount on the voucher adjustment" do + visit checkout_step_path(:details) + + choose "Delivery" + + click_button "Next - Payment method" + + # add Voucher + fill_in "Enter voucher code", with: voucher.code + click_button("Apply") + + # Choose payment ?? + click_on "Next - Order summary" + click_on "Complete order" + + # UI checks + expect(page).to have_content("Confirmed") + expect(page).to have_selector('#order_total', text: with_currency(0.00)) + expect(page).to have_selector('#tax-row', text: with_currency(1.15)) + + # Voucher + within "#line-items" do + expect(page).to have_content(voucher.code) + expect(page).to have_content(with_currency(-10.00)) + end + + # DB check + order_within_zone.reload + voucher_adjustment = order_within_zone.vouchers.first + + expect(voucher_adjustment.amount.to_f).to eq(-10) + expect(voucher_adjustment.included_tax.to_f).to eq(-1.15) + end + end 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 435d02d658..6cb7f45a9e 100644 --- a/spec/system/consumer/split_checkout_tax_not_incl_spec.rb +++ b/spec/system/consumer/split_checkout_tax_not_incl_spec.rb @@ -142,6 +142,47 @@ describe "As a consumer, I want to see adjustment breakdown" do expect(page).to have_selector('#order_total', text: with_currency(11.30)) expect(page).to have_selector('#tax-row', text: with_currency(1.30)) end + + context "when using a voucher" do + let!(:voucher) { Voucher.create(code: 'some_code', enterprise: distributor) } + + it "will include a tax included amount on the voucher adjustment" do + visit checkout_step_path(:details) + + choose "Delivery" + + click_button "Next - Payment method" + # add Voucher + fill_in "Enter voucher code", with: voucher.code + click_button("Apply") + + # Choose payment ?? + click_on "Next - Order summary" + click_on "Complete order" + + # UI checks + expect(page).to have_content("Confirmed") + expect(page).to have_selector('#order_total', text: with_currency(1.30)) + expect(page).to have_selector('#tax-row', text: with_currency(1.30)) + + # Voucher + within "#line-items" do + expect(page).to have_content(voucher.code) + expect(page).to have_content(with_currency(-8.85)) + + expect(page).to have_content("Tax #{voucher.code}") + expect(page).to have_content(with_currency(-1.15)) + end + + # DB check + order_within_zone.reload + voucher_adjustment = order_within_zone.vouchers.first + voucher_tax_adjustment = order_within_zone.vouchers.second + + expect(voucher_adjustment.amount.to_f).to eq(-8.85) + expect(voucher_tax_adjustment.amount.to_f).to eq(-1.15) + end + end end end From 5064bf5383cbc954de60f3a8e41363df6f73adaa Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 4 Apr 2023 09:53:40 +1000 Subject: [PATCH 14/39] Allow voucher adjustment to be created with an amount of 0 Although unlikely, we should still be able to create a voucher amount with. This can happen if the order.total is for instance. --- app/models/voucher.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/models/voucher.rb b/app/models/voucher.rb index 99707fd5e0..6522305857 100644 --- a/app/models/voucher.rb +++ b/app/models/voucher.rb @@ -94,8 +94,6 @@ class Voucher < ApplicationRecord def create_adjustment(label, order, mandatory = false, _state = "open", tax_category = nil) amount = compute_amount(order) - return if amount.zero? && !mandatory - adjustment_attributes = { amount: amount, originator: self, From 58469adfeb88d8901cab4ac100022dd3d9d03502 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 4 Apr 2023 11:06:32 +1000 Subject: [PATCH 15/39] Adjustments spec, add association test --- spec/models/spree/adjustment_spec.rb | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/spec/models/spree/adjustment_spec.rb b/spec/models/spree/adjustment_spec.rb index 2dde510c6e..927eb7d14c 100644 --- a/spec/models/spree/adjustment_spec.rb +++ b/spec/models/spree/adjustment_spec.rb @@ -7,6 +7,19 @@ module Spree let(:order) { build(:order) } let(:adjustment) { Spree::Adjustment.create(label: "Adjustment", amount: 5) } + describe "associations" do + it { is_expected.to have_one(:metadata) } + it { is_expected.to have_many(:adjustments) } + + it { is_expected.to belong_to(:adjustable) } + + it { is_expected.to belong_to(:adjustable) } + it { is_expected.to belong_to(:originator) } + it { is_expected.to belong_to(:order) } + it { is_expected.to belong_to(:tax_category) } + it { is_expected.to belong_to(:tax_rate) } + end + describe "scopes" do let!(:arbitrary_adjustment) { create(:adjustment, label: "Arbitrary") } let!(:return_authorization_adjustment) { From 4b5d6d7eacc13d69025f05bf70f63e18bd126cdd Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 4 Apr 2023 11:09:05 +1000 Subject: [PATCH 16/39] Fix voucher adjustments association Add missing, "inverse_of" and "dependent" options. Use :nullify as for "dependent" because we would want to keep the adjustments on order even if the voucher is deleted. --- app/models/spree/adjustment.rb | 2 ++ app/models/voucher.rb | 7 ++++++- spec/models/spree/adjustment_spec.rb | 1 + 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/app/models/spree/adjustment.rb b/app/models/spree/adjustment.rb index 06131597db..27adcbac42 100644 --- a/app/models/spree/adjustment.rb +++ b/app/models/spree/adjustment.rb @@ -44,6 +44,8 @@ module Spree belongs_to :tax_rate, -> { where spree_adjustments: { originator_type: 'Spree::TaxRate' } }, foreign_key: 'originator_id' + belongs_to :voucher, -> { where spree_adjustments: { originator_type: 'Spree::Voucher' } }, + foreign_key: 'originator_id', inverse_of: :adjustments validates :label, presence: true validates :amount, numericality: true diff --git a/app/models/voucher.rb b/app/models/voucher.rb index 6522305857..581b6ce707 100644 --- a/app/models/voucher.rb +++ b/app/models/voucher.rb @@ -7,7 +7,8 @@ class Voucher < ApplicationRecord belongs_to :enterprise - has_many :adjustments, as: :originator, class_name: 'Spree::Adjustment' + has_many :adjustments, as: :originator, class_name: 'Spree::Adjustment', inverse_of: :voucher, + dependent: :nullify validates :code, presence: true, uniqueness: { scope: :enterprise_id } @@ -91,6 +92,8 @@ class Voucher < ApplicationRecord # override the one from CalculatedAdjustments # Create an "open" adjustment which will be updated later once tax and other fees have # been applied to the order + # + # rubocop:disable Style/OptionalBooleanParameter def create_adjustment(label, order, mandatory = false, _state = "open", tax_category = nil) amount = compute_amount(order) @@ -106,11 +109,13 @@ class Voucher < ApplicationRecord order.adjustments.create(adjustment_attributes) end + # rubocop:enable Style/OptionalBooleanParameter # override the one from CalculatedAdjustments so we limit adjustment to the maximum amount # needed to cover the order, ie if the voucher covers more than the order.total we only need # to create an adjustment covering the order.total # Doesn't work with taxes for now + # TODO move this to a calculator def compute_amount(order) amount = calculator.compute(order) diff --git a/spec/models/spree/adjustment_spec.rb b/spec/models/spree/adjustment_spec.rb index 927eb7d14c..bbe7b8f245 100644 --- a/spec/models/spree/adjustment_spec.rb +++ b/spec/models/spree/adjustment_spec.rb @@ -18,6 +18,7 @@ module Spree it { is_expected.to belong_to(:order) } it { is_expected.to belong_to(:tax_category) } it { is_expected.to belong_to(:tax_rate) } + it { is_expected.to belong_to(:voucher) } end describe "scopes" do From 87cc525d2732acfbfb4f5e57f454e439f2d2c804 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 4 Apr 2023 11:24:22 +1000 Subject: [PATCH 17/39] Refactor #vouchers Use "has_many" instead of a method --- app/models/spree/order.rb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index 4a5ef3bc81..20f1b3225c 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -62,6 +62,9 @@ module Spree has_many :line_item_adjustments, through: :line_items, source: :adjustments has_many :shipment_adjustments, through: :shipments, source: :adjustments has_many :all_adjustments, class_name: 'Spree::Adjustment', dependent: :destroy + has_many :vouchers, -> { where(originator_type: 'Voucher') }, + class_name: 'Spree::Adjustment', + dependent: :destroy belongs_to :order_cycle belongs_to :distributor, class_name: 'Enterprise' @@ -618,10 +621,6 @@ module Spree end end - def vouchers - adjustments.where(originator_type: 'Voucher') - end - private def fee_handler From bb9d835bd8f049a81a233cad84eb742d4e414252 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 4 Apr 2023 11:46:42 +1000 Subject: [PATCH 18/39] Fix rubocop warning --- app/models/voucher.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/models/voucher.rb b/app/models/voucher.rb index 581b6ce707..4ce63e8657 100644 --- a/app/models/voucher.rb +++ b/app/models/voucher.rb @@ -7,7 +7,10 @@ class Voucher < ApplicationRecord belongs_to :enterprise - has_many :adjustments, as: :originator, class_name: 'Spree::Adjustment', inverse_of: :voucher, + has_many :adjustments, + as: :originator, + class_name: 'Spree::Adjustment', + inverse_of: :voucher, dependent: :nullify validates :code, presence: true, uniqueness: { scope: :enterprise_id } From a1ad25f217c934a327eaa6fcd70eb2dd9a82b10f Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 4 Apr 2023 13:36:50 +1000 Subject: [PATCH 19/39] Fix failing specs --- spec/controllers/split_checkout_controller_spec.rb | 2 +- spec/system/consumer/split_checkout_spec.rb | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/spec/controllers/split_checkout_controller_spec.rb b/spec/controllers/split_checkout_controller_spec.rb index 60c36e5ade..9c834e7123 100644 --- a/spec/controllers/split_checkout_controller_spec.rb +++ b/spec/controllers/split_checkout_controller_spec.rb @@ -275,7 +275,7 @@ describe SplitCheckoutController, type: :controller do context "when adding fails" do it "returns 422 and an error message" do # Makes adding the voucher fails - allow(voucher).to receive(:compute_amount).and_return(0) + allow(voucher).to receive(:create_adjustment).and_return(nil) allow(Voucher).to receive(:find_by).and_return(voucher) put :update, params: params diff --git a/spec/system/consumer/split_checkout_spec.rb b/spec/system/consumer/split_checkout_spec.rb index 91a85fe289..8914600d14 100644 --- a/spec/system/consumer/split_checkout_spec.rb +++ b/spec/system/consumer/split_checkout_spec.rb @@ -1115,6 +1115,8 @@ describe "As a consumer, I want to checkout my order" do before do # Add voucher to the order voucher.create_adjustment(voucher.code, order) + # Update order so voucher adjustment is properly taken into account + order.update_order! visit checkout_step_path(:summary) end From 94294fa1617faaa408ef980fe5a9ad69ac4f5759 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 4 Apr 2023 14:11:51 +1000 Subject: [PATCH 20/39] Voucher adjustment, add ordering by created_at It gives us a consistent result when calling #vouchers --- app/models/spree/order.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index 20f1b3225c..65d1b2aa44 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -62,7 +62,11 @@ module Spree has_many :line_item_adjustments, through: :line_items, source: :adjustments has_many :shipment_adjustments, through: :shipments, source: :adjustments has_many :all_adjustments, class_name: 'Spree::Adjustment', dependent: :destroy - has_many :vouchers, -> { where(originator_type: 'Voucher') }, + has_many :vouchers, + -> { + where(originator_type: 'Voucher') + .order("#{Spree::Adjustment.table_name}.created_at ASC") + }, class_name: 'Spree::Adjustment', dependent: :destroy From fe9b94a80ed2d8462865e681b2b8e512b9f5960b Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 11 Apr 2023 14:56:06 +1000 Subject: [PATCH 21/39] Fix cart disappearing bug after adding a voucher The split checkout page uses `mrujs` and `CableCar` to set the form as a remote one and perform `CableCar` operation if any : https://mrujs.com/how-tos/integrate-cablecar The previous solution broke the cart handled by angularJS, it looks like `morpdom` (https://mrujs.com/references/remote-forms-and-links Navigation Adapter section ) was doing something that angularJS didn't like when redirecting to the current page. With the power of `CableCar`, we now replace the #voucher-section on the split checkout page instead of doing a redirect. --- app/controllers/split_checkout_controller.rb | 20 ++++++++++++++--- app/views/split_checkout/_payment.html.haml | 20 +---------------- .../split_checkout/_voucher_section.html.haml | 22 +++++++++++++++++++ .../split_checkout_controller_spec.rb | 4 ++-- 4 files changed, 42 insertions(+), 24 deletions(-) create mode 100644 app/views/split_checkout/_voucher_section.html.haml diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb index a125ca6e20..9282fa9fba 100644 --- a/app/controllers/split_checkout_controller.rb +++ b/app/controllers/split_checkout_controller.rb @@ -31,7 +31,7 @@ class SplitCheckoutController < ::BaseController def update if add_voucher - return redirect_to checkout_step_path(:payment) + return render_voucher_section elsif @order.errors.present? return render_error end @@ -56,7 +56,7 @@ class SplitCheckoutController < ::BaseController adjustment = Spree::Adjustment.find_by(id: params[:adjustment_id]) adjustment.destroy - redirect_to checkout_step_path(:payment) + render_voucher_section end private @@ -72,6 +72,20 @@ class SplitCheckoutController < ::BaseController replace("#flashes", partial("shared/flashes", locals: { flashes: flash })) end + # Using the power of cable_car we replace only the #voucher_section instead of reloading the page + def render_voucher_section + render( + status: :ok, + operations: cable_car.replace( + "#voucher-section", + partial( + "split_checkout/voucher_section", + locals: { order: @order, voucher_adjustment: @order.vouchers.first } + ) + ) + ) + end + def order_error_messages # Remove ship_address.* errors if no shipping method is not selected remove_ship_address_errors if no_ship_address_needed? @@ -193,7 +207,7 @@ class SplitCheckoutController < ::BaseController end def add_voucher - return unless payment_step? && params[:order] && params[:order][:voucher_code] + return unless payment_step? && params[:order] && params[:order][:voucher_code].present? # Fetch Voucher voucher = Voucher.find_by(code: params[:order][:voucher_code], enterprise: @order.distributor) diff --git a/app/views/split_checkout/_payment.html.haml b/app/views/split_checkout/_payment.html.haml index d211686e8f..08e4a846b3 100644 --- a/app/views/split_checkout/_payment.html.haml +++ b/app/views/split_checkout/_payment.html.haml @@ -1,24 +1,6 @@ .medium-6 %div.checkout-substep{"data-controller": "paymentmethod"} - - if @order.distributor.vouchers.present? - .checkout-title - = t("split_checkout.step2.voucher.apply_voucher") - - .checkout-input - .two-columns-inputs.voucher - - if @voucher_adjustment.present? - %span.button.voucher-added - %i.ofn-i_051-check-big - = "#{@voucher_adjustment.originator.display_value} #{t("split_checkout.step2.voucher.voucher")}" - = link_to t("split_checkout.step2.voucher.remove_code"), checkout_destroy_path(adjustment_id: @voucher_adjustment.id), method: "delete", data: { confirm: t("split_checkout.step2.voucher.confirm_delete") } - - if @voucher_adjustment.originator.value > @order.total - .checkout-input - %span.formError.standalone - = t("split_checkout.step2.voucher.warning_forfeit_remaining_amount") - - else - = f.text_field :voucher_code, { placeholder: t("split_checkout.step2.voucher.placeholder"), class: "voucher" } - - # TODO: enable button when code entered - = f.submit t("split_checkout.step2.voucher.apply"), class: "button cancel voucher", disabled: false + = render "split_checkout/voucher_section", { order: @order, voucher_adjustment: @voucher_adjustment } %div.checkout-title = t("split_checkout.step2.payment_method.title") diff --git a/app/views/split_checkout/_voucher_section.html.haml b/app/views/split_checkout/_voucher_section.html.haml new file mode 100644 index 0000000000..10271cdedd --- /dev/null +++ b/app/views/split_checkout/_voucher_section.html.haml @@ -0,0 +1,22 @@ +%div#voucher-section + = form_with url: checkout_update_path(:payment), model: order, method: :put, data: { remote: "true" } do |f| + - if order.distributor.vouchers.present? + .checkout-title + = t("split_checkout.step2.voucher.apply_voucher") + + .checkout-input + .two-columns-inputs.voucher + - if voucher_adjustment.present? + %span.button.voucher-added + %i.ofn-i_051-check-big + = "#{voucher_adjustment.originator.display_value} #{t("split_checkout.step2.voucher.voucher")}" + = link_to t("split_checkout.step2.voucher.remove_code"), checkout_destroy_path(adjustment_id: voucher_adjustment.id), method: "delete", data: { confirm: t("split_checkout.step2.voucher.confirm_delete") } + - # TODO: this might not be true, ie payment method include fee which wouldn't be covered by voucher, tax implication raise total to be bigger than voucher other ? + - if voucher_adjustment.originator.value > order.total + .checkout-input + %span.formError.standalone + = t("split_checkout.step2.voucher.warning_forfeit_remaining_amount") + - else + = f.text_field :voucher_code, { placeholder: t("split_checkout.step2.voucher.placeholder"), class: "voucher" } + - # TODO: enable button when code entered + = f.submit t("split_checkout.step2.voucher.apply"), class: "button cancel voucher", disabled: false diff --git a/spec/controllers/split_checkout_controller_spec.rb b/spec/controllers/split_checkout_controller_spec.rb index 9c834e7123..0739a53537 100644 --- a/spec/controllers/split_checkout_controller_spec.rb +++ b/spec/controllers/split_checkout_controller_spec.rb @@ -251,7 +251,7 @@ describe SplitCheckoutController, type: :controller do it "adds a voucher to the order" do put :update, params: params - expect(response).to redirect_to checkout_step_path(:payment) + expect(response.status).to eq(200) expect(order.reload.vouchers.length).to eq(1) end @@ -292,7 +292,7 @@ describe SplitCheckoutController, type: :controller do delete :destroy, params: { adjustment_id: adjustment.id } - expect(response).to redirect_to checkout_step_path(:payment) + expect(response.status).to eq(200) expect(order.reload.vouchers.length).to eq(0) end end From 43ab881181f56c6fb054ee1f7dcb3522833e80e2 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Wed, 12 Apr 2023 11:18:44 +1000 Subject: [PATCH 22/39] Rename order association to voucher_adjustments The name vouchers is a bit confusing as the order is linked to a voucher adjutment and not the actual voucher --- app/controllers/concerns/checkout_callbacks.rb | 2 +- app/controllers/split_checkout_controller.rb | 4 ++-- app/models/spree/order.rb | 2 +- app/models/voucher.rb | 10 +++++----- spec/controllers/split_checkout_controller_spec.rb | 4 ++-- spec/models/spree/order_spec.rb | 6 +++--- spec/models/voucher_spec.rb | 10 +++++----- spec/system/consumer/split_checkout_spec.rb | 2 ++ spec/system/consumer/split_checkout_tax_incl_spec.rb | 2 +- .../consumer/split_checkout_tax_not_incl_spec.rb | 4 ++-- 10 files changed, 24 insertions(+), 22 deletions(-) diff --git a/app/controllers/concerns/checkout_callbacks.rb b/app/controllers/concerns/checkout_callbacks.rb index 7d0925adb9..159c6041ea 100644 --- a/app/controllers/concerns/checkout_callbacks.rb +++ b/app/controllers/concerns/checkout_callbacks.rb @@ -30,7 +30,7 @@ module CheckoutCallbacks @order.manual_shipping_selection = true @order.checkout_processing = true - @voucher_adjustment = @order.vouchers.first + @voucher_adjustment = @order.voucher_adjustments.first redirect_to(main_app.shop_path) && return if redirect_to_shop? redirect_to_cart_path && return unless valid_order_line_items? diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb index 9282fa9fba..3a28ed8217 100644 --- a/app/controllers/split_checkout_controller.rb +++ b/app/controllers/split_checkout_controller.rb @@ -80,7 +80,7 @@ class SplitCheckoutController < ::BaseController "#voucher-section", partial( "split_checkout/voucher_section", - locals: { order: @order, voucher_adjustment: @order.vouchers.first } + locals: { order: @order, voucher_adjustment: @order.voucher_adjustments.first } ) ) ) @@ -305,7 +305,7 @@ class SplitCheckoutController < ::BaseController @order.create_tax_charge! @order.update_order! - apply_voucher if @order.vouchers.present? + apply_voucher if @order.voucher_adjustments.present? end def apply_voucher diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index 65d1b2aa44..74a4f332d9 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -62,7 +62,7 @@ module Spree has_many :line_item_adjustments, through: :line_items, source: :adjustments has_many :shipment_adjustments, through: :shipments, source: :adjustments has_many :all_adjustments, class_name: 'Spree::Adjustment', dependent: :destroy - has_many :vouchers, + has_many :voucher_adjustments, -> { where(originator_type: 'Voucher') .order("#{Spree::Adjustment.table_name}.created_at ASC") diff --git a/app/models/voucher.rb b/app/models/voucher.rb index 4ce63e8657..94674cb7d3 100644 --- a/app/models/voucher.rb +++ b/app/models/voucher.rb @@ -21,10 +21,10 @@ class Voucher < ApplicationRecord return if order.nil? # Find open Voucher Adjustment - return if order.vouchers.empty? + return if order.voucher_adjustments.empty? - # We only support one voucher per order right now, we could just loop on vouchers - adjustment = order.vouchers.first + # We only support one voucher per order right now, we could just loop on voucher_adjustments + adjustment = order.voucher_adjustments.first # Recalculate value amount = adjustment.originator.compute_amount(order) @@ -46,7 +46,7 @@ class Voucher < ApplicationRecord # Adding the voucher tax part tax_amount = voucher_rate * order.additional_tax_total - adjustment = order.vouchers.first + adjustment = order.voucher_adjustments.first adjustment_attributes = { amount: tax_amount, originator: adjustment.originator, @@ -73,7 +73,7 @@ class Voucher < ApplicationRecord included_tax = voucher_rate * order.included_tax_total # Update Adjustment - adjustment = order.vouchers.first + adjustment = order.voucher_adjustments.first return unless amount != adjustment.amount || included_tax != 0 diff --git a/spec/controllers/split_checkout_controller_spec.rb b/spec/controllers/split_checkout_controller_spec.rb index 0739a53537..2af0f8de56 100644 --- a/spec/controllers/split_checkout_controller_spec.rb +++ b/spec/controllers/split_checkout_controller_spec.rb @@ -252,7 +252,7 @@ describe SplitCheckoutController, type: :controller do put :update, params: params expect(response.status).to eq(200) - expect(order.reload.vouchers.length).to eq(1) + expect(order.reload.voucher_adjustments.length).to eq(1) end context "when voucher doesn't exist" do @@ -293,7 +293,7 @@ describe SplitCheckoutController, type: :controller do delete :destroy, params: { adjustment_id: adjustment.id } expect(response.status).to eq(200) - expect(order.reload.vouchers.length).to eq(0) + expect(order.reload.voucher_adjustments.length).to eq(0) end end end diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index 7260ca431a..5b3d0ef71a 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -1431,12 +1431,12 @@ describe Spree::Order do end end - describe "#vouchers" do + describe "#voucher_adjustments" do let(:voucher) { Voucher.create(code: 'new_code', enterprise: order.distributor) } context "when no voucher adjustment" do it 'returns an empty array' do - expect(order.vouchers).to eq([]) + expect(order.voucher_adjustments).to eq([]) end end @@ -1444,7 +1444,7 @@ describe Spree::Order do order.save! expected_adjustments = Array.new(2) { voucher.create_adjustment(voucher.code, order) } - expect(order.vouchers).to eq(expected_adjustments) + expect(order.voucher_adjustments).to eq(expected_adjustments) end end end diff --git a/spec/models/voucher_spec.rb b/spec/models/voucher_spec.rb index fed7b91cd1..e01a3a0935 100644 --- a/spec/models/voucher_spec.rb +++ b/spec/models/voucher_spec.rb @@ -33,7 +33,7 @@ describe Voucher do let(:voucher) { Voucher.create(code: 'new_code', enterprise: enterprise) } context 'when voucher covers the order total' do - subject { order.vouchers.first } + subject { order.voucher_adjustments.first } let(:order) { create(:order_with_totals) } @@ -50,7 +50,7 @@ describe Voucher do end context 'with price included in order price' do - subject { order.vouchers.first } + subject { order.voucher_adjustments.first } let(:order) do create( @@ -115,7 +115,7 @@ describe Voucher do end it 'includes amount withou tax' do - adjustment = order.vouchers.first + adjustment = order.voucher_adjustments.first # voucher_rate = amount / order.total # -10 / 161 = -0.062111801 # amount = voucher_rate * (order.total - order.additional_tax_total) @@ -128,13 +128,13 @@ describe Voucher do # -10 / 161 = -0.062111801 # amount = voucher_rate * order.additional_tax_total # -0.0585 * 11 = -0.68 - tax_adjustment = order.vouchers.second + tax_adjustment = order.voucher_adjustments.second expect(tax_adjustment.amount.to_f).to eq(-0.68) expect(tax_adjustment.label).to match("Tax") end it 'moves the adjustment state to closed' do - adjustment = order.vouchers.first + adjustment = order.voucher_adjustments.first expect(adjustment.state).to eq('closed') end end diff --git a/spec/system/consumer/split_checkout_spec.rb b/spec/system/consumer/split_checkout_spec.rb index 8914600d14..336130fa0b 100644 --- a/spec/system/consumer/split_checkout_spec.rb +++ b/spec/system/consumer/split_checkout_spec.rb @@ -739,6 +739,7 @@ describe "As a consumer, I want to checkout my order" do it "adds a voucher to the order" do expect(page).to have_content("$10.00 Voucher") + expect(order.reload.voucher_adjustments.length).to eq(1) end end @@ -787,6 +788,7 @@ describe "As a consumer, I want to checkout my order" do within '.voucher' do expect(page).to have_button("Apply") end + expect(order.voucher_adjustments.length).to eq(0) end end end diff --git a/spec/system/consumer/split_checkout_tax_incl_spec.rb b/spec/system/consumer/split_checkout_tax_incl_spec.rb index 916e1db80f..5cb07f67bb 100644 --- a/spec/system/consumer/split_checkout_tax_incl_spec.rb +++ b/spec/system/consumer/split_checkout_tax_incl_spec.rb @@ -166,7 +166,7 @@ describe "As a consumer, I want to see adjustment breakdown" do # DB check order_within_zone.reload - voucher_adjustment = order_within_zone.vouchers.first + voucher_adjustment = order_within_zone.voucher_adjustments.first expect(voucher_adjustment.amount.to_f).to eq(-10) expect(voucher_adjustment.included_tax.to_f).to eq(-1.15) 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 6cb7f45a9e..4fa0f0531a 100644 --- a/spec/system/consumer/split_checkout_tax_not_incl_spec.rb +++ b/spec/system/consumer/split_checkout_tax_not_incl_spec.rb @@ -176,8 +176,8 @@ describe "As a consumer, I want to see adjustment breakdown" do # DB check order_within_zone.reload - voucher_adjustment = order_within_zone.vouchers.first - voucher_tax_adjustment = order_within_zone.vouchers.second + voucher_adjustment = order_within_zone.voucher_adjustments.first + voucher_tax_adjustment = order_within_zone.voucher_adjustments.second expect(voucher_adjustment.amount.to_f).to eq(-8.85) expect(voucher_tax_adjustment.amount.to_f).to eq(-1.15) From 3f37554ff9ab53424f734058b36ec4f872ae40f3 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Wed, 12 Apr 2023 13:52:08 +1000 Subject: [PATCH 23/39] Add VoucherAdjustmentsController and specs Refactor split checkout to use the new controller. But unfortunately we can't have nested form, so adding a voucher is still handled by SplitCheckoutController. --- app/controllers/split_checkout_controller.rb | 18 +++--- .../voucher_adjustments_controller.rb | 33 ++++++++++ app/views/split_checkout/_payment.html.haml | 2 +- ...haml => _voucher_section.cable_ready.haml} | 4 +- config/routes.rb | 4 +- spec/base_spec_helper.rb | 2 + .../split_checkout_controller_spec.rb | 16 ++--- spec/requests/voucher_adjustments_spec.rb | 61 +++++++++++++++++++ 8 files changed, 118 insertions(+), 22 deletions(-) create mode 100644 app/controllers/voucher_adjustments_controller.rb rename app/views/split_checkout/{_voucher_section.html.haml => _voucher_section.cable_ready.haml} (86%) create mode 100644 spec/requests/voucher_adjustments_spec.rb diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb index 3a28ed8217..93ad5c2684 100644 --- a/app/controllers/split_checkout_controller.rb +++ b/app/controllers/split_checkout_controller.rb @@ -31,7 +31,7 @@ class SplitCheckoutController < ::BaseController def update if add_voucher - return render_voucher_section + return render_voucher_section_or_redirect elsif @order.errors.present? return render_error end @@ -52,13 +52,6 @@ class SplitCheckoutController < ::BaseController render cable_ready: cable_car.redirect_to(url: checkout_step_path(:payment)) end - def destroy - adjustment = Spree::Adjustment.find_by(id: params[:adjustment_id]) - adjustment.destroy - - render_voucher_section - end - private def render_error @@ -72,11 +65,18 @@ class SplitCheckoutController < ::BaseController replace("#flashes", partial("shared/flashes", locals: { flashes: flash })) end + def render_voucher_section_or_redirect + respond_to do |format| + format.cable_ready { render_voucher_section } + format.html { redirect_to checkout_step_path(:payment) } + end + end + # Using the power of cable_car we replace only the #voucher_section instead of reloading the page def render_voucher_section render( status: :ok, - operations: cable_car.replace( + cable_ready: cable_car.replace( "#voucher-section", partial( "split_checkout/voucher_section", diff --git a/app/controllers/voucher_adjustments_controller.rb b/app/controllers/voucher_adjustments_controller.rb new file mode 100644 index 0000000000..e56832977c --- /dev/null +++ b/app/controllers/voucher_adjustments_controller.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +class VoucherAdjustmentsController < ::BaseController + include CablecarResponses + + def destroy + @order = current_order + + # TODO: do we need to check the user can delete voucher_adjustment + @order.voucher_adjustments.find_by(id: params[:id])&.destroy + + respond_to do |format| + format.cable_ready { render_voucher_section } + format.html { redirect_to checkout_step_path(:payment) } + end + end + + private + + # Using the power of cable_car we replace only the #voucher_section instead of reloading the page + def render_voucher_section + render( + status: :ok, + cable_ready: cable_car.replace( + "#voucher-section", + partial( + "split_checkout/voucher_section", + locals: { order: @order, voucher_adjustment: @order.voucher_adjustments.first } + ) + ) + ) + end +end diff --git a/app/views/split_checkout/_payment.html.haml b/app/views/split_checkout/_payment.html.haml index 08e4a846b3..82d3adcb3a 100644 --- a/app/views/split_checkout/_payment.html.haml +++ b/app/views/split_checkout/_payment.html.haml @@ -1,6 +1,6 @@ .medium-6 %div.checkout-substep{"data-controller": "paymentmethod"} - = render "split_checkout/voucher_section", { order: @order, voucher_adjustment: @voucher_adjustment } + = render partial: "split_checkout/voucher_section", formats: [:cable_ready], locals: { order: @order, voucher_adjustment: @voucher_adjustment } %div.checkout-title = t("split_checkout.step2.payment_method.title") diff --git a/app/views/split_checkout/_voucher_section.html.haml b/app/views/split_checkout/_voucher_section.cable_ready.haml similarity index 86% rename from app/views/split_checkout/_voucher_section.html.haml rename to app/views/split_checkout/_voucher_section.cable_ready.haml index 10271cdedd..aaaac04e3e 100644 --- a/app/views/split_checkout/_voucher_section.html.haml +++ b/app/views/split_checkout/_voucher_section.cable_ready.haml @@ -10,7 +10,7 @@ %span.button.voucher-added %i.ofn-i_051-check-big = "#{voucher_adjustment.originator.display_value} #{t("split_checkout.step2.voucher.voucher")}" - = link_to t("split_checkout.step2.voucher.remove_code"), checkout_destroy_path(adjustment_id: voucher_adjustment.id), method: "delete", data: { confirm: t("split_checkout.step2.voucher.confirm_delete") } + = link_to t("split_checkout.step2.voucher.remove_code"), voucher_adjustment_path(id: voucher_adjustment.id), method: "delete", data: { confirm: t("split_checkout.step2.voucher.confirm_delete") } - # TODO: this might not be true, ie payment method include fee which wouldn't be covered by voucher, tax implication raise total to be bigger than voucher other ? - if voucher_adjustment.originator.value > order.total .checkout-input @@ -19,4 +19,4 @@ - else = f.text_field :voucher_code, { placeholder: t("split_checkout.step2.voucher.placeholder"), class: "voucher" } - # TODO: enable button when code entered - = f.submit t("split_checkout.step2.voucher.apply"), class: "button cancel voucher", disabled: false + = f.submit t("split_checkout.step2.voucher.apply"), class: "button cancel voucher", disabled: false diff --git a/config/routes.rb b/config/routes.rb index 364c0425ed..be14086b9c 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -92,8 +92,6 @@ Openfoodnetwork::Application.routes.draw do put '/checkout/:step', to: 'split_checkout#update', as: :checkout_update end - delete '/checkout/payment', to: 'split_checkout#destroy', as: :checkout_destroy - # Redirects to the new checkout for any other 'step' (ie. /checkout/cart from the legacy checkout) get '/checkout/:other', to: redirect('/checkout') end @@ -123,6 +121,8 @@ Openfoodnetwork::Application.routes.draw do get '/:id/shop', to: 'enterprises#shop', as: 'enterprise_shop' get "/enterprises/:permalink", to: redirect("/") # Legacy enterprise URL + resources :voucher_adjustments, only: [:destroy] + get 'sitemap.xml', to: 'sitemap#index', defaults: { format: 'xml' } # Mount Spree's routes diff --git a/spec/base_spec_helper.rb b/spec/base_spec_helper.rb index 19206bd0b5..e30ddc382c 100644 --- a/spec/base_spec_helper.rb +++ b/spec/base_spec_helper.rb @@ -170,6 +170,8 @@ RSpec.configure do |config| config.include OpenFoodNetwork::ApiHelper, type: :controller config.include OpenFoodNetwork::ControllerHelper, type: :controller + config.include Devise::Test::IntegrationHelpers, type: :request + config.include Features::DatepickerHelper, type: :system config.include DownloadsHelper, type: :system end diff --git a/spec/controllers/split_checkout_controller_spec.rb b/spec/controllers/split_checkout_controller_spec.rb index 2af0f8de56..a08ca12576 100644 --- a/spec/controllers/split_checkout_controller_spec.rb +++ b/spec/controllers/split_checkout_controller_spec.rb @@ -249,6 +249,9 @@ describe SplitCheckoutController, type: :controller do end it "adds a voucher to the order" do + # Set the headers to simulate a cable_ready request + request.headers["accept"] = "text/vnd.cable-ready.json" + put :update, params: params expect(response.status).to eq(200) @@ -284,16 +287,13 @@ describe SplitCheckoutController, type: :controller do expect(flash[:error]).to match "There was an error while adding the voucher" end end - end - describe "removing a voucher" do - it "removes the voucher" do - adjustment = voucher.create_adjustment(voucher.code, order) + context "with an html request" do + it "redirects to the payment step" do + put :update, params: params - delete :destroy, params: { adjustment_id: adjustment.id } - - expect(response.status).to eq(200) - expect(order.reload.voucher_adjustments.length).to eq(0) + expect(response).to redirect_to(checkout_step_path(:payment)) + end end end end diff --git a/spec/requests/voucher_adjustments_spec.rb b/spec/requests/voucher_adjustments_spec.rb new file mode 100644 index 0000000000..056526ee67 --- /dev/null +++ b/spec/requests/voucher_adjustments_spec.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe VoucherAdjustmentsController, type: :request do + let(:user) { order.user } + let(:distributor) { create(:distributor_enterprise, with_payment_and_shipping: true) } + let(:order) { create( :order_with_line_items, line_items_count: 1, distributor: distributor) } + let(:voucher) { Voucher.create(code: 'some_code', enterprise: distributor) } + let!(:adjustment) { voucher.create_adjustment(voucher.code, order) } + + before do + Flipper.enable(:split_checkout) + + # Make sure the order is created by the order user, the factory doesn't set ip properly + order.created_by = user + order.save! + + sign_in user + end + + describe "DELETE voucher_adjustments/:id" do + let(:cable_ready_header) { { accept: "text/vnd.cable-ready.json" } } + + context "with a cable ready request" do + it "deletes the voucher adjustment" do + delete("/voucher_adjustments/#{adjustment.id}", headers: cable_ready_header) + + expect(order.voucher_adjustments.length).to eq(0) + end + + it "render a succesful response" do + delete("/voucher_adjustments/#{adjustment.id}", headers: cable_ready_header) + + expect(response).to be_successful + end + + context "when adjustment doesn't exits" do + it "does nothing" do + delete "/voucher_adjustments/-1", headers: cable_ready_header + + expect(order.voucher_adjustments.length).to eq(1) + end + + it "render a succesful response" do + delete "/voucher_adjustments/-1", headers: cable_ready_header + + expect(response).to be_successful + end + end + end + + context "with an html request" do + it "redirect to checkout payment step" do + delete "/voucher_adjustments/#{adjustment.id}" + + expect(response).to redirect_to(checkout_step_path(:payment)) + end + end + end +end From 56e981d3001dacc5ef40a440cf2c4c48bd66fd19 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Fri, 14 Apr 2023 14:46:52 +1000 Subject: [PATCH 24/39] Refactor Voucher to get rid of CalculatedAdjustments Voucher adjustment have complicated calculation that prevent us from using Spree::Calculator, and we need to override CalculatedAdjustments method, so no point using it. We still keep the same methods to stay as consitent as possible in regards to adjustments --- app/models/voucher.rb | 44 ++++++++++++------------------------- spec/models/voucher_spec.rb | 12 ---------- 2 files changed, 14 insertions(+), 42 deletions(-) diff --git a/app/models/voucher.rb b/app/models/voucher.rb index 94674cb7d3..bb65c33d21 100644 --- a/app/models/voucher.rb +++ b/app/models/voucher.rb @@ -3,8 +3,6 @@ class Voucher < ApplicationRecord acts_as_paranoid - include CalculatedAdjustments - belongs_to :enterprise has_many :adjustments, @@ -15,8 +13,6 @@ class Voucher < ApplicationRecord validates :code, presence: true, uniqueness: { scope: :enterprise_id } - before_validation :add_calculator - def self.adjust!(order) return if order.nil? @@ -29,6 +25,10 @@ class Voucher < ApplicationRecord # Recalculate value amount = adjustment.originator.compute_amount(order) + # It is quite possible to have an order with both tax included in and tax excluded from price. + # We should be able to caculate the relevant amount apply the current calculation. + # + # For now we just assume it is either all tax included in price or all tax excluded from price. if order.additional_tax_total.positive? handle_tax_excluded_from_price(order, amount) else @@ -42,7 +42,6 @@ class Voucher < ApplicationRecord def self.handle_tax_excluded_from_price(order, amount) voucher_rate = amount / order.total - # TODO: might need to use VoucherTax has originator (sub class of Voucher) # Adding the voucher tax part tax_amount = voucher_rate * order.additional_tax_total @@ -92,12 +91,12 @@ class Voucher < ApplicationRecord Spree::Money.new(value) end - # override the one from CalculatedAdjustments - # Create an "open" adjustment which will be updated later once tax and other fees have - # been applied to the order + # Ideally we would use `include CalculatedAdjustments` to be consistent with other adjustments, + # but vouchers have complicated calculation so we can't easily use Spree::Calculator. We keep + # the same method to stay as consistent as possible. # - # rubocop:disable Style/OptionalBooleanParameter - def create_adjustment(label, order, mandatory = false, _state = "open", tax_category = nil) + # Creates a new voucher adjustment for the given order + def create_adjustment(label, order) amount = compute_amount(order) adjustment_attributes = { @@ -105,32 +104,17 @@ class Voucher < ApplicationRecord originator: self, order: order, label: label, - mandatory: mandatory, + mandatory: false, state: "open", - tax_category: tax_category + tax_category: nil } order.adjustments.create(adjustment_attributes) end - # rubocop:enable Style/OptionalBooleanParameter - # override the one from CalculatedAdjustments so we limit adjustment to the maximum amount - # needed to cover the order, ie if the voucher covers more than the order.total we only need - # to create an adjustment covering the order.total - # Doesn't work with taxes for now - # TODO move this to a calculator + # We limit adjustment to the maximum amount needed to cover the order, ie if the voucher + # covers more than the order.total we only need to create an adjustment covering the order.total def compute_amount(order) - amount = calculator.compute(order) - - return -order.total if amount.abs > order.total - - amount - end - - private - - # For now voucher are only flat rate of 10 - def add_calculator - self.calculator = Calculator::FlatRate.new(preferred_amount: -value) + -value.clamp(0, order.total) end end diff --git a/spec/models/voucher_spec.rb b/spec/models/voucher_spec.rb index e01a3a0935..cbec1c7626 100644 --- a/spec/models/voucher_spec.rb +++ b/spec/models/voucher_spec.rb @@ -17,18 +17,6 @@ describe Voucher do it { is_expected.to validate_uniqueness_of(:code).scoped_to(:enterprise_id) } end - describe 'after_save' do - subject { Voucher.create(code: 'new_code', enterprise: enterprise) } - - it 'adds a FlateRate calculator' do - expect(subject.calculator.instance_of?(Calculator::FlatRate)).to be(true) - end - - it 'has a preferred_amount of -10' do - expect(subject.calculator.preferred_amount.to_f).to eq(-10) - end - end - describe '.adjust!' do let(:voucher) { Voucher.create(code: 'new_code', enterprise: enterprise) } From 17e32a89f88fc9e921df24219315e16ed3cb4178 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Fri, 14 Apr 2023 16:10:40 +1000 Subject: [PATCH 25/39] Add VoucherAdjustmentsService to handle calculation Refactor Voucher to move voucher adjustments calculation to VoucherAdjustmentsService --- app/controllers/split_checkout_controller.rb | 2 +- app/models/voucher.rb | 70 ---------- app/services/voucher_adjustments_service.rb | 75 ++++++++++ spec/models/voucher_spec.rb | 125 ----------------- .../voucher_adjustments_service_spec.rb | 131 ++++++++++++++++++ 5 files changed, 207 insertions(+), 196 deletions(-) create mode 100644 app/services/voucher_adjustments_service.rb create mode 100644 spec/services/voucher_adjustments_service_spec.rb diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb index 93ad5c2684..f2d4854182 100644 --- a/app/controllers/split_checkout_controller.rb +++ b/app/controllers/split_checkout_controller.rb @@ -309,7 +309,7 @@ class SplitCheckoutController < ::BaseController end def apply_voucher - Voucher.adjust!(@order) + VoucherAdjustmentsService.calculate(@order) # update order to take into account the voucher we applied @order.update_order! diff --git a/app/models/voucher.rb b/app/models/voucher.rb index bb65c33d21..33ef0f6075 100644 --- a/app/models/voucher.rb +++ b/app/models/voucher.rb @@ -13,76 +13,6 @@ class Voucher < ApplicationRecord validates :code, presence: true, uniqueness: { scope: :enterprise_id } - def self.adjust!(order) - return if order.nil? - - # Find open Voucher Adjustment - return if order.voucher_adjustments.empty? - - # We only support one voucher per order right now, we could just loop on voucher_adjustments - adjustment = order.voucher_adjustments.first - - # Recalculate value - amount = adjustment.originator.compute_amount(order) - - # It is quite possible to have an order with both tax included in and tax excluded from price. - # We should be able to caculate the relevant amount apply the current calculation. - # - # For now we just assume it is either all tax included in price or all tax excluded from price. - if order.additional_tax_total.positive? - handle_tax_excluded_from_price(order, amount) - else - handle_tax_included_in_price(order, amount) - end - - # Move to closed state - adjustment.close - end - - def self.handle_tax_excluded_from_price(order, amount) - voucher_rate = amount / order.total - - # Adding the voucher tax part - tax_amount = voucher_rate * order.additional_tax_total - - adjustment = order.voucher_adjustments.first - adjustment_attributes = { - amount: tax_amount, - originator: adjustment.originator, - order: order, - label: "Tax #{adjustment.label}", - mandatory: false, - state: 'closed', - tax_category: nil, - included_tax: 0 - } - order.adjustments.create(adjustment_attributes) - - # Update the adjustment amount - amount = voucher_rate * (order.total - order.additional_tax_total) - - adjustment.update_columns( - amount: amount, - updated_at: Time.zone.now - ) - end - - def self.handle_tax_included_in_price(order, amount) - voucher_rate = amount / order.total - included_tax = voucher_rate * order.included_tax_total - - # Update Adjustment - adjustment = order.voucher_adjustments.first - - return unless amount != adjustment.amount || included_tax != 0 - - adjustment.update_columns( - amount: amount, - included_tax: included_tax, - updated_at: Time.zone.now - ) - end - def value 10 end diff --git a/app/services/voucher_adjustments_service.rb b/app/services/voucher_adjustments_service.rb new file mode 100644 index 0000000000..2569010d95 --- /dev/null +++ b/app/services/voucher_adjustments_service.rb @@ -0,0 +1,75 @@ +# frozen_string_literal: true + +class VoucherAdjustmentsService + def self.calculate(order) + return if order.nil? + + # Find open Voucher Adjustment + return if order.voucher_adjustments.empty? + + # We only support one voucher per order right now, we could just loop on voucher_adjustments + adjustment = order.voucher_adjustments.first + + # Recalculate value + amount = adjustment.originator.compute_amount(order) + + # It is quite possible to have an order with both tax included in and tax excluded from price. + # We should be able to caculate the relevant amount apply the current calculation. + # + # For now we just assume it is either all tax included in price or all tax excluded from price. + if order.additional_tax_total.positive? + handle_tax_excluded_from_price(order, amount) + else + handle_tax_included_in_price(order, amount) + end + + # Move to closed state + adjustment.close + end + + def self.handle_tax_excluded_from_price(order, amount) + voucher_rate = amount / order.total + + # Adding the voucher tax part + tax_amount = voucher_rate * order.additional_tax_total + + adjustment = order.voucher_adjustments.first + adjustment_attributes = { + amount: tax_amount, + originator: adjustment.originator, + order: order, + label: "Tax #{adjustment.label}", + mandatory: false, + state: 'closed', + tax_category: nil, + included_tax: 0 + } + order.adjustments.create(adjustment_attributes) + + # Update the adjustment amount + amount = voucher_rate * (order.total - order.additional_tax_total) + + adjustment.update_columns( + amount: amount, + updated_at: Time.zone.now + ) + end + + def self.handle_tax_included_in_price(order, amount) + voucher_rate = amount / order.total + included_tax = voucher_rate * order.included_tax_total + + # Update Adjustment + adjustment = order.voucher_adjustments.first + + return unless amount != adjustment.amount || included_tax != 0 + + adjustment.update_columns( + amount: amount, + included_tax: included_tax, + updated_at: Time.zone.now + ) + end + + private_class_method :handle_tax_included_in_price, :handle_tax_excluded_from_price +end diff --git a/spec/models/voucher_spec.rb b/spec/models/voucher_spec.rb index cbec1c7626..3ef8792b1c 100644 --- a/spec/models/voucher_spec.rb +++ b/spec/models/voucher_spec.rb @@ -17,131 +17,6 @@ describe Voucher do it { is_expected.to validate_uniqueness_of(:code).scoped_to(:enterprise_id) } end - describe '.adjust!' do - let(:voucher) { Voucher.create(code: 'new_code', enterprise: enterprise) } - - context 'when voucher covers the order total' do - subject { order.voucher_adjustments.first } - - let(:order) { create(:order_with_totals) } - - it 'updates the adjustment amount to -order.total' do - voucher.create_adjustment(voucher.code, order) - - order.total = 6 - order.save! - - Voucher.adjust!(order) - - expect(subject.amount.to_f).to eq(-6.0) - end - end - - context 'with price included in order price' do - subject { order.voucher_adjustments.first } - - let(:order) do - create( - :order_with_taxes, - distributor: enterprise, - ship_address: create(:address), - product_price: 110, - tax_rate_amount: 0.10, - included_in_price: true, - tax_rate_name: "Tax 1" - ) - end - - before do - # create adjustment before tax are set - voucher.create_adjustment(voucher.code, order) - - # Update taxes - order.create_tax_charge! - order.update_shipping_fees! - order.update_order! - - Voucher.adjust!(order) - end - - it 'updates the adjustment included_tax' do - # voucher_rate = amount / order.total - # -10 / 150 = -0.066666667 - # included_tax = voucher_rate * order.included_tax_total - # -0.66666666 * 10 = -0.67 - expect(subject.included_tax.to_f).to eq(-0.67) - end - - it 'moves the adjustment state to closed' do - expect(subject.state).to eq('closed') - end - end - - context 'with price not included in order price' do - let(:order) do - create( - :order_with_taxes, - distributor: enterprise, - ship_address: create(:address), - product_price: 110, - tax_rate_amount: 0.10, - included_in_price: false, - tax_rate_name: "Tax 1" - ) - end - - before do - # create adjustment before tax are set - voucher.create_adjustment(voucher.code, order) - - # Update taxes - order.create_tax_charge! - order.update_shipping_fees! - order.update_order! - - Voucher.adjust!(order) - end - - it 'includes amount withou tax' do - adjustment = order.voucher_adjustments.first - # voucher_rate = amount / order.total - # -10 / 161 = -0.062111801 - # amount = voucher_rate * (order.total - order.additional_tax_total) - # -0.062111801 * (161 -11) = -9.32 - expect(adjustment.amount.to_f).to eq(-9.32) - end - - it 'creates a tax adjustment' do - # voucher_rate = amount / order.total - # -10 / 161 = -0.062111801 - # amount = voucher_rate * order.additional_tax_total - # -0.0585 * 11 = -0.68 - tax_adjustment = order.voucher_adjustments.second - expect(tax_adjustment.amount.to_f).to eq(-0.68) - expect(tax_adjustment.label).to match("Tax") - end - - it 'moves the adjustment state to closed' do - adjustment = order.voucher_adjustments.first - expect(adjustment.state).to eq('closed') - end - end - - context 'when no order given' do - it "doesn't blow up" do - expect { Voucher.adjust!(nil) }.to_not raise_error - end - end - - context 'when no voucher used on the given order' do - let(:order) { create(:order_with_line_items, line_items_count: 1, distributor: enterprise) } - - it "doesn't blow up" do - expect { Voucher.adjust!(order) }.to_not raise_error - end - end - end - describe '#compute_amount' do subject { Voucher.create(code: 'new_code', enterprise: enterprise) } diff --git a/spec/services/voucher_adjustments_service_spec.rb b/spec/services/voucher_adjustments_service_spec.rb new file mode 100644 index 0000000000..d2b95cdb78 --- /dev/null +++ b/spec/services/voucher_adjustments_service_spec.rb @@ -0,0 +1,131 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe VoucherAdjustmentsService do + describe '.calculate' do + let(:enterprise) { build(:enterprise) } + let(:voucher) { Voucher.create(code: 'new_code', enterprise: enterprise) } + + context 'when voucher covers the order total' do + subject { order.voucher_adjustments.first } + + let(:order) { create(:order_with_totals) } + + it 'updates the adjustment amount to -order.total' do + voucher.create_adjustment(voucher.code, order) + + order.total = 6 + order.save! + + VoucherAdjustmentsService.calculate(order) + + expect(subject.amount.to_f).to eq(-6.0) + end + end + + context 'with price included in order price' do + subject { order.voucher_adjustments.first } + + let(:order) do + create( + :order_with_taxes, + distributor: enterprise, + ship_address: create(:address), + product_price: 110, + tax_rate_amount: 0.10, + included_in_price: true, + tax_rate_name: "Tax 1" + ) + end + + before do + # create adjustment before tax are set + voucher.create_adjustment(voucher.code, order) + + # Update taxes + order.create_tax_charge! + order.update_shipping_fees! + order.update_order! + + VoucherAdjustmentsService.calculate(order) + end + + it 'updates the adjustment included_tax' do + # voucher_rate = amount / order.total + # -10 / 150 = -0.066666667 + # included_tax = voucher_rate * order.included_tax_total + # -0.66666666 * 10 = -0.67 + expect(subject.included_tax.to_f).to eq(-0.67) + end + + it 'moves the adjustment state to closed' do + expect(subject.state).to eq('closed') + end + end + + context 'with price not included in order price' do + let(:order) do + create( + :order_with_taxes, + distributor: enterprise, + ship_address: create(:address), + product_price: 110, + tax_rate_amount: 0.10, + included_in_price: false, + tax_rate_name: "Tax 1" + ) + end + + before do + # create adjustment before tax are set + voucher.create_adjustment(voucher.code, order) + + # Update taxes + order.create_tax_charge! + order.update_shipping_fees! + order.update_order! + + VoucherAdjustmentsService.calculate(order) + end + + it 'includes amount withou tax' do + adjustment = order.voucher_adjustments.first + # voucher_rate = amount / order.total + # -10 / 161 = -0.062111801 + # amount = voucher_rate * (order.total - order.additional_tax_total) + # -0.062111801 * (161 -11) = -9.32 + expect(adjustment.amount.to_f).to eq(-9.32) + end + + it 'creates a tax adjustment' do + # voucher_rate = amount / order.total + # -10 / 161 = -0.062111801 + # amount = voucher_rate * order.additional_tax_total + # -0.0585 * 11 = -0.68 + tax_adjustment = order.voucher_adjustments.second + expect(tax_adjustment.amount.to_f).to eq(-0.68) + expect(tax_adjustment.label).to match("Tax") + end + + it 'moves the adjustment state to closed' do + adjustment = order.voucher_adjustments.first + expect(adjustment.state).to eq('closed') + end + end + + context 'when no order given' do + it "doesn't blow up" do + expect { VoucherAdjustmentsService.calculate(nil) }.to_not raise_error + end + end + + context 'when no voucher used on the given order' do + let(:order) { create(:order_with_line_items, line_items_count: 1, distributor: enterprise) } + + it "doesn't blow up" do + expect { VoucherAdjustmentsService.calculate(order) }.to_not raise_error + end + end + end +end From 2f1fe6d096b6d1f87f9bdf7bc632be269b7dcf5e Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 17 Apr 2023 11:43:24 +1000 Subject: [PATCH 26/39] Fix rubocop warning --- spec/system/consumer/split_checkout_spec.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/spec/system/consumer/split_checkout_spec.rb b/spec/system/consumer/split_checkout_spec.rb index 336130fa0b..d487365786 100644 --- a/spec/system/consumer/split_checkout_spec.rb +++ b/spec/system/consumer/split_checkout_spec.rb @@ -33,7 +33,7 @@ describe "As a consumer, I want to checkout my order" do let(:enterprise_fee) { create(:enterprise_fee, amount: 1.23, tax_category: fee_tax_category) } let(:free_shipping_with_required_address) { - create(:shipping_method, require_ship_address: true, + create(:shipping_method, require_ship_address: true, name: "A Free Shipping with required address") } let(:free_shipping) { @@ -758,7 +758,8 @@ describe "As a consumer, I want to checkout my order" do click_button("Apply") expect(page).to have_content( - "Your voucher value is more than your order. By using this voucher you are forfeiting the remaining value." + "Your voucher value is more than your order. " \ + "By using this voucher you are forfeiting the remaining value." ) end end From a2e1e6ca33fabc6abede60dea0e1d080efe03732 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 17 Apr 2023 11:45:10 +1000 Subject: [PATCH 27/39] Review comment, use dig to access voucher_code param --- app/controllers/split_checkout_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb index f2d4854182..3c3cf45a59 100644 --- a/app/controllers/split_checkout_controller.rb +++ b/app/controllers/split_checkout_controller.rb @@ -207,7 +207,7 @@ class SplitCheckoutController < ::BaseController end def add_voucher - return unless payment_step? && params[:order] && params[:order][:voucher_code].present? + return unless payment_step? && params.dig(:order, :voucher_code).present? # Fetch Voucher voucher = Voucher.find_by(code: params[:order][:voucher_code], enterprise: @order.distributor) From 815dcbcefe9d9221d25b0b66229a603672c0e1cc Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 17 Apr 2023 12:06:02 +1000 Subject: [PATCH 28/39] Fix translations --- app/views/split_checkout/_voucher_section.cable_ready.haml | 2 +- config/locales/en.yml | 4 ++-- spec/system/consumer/split_checkout_spec.rb | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/views/split_checkout/_voucher_section.cable_ready.haml b/app/views/split_checkout/_voucher_section.cable_ready.haml index aaaac04e3e..a989f14af5 100644 --- a/app/views/split_checkout/_voucher_section.cable_ready.haml +++ b/app/views/split_checkout/_voucher_section.cable_ready.haml @@ -9,7 +9,7 @@ - if voucher_adjustment.present? %span.button.voucher-added %i.ofn-i_051-check-big - = "#{voucher_adjustment.originator.display_value} #{t("split_checkout.step2.voucher.voucher")}" + = t("split_checkout.step2.voucher.voucher", voucher_amount: voucher_adjustment.originator.display_value) = link_to t("split_checkout.step2.voucher.remove_code"), voucher_adjustment_path(id: voucher_adjustment.id), method: "delete", data: { confirm: t("split_checkout.step2.voucher.confirm_delete") } - # TODO: this might not be true, ie payment method include fee which wouldn't be covered by voucher, tax implication raise total to be bigger than voucher other ? - if voucher_adjustment.originator.value > order.total diff --git a/config/locales/en.yml b/config/locales/en.yml index 0dcfdd7585..889ffa5836 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -2056,12 +2056,12 @@ en: submit: Next - Order summary cancel: Back to Your details voucher: - voucher: Voucher + voucher: "%{voucher_amount} Voucher" apply_voucher: Apply voucher apply: Apply placeholder: Enter voucher code remove_code: Remove code - confirm_delete: Are you sure you want to remove the voucher ? + confirm_delete: Are you sure you want to remove the voucher? warning_forfeit_remaining_amount: Your voucher value is more than your order. By using this voucher you are forfeiting the remaining value. step3: delivery_details: diff --git a/spec/system/consumer/split_checkout_spec.rb b/spec/system/consumer/split_checkout_spec.rb index d487365786..420389b087 100644 --- a/spec/system/consumer/split_checkout_spec.rb +++ b/spec/system/consumer/split_checkout_spec.rb @@ -782,7 +782,7 @@ describe "As a consumer, I want to checkout my order" do end it "removes voucher" do - accept_confirm "Are you sure you want to remove the voucher ?" do + accept_confirm "Are you sure you want to remove the voucher?" do click_on "Remove code" end From e5f14177d33e71d8f23602788a6fe3b2d5d7fd5f Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Fri, 21 Apr 2023 16:12:43 +1000 Subject: [PATCH 29/39] Fix rubocop warning I guess the new cops are effective :) --- 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 e56832977c..1eb59e3a03 100644 --- a/app/controllers/voucher_adjustments_controller.rb +++ b/app/controllers/voucher_adjustments_controller.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -class VoucherAdjustmentsController < ::BaseController +class VoucherAdjustmentsController < BaseController include CablecarResponses def destroy From aa526a639c77b96f641ea978eea6dc1802c9338c Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 1 May 2023 16:28:07 +1000 Subject: [PATCH 30/39] Checkout payment page, enable voucher "apply" button when code entered The "apply" button is disabled by default. If left enabled, a customer could try to apply an empty voucher, which results in system trying to move to the order summary step, an unexpected behaviour! We only enable the button when something is entered in the input. --- .../_voucher_section.cable_ready.haml | 7 +- .../toggle_button_disabled_controller.js | 22 +++++ .../toggle_button_disabled_controller_test.js | 82 +++++++++++++++++++ spec/system/consumer/split_checkout_spec.rb | 4 +- 4 files changed, 109 insertions(+), 6 deletions(-) create mode 100644 app/webpacker/controllers/toggle_button_disabled_controller.js create mode 100644 spec/javascripts/stimulus/toggle_button_disabled_controller_test.js diff --git a/app/views/split_checkout/_voucher_section.cable_ready.haml b/app/views/split_checkout/_voucher_section.cable_ready.haml index a989f14af5..cb0374f33c 100644 --- a/app/views/split_checkout/_voucher_section.cable_ready.haml +++ b/app/views/split_checkout/_voucher_section.cable_ready.haml @@ -5,7 +5,7 @@ = t("split_checkout.step2.voucher.apply_voucher") .checkout-input - .two-columns-inputs.voucher + .two-columns-inputs.voucher{"data-controller": "toggle-button-disabled"} - if voucher_adjustment.present? %span.button.voucher-added %i.ofn-i_051-check-big @@ -17,6 +17,5 @@ %span.formError.standalone = t("split_checkout.step2.voucher.warning_forfeit_remaining_amount") - else - = f.text_field :voucher_code, { placeholder: t("split_checkout.step2.voucher.placeholder"), class: "voucher" } - - # TODO: enable button when code entered - = f.submit t("split_checkout.step2.voucher.apply"), class: "button cancel voucher", disabled: false + = f.text_field :voucher_code, data: { action: "input->toggle-button-disabled#inputIsChanged", }, placeholder: t("split_checkout.step2.voucher.placeholder") , class: "voucher" + = f.submit t("split_checkout.step2.voucher.apply"), disabled: true, class: "button cancel voucher", data: { "toggle-button-disabled-target": "button" } diff --git a/app/webpacker/controllers/toggle_button_disabled_controller.js b/app/webpacker/controllers/toggle_button_disabled_controller.js new file mode 100644 index 0000000000..6b407d9809 --- /dev/null +++ b/app/webpacker/controllers/toggle_button_disabled_controller.js @@ -0,0 +1,22 @@ +import { Controller } from "stimulus"; + +export default class extends Controller { + static targets = ["button"]; + + connect() { + // Hacky way to go arount mrjus automatically enabling/disabling form element + setTimeout(() => { + if (this.hasButtonTarget) { + this.buttonTarget.disabled = true; + } + }, 100); + } + + inputIsChanged(e) { + if (e.target.value !== "") { + this.buttonTarget.disabled = false; + } else { + this.buttonTarget.disabled = true; + } + } +} diff --git a/spec/javascripts/stimulus/toggle_button_disabled_controller_test.js b/spec/javascripts/stimulus/toggle_button_disabled_controller_test.js new file mode 100644 index 0000000000..af25010124 --- /dev/null +++ b/spec/javascripts/stimulus/toggle_button_disabled_controller_test.js @@ -0,0 +1,82 @@ +/** + * @jest-environment jsdom + */ + +import { Application } from "stimulus" +import toggle_button_disabled_controller from "../../../app/webpacker/controllers/toggle_button_disabled_controller" + +describe("ButtonEnableToggleController", () => { + beforeAll(() => { + const application = Application.start() + application.register("toggle-button-disabled", toggle_button_disabled_controller) + jest.useFakeTimers() + }) + + beforeEach(() => { + document.body.innerHTML = ` +
+ + +
+ ` + }) + + describe("#connect", () => { + it("disables the target submit button", () => { + jest.runAllTimers(); + + const submit = document.getElementById("test-submit") + expect(submit.disabled).toBe(true) + }) + + describe("when no button present", () => { + beforeEach(() => { + document.body.innerHTML = ` +
+ +
+ ` + }) + + // I am not sure if it's possible to manually trigger the loading/connect of the controller to + // try catch the error, so leaving as this. It will break if the missing target isn't handled + // properly + it("doesn't break", () => { + jest.runAllTimers() + }) + }) + }) + + describe("#formIsChanged", () => { + let input + let submit + + beforeEach(() => { + jest.runAllTimers() + input = document.getElementById("test-input") + submit = document.getElementById("test-submit") + }) + + describe("when the input value is not empty", () => { + it("enables the target button", () => { + input.value = "test" + input.dispatchEvent(new Event("input")); + + expect(submit.disabled).toBe(false) + }) + }) + + describe("when the input value is empty", () => { + it("disables the target button", () => { + // setting up state where target button is enabled + input.value = "test" + input.dispatchEvent(new Event("input")); + + input.value = "" + input.dispatchEvent(new Event("input")); + + expect(submit.disabled).toBe(true) + }) + }) + }) +}) diff --git a/spec/system/consumer/split_checkout_spec.rb b/spec/system/consumer/split_checkout_spec.rb index 420389b087..09965f0484 100644 --- a/spec/system/consumer/split_checkout_spec.rb +++ b/spec/system/consumer/split_checkout_spec.rb @@ -787,7 +787,7 @@ describe "As a consumer, I want to checkout my order" do end within '.voucher' do - expect(page).to have_button("Apply") + expect(page).to have_button("Apply", disabled: true) end expect(order.voucher_adjustments.length).to eq(0) end @@ -1024,7 +1024,7 @@ describe "As a consumer, I want to checkout my order" do end context "when the terms have been accepted in the past" do - + context "with a dedicated ToS file" do before do From 236f6926d9ce6b126a825237ba02e88c1fbd82e8 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 1 May 2023 16:32:42 +1000 Subject: [PATCH 31/39] Cleaning up left over TODOs --- app/controllers/voucher_adjustments_controller.rb | 1 - app/views/split_checkout/_voucher_section.cable_ready.haml | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/app/controllers/voucher_adjustments_controller.rb b/app/controllers/voucher_adjustments_controller.rb index 1eb59e3a03..b45b60fd9b 100644 --- a/app/controllers/voucher_adjustments_controller.rb +++ b/app/controllers/voucher_adjustments_controller.rb @@ -6,7 +6,6 @@ class VoucherAdjustmentsController < BaseController def destroy @order = current_order - # TODO: do we need to check the user can delete voucher_adjustment @order.voucher_adjustments.find_by(id: params[:id])&.destroy respond_to do |format| diff --git a/app/views/split_checkout/_voucher_section.cable_ready.haml b/app/views/split_checkout/_voucher_section.cable_ready.haml index cb0374f33c..26734410b5 100644 --- a/app/views/split_checkout/_voucher_section.cable_ready.haml +++ b/app/views/split_checkout/_voucher_section.cable_ready.haml @@ -11,7 +11,7 @@ %i.ofn-i_051-check-big = t("split_checkout.step2.voucher.voucher", voucher_amount: voucher_adjustment.originator.display_value) = link_to t("split_checkout.step2.voucher.remove_code"), voucher_adjustment_path(id: voucher_adjustment.id), method: "delete", data: { confirm: t("split_checkout.step2.voucher.confirm_delete") } - - # TODO: this might not be true, ie payment method include fee which wouldn't be covered by voucher, tax implication raise total to be bigger than voucher other ? + - # This might not be true, ie payment method including a fee which wouldn't be covered by voucher or tax implication raising total to be bigger than the voucher amount ? - if voucher_adjustment.originator.value > order.total .checkout-input %span.formError.standalone From 0a249d7722ab545e0a99cad614eba6679960ef1d Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 2 May 2023 13:25:59 +1000 Subject: [PATCH 32/39] Fix ButtonEnableToggleController to remove hacky button disable As per review comment, use data-disable-with="false" do prevent Rails from automatically enabling the "Apply" button. We can then remove the timeout hack. --- .../_voucher_section.cable_ready.haml | 2 +- .../toggle_button_disabled_controller.js | 14 ++++++++------ .../toggle_button_disabled_controller_test.js | 15 +++++++-------- 3 files changed, 16 insertions(+), 15 deletions(-) diff --git a/app/views/split_checkout/_voucher_section.cable_ready.haml b/app/views/split_checkout/_voucher_section.cable_ready.haml index 26734410b5..08941d4fb0 100644 --- a/app/views/split_checkout/_voucher_section.cable_ready.haml +++ b/app/views/split_checkout/_voucher_section.cable_ready.haml @@ -18,4 +18,4 @@ = t("split_checkout.step2.voucher.warning_forfeit_remaining_amount") - else = f.text_field :voucher_code, data: { action: "input->toggle-button-disabled#inputIsChanged", }, placeholder: t("split_checkout.step2.voucher.placeholder") , class: "voucher" - = f.submit t("split_checkout.step2.voucher.apply"), disabled: true, class: "button cancel voucher", data: { "toggle-button-disabled-target": "button" } + = f.submit t("split_checkout.step2.voucher.apply"), disabled: true, class: "button cancel voucher", "data-disable-with": false, data: { "toggle-button-disabled-target": "button" } diff --git a/app/webpacker/controllers/toggle_button_disabled_controller.js b/app/webpacker/controllers/toggle_button_disabled_controller.js index 6b407d9809..7ef9233217 100644 --- a/app/webpacker/controllers/toggle_button_disabled_controller.js +++ b/app/webpacker/controllers/toggle_button_disabled_controller.js @@ -1,15 +1,17 @@ import { Controller } from "stimulus"; +// Since Rails 7 it adds "data-disabled-with" property to submit, you'll need to add +// 'data-disable-with="false' for this to function as expected, ie: +// +// +// export default class extends Controller { static targets = ["button"]; connect() { - // Hacky way to go arount mrjus automatically enabling/disabling form element - setTimeout(() => { - if (this.hasButtonTarget) { - this.buttonTarget.disabled = true; - } - }, 100); + if (this.hasButtonTarget) { + this.buttonTarget.disabled = true; + } } inputIsChanged(e) { diff --git a/spec/javascripts/stimulus/toggle_button_disabled_controller_test.js b/spec/javascripts/stimulus/toggle_button_disabled_controller_test.js index af25010124..2c367fdb11 100644 --- a/spec/javascripts/stimulus/toggle_button_disabled_controller_test.js +++ b/spec/javascripts/stimulus/toggle_button_disabled_controller_test.js @@ -9,22 +9,24 @@ describe("ButtonEnableToggleController", () => { beforeAll(() => { const application = Application.start() application.register("toggle-button-disabled", toggle_button_disabled_controller) - jest.useFakeTimers() }) beforeEach(() => { document.body.innerHTML = `
- +
` }) describe("#connect", () => { it("disables the target submit button", () => { - jest.runAllTimers(); - const submit = document.getElementById("test-submit") expect(submit.disabled).toBe(true) }) @@ -41,9 +43,7 @@ describe("ButtonEnableToggleController", () => { // I am not sure if it's possible to manually trigger the loading/connect of the controller to // try catch the error, so leaving as this. It will break if the missing target isn't handled // properly - it("doesn't break", () => { - jest.runAllTimers() - }) + it("doesn't break", () => {}) }) }) @@ -52,7 +52,6 @@ describe("ButtonEnableToggleController", () => { let submit beforeEach(() => { - jest.runAllTimers() input = document.getElementById("test-input") submit = document.getElementById("test-submit") }) From 92bcd937dc248eee3a0d2d334a0e5dd4f614c952 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 2 May 2023 13:40:31 +1000 Subject: [PATCH 33/39] Per review comment, remove form object from the partial This partial is rendered inside another
element, nested form don't work. --- .../_voucher_section.cable_ready.haml | 38 +++++++++---------- 1 file changed, 18 insertions(+), 20 deletions(-) diff --git a/app/views/split_checkout/_voucher_section.cable_ready.haml b/app/views/split_checkout/_voucher_section.cable_ready.haml index 08941d4fb0..7708716eda 100644 --- a/app/views/split_checkout/_voucher_section.cable_ready.haml +++ b/app/views/split_checkout/_voucher_section.cable_ready.haml @@ -1,21 +1,19 @@ %div#voucher-section - = form_with url: checkout_update_path(:payment), model: order, method: :put, data: { remote: "true" } do |f| - - if order.distributor.vouchers.present? - .checkout-title - = t("split_checkout.step2.voucher.apply_voucher") - - .checkout-input - .two-columns-inputs.voucher{"data-controller": "toggle-button-disabled"} - - if voucher_adjustment.present? - %span.button.voucher-added - %i.ofn-i_051-check-big - = t("split_checkout.step2.voucher.voucher", voucher_amount: voucher_adjustment.originator.display_value) - = link_to t("split_checkout.step2.voucher.remove_code"), voucher_adjustment_path(id: voucher_adjustment.id), method: "delete", data: { confirm: t("split_checkout.step2.voucher.confirm_delete") } - - # This might not be true, ie payment method including a fee which wouldn't be covered by voucher or tax implication raising total to be bigger than the voucher amount ? - - if voucher_adjustment.originator.value > order.total - .checkout-input - %span.formError.standalone - = t("split_checkout.step2.voucher.warning_forfeit_remaining_amount") - - else - = f.text_field :voucher_code, data: { action: "input->toggle-button-disabled#inputIsChanged", }, placeholder: t("split_checkout.step2.voucher.placeholder") , class: "voucher" - = f.submit t("split_checkout.step2.voucher.apply"), disabled: true, class: "button cancel voucher", "data-disable-with": false, data: { "toggle-button-disabled-target": "button" } + - if order.distributor.vouchers.present? + .checkout-title + = t("split_checkout.step2.voucher.apply_voucher") + .checkout-input + .two-columns-inputs.voucher{"data-controller": "toggle-button-disabled"} + - if voucher_adjustment.present? + %span.button.voucher-added + %i.ofn-i_051-check-big + = t("split_checkout.step2.voucher.voucher", voucher_amount: voucher_adjustment.originator.display_value) + = link_to t("split_checkout.step2.voucher.remove_code"), voucher_adjustment_path(id: voucher_adjustment.id), method: "delete", data: { confirm: t("split_checkout.step2.voucher.confirm_delete") } + - # This might not be true, ie payment method including a fee which wouldn't be covered by voucher or tax implication raising total to be bigger than the voucher amount ? + - if voucher_adjustment.originator.value > order.total + .checkout-input + %span.formError.standalone + = t("split_checkout.step2.voucher.warning_forfeit_remaining_amount") + - else + = text_field_tag "[order][voucher_code]", params.dig(:order, :voucher_code), data: { action: "input->toggle-button-disabled#inputIsChanged", }, placeholder: t("split_checkout.step2.voucher.placeholder") , class: "voucher" + = submit_tag t("split_checkout.step2.voucher.apply"), disabled: true, class: "button cancel voucher", "data-disable-with": false, data: { "toggle-button-disabled-target": "button" } From b80274f49d9844ffca0b56024d9993f26e571343 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 2 May 2023 14:12:31 +1000 Subject: [PATCH 34/39] Per review comment, Use named value on voucher submit button to distinguish between submission types The voucher apply button is inside form that has another a submit button, it leads to a weird situation where either one will submit the whole payments page form. Adding a named parameter on the voucher apply button means we can distinguish between the two by checking for the presence of params[:apply_voucher]. --- app/controllers/split_checkout_controller.rb | 19 +++++++++++++------ .../_voucher_section.cable_ready.haml | 2 +- .../split_checkout_controller_spec.rb | 2 ++ 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb index 3c3cf45a59..4a4d1f0aca 100644 --- a/app/controllers/split_checkout_controller.rb +++ b/app/controllers/split_checkout_controller.rb @@ -30,11 +30,7 @@ class SplitCheckoutController < ::BaseController end def update - if add_voucher - return render_voucher_section_or_redirect - elsif @order.errors.present? - return render_error - end + return process_voucher if params[:apply_voucher].present? if confirm_order || update_order return if performed? @@ -206,8 +202,19 @@ class SplitCheckoutController < ::BaseController selected_shipping_method.first.require_ship_address == false end + def process_voucher + if add_voucher + render_voucher_section_or_redirect + elsif @order.errors.present? + render_error + end + end + def add_voucher - return unless payment_step? && params.dig(:order, :voucher_code).present? + if params.dig(:order, :voucher_code).blank? + @order.errors.add(:voucher, I18n.t('split_checkout.errors.voucher_not_found')) + return false + end # Fetch Voucher voucher = Voucher.find_by(code: params[:order][:voucher_code], enterprise: @order.distributor) diff --git a/app/views/split_checkout/_voucher_section.cable_ready.haml b/app/views/split_checkout/_voucher_section.cable_ready.haml index 7708716eda..786eff7288 100644 --- a/app/views/split_checkout/_voucher_section.cable_ready.haml +++ b/app/views/split_checkout/_voucher_section.cable_ready.haml @@ -16,4 +16,4 @@ = t("split_checkout.step2.voucher.warning_forfeit_remaining_amount") - else = text_field_tag "[order][voucher_code]", params.dig(:order, :voucher_code), data: { action: "input->toggle-button-disabled#inputIsChanged", }, placeholder: t("split_checkout.step2.voucher.placeholder") , class: "voucher" - = submit_tag t("split_checkout.step2.voucher.apply"), disabled: true, class: "button cancel voucher", "data-disable-with": false, data: { "toggle-button-disabled-target": "button" } + = submit_tag t("split_checkout.step2.voucher.apply"), name: "apply_voucher", disabled: true, class: "button cancel voucher", "data-disable-with": false, data: { "toggle-button-disabled-target": "button" } diff --git a/spec/controllers/split_checkout_controller_spec.rb b/spec/controllers/split_checkout_controller_spec.rb index a08ca12576..db3571d81b 100644 --- a/spec/controllers/split_checkout_controller_spec.rb +++ b/spec/controllers/split_checkout_controller_spec.rb @@ -242,6 +242,7 @@ describe SplitCheckoutController, type: :controller do describe "adding a voucher" do let(:checkout_params) do { + apply_voucher: "true", order: { voucher_code: voucher.code } @@ -261,6 +262,7 @@ describe SplitCheckoutController, type: :controller do context "when voucher doesn't exist" do let(:checkout_params) do { + apply_voucher: "true", order: { voucher_code: "non_voucher" } From 9b1725d39fe88f6d6c3ebad13a5d95c35fea1136 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 1 May 2023 15:50:38 +0100 Subject: [PATCH 35/39] Fix model name in adjustment association --- app/models/spree/adjustment.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/spree/adjustment.rb b/app/models/spree/adjustment.rb index 27adcbac42..e0a4c1c5e5 100644 --- a/app/models/spree/adjustment.rb +++ b/app/models/spree/adjustment.rb @@ -44,7 +44,7 @@ module Spree belongs_to :tax_rate, -> { where spree_adjustments: { originator_type: 'Spree::TaxRate' } }, foreign_key: 'originator_id' - belongs_to :voucher, -> { where spree_adjustments: { originator_type: 'Spree::Voucher' } }, + belongs_to :voucher, -> { where spree_adjustments: { originator_type: 'Voucher' } }, foreign_key: 'originator_id', inverse_of: :adjustments validates :label, presence: true From b67f5ae1540b326d6e7e106a5c56c0e02d5b908d Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 2 May 2023 14:46:21 +1000 Subject: [PATCH 36/39] Fixing Rubocop issue My editor automatically remove blank character on empty line, that's why rubocop got grumpy here. --- spec/system/consumer/split_checkout_spec.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/spec/system/consumer/split_checkout_spec.rb b/spec/system/consumer/split_checkout_spec.rb index 09965f0484..f0e5fb6fc6 100644 --- a/spec/system/consumer/split_checkout_spec.rb +++ b/spec/system/consumer/split_checkout_spec.rb @@ -1024,8 +1024,6 @@ describe "As a consumer, I want to checkout my order" do end context "when the terms have been accepted in the past" do - - context "with a dedicated ToS file" do before do TermsOfServiceFile.create!( From d29119f5c57d9505a76a58702731ef26a441ff85 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Fri, 5 May 2023 13:12:59 +1000 Subject: [PATCH 37/39] Remove non need belongs_to associations from Adjustments It turns out the "tax_rate" association isn't used and wasn't working. Same for the "voucher" one, which I added to be consistent with existing code. Both of these weren't caught by the specs because you can't test associations with a custome relation with 'shouda-matchers' see: https://github.com/thoughtbot/shoulda-matchers/issues/981 --- app/models/spree/adjustment.rb | 5 ----- app/models/voucher.rb | 1 - spec/models/spree/adjustment_spec.rb | 4 ---- 3 files changed, 10 deletions(-) diff --git a/app/models/spree/adjustment.rb b/app/models/spree/adjustment.rb index e0a4c1c5e5..57ef3d549c 100644 --- a/app/models/spree/adjustment.rb +++ b/app/models/spree/adjustment.rb @@ -42,11 +42,6 @@ module Spree belongs_to :order, class_name: "Spree::Order" belongs_to :tax_category, class_name: 'Spree::TaxCategory' - belongs_to :tax_rate, -> { where spree_adjustments: { originator_type: 'Spree::TaxRate' } }, - foreign_key: 'originator_id' - belongs_to :voucher, -> { where spree_adjustments: { originator_type: 'Voucher' } }, - foreign_key: 'originator_id', inverse_of: :adjustments - validates :label, presence: true validates :amount, numericality: true diff --git a/app/models/voucher.rb b/app/models/voucher.rb index 33ef0f6075..376c1979dd 100644 --- a/app/models/voucher.rb +++ b/app/models/voucher.rb @@ -8,7 +8,6 @@ class Voucher < ApplicationRecord has_many :adjustments, as: :originator, class_name: 'Spree::Adjustment', - inverse_of: :voucher, dependent: :nullify validates :code, presence: true, uniqueness: { scope: :enterprise_id } diff --git a/spec/models/spree/adjustment_spec.rb b/spec/models/spree/adjustment_spec.rb index bbe7b8f245..66d243fd59 100644 --- a/spec/models/spree/adjustment_spec.rb +++ b/spec/models/spree/adjustment_spec.rb @@ -11,14 +11,10 @@ module Spree it { is_expected.to have_one(:metadata) } it { is_expected.to have_many(:adjustments) } - it { is_expected.to belong_to(:adjustable) } - it { is_expected.to belong_to(:adjustable) } it { is_expected.to belong_to(:originator) } it { is_expected.to belong_to(:order) } it { is_expected.to belong_to(:tax_category) } - it { is_expected.to belong_to(:tax_rate) } - it { is_expected.to belong_to(:voucher) } end describe "scopes" do From ccff3379ea8f554547524d9212dcb481b8ea5e75 Mon Sep 17 00:00:00 2001 From: David Cook Date: Wed, 10 May 2023 16:56:39 +1000 Subject: [PATCH 38/39] Update schema.rb On a freshly mirrored prod db, I found these changes. I don't know why.. but hopefully this is correct. --- db/schema.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/db/schema.rb b/db/schema.rb index 41b3d45046..f0935b6e16 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1192,11 +1192,11 @@ ActiveRecord::Schema[7.0].define(version: 2023_04_24_141213) do create_table "vouchers", force: :cascade do |t| t.string "code", limit: 255, null: false - t.datetime "expiry_date" + t.datetime "expiry_date", precision: nil t.datetime "created_at", null: false t.datetime "updated_at", null: false t.bigint "enterprise_id" - t.datetime "deleted_at" + t.datetime "deleted_at", precision: nil t.index ["code", "enterprise_id"], name: "index_vouchers_on_code_and_enterprise_id", unique: true t.index ["deleted_at"], name: "index_vouchers_on_deleted_at" t.index ["enterprise_id"], name: "index_vouchers_on_enterprise_id" From 5eb6097101b1f920f0ce804a3bab28d84e12fa2d Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Fri, 12 May 2023 15:01:52 +1000 Subject: [PATCH 39/39] Fix error handling when creating a voucher adjustment I wrongly assumed that `voucher.create_adjustment` would return nil if failing to create an adjustment. I will in fact return an adjustment object with errors. --- app/controllers/split_checkout_controller.rb | 3 ++- spec/controllers/split_checkout_controller_spec.rb | 9 ++++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb index 4a4d1f0aca..1ccf66f27c 100644 --- a/app/controllers/split_checkout_controller.rb +++ b/app/controllers/split_checkout_controller.rb @@ -226,8 +226,9 @@ class SplitCheckoutController < ::BaseController adjustment = voucher.create_adjustment(voucher.code, @order) - if adjustment.nil? + if !adjustment.valid? @order.errors.add(:voucher, I18n.t('split_checkout.errors.add_voucher_error')) + adjustment.errors.each { |error| @order.errors.import(error) } return false end diff --git a/spec/controllers/split_checkout_controller_spec.rb b/spec/controllers/split_checkout_controller_spec.rb index db3571d81b..774be4ca76 100644 --- a/spec/controllers/split_checkout_controller_spec.rb +++ b/spec/controllers/split_checkout_controller_spec.rb @@ -279,14 +279,17 @@ describe SplitCheckoutController, type: :controller do context "when adding fails" do it "returns 422 and an error message" do - # Makes adding the voucher fails - allow(voucher).to receive(:create_adjustment).and_return(nil) + # Create a non valid adjustment + adjustment = build(:adjustment, label: nil) + allow(voucher).to receive(:create_adjustment).and_return(adjustment) allow(Voucher).to receive(:find_by).and_return(voucher) put :update, params: params expect(response.status).to eq 422 - expect(flash[:error]).to match "There was an error while adding the voucher" + expect(flash[:error]).to match( + "There was an error while adding the voucher and Label can't be blank" + ) end end