From d9efd10ac0758c7522ce770669bcde4073e9f70a Mon Sep 17 00:00:00 2001 From: Mohamed ABDELLANI Date: Sun, 16 Apr 2023 08:59:40 +0100 Subject: [PATCH] update attributes relevant for the comparaison --- app/models/invoice/data_presenter.rb | 170 ++++++++++-------- app/models/invoice/data_presenter/address.rb | 2 + .../invoice/data_presenter/adjustment.rb | 7 +- .../invoice/data_presenter/distributor.rb | 1 - .../invoice/data_presenter/line_item.rb | 8 +- app/models/invoice/data_presenter/payment.rb | 4 +- .../invoice/data_presenter/payment_method.rb | 3 +- .../invoice/data_presenter/shipping_method.rb | 3 +- app/models/invoice/data_presenter/variant.rb | 2 +- .../invoice/data_presenter_attributes.rb | 72 +++++--- .../invoice/line_item_serializer.rb | 8 +- app/serializers/invoice/order_serializer.rb | 7 +- .../invoice/payment_method_serializer.rb | 2 +- app/serializers/invoice/payment_serializer.rb | 2 +- app/serializers/invoice/variant_serializer.rb | 4 +- app/services/order_invoice_comparator.rb | 46 +++-- .../services/order_invoice_comparator_spec.rb | 92 +++++++--- 17 files changed, 261 insertions(+), 172 deletions(-) diff --git a/app/models/invoice/data_presenter.rb b/app/models/invoice/data_presenter.rb index 8772def6b6..4d0118a4d3 100644 --- a/app/models/invoice/data_presenter.rb +++ b/app/models/invoice/data_presenter.rb @@ -1,102 +1,114 @@ -class Invoice::DataPresenter - attr_reader :invoice - delegate :data, :date, to: :invoice +# frozen_string_literal: true - FINALIZED_NON_SUCCESSFUL_STATES = %w(canceled returned).freeze +class Invoice + class DataPresenter + attr_reader :invoice - extend Invoice::DataPresenterAttributes + delegate :data, :date, to: :invoice - attributes :included_tax_total, :additional_tax_total, :state, :total, :payment_total, - :currency - attributes :number, :note, :special_instructions, prefix: :order - attributes_with_presenter :order_cycle, :distributor, :customer, :ship_address, - :shipping_method, :bill_address + FINALIZED_NON_SUCCESSFUL_STATES = %w(canceled returned).freeze - array_attribute :sorted_line_items, class_name: 'LineItem' - array_attribute :all_eligible_adjustments, class_name: 'Adjustment' - array_attribute :payments, class_name: 'Payment' + extend Invoice::DataPresenterAttributes - relevant_attributes :order_note, :distributor, :sorted_line_items + attributes :additional_tax_total, :currency, :included_tax_total, :payment_total, + :shipping_method_id, :state, :total + attributes :number, :note, :special_instructions, prefix: :order + attributes_with_presenter :bill_address, :customer, :distributor, :ship_address, + :shipping_method, :order_cycle - def initialize(invoice) - @invoice = invoice - end - - def has_taxes_included - included_tax_total > 0 - end + array_attribute :sorted_line_items, class_name: 'LineItem' + array_attribute :all_eligible_adjustments, class_name: 'Adjustment' + array_attribute :payments, class_name: 'Payment' - def total_tax - additional_tax_total + included_tax_total - end + # if any of the following attributes is updated, a new invoice should be generated + invoice_generation_attributes :additional_tax_total, :all_eligible_adjustments, :bill_address, + :included_tax_total, :payments, :payment_total, :ship_address, + :shipping_method_id, :sorted_line_items, :total - def order_completed_at - return nil if data[:completed_at].blank? + # if any of the following attributes is updated, the latest invoice should be updated + invoice_update_attributes :order_note, :order_special_instructions, :state, + :all_eligible_adjustments, :payments - Time.zone.parse(data[:completed_at]) - end - - def checkout_adjustments(exclude: [], reject_zero_amount: true) - adjustments = all_eligible_adjustments - - if exclude.include? :line_item - adjustments.reject! { |a| - a.adjustable_type == 'Spree::LineItem' - } + def initialize(invoice) + @invoice = invoice end - if reject_zero_amount - adjustments.reject! { |a| a.amount == 0 } + def has_taxes_included + included_tax_total > 0 end - adjustments - end - - def invoice_date - date - end - - def paid? - data[:payment_state] == 'paid' || data[:payment_state] == 'credit_owed' - end - - def outstanding_balance? - !new_outstanding_balance.zero? - end - - def new_outstanding_balance - if state.in?(FINALIZED_NON_SUCCESSFUL_STATES) - -payment_total - else - total - payment_total + def total_tax + additional_tax_total + included_tax_total end - end - def outstanding_balance_label - new_outstanding_balance.negative? ? I18n.t(:credit_owed) : I18n.t(:balance_due) - end + def order_completed_at + return nil if data[:completed_at].blank? - def last_payment - payments.max_by(&:created_at) - end + Time.zone.parse(data[:completed_at]) + end - def last_payment_method - last_payment&.payment_method - end + def checkout_adjustments(exclude: [], reject_zero_amount: true) + adjustments = all_eligible_adjustments - def display_outstanding_balance - Spree::Money.new(new_outstanding_balance, currency: currency) - end + if exclude.include? :line_item + adjustments.reject! { |a| + a.adjustable_type == 'Spree::LineItem' + } + end - def display_checkout_tax_total - Spree::Money.new(total_tax, currency: currency) - end + if reject_zero_amount + adjustments.reject! { |a| a.amount == 0 } + end - def display_checkout_total_less_tax - Spree::Money.new(total - total_tax, currency: currency) - end + adjustments + end - def display_total - Spree::Money.new(total, currency: currency) + def invoice_date + date + end + + def paid? + data[:payment_state] == 'paid' || data[:payment_state] == 'credit_owed' + end + + def outstanding_balance? + !new_outstanding_balance.zero? + end + + def new_outstanding_balance + if state.in?(FINALIZED_NON_SUCCESSFUL_STATES) + -payment_total + else + total - payment_total + end + end + + def outstanding_balance_label + new_outstanding_balance.negative? ? I18n.t(:credit_owed) : I18n.t(:balance_due) + end + + def last_payment + payments.max_by(&:created_at) + end + + def last_payment_method + last_payment&.payment_method + end + + def display_outstanding_balance + Spree::Money.new(new_outstanding_balance, currency: currency) + end + + def display_checkout_tax_total + Spree::Money.new(total_tax, currency: currency) + end + + def display_checkout_total_less_tax + Spree::Money.new(total - total_tax, currency: currency) + end + + def display_total + Spree::Money.new(total, currency: currency) + end end end diff --git a/app/models/invoice/data_presenter/address.rb b/app/models/invoice/data_presenter/address.rb index 4aca60a7bb..a3a980270e 100644 --- a/app/models/invoice/data_presenter/address.rb +++ b/app/models/invoice/data_presenter/address.rb @@ -1,6 +1,8 @@ class Invoice::DataPresenter::Address < Invoice::DataPresenter::Base attributes :firstname, :lastname, :address1, :address2, :city, :zipcode, :company, :phone attributes_with_presenter :state + invoice_generation_attributes :firstname, :lastname, :address1, :address2, :city, :zipcode, :company, :phone + def full_name "#{firstname} #{lastname}".strip end diff --git a/app/models/invoice/data_presenter/adjustment.rb b/app/models/invoice/data_presenter/adjustment.rb index 9b514235a9..9d2ef5e21c 100644 --- a/app/models/invoice/data_presenter/adjustment.rb +++ b/app/models/invoice/data_presenter/adjustment.rb @@ -1,7 +1,8 @@ class Invoice::DataPresenter::Adjustment < Invoice::DataPresenter::Base - attributes :adjustable_type, :label, :included_tax_total, :additional_tax_total, :amount, - :currency - + attributes :additional_tax_total, :adjustable_type, :amount, :currency, :included_tax_total, + :label + invoice_generation_attributes :additional_tax_total, :adjustable_type, :amount, :included_tax_total + invoice_update_attributes :label def display_amount Spree::Money.new(amount, currency: currency) end diff --git a/app/models/invoice/data_presenter/distributor.rb b/app/models/invoice/data_presenter/distributor.rb index 2945305d09..d9af10520a 100644 --- a/app/models/invoice/data_presenter/distributor.rb +++ b/app/models/invoice/data_presenter/distributor.rb @@ -1,7 +1,6 @@ class Invoice::DataPresenter::Distributor < Invoice::DataPresenter::Base attributes :name, :abn, :acn, :logo_url, :display_invoice_logo, :invoice_text, :email_address attributes_with_presenter :contact, :address, :business_address - relevant_attributes :name def display_invoice_logo? display_invoice_logo == true diff --git a/app/models/invoice/data_presenter/line_item.rb b/app/models/invoice/data_presenter/line_item.rb index 1f4502560e..778eeedb93 100644 --- a/app/models/invoice/data_presenter/line_item.rb +++ b/app/models/invoice/data_presenter/line_item.rb @@ -1,7 +1,9 @@ -class Invoice::DataPresenter::LineItem < Invoice::DataPresenter::Base - attributes :quantity, :price_with_adjustments, :added_tax, :included_tax, :currency +class Invoice::DataPresenter::LineItem < Invoice::DataPresenter::Base + attributes :added_tax, :currency, :included_tax, :price_with_adjustments, :quantity, :variant_id attributes_with_presenter :variant - relevant_attributes :quantity + invoice_generation_attributes :added_tax, :included_tax, :price_with_adjustments, + :quantity, :variant_id + delegate :name_to_display, :options_text, to: :variant def display_amount_with_adjustments diff --git a/app/models/invoice/data_presenter/payment.rb b/app/models/invoice/data_presenter/payment.rb index 95b0490a10..f3125521fe 100644 --- a/app/models/invoice/data_presenter/payment.rb +++ b/app/models/invoice/data_presenter/payment.rb @@ -1,6 +1,8 @@ class Invoice::DataPresenter::Payment < Invoice::DataPresenter::Base - attributes :amount, :currency, :state + attributes :amount, :currency, :state, :payment_method_id attributes_with_presenter :payment_method + invoice_generation_attributes :amount, :payment_method_id + invoice_update_attributes :state def created_at datetime = data&.[](:created_at) diff --git a/app/models/invoice/data_presenter/payment_method.rb b/app/models/invoice/data_presenter/payment_method.rb index 0f0027c305..2361ea64ef 100644 --- a/app/models/invoice/data_presenter/payment_method.rb +++ b/app/models/invoice/data_presenter/payment_method.rb @@ -1,3 +1,4 @@ class Invoice::DataPresenter::PaymentMethod < Invoice::DataPresenter::Base - attributes :name, :description + attributes :id,:name, :description + invoice_generation_attributes :id end diff --git a/app/models/invoice/data_presenter/shipping_method.rb b/app/models/invoice/data_presenter/shipping_method.rb index 8a61f9dc52..8c79cd4f6e 100644 --- a/app/models/invoice/data_presenter/shipping_method.rb +++ b/app/models/invoice/data_presenter/shipping_method.rb @@ -1,3 +1,4 @@ class Invoice::DataPresenter::ShippingMethod < Invoice::DataPresenter::Base - attributes :name, :require_ship_address + attributes :id, :name, :require_ship_address + invoice_generation_attributes :id end diff --git a/app/models/invoice/data_presenter/variant.rb b/app/models/invoice/data_presenter/variant.rb index 57aad76325..1ad46bc389 100644 --- a/app/models/invoice/data_presenter/variant.rb +++ b/app/models/invoice/data_presenter/variant.rb @@ -1,5 +1,5 @@ class Invoice::DataPresenter::Variant < Invoice::DataPresenter::Base - attributes :display_name, :options_text + attributes :id, :display_name, :options_text attributes_with_presenter :product def name_to_display diff --git a/app/models/invoice/data_presenter_attributes.rb b/app/models/invoice/data_presenter_attributes.rb index 8d98ef97a1..c6ecc38518 100644 --- a/app/models/invoice/data_presenter_attributes.rb +++ b/app/models/invoice/data_presenter_attributes.rb @@ -1,42 +1,58 @@ -module Invoice::DataPresenterAttributes - extend ActiveSupport::Concern +# frozen_string_literal: true - def attributes(*attributes,prefix: nil) - attributes.each do |attribute| - define_method([prefix,attribute].reject(&:blank?).join("_")) do - data&.[](attribute) +class Invoice + module DataPresenterAttributes + extend ActiveSupport::Concern + + def attributes(*attributes, prefix: nil) + attributes.each do |attribute| + define_method([prefix, attribute].reject(&:blank?).join("_")) do + data&.[](attribute) + end end end - end - def attributes_with_presenter(*attributes) - attributes.each do |attribute| - define_method(attribute) do - instance_variable = instance_variable_get("@#{attribute}") + def attributes_with_presenter(*attributes) + attributes.each do |attribute| + define_method(attribute) do + instance_variable = instance_variable_get("@#{attribute}") + return instance_variable if instance_variable + + instance_variable_set("@#{attribute}", + Invoice::DataPresenter.const_get( + attribute.to_s.classify + ).new(data&.[](attribute))) + end + end + end + + def array_attribute(attribute_name, class_name: nil) + define_method(attribute_name) do + instance_variable = instance_variable_get("@#{attribute_name}") return instance_variable if instance_variable - instance_variable_set("@#{attribute}", - Invoice::DataPresenter.const_get(attribute.to_s.classify).new(data&.[](attribute))) + instance_variable_set("@#{attribute_name}", + data&.[](attribute_name)&.map { |item| + Invoice::DataPresenter.const_get(class_name).new(item) + }) end end - end - def array_attribute(attribute_name,class_name: nil) - define_method(attribute_name) do - instance_variable = instance_variable_get("@#{attribute_name}") - return instance_variable if instance_variable - - instance_variable_set("@#{attribute_name}", - data&.[](attribute_name)&.map { |item| - Invoice::DataPresenter.const_get(class_name).new(item) - }) + # if one of the list attributes is updated, the invoice needs to be regenerated + def invoice_generation_attributes(*attributes) + define_method(:invoice_generation_values) do + attributes.map do |attribute| + public_send(attribute) + end + end end - end - def relevant_attributes(*attributes) - define_method(:relevant_values) do - attributes.map do |attribute| - send(attribute) + # if one of the list attributes is updated, the invoice needs to be updated + def invoice_update_attributes(*attributes) + define_method(:invoice_update_values) do + attributes.map do |attribute| + public_send(attribute) + end end end end diff --git a/app/serializers/invoice/line_item_serializer.rb b/app/serializers/invoice/line_item_serializer.rb index 87a8d38ad7..bda17dc664 100644 --- a/app/serializers/invoice/line_item_serializer.rb +++ b/app/serializers/invoice/line_item_serializer.rb @@ -1,8 +1,4 @@ class Invoice::LineItemSerializer < ActiveModel::Serializer - attributes :quantity, - :price_with_adjustments, - :added_tax, - :included_tax - - has_one :variant, serializer: Invoice::VariantSerializer + attributes :added_tax, :currency, :included_tax, :price_with_adjustments, :quantity, :variant_id + has_one :variant, serializer: Invoice::VariantSerializer end diff --git a/app/serializers/invoice/order_serializer.rb b/app/serializers/invoice/order_serializer.rb index fe313ee7bd..7e4ae40ac7 100644 --- a/app/serializers/invoice/order_serializer.rb +++ b/app/serializers/invoice/order_serializer.rb @@ -1,6 +1,7 @@ class Invoice::OrderSerializer < ActiveModel::Serializer attributes :number, :special_instructions, :note, :payment_state, :total, :payment_total, :state, - :currency, :additional_tax_total, :included_tax_total, :completed_at, :has_taxes_included + :currency, :additional_tax_total, :included_tax_total, :completed_at, :has_taxes_included, + :shipping_method_id has_one :order_cycle, serializer: Invoice::OrderCycleSerializer has_one :customer, serializer: Invoice::CustomerSerializer has_one :distributor, serializer: Invoice::EnterpriseSerializer @@ -19,4 +20,8 @@ class Invoice::OrderSerializer < ActiveModel::Serializer def completed_at object.completed_at.to_s end + + def shipping_method_id + object.shipping_method&.id + end end diff --git a/app/serializers/invoice/payment_method_serializer.rb b/app/serializers/invoice/payment_method_serializer.rb index f106b47379..8fee4a859e 100644 --- a/app/serializers/invoice/payment_method_serializer.rb +++ b/app/serializers/invoice/payment_method_serializer.rb @@ -1,3 +1,3 @@ class Invoice::PaymentMethodSerializer < ActiveModel::Serializer - attributes :name, :description + attributes :id, :name, :description end diff --git a/app/serializers/invoice/payment_serializer.rb b/app/serializers/invoice/payment_serializer.rb index e8a27e0bd0..74ae234f45 100644 --- a/app/serializers/invoice/payment_serializer.rb +++ b/app/serializers/invoice/payment_serializer.rb @@ -1,5 +1,5 @@ class Invoice::PaymentSerializer < ActiveModel::Serializer - attributes :state, :created_at, :amount, :currency + attributes :state, :created_at, :amount, :currency, :payment_method_id has_one :payment_method, serializer: Invoice::PaymentMethodSerializer def created_at diff --git a/app/serializers/invoice/variant_serializer.rb b/app/serializers/invoice/variant_serializer.rb index 94b316f4cd..13d96f5f31 100644 --- a/app/serializers/invoice/variant_serializer.rb +++ b/app/serializers/invoice/variant_serializer.rb @@ -1,4 +1,4 @@ class Invoice::VariantSerializer < ActiveModel::Serializer - attributes :display_name, :options_text + attributes :id, :display_name, :options_text has_one :product, serializer: Invoice::ProductSerializer -end \ No newline at end of file +end diff --git a/app/services/order_invoice_comparator.rb b/app/services/order_invoice_comparator.rb index 04e1b68627..e2cb976260 100644 --- a/app/services/order_invoice_comparator.rb +++ b/app/services/order_invoice_comparator.rb @@ -1,29 +1,45 @@ +# frozen_string_literal: true + class OrderInvoiceComparator - def equal?(invoice1, invoice2) - # We'll use a recursive BFS algorithm to find if current state of invoice is outdated + def can_generate_new_invoice?(current_state_invoice, latest_invoice) + # We'll use a recursive BFS algorithm to find if the invoice is outdated # the root will be the order # On each node, we'll a list of relevant attributes that will be used on the comparison - bfs(invoice1.presenter, invoice2.presenter) + different?(current_state_invoice.presenter, latest_invoice.presenter, + invoice_generation_selector) end - def bfs(node1, node2) - simple_values1, presenters1 = group_relevant_values(node1) - simple_values2, presenters2 = group_relevant_values(node2) - return false if simple_values1 != simple_values2 + def can_update_latest_invoice?(current_state_invoice, latest_invoice) + different?(current_state_invoice.presenter, latest_invoice.presenter, invoice_update_selector) + end - return false if presenters1.size != presenters2.size + def different?(node1, node2, attributes_selector) + simple_values1, presenters1 = attributes_selector.call(node1) + simple_values2, presenters2 = attributes_selector.call(node2) + return true if simple_values1 != simple_values2 - presenters1.zip(presenters2).each do |presenter1, presenter2| - return false unless bfs(presenter1, presenter2) + return true if presenters1.size != presenters2.size + + presenters1.zip(presenters2).any? do |presenter1, presenter2| + different?(presenter1, presenter2, attributes_selector) end - true end - def group_relevant_values(node) - return [[], []] unless node.respond_to?(:relevant_values) + def invoice_generation_selector + values_selector(:invoice_generation_values) + end - grouped = node.relevant_values.group_by(&grouper) - [grouped[:simple] || [], grouped[:presenters]&.flatten || []] + def invoice_update_selector + values_selector(:invoice_update_values) + end + + def values_selector(attribute) + proc do |node| + return [[], []] unless node.respond_to?(attribute) + + grouped = node.public_send(attribute).group_by(&grouper) + [grouped[:simple] || [], grouped[:presenters]&.flatten || []] + end end def grouper diff --git a/spec/services/order_invoice_comparator_spec.rb b/spec/services/order_invoice_comparator_spec.rb index 3a2371ef23..20e743dd61 100644 --- a/spec/services/order_invoice_comparator_spec.rb +++ b/spec/services/order_invoice_comparator_spec.rb @@ -3,52 +3,88 @@ require 'spec_helper' describe OrderInvoiceComparator do - describe '#equal?' do + describe '#can_generate_new_invoice?' do let!(:order) { create(:completed_order_with_fees) } - let(:current_state_invoice){ order.current_state_invoice } let!(:invoice){ create(:invoice, order: order) } + let(:current_state_invoice){ order.current_state_invoice } + let(:subject) { + OrderInvoiceComparator.new.can_generate_new_invoice?(current_state_invoice, invoice) + } context "changes on the order object" do it "returns true if the order didn't change" do - expect(OrderInvoiceComparator.new.equal?(current_state_invoice, invoice)).to be true + expect(subject).to be false end - + it "returns true if a relevant attribute changes" do - order.update!(note: 'THIS IS AN UPDATE') - - expect(OrderInvoiceComparator.new.equal?(current_state_invoice, invoice)).to be false + Spree::Order.where(id: order.id).update_all(payment_total: order.payment_total + 10) + order.reload + expect(subject).to be true end - + it "returns true if a non-relevant attribute changes" do - order.update!(last_ip_address: "192.168.172.165") - - expect(OrderInvoiceComparator.new.equal?(current_state_invoice, invoice)).to be true + order.update!(note: "THIS IS A NEW NOTE") + expect(subject).to be false end end - context "change on associate objects (belong_to)" do + context "a non-relevant associated model is updated" do let(:distributor){ order.distributor } - - it "returns false if the distributor change relavant attribute" do - distributor.update!(name: 'THIS IS A NEW NAME') - - expect(OrderInvoiceComparator.new.equal?(current_state_invoice, invoice)).to be false - end - - it "returns true if the distributor change non-relavant attribute" do - distributor.update!(description: 'THIS IS A NEW DESCRIPTION') - - expect(OrderInvoiceComparator.new.equal?(current_state_invoice, invoice)).to be true + it "returns false" do + distributor.update!(name: 'THIS IS A NEW NAME', abn: 'This is a new ABN') + expect(subject).to be false end end - context "changes on associate objects (has_many)" do + context "a relevant associated object is updated" do let(:line_item){ order.line_items.first } - it "return true if relavant attribute change" do + it "return true" do line_item.update!(quantity: line_item.quantity + 1) - - expect(OrderInvoiceComparator.new.equal?(current_state_invoice, invoice)).to be false + expect(subject).to be true end end end -end \ No newline at end of file + + describe '#can_update_latest_invoice?' do + let!(:order) { create(:completed_order_with_fees) } + let!(:invoice){ create(:invoice, order: order) } + let(:current_state_invoice){ order.current_state_invoice } + let(:subject) { + OrderInvoiceComparator.new.can_update_latest_invoice?(current_state_invoice, invoice) + } + + context "changes on the order object" do + it "returns true if the order didn't change" do + expect(subject).to be false + end + + it "returns true if a relevant attribute changes" do + order.update!(note: "THIS IS A NEW NOTE") + expect(subject).to be true + end + + it "returns false if a non-relevant attribute changes" do + Spree::Order.where(id: order.id).update_all(payment_total: order.payment_total + 10) + order.reload + expect(subject).to be false + end + end + + context "a non-relevant associated model is updated" do + let(:distributor){ order.distributor } + it "returns false" do + distributor.update!(name: 'THIS IS A NEW NAME', abn: 'This is a new ABN') + expect(subject).to be false + end + end + + context "a relevant associated object is updated" do + let(:payment){ order.payments.first } + it "return true" do + expect(payment.state).to_not eq 'completed' + payment.update!(state: 'completed') + expect(subject).to be true + end + end + end +end