Merge pull request #11002 from openfoodfoundation/voucher-prep

Vouchers part 1

app/controllers/split_checkout_controller.rb

@@ -24,7 +24,6 @@ class SplitCheckoutController < ::BaseController
   def edit
     redirect_to_step_based_on_order unless params[:step]
     check_step if params[:step]
-    recalculate_tax if params[:step] == "summary"
 
     flash_error_when_no_shipping_method_available if available_shipping_methods.none?
   end
@@ -204,6 +203,7 @@ class SplitCheckoutController < ::BaseController
 
   def process_voucher
     if add_voucher
+      VoucherAdjustmentsService.calculate(@order)
       render_voucher_section_or_redirect
     elsif @order.errors.present?
       render_error
@@ -226,7 +226,7 @@ class SplitCheckoutController < ::BaseController
 
     adjustment = voucher.create_adjustment(voucher.code, @order)
 
-    if !adjustment.valid?
+    unless adjustment.valid?
       @order.errors.add(:voucher, I18n.t('split_checkout.errors.add_voucher_error'))
       adjustment.errors.each { |error| @order.errors.import(error) }
       return false
@@ -308,18 +308,4 @@ class SplitCheckoutController < ::BaseController
       redirect_to checkout_step_path(:payment) if params[:step] == "summary"
     end
   end
-
-  def recalculate_tax
-    @order.create_tax_charge!
-    @order.update_order!
-
-    apply_voucher if @order.voucher_adjustments.present?
-  end
-
-  def apply_voucher
-    VoucherAdjustmentsService.calculate(@order)
-
-    # update order to take into account the voucher we applied
-    @order.update_order!
-  end
 end

app/controllers/spree/admin/orders/customer_details_controller.rb

@@ -24,7 +24,6 @@ module Spree
             end
 
             refresh_shipment_rates
-            recalculate_taxes
             OrderWorkflow.new(@order).advance_to_payment
 
             flash[:success] = Spree.t('customer_details_updated')
@@ -52,15 +51,6 @@ module Spree
           @order.shipments.map(&:refresh_rates)
         end
 
