Merge pull request #11117 from rioug/10857-voucher-error-moving-between-summary-and-cart-take2

[vouchers] error moving between summary and cart pages

app/controllers/spree/orders_controller.rb

@@ -69,6 +69,10 @@ module Spree
       if @order.contents.update_cart(order_params)
         @order.recreate_all_fees! # Enterprise fees on line items and on the order itself
 
+        # Re apply the voucher
+        VoucherAdjustmentsService.new(@order).update
+        @order.update_totals_and_states
+
         if @order.complete?
           @order.update_payment_fees!
           @order.create_tax_charge!

app/controllers/voucher_adjustments_controller.rb

@@ -5,9 +5,6 @@ class VoucherAdjustmentsController < BaseController
 
   def create
     if add_voucher
-      VoucherAdjustmentsService.calculate(@order)
-      @order.update_totals_and_states
-
       update_payment_section
     elsif @order.errors.present?
       render_error
@@ -21,6 +18,9 @@ class VoucherAdjustmentsController < BaseController
       @order.voucher_adjustments.where(originator_id: adjustment.originator_id)&.destroy_all
     end
 
+    # Update order to make sure we display the appropriate payment method
+    @order.update_totals_and_states
+
     update_payment_section
   end
 
@@ -45,12 +45,17 @@ class VoucherAdjustmentsController < BaseController
 
     adjustment = voucher.create_adjustment(voucher.code, @order)
 
-    unless adjustment.valid?
+    unless adjustment.persisted?
       @order.errors.add(:voucher_code, I18n.t('split_checkout.errors.add_voucher_error'))
       adjustment.errors.each { |error| @order.errors.import(error) }
       return false
     end
 
+    clear_payments
+
+    VoucherAdjustmentsService.new(@order).update
+    @order.update_totals_and_states
+
     true
   end
 
@@ -78,4 +83,9 @@ class VoucherAdjustmentsController < BaseController
   def voucher_params
     params.require(:order).permit(:voucher_code)
   end
+
+  # Clear payments and payment fees, to not affect voucher adjustment calculation
+  def clear_payments
+    @order.payments.incomplete.destroy_all
+  end
 end

app/models/spree/order/checkout.rb

@@ -81,6 +81,12 @@ module Spree
                 order.create_tax_charge!
                 order.update_totals_and_states
               end
+
+              after_transition to: :confirmation do |order|
+                VoucherAdjustmentsService.new(order).update
+                order.update_totals_and_states
+              end
+
               after_transition to: :complete, do: :finalize!
               after_transition to: :resumed,  do: :after_resume
               after_transition to: :canceled, do: :after_cancel

app/models/voucher.rb

@@ -21,12 +21,11 @@ class Voucher < ApplicationRecord
   # but vouchers have complicated calculation so we can't easily use Spree::Calculator. We keep
   # the same method to stay as consistent as possible.
   #
-  # Creates a new voucher adjustment for the given order
+  # Creates a new voucher adjustment for the given order with an amount of 0
+  # The amount will be calculated via VoucherAdjustmentsService#update
   def create_adjustment(label, order)
