Merge pull request #11219 from abdellani/fix-tax-rates-rendering-when-invoice-enabled

fix Viewing an invoice with the instance's invoice setting set to alternative model leads to an error 500

.rubocop_todo.yml

@@ -210,6 +210,7 @@ Metrics/ClassLength:
     - 'app/controllers/spree/admin/users_controller.rb'
     - 'app/controllers/spree/orders_controller.rb'
     - 'app/models/enterprise.rb'
+    - 'app/models/invoice/data_presenter.rb'
     - 'app/models/order_cycle.rb'
     - 'app/models/product_import/entry_processor.rb'
     - 'app/models/product_import/entry_validator.rb'

app/models/invoice/data_presenter.rb

@@ -2,6 +2,7 @@
 
 class Invoice
   class DataPresenter
+    include ::ActionView::Helpers::NumberHelper
     attr_reader :invoice
 
     delegate :data, to: :invoice
@@ -50,9 +51,9 @@ class Invoice
     end
 
     def checkout_adjustments(exclude: [], reject_zero_amount: true)
-      adjustments = all_eligible_adjustments
+      adjustments = all_eligible_adjustments.map(&:clone)
 
-      adjustments.reject! { |a| a.originator_type == 'Spree::TaxRate' }
+      adjustments.reject! { |a| a.originator.type == 'Spree::TaxRate' }
 
       if exclude.include? :line_item
         adjustments.reject! { |a|
@@ -68,11 +69,10 @@ class Invoice
     end
 
     def display_checkout_taxes_hash
-      totals = OrderTaxAdjustmentsFetcher.new(nil).totals(all_tax_adjustments)
-
-      totals.map do |tax_rate, tax_amount|
+      tax_adjustment_totals.map do |tax_rate_id, tax_amount|
+        tax_rate = tax_rate_by_id[tax_rate_id]
         {
-          amount: Spree::Money.new(tax_amount, currency: order.currency),
+          amount: Spree::Money.new(tax_amount, currency:),
           percentage: number_to_percentage(tax_rate.amount * 100, precision: 1),
           rate_amount: tax_rate.amount,
         }
@@ -83,8 +83,20 @@ class Invoice
       I18n.l(invoice_date.to_date, format: :long)
     end
 
+    def tax_adjustment_totals
+      all_tax_adjustments.each_with_object(Hash.new(0)) do |adjustment, totals|
+        totals[adjustment.originator.id] += adjustment.amount
+      end
+    end
+
+    def tax_rate_by_id
+      all_tax_adjustments.each_with_object({}) do |adjustment, tax_rates|
+        tax_rates[adjustment.originator.id] = adjustment.originator
+      end
+    end
+
     def all_tax_adjustments
-      all_eligible_adjustments.select { |a| a.originator_type == 'Spree::TaxRate' }
+      all_eligible_adjustments.select { |a| a.originator.type == 'Spree::TaxRate' }
     end
 
     def paid?

app/models/invoice/data_presenter/adjustable.rb

@@ -0,0 +1,20 @@
+# frozen_string_literal: false
+
+class Invoice
+  class DataPresenter
+    class Adjustable < Invoice::DataPresenter::Base
+      attributes :id, :type, :currency, :included_tax_total, :additional_tax_total, :amount
+
+      def display_taxes(display_zero: false)
+        if included_tax_total.positive?
+          amount = Spree::Money.new(included_tax_total, currency:)
+          I18n.t(:tax_amount_included, amount:)
+        elsif additional_tax_total.positive?
+          Spree::Money.new(additional_tax_total, currency:)
+        elsif display_zero
+          Spree::Money.new(0.00, currency:)
+        end
+      end
+    end
+  end
+end

app/models/invoice/data_presenter/adjustment.rb

@@ -4,7 +4,10 @@ class Invoice
   class DataPresenter
     class Adjustment < Invoice::DataPresenter::Base
       attributes :additional_tax_total, :adjustable_type, :amount, :currency, :included_tax_total,
-                 :label, :originator_type
+                 :label
+      array_attribute :tax_rates, class_name: 'TaxRate'
+      attributes_with_presenter :originator, class_name: 'AdjustmentOriginator'
+      attributes_with_presenter :adjustable
       invoice_generation_attributes :additional_tax_total, :adjustable_type, :amount,
                                     :included_tax_total
       invoice_update_attributes :label
@@ -23,6 +26,10 @@ class Invoice
           Spree::Money.new(0.00, currency:)
         end
       end
+
+      def display_adjustment_tax_rates
+        tax_rates.map { |tr| number_to_percentage(tr.amount * 100, precision: 1) }.join(", ")
+      end
     end
   end
 end

app/models/invoice/data_presenter/adjustment_originator.rb

@@ -0,0 +1,9 @@
+# frozen_string_literal: false
+
+class Invoice
+  class DataPresenter
+    class AdjustmentOriginator < Invoice::DataPresenter::Base
+      attributes :id, :type, :amount
+    end
+  end
+end

app/models/invoice/data_presenter/base.rb

@@ -3,6 +3,7 @@
 class Invoice
   class DataPresenter
     class Base
+      include ::ActionView::Helpers::NumberHelper
       attr_reader :data
 
       def initialize(data)

app/models/invoice/data_presenter/line_item.rb

@@ -6,6 +6,7 @@ class Invoice
       attributes :added_tax, :currency, :included_tax, :price_with_adjustments, :quantity,
                  :variant_id
       attributes_with_presenter :variant
+      array_attribute :tax_rates, class_name: 'TaxRate'
       invoice_generation_attributes :added_tax, :included_tax, :price_with_adjustments,
                                     :quantity, :variant_id
 
@@ -28,6 +29,10 @@ class Invoice
           Spree::Money.new(0.00, currency:)
         end
       end
+
+      def display_line_item_tax_rates
+        tax_rates.map { |tr| number_to_percentage(tr.amount * 100, precision: 1) }.join(", ")
+      end
     end
   end
 end

app/models/invoice/data_presenter/tax_rate.rb

@@ -0,0 +1,9 @@
+# frozen_string_literal: false
+
+class Invoice
+  class DataPresenter
+    class TaxRate < Invoice::DataPresenter::Base
+      attributes :id, :amount
+    end
+  end
+end

app/models/invoice/data_presenter_attributes.rb

@@ -12,7 +12,7 @@ class Invoice
       end
     end
 
-    def attributes_with_presenter(*attributes)
+    def attributes_with_presenter(*attributes, class_name: nil)
       attributes.each do |attribute|
         define_method(attribute) do
           instance_variable = instance_variable_get("@#{attribute}")
@@ -20,7 +20,7 @@ class Invoice
 
           instance_variable_set("@#{attribute}",
                                 Invoice::DataPresenter.const_get(
-                                  attribute.to_s.classify
+                                  class_name.presence || attribute.to_s.classify
                                 ).new(data&.[](attribute)))
         end
       end

app/serializers/invoice/adjustable_serializer.rb

@@ -0,0 +1,18 @@
+# frozen_string_literal: false
+
+class Invoice
+  class AdjustableSerializer < ActiveModel::Serializer
+    attributes :id, :type, :currency, :included_tax_total, :additional_tax_total, :amount
+    def type
+      object.class.name
+    end
+
+    [:currency, :included_tax_total, :additional_tax_total, :amount].each do |method|
+      define_method method do
+        return nil unless object.respond_to?(method)
+
+        object.public_send(method).to_f
+      end
+    end
+  end
+end

app/serializers/invoice/adjustment_originator_serializer.rb

@@ -0,0 +1,16 @@
+# frozen_string_literal: false
+
+class Invoice
+  class AdjustmentOriginatorSerializer < ActiveModel::Serializer
+    attributes :id, :type, :amount
+    def type
+      object.class.name
+    end
+
+    def amount
+      return nil unless object.respond_to?(:amount)
+
+      object.amount.to_f
+    end
+  end
+end

app/serializers/invoice/adjustment_serializer.rb

@@ -4,5 +4,12 @@ class Invoice
   class AdjustmentSerializer < ActiveModel::Serializer
     attributes :adjustable_type, :label, :included_tax_total, :additional_tax_total, :amount,
                :currency
+    has_one :originator, serializer: Invoice::AdjustmentOriginatorSerializer
+    has_one :adjustable, serializer: Invoice::AdjustableSerializer
+    has_many :tax_rates, serializer: Invoice::TaxRateSerializer
+
+    def tax_rates
+      TaxRateFinder.tax_rates_of(object)
+    end
   end
 end

app/serializers/invoice/line_item_serializer.rb

@@ -5,5 +5,6 @@ class Invoice
     attributes :id, :added_tax, :currency, :included_tax, :price_with_adjustments, :quantity,
                :variant_id
     has_one :variant, serializer: Invoice::VariantSerializer
+    has_many :tax_rates, serializer: Invoice::TaxRateSerializer
   end
 end

app/serializers/invoice/tax_rate_serializer.rb

@@ -0,0 +1,7 @@
+# frozen_string_literal: false
+
+class Invoice
+  class TaxRateSerializer < ActiveModel::Serializer
+    attributes :id, :amount
+  end
+end

app/views/spree/admin/orders/_invoice_table3.html.haml

@@ -25,14 +25,14 @@
           = item.display_amount_with_adjustments
       
     - @order.checkout_adjustments(exclude: [:line_item]).reverse_each do |adjustment|
-      - taxable = adjustment#.adjustable_type == "Spree::Shipment" ? adjustment.adjustable : adjustment
+      - taxable = adjustment.adjustable_type == "Spree::Shipment" ? adjustment.adjustable : adjustment
       %tr
         %td
           %strong= "#{raw(adjustment.label)}"
         %td{:align => "right"}
           1
         %td{:align => "right"}
-          = adjustment.display_taxes
+          = taxable.display_taxes
         %td{:align => "right"}
           = adjustment.display_amount
   %tfoot

app/views/spree/admin/orders/_invoice_table4.html.haml

@@ -28,7 +28,7 @@
           = item.display_amount_with_adjustments
         - if @order.total_tax > 0
           %td{:align => "right"}
-            = display_line_item_tax_rates(item)
+            = item.display_line_item_tax_rates
 
     - @order.checkout_adjustments(exclude: [:line_item]).reverse_each do |adjustment|
       %tr
@@ -40,7 +40,7 @@
           = adjustment.display_amount
         - if @order.total_tax > 0
           %td{:align => "right"}
-            = display_adjustment_tax_rates(adjustment)
+            = adjustment.display_adjustment_tax_rates
   %tfoot
     %tr
       %td{:align => "right", :colspan => "3"}

spec/system/admin/invoice_print_spec.rb

@@ -35,7 +35,15 @@ describe '
     Capybara.use_default_driver
   end
 
-  describe "that contains right Payment Description at Checkout information" do
+  shared_examples "contains right Payment Description at Checkout information" do
+    let(:url_params) {
+      if OpenFoodNetwork::FeatureToggle.enabled?(:invoices)
+        { invoice_id: order.invoices.first.id }
+      else
+        {}
+      end
+    }
+
     let!(:payment_method1) do
       create(:stripe_sca_payment_method, distributors: [distributor], description: "description1")
     end
@@ -45,8 +53,9 @@ describe '
 
     context "with no payment" do
       it "do not display the payment description information" do
+        order.invoices.create!
         login_as_admin
-        visit spree.print_admin_order_path(order)
+        visit spree.print_admin_order_path(order, params: url_params)
         convert_pdf_to_page
         expect(page).to have_no_content 'Payment Description at Checkout'
       end
@@ -58,11 +67,12 @@ describe '
       end
       before do
         order.save!
+        order.invoices.create!
       end
 
       it "display the payment description section" do
         login_as_admin
-        visit spree.print_admin_order_path(order)
+        visit spree.print_admin_order_path(order, params: url_params)
         convert_pdf_to_page
         expect(page).to have_content 'Payment Description at Checkout'
         expect(page).to have_content 'description1'
@@ -76,13 +86,15 @@ describe '
                                                        payment_method: payment_method1,
                                                        created_at: 1.day.ago)
         order.payments << create(:payment, order:, state: 'failed',
-                                           payment_method: payment_method2, created_at: 2.days.ago)
+                                           payment_method: payment_method2,
+                                           created_at: 2.days.ago)
         order.save!
