From 8cc4c6a63f41a821955eb17dbc2f2f50c9c67e49 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Fri, 15 Jan 2021 12:00:01 +0000
Subject: [PATCH 1/3] Introduce soft-deprecation strategy when modifying tax
 rates

If an admin changes the amount on a tax rate, and that rate has been used by adjustments in the past, we need to soft-delete and clone it to preserve the data integrity of previous adjustments that were created using that rate.
---
 .../spree/admin/tax_rates_controller.rb       | 36 +++++++++++++
 .../spree/admin/tax_rates_controller_spec.rb  | 54 +++++++++++++++++++
 2 files changed, 90 insertions(+)
 create mode 100644 spec/controllers/spree/admin/tax_rates_controller_spec.rb

diff --git a/app/controllers/spree/admin/tax_rates_controller.rb b/app/controllers/spree/admin/tax_rates_controller.rb
index 2df303bc75..e3389b3e0c 100644
--- a/app/controllers/spree/admin/tax_rates_controller.rb
+++ b/app/controllers/spree/admin/tax_rates_controller.rb
@@ -3,8 +3,44 @@ module Spree
     class TaxRatesController < ::Admin::ResourceController
       before_action :load_data
 
+      def update
+        return super unless amount_changed? && associated_adjustments?
+
+        # If a TaxRate is modified in production and the amount is changed, we need to clone
+        # and soft-delete it to preserve associated data on previous orders. For example; previous
+        # orders will have adjustments created with that rate. Those old orders will keep the
+        # rate they had when they were created, and new orders will have the new rate applied.
+
+        cloned_rate = clone_tax_rate(@tax_rate)
+        cloned_rate.assign_attributes(permitted_resource_params)
+
+        if cloned_rate.save
+          @tax_rate.destroy
+
+          redirect_to location_after_save,
+                      flash: { success: flash_message_for(cloned_rate, :successfully_updated) }
+        else
+          redirect_to spree.edit_admin_tax_rate_path(@tax_rate),
+                      flash: { error: cloned_rate.errors.full_messages.to_sentence }
+        end
+      end
+
       private
 
+      def amount_changed?
+        BigDecimal(permitted_resource_params[:amount]) != @tax_rate.amount
+      end
+
+      def associated_adjustments?
+        Spree::Adjustment.where(originator: @tax_rate).exists?
+      end
+
+      def clone_tax_rate(tax_rate)
+        cloned_rate = tax_rate.deep_dup
+        cloned_rate.calculator = tax_rate.calculator.deep_dup
+        cloned_rate
+      end
+
       def load_data
         @available_zones = Zone.order(:name)
         @available_categories = TaxCategory.order(:name)
diff --git a/spec/controllers/spree/admin/tax_rates_controller_spec.rb b/spec/controllers/spree/admin/tax_rates_controller_spec.rb
new file mode 100644
index 0000000000..266edc9430
--- /dev/null
+++ b/spec/controllers/spree/admin/tax_rates_controller_spec.rb
@@ -0,0 +1,54 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+module Spree
+  module Admin
+    describe TaxRatesController, type: :controller do
+      include AuthenticationHelper
+
+      let!(:tax_rate) {
+        create(:tax_rate, name: "Original Rate", amount: 0.1, calculator: build(:calculator))
+      }
+
+      describe "#update" do
+        before { controller_login_as_admin }
+
+        context "when the tax rate has associated adjustments" do
+          let!(:adjustment) { create(:adjustment, originator: tax_rate) }
+
+          context "when the amount is not changed" do
+            it "updates the record" do
+              expect {
+                spree_put :update, id: tax_rate.id, tax_rate: { name: "Updated Rate", amount: 0.1 }
+              }.to_not change{ Spree::TaxRate.with_deleted.count }
+
+              expect(response).to redirect_to spree.admin_tax_rates_url
+              expect(tax_rate.reload.name).to eq "Updated Rate"
+              expect(tax_rate.amount).to eq 0.1
+            end
+          end
+
+          context "when the amount is changed" do
+            it "duplicates the record and soft-deletes the duplicate" do
+              expect {
+                spree_put :update, id: tax_rate.id, tax_rate: { name: "Changed Rate", amount: 0.5 }
+              }.to change{ Spree::TaxRate.with_deleted.count }.by(1)
+
+              expect(response).to redirect_to spree.admin_tax_rates_url
+
+              deprecated_rate = tax_rate.reload
+              expect(deprecated_rate.name).to eq "Original Rate"
+              expect(deprecated_rate.amount).to eq 0.1
+              expect(deprecated_rate.deleted?).to be true
+
+              updated_rate = Spree::TaxRate.last
+              expect(updated_rate.name).to eq "Changed Rate"
+              expect(updated_rate.amount).to eq 0.5
+            end
+          end
+        end
+      end
+    end
+  end
+end