-        def recalculate_taxes
-          # If the order's address has been changed, the tax zone could be different,
-          # which means a different set of tax rates might be applicable.
-          @order.create_tax_charge!
-          Spree::TaxRate.adjust(@order, @order.adjustments.admin)
-
-          @order.update_totals_and_states
-        end
-
         def order_params
           params.require(:order).permit(
             :email,

app/models/spree/adjustment.rb

@@ -67,6 +67,8 @@ module Spree
     scope :charge, -> { where('amount >= 0') }
     scope :credit, -> { where('amount < 0') }
     scope :return_authorization, -> { where(originator_type: "Spree::ReturnAuthorization") }
+    scope :voucher, -> { where(originator_type: "Voucher") }
+    scope :non_voucher, -> { where.not(originator_type: "Voucher") }
     scope :inclusive, -> { where(included: true) }
     scope :additional, -> { where(included: false) }
     scope :legacy_tax, -> { additional.tax.where(adjustable_type: "Spree::Order") }

app/models/spree/order.rb

@@ -87,15 +87,6 @@ module Spree
     delegate :create_line_item_fees!, :create_order_fees!, :update_order_fees!,
              :update_line_item_fees!, :recreate_all_fees!, to: :fee_handler
 
-    # Needs to happen before save_permalink is called
-    before_validation :set_currency
-    before_validation :generate_order_number, if: :new_record?
-    before_validation :clone_billing_address, if: :use_billing?
-    before_validation :ensure_customer
-
-    before_create :link_by_email
-    after_create :create_tax_charge!
-
     validates :customer, presence: true, if: :require_customer?
     validate :products_available_from_new_distribution, if: lambda {
       distributor_id_changed? || order_cycle_id_changed?
@@ -108,16 +99,25 @@ module Spree
     validates :order_cycle, presence: true, on: :require_distribution
     validates :distributor, presence: true, on: :require_distribution
 
-    make_permalink field: :number
+    before_validation :set_currency
+    before_validation :generate_order_number, if: :new_record?
+    before_validation :clone_billing_address, if: :use_billing?
+    before_validation :ensure_customer
 
+    before_create :link_by_email
     before_save :update_shipping_fees!, if: :complete?
     before_save :update_payment_fees!, if: :complete?
 
+    after_create :create_tax_charge!
+    after_save :reapply_tax_on_changed_address
+
     after_save_commit DefaultAddressUpdater
 
+    make_permalink field: :number
+
     attribute :send_cancellation_email, type: :boolean, default: true
     attribute :restock_items, type: :boolean, default: true
-    # -- Scopes
+
     scope :not_empty, -> {
       left_outer_joins(:line_items).where.not(spree_line_items: { id: nil })
     }
@@ -170,6 +170,11 @@ module Spree
       line_items.inject(0.0) { |sum, li| sum + li.amount }
     end
 
+    # Order total without any applied discounts from vouchers
+    def pre_discount_total
+      item_total + all_adjustments.additional.eligible.non_voucher.sum(:amount)
+    end
+
     def currency
       self[:currency] || Spree::Config[:currency]
     end
@@ -316,12 +321,13 @@ module Spree
     # Creates new tax charges if there are any applicable rates. If prices already
     # include taxes then price adjustments are created instead.
     def create_tax_charge!
-      return if state.in?(["cart", "address", "delivery"])
+      return if before_payment_state?
 
       clear_legacy_taxes!
 
       Spree::TaxRate.adjust(self, line_items)
       Spree::TaxRate.adjust(self, shipments) if shipments.any?
+      Spree::TaxRate.adjust(self, adjustments.admin) if adjustments.admin.any?
       fee_handler.tax_enterprise_fees!
     end
 
@@ -573,8 +579,20 @@ module Spree
       end
     end
 
+    def before_payment_state?
+      state.in?(["cart", "address", "delivery"])
+    end
+
     private
 
+    def reapply_tax_on_changed_address
+      return if before_payment_state?
+      return unless tax_address&.saved_changes?
+
+      create_tax_charge!
+      update_totals_and_states
+    end
+
     def deliver_order_confirmation_email
       return if subscription.present?
 

app/models/spree/order/checkout.rb

@@ -77,7 +77,10 @@ module Spree
               before_transition to: :delivery, do: :ensure_available_shipping_rates
               before_transition to: :confirmation, do: :validate_payment_method!
 
-              after_transition to: :payment, do: :create_tax_charge!
+              after_transition to: :payment do |order|
+                order.create_tax_charge!
+                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
@@ -144,7 +147,7 @@ module Spree
           private
 
           def after_cancel
-            shipments.each(&:cancel!)
+            shipments.reject(&:canceled?).each(&:cancel!)
             payments.checkout.each(&:void!)
 
             OrderMailer.cancel_email(id).deliver_later if send_cancellation_email

app/models/voucher.rb

@@ -41,6 +41,6 @@ class Voucher < ApplicationRecord
   # 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.clamp(0, order.total)
+    -amount.clamp(0, order.pre_discount_total)
   end
 end

app/services/order_fees_handler.rb

@@ -21,7 +21,7 @@ class OrderFeesHandler
       create_order_fees!
     end
 
-    tax_enterprise_fees!
+    tax_enterprise_fees! unless order.before_payment_state?
     order.update_order!
   end
 

app/services/order_workflow.rb

@@ -24,13 +24,13 @@ class OrderWorkflow
   end
 
   def advance_to_payment
-    return unless order.state.in? ["cart", "address", "delivery"]
+    return unless order.before_payment_state?
 
     advance_to_state("payment", advance_order_options)
   end
 
   def advance_checkout(options = {})
-    advance_to = order.state.in?(["cart", "address", "delivery"]) ? "payment" : "confirmation"
+    advance_to = order.before_payment_state? ? "payment" : "confirmation"
 
     advance_to_state(advance_to, advance_order_options.merge(options))
   end

app/views/layouts/darkswarm.html.haml

@@ -46,7 +46,6 @@
           = yield
 
           #footer
-    %loading
 
     = yield :scripts
     = inject_current_hub

spec/controllers/spree/admin/orders_controller_spec.rb

@@ -57,7 +57,7 @@ describe Spree::Admin::OrdersController, type: :controller do
 
         it "updates fees and taxes and redirects to order details page" do
           expect(order).to receive(:recreate_all_fees!)
-          expect(order).to receive(:create_tax_charge!)
+          expect(order).to receive(:create_tax_charge!).at_least :once
 
           spree_put :update, params
 

spec/models/proxy_order_spec.rb

@@ -118,6 +118,7 @@ describe ProxyOrder, type: :model do
                                        }
           break unless order.next! while !order.completed?
           order.cancel
+          order.reload
         end
 
         it "returns true, clears canceled_at and resumes the order" do

spec/models/spree/adjustment_spec.rb

@@ -250,6 +250,7 @@ module Spree
           context "when the shipment has an inclusive tax rate" do
             it "calculates the shipment tax from the tax rate" do
               order.shipments = [shipment]
+              order.state = "payment"
               order.create_tax_charge!
               order.update_totals
 
@@ -274,6 +275,7 @@ module Spree
 
             it "records the tax on the shipment's adjustments" do
               order.shipments = [shipment]
+              order.state = "payment"
               order.create_tax_charge!
               order.update_totals
 
@@ -355,6 +357,7 @@ module Spree
 
         context "when enterprise fees have a fixed tax_category" do
           before do
+            order.update(state: "payment")
             order.recreate_all_fees!
           end
 
@@ -440,6 +443,11 @@ module Spree
                                       calculator: ::Calculator::FlatRate.new(preferred_amount: 50.0))
             }
 