+        order.invoices.create!
       end
 
       it "display the payment description section and use the one from the completed payment" do
         login_as_admin
-        visit spree.print_admin_order_path(order)
+        visit spree.print_admin_order_path(order, params: url_params)
         convert_pdf_to_page
         expect(page).to have_content 'Payment Description at Checkout'
         expect(page).to have_content 'description1'
@@ -99,29 +111,37 @@ describe '
                                                        payment_method: payment_method2,
                                                        created_at: 1.day.ago)
         order.save!
+        order.invoices.create!
       end
 
       it "display the payment description section and use the one from the last payment" do
         login_as_admin
-        visit spree.print_admin_order_path(order)
+        visit spree.print_admin_order_path(order, params: url_params)
         convert_pdf_to_page
         expect(page).to have_content 'Payment Description at Checkout'
         expect(page).to have_content 'description2'
       end
     end
   end
-
   shared_examples "Check display on each invoice: legacy and alternative" do |alternative_invoice|
     let!(:completed_order) do
       create(:completed_order_with_fees, distributor:, order_cycle:,
                                          user: create(:user, email: "xxxxxx@example.com"),
                                          bill_address: create(:address, phone: '1234567890'))
     end
+    let(:url_params) {
+      if OpenFoodNetwork::FeatureToggle.enabled?(:invoices)
+        { invoice_id: completed_order.invoices.first.id }
+      else
+        {}
+      end
+    }
 
     before do
