[vouchers] error moving between summary and cart pages

app/controllers/spree/orders_controller.rb

@@ -69,6 +69,10 @@ def update
       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
@@ -17,6 +14,9 @@ def create
   def destroy
     @order.voucher_adjustments.find_by(id: params[:id])&.destroy
 
+    # Update order to make sure we display the appropriate payment method
+    @order.update_totals_and_states
+
     update_payment_section
   end
 
@@ -41,12 +41,17 @@ def add_voucher
 
     adjustment = voucher.create_adjustment(voucher.code, @order)
 
-    unless adjustment.valid?
+    unless adjustment.persisted?
       @order.errors.add(:voucher_code, I18n.t('split_checkout.errors.add_voucher_error'))
       adjustment.errors.each { |error| @order.errors.import(error) }
       return false
     end
 
+    clear_payments
+
+    VoucherAdjustmentsService.new(@order).update
+    @order.update_totals_and_states
+
     true
   end
 
@@ -74,4 +79,10 @@ def render_error
   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.all_adjustments.payment_fee.destroy_all
+    @order.payments.clear
+  end
 end

app/models/spree/order/checkout.rb

@@ -81,6 +81,12 @@ def self.define_state_machine!
                 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 @@ def display_value
   # but vouchers have complicated calculation so we can't easily use Spree::Calculator. We keep
   # the same method to stay as consistent as possible.
   #
-  # Creates a new voucher adjustment for the given order
+  # 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
+
+    update_tax_adjustment_for(adjustment, tax_amount)
+
+    # Update the adjustment amount
+    adjustment_amount = voucher_rate * (@order.pre_discount_total - @order.additional_tax_total)
+
+    adjustment.update_columns(
+      amount: adjustment_amount,
+      updated_at: Time.zone.now
+    )
+  end
 
-    adjustment = order.voucher_adjustments.first
+  def update_tax_adjustment_for(adjustment, tax_amount)
     adjustment_attributes = {
-      amount: tax_amount,
       originator: adjustment.originator,
-      order: order,
+      order: @order,
       label: "Tax #{adjustment.label}",
       mandatory: false,
-      state: 'closed',
+      state: 'open',
       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
-    )
+    # 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 self.handle_tax_included_in_price(order, amount)
-    voucher_rate = amount / order.total
-    included_tax = voucher_rate * order.included_tax_total
+  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 @@ def self.handle_tax_included_in_price(order, amount)
       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 @@ def update_payment_state
       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 @@
     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 @@
   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 @@
         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 @@
       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"