+            before do
+              order.update(state: "payment")
+              order.create_tax_charge!
+            end
+
             describe "when the tax rate includes the tax in the price" do
               it "records no tax on the enterprise fee adjustments" do
                 # EnterpriseFee tax category is nil and inheritance only applies to per item fees
@@ -471,6 +479,11 @@ module Spree
                                       calculator: ::Calculator::PerItem.new(preferred_amount: 50.0))
             }
 
+            before do
+              order.update(state: "payment")
+              order.create_tax_charge!
+            end
+
             describe "when the tax rate includes the tax in the price" do
               it "records the correct amount in a tax adjustment" do
                 # Applying product tax rate of 0.2 to enterprise fee of $50
@@ -522,6 +535,8 @@ module Spree
           tax_category.tax_rates << tax_rate
           allow(order).to receive(:tax_zone) { zone }
           order.line_items << create(:line_item, variant: variant, quantity: 5)
+          order.update(state: "payment")
+          order.create_tax_charge!
         end
 
         context "with included taxes" do

spec/models/spree/order_spec.rb

@@ -336,6 +336,7 @@ describe Spree::Order do
 
     before do
       order.cancel!
+      order.reload
       order.resume!
     end
 

spec/models/voucher_spec.rb

@@ -20,20 +20,21 @@ describe Voucher do
   end
 
   describe '#compute_amount' do
-    subject { create(:voucher, code: 'new_code', enterprise: enterprise, amount: 10) }
-
     let(:order) { create(:order_with_totals) }
 
-    it 'returns -10' do
-      expect(subject.compute_amount(order).to_f).to eq(-10)
+    context 'when order total is more than the voucher' do
+      subject { create(:voucher, code: 'new_code', enterprise: enterprise, amount: 5) }
+
+      it 'uses the voucher total' do
+        expect(subject.compute_amount(order).to_f).to eq(-5)
+      end
     end
 
-    context 'when order total is smaller than 10' do
-      it 'returns minus the order total' do
-        order.total = 6
-        order.save!
+    context 'when order total is less than the voucher' do
+      subject { create(:voucher, code: 'new_code', enterprise: enterprise, amount: 20) }
 