+      completed_order.invoices.create!
       allow(Spree::Config).to receive(:invoice_style2?).and_return(alternative_invoice)
       login_as_admin
-      visit spree.print_admin_order_path(completed_order)
+      visit spree.print_admin_order_path(completed_order, params: url_params)
       convert_pdf_to_page
     end
 
@@ -131,10 +151,7 @@ describe '
     end
   end
 
-  it_behaves_like "Check display on each invoice: legacy and alternative", false
-  it_behaves_like "Check display on each invoice: legacy and alternative", true
-
-  describe "an order with taxes" do
+  shared_examples "order with tax" do
     let(:user1) { create(:user, enterprises: [distributor]) }
     let!(:zone) { create(:zone_with_member) }
     let(:address) { create(:address) }
@@ -146,8 +163,12 @@ describe '
       let(:enterprise_fee_rate_included) {
         create(:tax_rate, amount: 0.15, included_in_price: true, zone:)
       }
-      let(:shipping_tax_category) { create(:tax_category, tax_rates: [shipping_tax_rate_included]) }
-      let(:fee_tax_category) { create(:tax_category, tax_rates: [enterprise_fee_rate_included]) }
+      let(:shipping_tax_category) {
+        create(:tax_category, tax_rates: [shipping_tax_rate_included])
+      }
+      let(:fee_tax_category) {
+        create(:tax_category, tax_rates: [enterprise_fee_rate_included])
+      }
       let!(:shipping_method) {
         create(:shipping_method_with, :expensive_name, distributors: [distributor],
                                                        tax_category: shipping_tax_category)
@@ -158,9 +179,11 @@ describe '
                                 calculator: Calculator::FlatRate.new(preferred_amount: 120.0))
       }
       let!(:order_cycle) {
-        create(:simple_order_cycle, coordinator: distributor,
-                                    coordinator_fees: [enterprise_fee], distributors: [distributor],
-                                    variants: [product1.variants.first, product2.variants.first])
+        create(:simple_order_cycle,
+               coordinator: distributor,
+               coordinator_fees: [enterprise_fee],
+               distributors: [distributor],
+               variants: [product1.variants.first, product2.variants.first])
       }
 
       let!(:order1) {
@@ -185,6 +208,14 @@ describe '
                            order: order1)
       }
 