-    amount = compute_amount(order)
-
     adjustment_attributes = {
-      amount: amount,
+      amount: 0,
       originator: self,
       order: order,
       label: label,

app/services/voucher_adjustments_service.rb

@@ -1,68 +1,80 @@
 # frozen_string_literal: true
 
 class VoucherAdjustmentsService
-  def self.calculate(order)
-    return if order.nil?
+  def initialize(order)
+    @order = order
+  end
+
+  def update
+    return if @order.nil?
 
     # Find open Voucher Adjustment
-    return if order.voucher_adjustments.empty?
+    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
+    adjustment = @order.voucher_adjustments.first
 
-    # Recalculate value
-    amount = adjustment.originator.compute_amount(order)
+    # Calculate 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)
-    elsif order.included_tax_total.positive?
-      handle_tax_included_in_price(order, amount)
+    if @order.additional_tax_total.positive?
+      handle_tax_excluded_from_price(amount)
+    elsif @order.included_tax_total.positive?
+      handle_tax_included_in_price(amount)
     else
       adjustment.amount = amount
+      adjustment.save
     end
-
-    # Move to closed state
-    adjustment.close
   end
 
-  def self.handle_tax_excluded_from_price(order, amount)
-    voucher_rate = amount / order.total
+  private
+
+  def handle_tax_excluded_from_price(amount)
+    voucher_rate = amount / @order.pre_discount_total
+
+    adjustment = @order.voucher_adjustments.first
 
     # Adding the voucher tax part
-    tax_amount = voucher_rate * order.additional_tax_total
+    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_tax_adjustment_for(adjustment, tax_amount)
 
     # Update the adjustment amount
-    amount = voucher_rate * (order.total - order.additional_tax_total)
+    adjustment_amount = voucher_rate * (@order.pre_discount_total - @order.additional_tax_total)
 
     adjustment.update_columns(
-      amount: amount,
+      amount: adjustment_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
+  def update_tax_adjustment_for(adjustment, tax_amount)
+    adjustment_attributes = {
+      originator: adjustment.originator,
+      order: @order,
+      label: "Tax #{adjustment.label}",
+      mandatory: false,
+      state: 'open',
+      tax_category: nil,
+      included_tax: 0
+    }
+
+    # Update the amount if tax adjustment already exist, create if not
+    tax_adjustment = @order.adjustments.find_or_initialize_by(adjustment_attributes)
+    tax_adjustment.amount = tax_amount
+    tax_adjustment.save
+  end
+
+  def handle_tax_included_in_price(amount)
+    voucher_rate = amount / @order.pre_discount_total
+    included_tax = voucher_rate * @order.included_tax_total
 
     # Update Adjustment
-    adjustment = order.voucher_adjustments.first
+    adjustment = @order.voucher_adjustments.first
 
     return unless amount != adjustment.amount || included_tax != 0
 
@@ -72,6 +84,4 @@ class VoucherAdjustmentsService
       updated_at: Time.zone.now
     )
   end
-
-  private_class_method :handle_tax_included_in_price, :handle_tax_excluded_from_price
 end

engines/order_management/app/services/order_management/order/updater.rb

@@ -137,7 +137,10 @@ module OrderManagement
       end
 
       def update_all_adjustments
-        order.all_adjustments.reload.each(&:update_adjustment!)
+        # Voucher are modelled as a Spree::Adjustment but  they don't behave like all the other
+        # adjustments, so we don't want voucher adjustment to be updated here.
+        # Calculation are handled by VoucherAdjustmentsService.calculate
+        order.all_adjustments.non_voucher.reload.each(&:update_adjustment!)
       end
 
       def before_save_hook

spec/models/voucher_spec.rb

@@ -45,8 +45,8 @@ describe Voucher do
     let(:voucher) { create(:voucher, code: 'new_code', enterprise: enterprise, amount: 25) }
     let(:order) { create(:order_with_line_items, line_items_count: 3, distributor: enterprise) }
 
-    it 'includes the full voucher amount' do
-      expect(adjustment.amount.to_f).to eq(-25.0)
+    it 'includes an amount of 0' do
+      expect(adjustment.amount.to_f).to eq(0.0)
     end
 
     it 'has no included_tax' do

spec/requests/voucher_adjustments_spec.rb

@@ -8,7 +8,7 @@ describe VoucherAdjustmentsController, type: :request do
   let(:distributor) { create(:distributor_enterprise, with_payment_and_shipping: true) }
   let(:order_cycle) { create(:order_cycle, distributors: [distributor]) }
   let(:exchange) { order_cycle.exchanges.outgoing.first }
-  let(:order) do
+  let!(:order) do
     create(
       :order_with_line_items,
       line_items_count: 1,
@@ -61,9 +61,28 @@ describe VoucherAdjustmentsController, type: :request do
         post "/voucher_adjustments", params: params
 
         expect(response).to be_unprocessable
-        expect(flash[:error]).to match(
-          "There was an error while adding the voucher and Label can't be blank"
-        )
+        expect(flash[:error]).to match("Voucher code There was an error while adding the voucher")
+      end
+    end
+
+    context "when the order has a payment and payment feed" do
+      let(:payment_method) { create(:payment_method, calculator: calculator) }
+      let(:calculator) { Calculator::FlatPercentItemTotal.new(preferred_flat_percent: 10) }
+
+      before do
+        create(:payment, order: order, payment_method: payment_method, amount: order.total)
+      end
+
+      it "removes existing payments" do
+        expect do
+          post "/voucher_adjustments", params: params
+        end.to change { order.reload.payments.count }.from(1).to(0)
+      end
+
+      it "removes existing payment fees" do
+        expect do
+          post "/voucher_adjustments", params: params
+        end.to change { order.reload.all_adjustments.payment_fee.count }.from(1).to(0)
       end
     end
   end
@@ -83,7 +102,7 @@ describe VoucherAdjustmentsController, type: :request do
       expect(response).to be_successful
     end
 
-    context "when adjustment doesn't exits" do
+    context "when adjustment doesn't exist" do
       it "does nothing" do
         delete "/voucher_adjustments/-1"
 

spec/services/voucher_adjustments_service_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 describe VoucherAdjustmentsService do
-  describe '.calculate' do
+  describe '#update' do
     let(:enterprise) { build(:enterprise) }
     let(:voucher) { create(:voucher, code: 'new_code', enterprise: enterprise, amount: 10) }
 
@@ -16,13 +16,13 @@ describe VoucherAdjustmentsService do
         voucher.create_adjustment(voucher.code, order)
         order.update_columns(item_total: 6)
 
-        VoucherAdjustmentsService.calculate(order)
+        VoucherAdjustmentsService.new(order).update
 
         expect(subject.amount.to_f).to eq(-6.0)
       end
     end
 
-    context 'with price included in order price' do
+    context 'with tax included in order price' do
       subject { order.voucher_adjustments.first }
 
       let(:order) do
@@ -46,23 +46,51 @@ describe VoucherAdjustmentsService do
         order.update_shipping_fees!
         order.update_order!
 
-        VoucherAdjustmentsService.calculate(order)
+        VoucherAdjustmentsService.new(order).update
       end
 
       it 'updates the adjustment included_tax' do
         # voucher_rate = amount / order.total
-        # -10 / 150 = -0.066666667
+        # -10 / 160 = -0.0625
         # included_tax = voucher_rate * order.included_tax_total
-        # -0.66666666 * 10 = -0.67
-        expect(subject.included_tax.to_f).to eq(-0.67)
+        # -0.0625 * 10 = -0.625
+        expect(subject.included_tax.to_f).to eq(-0.63)
       end
 
-      it 'moves the adjustment state to closed' do
-        expect(subject.state).to eq('closed')
+      context "when re calculating" do
+        it "does not update the adjustment amount" do
+          expect do
+            VoucherAdjustmentsService.new(order).update
+          end.not_to change { subject.reload.amount }
+        end
+
+        it "does not update the tax adjustment" do
+          expect do
+            VoucherAdjustmentsService.new(order).update
+          end.not_to change { subject.reload.included_tax }
+        end
+
+        context "when the order changed" do
+          before do
+            order.update_columns(item_total: 200)
+          end
+
+          it "does update the adjustment amount" do
+            expect do
+              VoucherAdjustmentsService.new(order).update
+            end.not_to change { subject.reload.amount }
+          end
+
+          it "updates the tax adjustment" do
+            expect do
+              VoucherAdjustmentsService.new(order).update
+            end.to change { subject.reload.included_tax }
+          end
+        end
       end
     end
 
-    context 'with price not included in order price' do
+    context 'with tax not included in order price' do
       let(:order) do
         create(
           :order_with_taxes,
@@ -74,6 +102,8 @@ describe VoucherAdjustmentsService do
           tax_rate_name: "Tax 1"
         )
       end
+      let(:adjustment) { order.voucher_adjustments.first }
+      let(:tax_adjustment) { order.voucher_adjustments.second }
 
       before do
         # create adjustment before tax are set
@@ -84,37 +114,62 @@ describe VoucherAdjustmentsService do
         order.update_shipping_fees!
         order.update_order!
 
-        VoucherAdjustmentsService.calculate(order)
+        VoucherAdjustmentsService.new(order).update
       end
 
-      it 'includes amount withou tax' do
-        adjustment = order.voucher_adjustments.first
+      it 'includes amount without tax' do
         # voucher_rate = amount / order.total
-        # -10 / 161 = -0.062111801
+        # -10 / 171 = -0.058479532
         # amount = voucher_rate * (order.total - order.additional_tax_total)
-        # -0.062111801 * (161 -11) = -9.32
-        expect(adjustment.amount.to_f).to eq(-9.32)
+        # -0.058479532 * (171 -11) = -9.36
+        expect(adjustment.amount.to_f).to eq(-9.36)
       end
 
       it 'creates a tax adjustment' do
         # voucher_rate = amount / order.total
-        # -10 / 161 = -0.062111801
+        # -10 / 171 = -0.058479532
         # 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)
+        # -0.058479532 * 11 = -0.64
+        expect(tax_adjustment.amount.to_f).to eq(-0.64)
         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')
+      context "when re calculating" do
+        it "does not update the adjustment amount" do
+          expect do
+            VoucherAdjustmentsService.new(order).update
+          end.not_to change { adjustment.reload.amount }
+        end
+
+        it "does not update the tax adjustment" do
+          expect do
+            VoucherAdjustmentsService.new(order).update
+          end.not_to change { tax_adjustment.reload.amount }
+        end
+
+        context "when the order changed" do
+          before do
+            order.update_columns(item_total: 200)
+          end
+
+          it "updates the adjustment amount" do
+            expect do
+              VoucherAdjustmentsService.new(order).update
+            end.to change { adjustment.reload.amount }
+          end
+
+          it "updates the tax adjustment" do
+            expect do
+              VoucherAdjustmentsService.new(order).update
+            end.to change { tax_adjustment.reload.amount }
+          end
+        end
       end
     end
 
     context 'when no order given' do
       it "doesn't blow up" do
-        expect { VoucherAdjustmentsService.calculate(nil) }.to_not raise_error
+        expect { VoucherAdjustmentsService.new(nil).update }.to_not raise_error
       end
     end
 
@@ -122,7 +177,7 @@ describe VoucherAdjustmentsService 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
+        expect { VoucherAdjustmentsService.new(order).update }.to_not raise_error
       end
     end
   end

spec/system/consumer/split_checkout_spec.rb

@@ -724,18 +724,20 @@ describe "As a consumer, I want to checkout my order" do
             create(:voucher, code: 'some_code', enterprise: distributor, amount: 15)
           end
 
-          before do
-            visit checkout_step_path(:payment)
-          end
-
           it "shows voucher input" do
+            visit checkout_step_path(:payment)
+            expect(page).to have_field "Enter voucher code"
             expect(page).to have_content "Apply voucher"
           end
 
           describe "adding voucher to the order" do
+            before do
+              visit checkout_step_path(:payment)
+            end
+
             shared_examples "adding voucher to the order" do
               before do
-                fill_in "Enter voucher code", with: voucher.code
+                fill_in "Enter voucher code", with: "some_code"
                 click_button("Apply")
               end
 
@@ -756,7 +758,7 @@ describe "As a consumer, I want to checkout my order" do
               it_behaves_like "adding voucher to the order"
 
               it "shows a warning message" do
-                fill_in "Enter voucher code", with: voucher.code
+                fill_in "Enter voucher code", with: "some_code"
                 click_button("Apply")
 
                 expect(page).to have_content(
@@ -764,6 +766,16 @@ describe "As a consumer, I want to checkout my order" do
                   "you may not be able to spend the remaining value."
                 )
               end
+
+              it "proceeds without requiring payment" do
+                fill_in "Enter voucher code", with: "some_code"
+                click_button("Apply")
+
+                expect(page).to have_content "No payment required"
+                click_button "Next - Order summary"
+                # Expect to be on the Order Summary page
+                expect(page).to have_content "Delivery details"
+              end
             end
 
             context "voucher doesn't exist" do
@@ -778,22 +790,45 @@ describe "As a consumer, I want to checkout my order" do
 
           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
+              add_voucher_to_order(voucher, order)
+
+              visit checkout_step_path(:payment)
 
-            it "removes voucher" do
               accept_confirm "Are you sure you want to remove the voucher?" do
                 click_on "Remove code"
               end
+            end
 
+            it "removes voucher" do
               within '#voucher-section' do
                 expect(page).to have_button("Apply", disabled: true)
+                expect(page).to have_field "Enter voucher code" # Currently no confirmation msg
               end
 
+              expect(page).not_to have_content "No payment required"
               expect(order.voucher_adjustments.length).to eq(0)
             end
+
+            it "can re-enter a voucher" do
+              # Re-enter a voucher code
+              fill_in "Enter voucher code", with: "some_code"
+              click_button("Apply")
+
+              expect(page).to have_content("$15.00 Voucher")
+              expect(order.reload.voucher_adjustments.length).to eq(1)
+
+              expect(page).to have_content "No payment required"
+
+              click_button "Next - Order summary"
+              # Expect to be on the Order Summary page
+              expect(page).to have_content "Delivery details"
+            end
+
+            it "can proceed with payment" do
+              click_button "Next - Order summary"
+              # Expect to be on the Order Summary page
+              expect(page).to have_content "Delivery details"
+            end
           end
         end
       end
@@ -1143,9 +1178,7 @@ describe "As a consumer, I want to checkout my order" do
         let(:voucher) { create(:voucher, code: 'some_code', enterprise: distributor, amount: 6) }
 
         before do
-          voucher.create_adjustment(voucher.code, order)
-          order.update_totals
-          VoucherAdjustmentsService.calculate(order)
+          add_voucher_to_order(voucher, order)
 
           visit checkout_step_path(:summary)
         end
@@ -1199,4 +1232,10 @@ describe "As a consumer, I want to checkout my order" do
       end
     end
   end
+
+  def add_voucher_to_order(voucher, order)
+    voucher.create_adjustment(voucher.code, order)
+    VoucherAdjustmentsService.new(order).update
+    order.update_totals_and_states
+  end
 end

spec/system/consumer/split_checkout_tax_incl_spec.rb

@@ -47,9 +47,12 @@ describe "As a consumer, I want to see adjustment breakdown" do
                             calculator: Calculator::FlatRate.new(preferred_amount: 0.00))
   }
   let!(:order_within_zone) {
-    create(:order, order_cycle: order_cycle, distributor: distributor, user: user_within_zone,
-                   bill_address: address_within_zone, ship_address: address_within_zone,
-                   state: "cart", line_items: [create(:line_item, variant: variant_with_tax)])
+    create(
+      :order,
+      order_cycle: order_cycle, distributor: distributor, user: user_within_zone,
+      bill_address: address_within_zone, ship_address: address_within_zone,
+      state: "cart", line_items: [create(:line_item, variant: variant_with_tax, quantity: 1)]
+    )
   }
   let!(:order_outside_zone) {
     create(:order, order_cycle: order_cycle, distributor: distributor, user: user_outside_zone,
@@ -120,7 +123,7 @@ describe "As a consumer, I want to see adjustment breakdown" do
           click_button "Next - Payment method"
 
           # add Voucher
-          fill_in "Enter voucher code", with: voucher.code
+          fill_in "Enter voucher code", with: "some_code"
           click_button("Apply")
 
           # Choose payment
@@ -139,11 +142,64 @@ describe "As a consumer, I want to see adjustment breakdown" do
           end
 
           # DB check
-          order_within_zone.reload
-          voucher_adjustment = order_within_zone.voucher_adjustments.first
+          assert_db_voucher_adjustment(-10.00, -1.15)
+        end
 
-          expect(voucher_adjustment.amount.to_f).to eq(-10)
-          expect(voucher_adjustment.included_tax.to_f).to eq(-1.15)
+        describe "moving between summary to summary via edit cart" do
+          let!(:voucher) do
+            create(:voucher, code: 'good_code', enterprise: distributor, amount: 15)
+          end
+
+          it "recalculate the tax component properly" do
+            visit checkout_step_path(:details)
+            proceed_to_payment
+
+            # add Voucher
+            fill_in "Enter voucher code", with: "good_code"
+            click_button("Apply")
+
+            proceed_to_summary
+
+            assert_db_voucher_adjustment(-10.00, -1.15)
+
+            # Click on edit link
+            within "div", text: /Order details/ do
+              # It's a bit brittle, but the scoping doesn't seem to work
+              all(".summary-edit").last.click
+            end
+
+            # Update quantity
+            within ".cart-item-quantity" do
+              fill_in "order_line_items_attributes_0_quantity", with: "2"
+            end
+
+            click_button("Update")
+
+            # Check adjustment has been recalculated
+            assert_db_voucher_adjustment(-15.00, -1.73)
+
+            within "#cart-container" do
+              click_link("Checkout")
+            end
+
+            # Go back to payment step
+            proceed_to_payment
+
+            # Check voucher is still there
+            expect(page).to have_content("$15.00 Voucher")
+
+            # Go to summary
+            proceed_to_summary
+
+            # Check voucher value
+            within ".summary-right" do
+              expect(page).to have_content "good_code"
+              expect(page).to have_content "-15"
+            end
+
+            # Check adjustment has been recalculated, we are not expecting any changes here
+            assert_db_voucher_adjustment(-15.00, -1.73)
+          end
         end
       end
     end
@@ -190,4 +246,10 @@ describe "As a consumer, I want to see adjustment breakdown" do
     expect(order_outside_zone.included_tax_total).to eq(0.0)
     expect(order_outside_zone.additional_tax_total).to eq(0.0)
   end
+
+  def assert_db_voucher_adjustment(amount, tax_amount)
+    adjustment = order_within_zone.voucher_adjustments.first
+    expect(adjustment.amount.to_f).to eq(amount)
+    expect(adjustment.included_tax.to_f).to eq(tax_amount)
+  end
 end

spec/system/consumer/split_checkout_tax_not_incl_spec.rb

@@ -129,7 +129,7 @@ describe "As a consumer, I want to see adjustment breakdown" do
           click_button "Next - Payment method"
 
           # add Voucher
-          fill_in "Enter voucher code", with: voucher.code
+          fill_in "Enter voucher code", with: "some_code"
           click_button("Apply")
           expect(page).to have_selector ".voucher-added"