Merge pull request #6678 from Matt-Yorkley/soft-deprecate-tax-rates

Introduce soft-deprecation strategy when modifying tax rates

app/controllers/spree/admin/tax_rates_controller.rb

@@ -3,8 +3,38 @@ 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?
+
+        transition_tax_rate
+      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 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
         @available_zones = Zone.order(:name)
         @available_categories = TaxCategory.order(:name)

app/services/tax_rate_updater.rb

@@ -0,0 +1,37 @@
+# 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!
+    ActiveRecord::Base.transaction do
+      updated_rate.save && current_rate.destroy
+    end
+  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

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

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