+      let(:url_params) {
+        if OpenFoodNetwork::FeatureToggle.enabled?(:invoices)
+          { invoice_id: order1.invoices.first.id }
+        else
+          {}
+        end
+      }
+
       before do
         order1.reload
         while !order1.delivery?
@@ -201,13 +232,14 @@ describe '
         while !order1.complete?
           break if !order1.next!
         end
+        order1.invoices.create!
       end
 
       context "legacy invoice" do
         before do
           allow(Spree::Config).to receive(:invoice_style2?).and_return(false)
           login_as_admin
-          visit spree.print_admin_order_path(order1)
+          visit spree.print_admin_order_path(order1, params: url_params)
           convert_pdf_to_page
         end
 
@@ -225,8 +257,12 @@ describe '
           expect(page).to have_content "(1g)" # display as
           expect(page).to have_content "3 $250.08 $1,500.45"
           # Enterprise fee
-          expect(page).to have_content "Whole order - #{enterprise_fee.name} fee by coordinator " \
-                                       "#{user1.enterprises.first.name} 1 $15.65 (included) $120.00"
+          expect(page).to have_content "Whole order - #{
+                                        enterprise_fee.name
+                                      } fee by coordinator " \
+                                       "#{
+                                        user1.enterprises.first.name
+                                      } 1 $15.65 (included) $120.00"
           # Shipping
           expect(page).to have_content "Shipping 1 $9.14 (included) $100.55"
           # Order Totals