From 49c616c33cbf1868cf741985f63bfea6ef6729ea Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Sat, 16 Jan 2021 16:39:46 +0000
Subject: [PATCH 2/3] Extract tax rate transition logic to service

---
 .../spree/admin/tax_rates_controller.rb       | 36 +++++++---------
 app/services/tax_rate_updater.rb              | 35 +++++++++++++++
 spec/services/tax_rate_updater_spec.rb        | 43 +++++++++++++++++++
 3 files changed, 93 insertions(+), 21 deletions(-)
 create mode 100644 app/services/tax_rate_updater.rb
 create mode 100644 spec/services/tax_rate_updater_spec.rb

diff --git a/app/controllers/spree/admin/tax_rates_controller.rb b/app/controllers/spree/admin/tax_rates_controller.rb
index e3389b3e0c..b1846bff65 100644
--- a/app/controllers/spree/admin/tax_rates_controller.rb
+++ b/app/controllers/spree/admin/tax_rates_controller.rb
@@ -3,26 +3,12 @@ module Spree
     class TaxRatesController < ::Admin::ResourceController
       before_action :load_data
 
+      delegate :transition_rate!, :updated_rate, to: :updater
+
       def update
         return super unless amount_changed? && associated_adjustments?
 
-        # If a TaxRate is modified in production and the amount is changed, we need to clone
-        # and soft-delete it to preserve associated data on previous orders. For example; previous
-        # orders will have adjustments created with that rate. Those old orders will keep the
-        # rate they had when they were created, and new orders will have the new rate applied.
-
-        cloned_rate = clone_tax_rate(@tax_rate)
-        cloned_rate.assign_attributes(permitted_resource_params)
-
-        if cloned_rate.save
-          @tax_rate.destroy
-
-          redirect_to location_after_save,
-                      flash: { success: flash_message_for(cloned_rate, :successfully_updated) }
-        else
-          redirect_to spree.edit_admin_tax_rate_path(@tax_rate),
-                      flash: { error: cloned_rate.errors.full_messages.to_sentence }
-        end
+        transition_tax_rate
       end
 
       private
@@ -35,10 +21,18 @@ module Spree
         Spree::Adjustment.where(originator: @tax_rate).exists?
       end
 
-      def clone_tax_rate(tax_rate)
-        cloned_rate = tax_rate.deep_dup
-        cloned_rate.calculator = tax_rate.calculator.deep_dup
-        cloned_rate
+      def transition_tax_rate
+        if transition_rate!
+          redirect_to location_after_save,
+                      flash: { success: flash_message_for(updated_rate, :successfully_updated) }
+        else
+          redirect_to spree.edit_admin_tax_rate_path(@tax_rate),
+                      flash: { error: updated_rate.errors.full_messages.to_sentence }
+        end
+      end
+
+      def updater
+        @updater ||= TaxRateUpdater.new(@tax_rate, permitted_resource_params)
       end
 
       def load_data
