From a584f9aaecd2e9d174af50c43b49d9a7b379150f Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 18 Jul 2023 14:39:01 +1000 Subject: [PATCH] Refactor VoucherAdjustmentsService It's more inline with existing coding style --- app/controllers/spree/orders_controller.rb | 2 +- app/models/spree/order/checkout.rb | 2 +- app/models/voucher.rb | 2 +- app/services/voucher_adjustments_service.rb | 48 ++++++++++--------- .../voucher_adjustments_service_spec.rb | 28 +++++------ spec/system/consumer/split_checkout_spec.rb | 2 +- 6 files changed, 44 insertions(+), 40 deletions(-) diff --git a/app/controllers/spree/orders_controller.rb b/app/controllers/spree/orders_controller.rb index 57b4983fde..24895ecf1a 100644 --- a/app/controllers/spree/orders_controller.rb +++ b/app/controllers/spree/orders_controller.rb @@ -70,7 +70,7 @@ module Spree @order.recreate_all_fees! # Enterprise fees on line items and on the order itself # Re apply the voucher - VoucherAdjustmentsService.calculate(@order) + VoucherAdjustmentsService.new(@order).calculate @order.update_totals_and_states if @order.complete? diff --git a/app/models/spree/order/checkout.rb b/app/models/spree/order/checkout.rb index 030df13964..20eb10da04 100644 --- a/app/models/spree/order/checkout.rb +++ b/app/models/spree/order/checkout.rb @@ -83,7 +83,7 @@ module Spree end after_transition to: :confirmation do |order| - VoucherAdjustmentsService.calculate(order) + VoucherAdjustmentsService.new(order).calculate order.update_totals_and_states end diff --git a/app/models/voucher.rb b/app/models/voucher.rb index 60bf1b8cf2..f74fa6101c 100644 --- a/app/models/voucher.rb +++ b/app/models/voucher.rb @@ -22,7 +22,7 @@ class Voucher < ApplicationRecord # the same method to stay as consistent as possible. # # Creates a new voucher adjustment for the given order with an amount of 0 - # The amount will be calculated via VoucherAdjustmentsService.calculate + # The amount will be calculated via VoucherAdjustmentsService#calculate def create_adjustment(label, order) adjustment_attributes = { amount: 0, diff --git a/app/services/voucher_adjustments_service.rb b/app/services/voucher_adjustments_service.rb index 3b0201d0af..3d23f8e11e 100644 --- a/app/services/voucher_adjustments_service.rb +++ b/app/services/voucher_adjustments_service.rb @@ -1,43 +1,49 @@ # frozen_string_literal: true class VoucherAdjustmentsService - def self.calculate(order) - return if order.nil? + def initialize(order) + @order = order + end + + def calculate + 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 # Calculate value - amount = adjustment.originator.compute_amount(order) + 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 end - def self.handle_tax_excluded_from_price(order, amount) - voucher_rate = amount / order.pre_discount_total + private - adjustment = order.voucher_adjustments.first + 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_attributes = { originator: adjustment.originator, - order: order, + order: @order, label: "Tax #{adjustment.label}", mandatory: false, state: 'open', @@ -46,12 +52,12 @@ class VoucherAdjustmentsService } # Update the amount if tax adjustment already exist, create if not - tax_adjustment = order.adjustments.find_or_initialize_by(adjustment_attributes) + tax_adjustment = @order.adjustments.find_or_initialize_by(adjustment_attributes) tax_adjustment.amount = tax_amount tax_adjustment.save # Update the adjustment amount - adjustment_amount = voucher_rate * (order.pre_discount_total - order.additional_tax_total) + adjustment_amount = voucher_rate * (@order.pre_discount_total - @order.additional_tax_total) adjustment.update_columns( amount: adjustment_amount, @@ -59,12 +65,12 @@ class VoucherAdjustmentsService ) end - def self.handle_tax_included_in_price(order, amount) - voucher_rate = amount / order.pre_discount_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 @@ -74,6 +80,4 @@ class VoucherAdjustmentsService updated_at: Time.zone.now ) end - - private_class_method :handle_tax_included_in_price, :handle_tax_excluded_from_price end diff --git a/spec/services/voucher_adjustments_service_spec.rb b/spec/services/voucher_adjustments_service_spec.rb index 7cc56e0b81..0d61de9121 100644 --- a/spec/services/voucher_adjustments_service_spec.rb +++ b/spec/services/voucher_adjustments_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe VoucherAdjustmentsService do - describe '.calculate' do + describe '#calculate' do let(:enterprise) { build(:enterprise) } let(:voucher) { create(:voucher, code: 'new_code', enterprise: enterprise, amount: 10) } @@ -16,7 +16,7 @@ describe VoucherAdjustmentsService do voucher.create_adjustment(voucher.code, order) order.update_columns(item_total: 6) - VoucherAdjustmentsService.calculate(order) + VoucherAdjustmentsService.new(order).calculate expect(subject.amount.to_f).to eq(-6.0) end @@ -46,7 +46,7 @@ describe VoucherAdjustmentsService do order.update_shipping_fees! order.update_order! - VoucherAdjustmentsService.calculate(order) + VoucherAdjustmentsService.new(order).calculate end it 'updates the adjustment included_tax' do @@ -60,13 +60,13 @@ describe VoucherAdjustmentsService do context "when re calculating" do it "does not update the adjustment amount" do expect do - VoucherAdjustmentsService.calculate(order) + VoucherAdjustmentsService.new(order).calculate end.not_to change { subject.reload.amount } end it "does not update the tax adjustment" do expect do - VoucherAdjustmentsService.calculate(order) + VoucherAdjustmentsService.new(order).calculate() end.not_to change { subject.reload.included_tax } end @@ -77,13 +77,13 @@ describe VoucherAdjustmentsService do it "does update the adjustment amount" do expect do - VoucherAdjustmentsService.calculate(order) + VoucherAdjustmentsService.new(order).calculate end.not_to change { subject.reload.amount } end it "updates the tax adjustment" do expect do - VoucherAdjustmentsService.calculate(order) + VoucherAdjustmentsService.new(order).calculate end.to change { subject.reload.included_tax } end end @@ -114,7 +114,7 @@ describe VoucherAdjustmentsService do order.update_shipping_fees! order.update_order! - VoucherAdjustmentsService.calculate(order) + VoucherAdjustmentsService.new(order).calculate end it 'includes amount without tax' do @@ -137,13 +137,13 @@ describe VoucherAdjustmentsService do context "when re calculating" do it "does not update the adjustment amount" do expect do - VoucherAdjustmentsService.calculate(order) + VoucherAdjustmentsService.new(order).calculate end.not_to change { adjustment.reload.amount } end it "does not update the tax adjustment" do expect do - VoucherAdjustmentsService.calculate(order) + VoucherAdjustmentsService.new(order).calculate end.not_to change { tax_adjustment.reload.amount } end @@ -154,13 +154,13 @@ describe VoucherAdjustmentsService do it "updates the adjustment amount" do expect do - VoucherAdjustmentsService.calculate(order) + VoucherAdjustmentsService.new(order).calculate end.to change { adjustment.reload.amount } end it "updates the tax adjustment" do expect do - VoucherAdjustmentsService.calculate(order) + VoucherAdjustmentsService.new(order).calculate end.to change { tax_adjustment.reload.amount } end end @@ -169,7 +169,7 @@ describe VoucherAdjustmentsService do context 'when no order given' do it "doesn't blow up" do - expect { VoucherAdjustmentsService.calculate(nil) }.to_not raise_error + expect { VoucherAdjustmentsService.new(nil).calculate }.to_not raise_error end end @@ -177,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).calculate }.to_not raise_error end end end diff --git a/spec/system/consumer/split_checkout_spec.rb b/spec/system/consumer/split_checkout_spec.rb index 2f957238d6..8d19772eda 100644 --- a/spec/system/consumer/split_checkout_spec.rb +++ b/spec/system/consumer/split_checkout_spec.rb @@ -1119,7 +1119,7 @@ describe "As a consumer, I want to checkout my order" do before do voucher.create_adjustment(voucher.code, order) order.update_totals - VoucherAdjustmentsService.calculate(order) + VoucherAdjustmentsService.new(order).calculate visit checkout_step_path(:summary) end