@@ -240,7 +276,7 @@ describe '
         before do
           allow(Spree::Config).to receive(:invoice_style2?).and_return(true)
           login_as_admin
-          visit spree.print_admin_order_path(order1)
+          visit spree.print_admin_order_path(order1, params: url_params)
           convert_pdf_to_page
         end
 
@@ -280,7 +316,9 @@ describe '
       let(:enterprise_fee_rate_added) {
         create(:tax_rate, amount: 0.15, included_in_price: false, zone:)
       }
-      let(:shipping_tax_category) { create(:tax_category, tax_rates: [shipping_tax_rate_added]) }
+      let(:shipping_tax_category) {
+        create(:tax_category, tax_rates: [shipping_tax_rate_added])
+      }
       let(:fee_tax_category) { create(:tax_category, tax_rates: [enterprise_fee_rate_added]) }
       let!(:shipping_method) {
         create(:shipping_method_with, :expensive_name, distributors: [distributor],
@@ -293,7 +331,8 @@ describe '
       }
       let(:order_cycle2) {
         create(:simple_order_cycle, coordinator: distributor,
-                                    coordinator_fees: [enterprise_fee], distributors: [distributor],
+                                    coordinator_fees: [enterprise_fee],
+                                    distributors: [distributor],
                                     variants: [product3.variants.first, product4.variants.first])
       }
 
@@ -360,8 +399,12 @@ describe '
           # header
           expect(page).to have_content "Item Qty GST Price"
           # Enterprise fee
-          expect(page).to have_content "Whole order - #{enterprise_fee.name} fee by coordinator " \
-                                       "#{user1.enterprises.first.name} 1 $18.00 $120.00"
+          expect(page).to have_content "Whole order - #{
+                                        enterprise_fee.name
+                                      } fee by coordinator " \
+                                       "#{
+                                        user1.enterprises.first.name
+                                      } 1 $18.00 $120.00"
           # Shipping
           expect(page).to have_content "Shipping 1 $10.06 $100.55"
           # Order Totals
@@ -407,6 +450,25 @@ describe '
       end
     end
   end
+
+  context "when invoice feature is not enabled" do
+    before do
+      Flipper.disable(:invoices)
+    end
+    it_behaves_like "contains right Payment Description at Checkout information"
+    it_behaves_like "Check display on each invoice: legacy and alternative", false
+    it_behaves_like "Check display on each invoice: legacy and alternative", true
+    it_behaves_like "order with tax"
+  end
+  context "when invoice feature is enabled" do
+    before do
+      Flipper.enable(:invoice)
+    end
+    it_behaves_like "contains right Payment Description at Checkout information"
+    it_behaves_like "Check display on each invoice: legacy and alternative", false
+    it_behaves_like "Check display on each invoice: legacy and alternative", true
+    it_behaves_like "order with tax"
+  end
 end
 
 def convert_pdf_to_page