diff --git a/app/services/tax_rate_updater.rb b/app/services/tax_rate_updater.rb
new file mode 100644
index 0000000000..683b76b9ab
--- /dev/null
+++ b/app/services/tax_rate_updater.rb
@@ -0,0 +1,35 @@
+# frozen_string_literal: true
+
+# If a TaxRate is modified in production and the amount is changed, we need to clone
+# and soft-delete it to preserve associated data on previous orders. For example; previous
+# orders will have adjustments created with that rate. Those old orders will keep the
+# rate they had when they were created, and new orders will have the new rate applied.
+
+class TaxRateUpdater
+  def initialize(current_rate, permitted_params)
+    @current_rate = current_rate
+    @permitted_params = permitted_params
+  end
+
+  def updated_rate
+    @updated_rate ||= begin
+      clone = clone_tax_rate!
+      clone.assign_attributes(permitted_params)
+      clone
+    end
+  end
+
+  def transition_rate!
+    updated_rate.save && current_rate.destroy
+  end
+
+  private
+
+  attr_reader :current_rate, :permitted_params
+
+  def clone_tax_rate!
+    cloned_rate = current_rate.deep_dup
+    cloned_rate.calculator = current_rate.calculator.deep_dup
+    cloned_rate
+  end
+end
diff --git a/spec/services/tax_rate_updater_spec.rb b/spec/services/tax_rate_updater_spec.rb
new file mode 100644
index 0000000000..6f030f9108
--- /dev/null
+++ b/spec/services/tax_rate_updater_spec.rb
@@ -0,0 +1,43 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe TaxRateUpdater do
+  let!(:old_tax_rate) {
+    create(:tax_rate, name: "Test Rate", amount: 0.2, calculator: Calculator::DefaultTax.new)
+  }
+  let(:params) { { amount: 0.5 } }
+  let(:service) { TaxRateUpdater.new(old_tax_rate, params) }
+  let(:new_tax_rate) { service.updated_rate }
+
+  describe "#updated_rate" do
+    it "returns a cloned (unsaved) tax rate with the new attributes assigned" do
+      expect(new_tax_rate).to_not be old_tax_rate
+      expect(new_tax_rate.amount).to eq params[:amount]
+      expect(new_tax_rate.id).to be_nil
+      expect(new_tax_rate.calculator.class).to eq old_tax_rate.calculator.class
+      expect(new_tax_rate).to be_valid
+    end
+  end
+
+  describe "#transition_rate!" do
+    it "saves the new tax_rate and deletes the old tax_rate" do
+      expect(new_tax_rate).to receive(:save).and_call_original
+      expect(old_tax_rate).to receive(:destroy).and_call_original
+
+      expect(service.transition_rate!).to be_truthy
+
+      expect(new_tax_rate.reload.persisted?).to be true
+      expect(old_tax_rate.reload.deleted?).to be true
+    end
+
+    context "when saving the new tax_rate fails" do
+      it "does not delete the old tax_rate and returns a falsey value" do
+        expect(new_tax_rate).to receive(:save) { false }
+        expect(old_tax_rate).to_not receive(:destroy)
+
+        expect(service.transition_rate!).to_not be_truthy
+      end
+    end
+  end
+end

From 389e149ded1f253b09b5e1b4fa739153b968f334 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Wed, 3 Feb 2021 11:54:54 +0000
Subject: [PATCH 3/3] Wrap updates in a transaction block

Ensures the operation will be rolled ack if either saving the new record or deleting the old record fail
---
 app/services/tax_rate_updater.rb | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/app/services/tax_rate_updater.rb b/app/services/tax_rate_updater.rb
index 683b76b9ab..d02754640f 100644
--- a/app/services/tax_rate_updater.rb
+++ b/app/services/tax_rate_updater.rb
@@ -20,7 +20,9 @@ class TaxRateUpdater
   end
 
   def transition_rate!
-    updated_rate.save && current_rate.destroy
+    ActiveRecord::Base.transaction do
+      updated_rate.save && current_rate.destroy
+    end
   end
 
   private