-        expect(subject.compute_amount(order).to_f).to eq(-6)
+      it 'matches the order total' do
+        expect(subject.compute_amount(order).to_f).to eq(-10)
       end
     end
   end

spec/services/voucher_adjustments_service_spec.rb

@@ -14,9 +14,7 @@ describe VoucherAdjustmentsService do
 
       it 'updates the adjustment amount to -order.total' do
         voucher.create_adjustment(voucher.code, order)
-
-        order.total = 6
-        order.save!
+        order.update_columns(item_total: 6)
 
         VoucherAdjustmentsService.calculate(order)
 

spec/system/admin/bulk_order_management_spec.rb

@@ -945,6 +945,9 @@ describe '
         let!(:li2) { create(:line_item_with_shipment, order: o2 ) }
 
         before :each do
+          o1.update_totals_and_states
+          o2.update_totals_and_states
+
           visit_bulk_order_management
         end
 

spec/system/consumer/split_checkout_spec.rb

@@ -1114,11 +1114,8 @@ describe "As a consumer, I want to checkout my order" do
         let(:voucher) { create(:voucher, code: 'some_code', enterprise: distributor, amount: 6) }
 
         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!
+          order.update_totals
           VoucherAdjustmentsService.calculate(order)
 
           visit checkout_step_path(:summary)

spec/system/consumer/split_checkout_tax_not_incl_spec.rb

@@ -10,8 +10,12 @@ describe "As a consumer, I want to see adjustment breakdown" do
   include AuthenticationHelper
   include WebHelper
 
-  let!(:address_within_zone) { create(:address, state: Spree::State.first) }
-  let!(:address_outside_zone) { create(:address, state: Spree::State.second) }
+  let!(:state) { create(:state, name: "Victoria") }
+  let!(:zone) { create(:zone_with_state_member, default_tax: false, member: state) }
+  let!(:address_within_zone) { create(:address, state: state) }
+  let!(:address_outside_zone) {
+    create(:address, state: create(:state, name: "Timbuktu", country: state.country))
+  }
   let(:user_within_zone) {
     create(:user, bill_address: address_within_zone,
                   ship_address: address_within_zone)
@@ -20,8 +24,7 @@ describe "As a consumer, I want to see adjustment breakdown" do
     create(:user, bill_address: address_outside_zone,
                   ship_address: address_outside_zone)
   }
-  let!(:zone) { create(:zone_with_state_member, name: 'Victoria', default_tax: false) }
-  let!(:tax_category) { create(:tax_category, name: "Veggies", is_default: "f") }
+  let!(:tax_category) { create(:tax_category, name: "Veggies", is_default: false) }
   let!(:tax_rate) {
     create(:tax_rate, name: "Tax rate - included or not", amount: 0.13,
                       zone: zone, tax_category: tax_category, included_in_price: false)
@@ -62,7 +65,7 @@ describe "As a consumer, I want to see adjustment breakdown" do
     Spree::Config.set(tax_using_ship_address: true)
   end
 
-  pending "a not-included tax" do
+  describe "a not-included tax" do
     before do
       zone.update!(default_tax: false)
       tax_rate.update!(included_in_price: false)
@@ -99,12 +102,12 @@ describe "As a consumer, I want to see adjustment breakdown" do
         choose "Delivery"
 
         click_button "Next - Payment method"
-        click_on "Next - Order summary"
-        click_on "Complete order"
 
         # DB checks
-        order_within_zone.reload
-        expect(order_within_zone.additional_tax_total).to eq(1.3)
+        expect(order_within_zone.reload.additional_tax_total).to eq(1.3)
+
+        click_on "Next - Order summary"
+        click_on "Complete order"
 
         # UI checks
         expect(page).to have_content("Confirmed")
@@ -112,7 +115,7 @@ describe "As a consumer, I want to see adjustment breakdown" do
         expect(page).to have_selector('#tax-row', text: with_currency(1.30))
       end
 
-      context "when using a voucher" do
+      pending "when using a voucher" do
         let!(:voucher) do
           create(:voucher, code: 'some_code', enterprise: distributor, amount: 10)
         end