Update Solidus to work correctly with non-promotion line-level adjust…

…ments In PR 1933 we starting allowing allow non-promotion adjustments on line items and shipments. However, we didn't update some other important code (including the DefaultTax calculator) to consider these adjustments. This commit: - Adds a `total_before_tax` method on LineItem, Shipment, and ShippingRate. - Updates Calculator::DefaultTax to use this method when calculating taxes. - Updates all other code inside Solidus to use this method. - Deprecates the old `discounted_amount` methods.
solidusio · Sep 13, 2017 · 5d61419 · 5d61419
1 parent 06874ef
commit 5d61419
Show file tree

Hide file tree

Showing 15 changed files with 108 additions and 40 deletions.
diff --git a/core/app/models/spree/calculator/default_tax.rb b/core/app/models/spree/calculator/default_tax.rb
@@ -13,7 +13,7 @@ def compute_order(order)
         rate.tax_categories.include?(line_item.tax_category)
       end
 
-      line_items_total = matched_line_items.sum(&:discounted_amount)
+      line_items_total = matched_line_items.sum(&:total_before_tax)
       if rate.included_in_price
         round_to_two_places(line_items_total - ( line_items_total / (1 + rate.amount) ) )
       else
@@ -27,7 +27,7 @@ def compute_item(item)
       if rate.included_in_price
         deduced_total_by_rate(item, rate)
       else
-        round_to_two_places(item.discounted_amount * rate.amount)
+        round_to_two_places(item.total_before_tax * rate.amount)
       end
     end
 
@@ -47,7 +47,7 @@ def round_to_two_places(amount)
 
     def deduced_total_by_rate(item, rate)
       round_to_two_places(
-        rate.amount * item.discounted_amount / (1 + sum_of_included_tax_rates(item))
+        rate.amount * item.total_before_tax / (1 + sum_of_included_tax_rates(item))
       )
     end
 

diff --git a/core/app/models/spree/calculator/returns/default_refund_amount.rb b/core/app/models/spree/calculator/returns/default_refund_amount.rb
@@ -16,12 +16,12 @@ def weighted_order_adjustment_amount(inventory_unit)
       end
 
       def weighted_line_item_amount(inventory_unit)
-        inventory_unit.line_item.discounted_amount * percentage_of_line_item(inventory_unit)
+        inventory_unit.line_item.total_before_tax * percentage_of_line_item(inventory_unit)
       end
 
       def percentage_of_order_total(inventory_unit)
-        return 0.0 if inventory_unit.order.discounted_item_amount.zero?
-        weighted_line_item_amount(inventory_unit) / inventory_unit.order.discounted_item_amount
+        return 0.0 if inventory_unit.order.item_total_before_tax.zero?
+        weighted_line_item_amount(inventory_unit) / inventory_unit.order.item_total_before_tax
       end
 
       def percentage_of_line_item(inventory_unit)

diff --git a/core/app/models/spree/line_item.rb b/core/app/models/spree/line_item.rb
@@ -57,6 +57,13 @@ def amount
     def discounted_amount
       amount + promo_total
     end
+    deprecate discounted_amount: :total_before_tax, deprecator: Spree::Deprecation
+
+    # @return [BigDecimal] the amount of this item, taking into consideration
+    #   all non-tax adjustments.
+    def total_before_tax
+      amount + adjustments.select { |a| !a.tax? && a.eligible? }.sum(&:amount)
+    end
 
     # @return [BigDecimal] the amount of this line item, taking into
     #   consideration all its adjustments.
@@ -68,13 +75,16 @@ def final_amount
     # @return [BigDecimal] the amount of this line item before included tax
     # @note just like `amount`, this does not include any additional tax
     def pre_tax_amount
-      discounted_amount - included_tax_total
+      total_before_tax - included_tax_total
     end
 
     extend Spree::DisplayMoney
     money_methods :amount, :discounted_amount, :final_amount, :pre_tax_amount, :price,
-                  :included_tax_total, :additional_tax_total
+                  :included_tax_total, :additional_tax_total,
+                  :total_before_tax
+    deprecate display_discounted_amount: :display_total_before_tax, deprecator: Spree::Deprecation
     alias discounted_money display_discounted_amount
+    deprecate discounted_money: :display_total_before_tax, deprecator: Spree::Deprecation
 
     # @return [Spree::Money] the price of this line item
     alias money_price display_price

diff --git a/core/app/models/spree/order.rb b/core/app/models/spree/order.rb
@@ -194,6 +194,11 @@ def pre_tax_item_amount
     def discounted_item_amount
       line_items.to_a.sum(&:discounted_amount)
     end
+    deprecate discounted_item_amount: :item_total_before_tax, deprecator: Spree::Deprecation
+
+    def item_total_before_tax
+      line_items.to_a.sum(&:total_before_tax)
+    end
 
     def currency
       self[:currency] || Spree::Config[:currency]

diff --git a/core/app/models/spree/return_authorization.rb b/core/app/models/spree/return_authorization.rb
@@ -46,7 +46,7 @@ def currency
     end
 
     def refundable_amount
-      order.discounted_item_amount + order.promo_total
+      order.item_total_before_tax + order.promo_total
     end
 
     def customer_returned_items?

diff --git a/core/app/models/spree/shipment.rb b/core/app/models/spree/shipment.rb
@@ -89,7 +89,11 @@ def can_transition_from_canceled_to_ready?
     end
 
     extend DisplayMoney
-    money_methods :cost, :amount, :discounted_cost, :final_price, :item_cost
+    money_methods(
+      :cost, :amount, :discounted_cost, :final_price, :item_cost,
+      :total_before_tax,
+    )
+    deprecate display_discounted_cost: :display_total_before_tax, deprecator: Spree::Deprecation
     alias_attribute :amount, :cost
 
     def add_shipping_method(shipping_method, selected = false)
@@ -116,10 +120,14 @@ def currency
     def discounted_cost
       cost + promo_total
     end
+    deprecate discounted_cost: :total_before_tax, deprecator: Spree::Deprecation
     alias discounted_amount discounted_cost
+    deprecate discounted_amount: :total_before_tax, deprecator: Spree::Deprecation
 
-    def editable_by?(_user)
-      !shipped?
+    # @return [BigDecimal] the amount of this item, taking into consideration
+    #   all non-tax adjustments.
+    def total_before_tax
+      amount + adjustments.select { |a| !a.tax? && a.eligible? }.sum(&:amount)
     end
 
     def final_price
@@ -130,6 +138,10 @@ def final_price_with_items
       item_cost + final_price
     end
 
+    def editable_by?(_user)
+      !shipped?
+    end
+
     # Decrement the stock counts for all pending inventory units in this
     # shipment and mark.
     # Any previous non-pending inventory units are skipped as their stock had
@@ -164,7 +176,7 @@ def line_items
     end
 
     def pre_tax_amount
-      discounted_amount - included_tax_total
+      total_before_tax - included_tax_total
     end
 
     def ready_or_pending?

diff --git a/core/app/models/spree/shipping_rate.rb b/core/app/models/spree/shipping_rate.rb
@@ -17,6 +17,8 @@ class ShippingRate < Spree::Base
     alias_attribute :amount, :cost
 
     alias_method :discounted_amount, :amount
+    deprecate discounted_amount: :total_before_tax, deprecator: Spree::Deprecation
+    alias_method :total_before_tax, :amount
 
     extend DisplayMoney
     money_methods :amount

diff --git a/core/app/views/spree/order_mailer/confirm_email.html.erb b/core/app/views/spree/order_mailer/confirm_email.html.erb
@@ -45,7 +45,7 @@
           <tr>
             <td></td>
             <td><%= Spree.t(:shipping) %> <%= name %>:</td>
-            <td><%= Spree::Money.new(shipments.sum(&:discounted_cost), currency: @order.currency) %></td>
+            <td><%= Spree::Money.new(shipments.sum(&:total_before_tax), currency: @order.currency) %></td>
           </tr>
         <% end %>
         <% if @order.all_adjustments.eligible.tax.exists? %>

diff --git a/core/app/views/spree/order_mailer/confirm_email.text.erb b/core/app/views/spree/order_mailer/confirm_email.text.erb
@@ -19,7 +19,7 @@
 <% end %>
 
 <% @order.shipments.group_by { |s| s.selected_shipping_rate.try(:name) }.each do |name, shipments| %>
-<%= Spree.t(:shipping) %>: <%= name %> <%= Spree::Money.new(shipments.sum(&:discounted_cost), currency: @order.currency) %>
+<%= Spree.t(:shipping) %>: <%= name %> <%= Spree::Money.new(shipments.sum(&:total_before_tax), currency: @order.currency) %>
 <% end %>
 
 <% if @order.all_adjustments.eligible.tax.exists? %>

diff --git a/core/spec/models/spree/calculator/default_tax_spec.rb b/core/spec/models/spree/calculator/default_tax_spec.rb
@@ -109,7 +109,14 @@
   end
 
   shared_examples_for 'computing any item' do
-    let(:promo_total) { 0 }
+    let(:adjustment_total) { 0 }
+    let(:adjustments) do
+      if adjustment_total.zero?
+        []
+      else
+       [Spree::Adjustment.new(included: false, source: nil, amount: adjustment_total)]
+      end
+    end
     let(:order) { build_stubbed(:order, ship_address: address) }
 
     context "when tax is included in price" do
@@ -129,19 +136,19 @@
           end
         end
 
-        context "when line item is discounted" do
-          let(:promo_total) { -1 }
+        context "when line item is adjusted" do
+          let(:adjustment_total) { -1 }
 
-          it "should be equal to the item's discounted total * rate" do
+          it "should be equal to the item's adjusted total * rate" do
             expect(calculator.compute(item)).to eql 1.38
           end
         end
       end
     end
 
     context "when tax is not included in price" do
-      context "when the line item is discounted" do
-        let(:promo_total) { -1 }
+      context "when the item has an adjustment" do
+        let(:adjustment_total) { -1 }
 
         it "should be equal to the item's pre-tax total * rate" do
           expect(calculator.compute(item)).to eq(1.45)
@@ -180,7 +187,7 @@
         :line_item,
         price: 10,
         quantity: 3,
-        promo_total: promo_total,
+        adjustments: adjustments,
         order: order,
         tax_category: tax_category
       )
@@ -209,7 +216,7 @@
       build_stubbed(
         :shipment,
         cost: 30,
-        promo_total: promo_total,
+        adjustments: adjustments,
         order: order,
         shipping_rates: [shipping_rate]
       )
@@ -234,12 +241,12 @@
     end
 
     let(:item) do
-      # cost and discounted_amount for shipping rates are the same as they
-      # can not be discounted. for the sake of passing tests, the cost is
+      # cost and adjusted amount for shipping rates are the same as they
+      # can not be adjusted. for the sake of passing tests, the cost is
       # adjusted here.
       build_stubbed(
         :shipping_rate,
-        cost: 30 + promo_total,
+        cost: 30 + adjustment_total,
         selected: true,
         shipping_method: shipping_method,
         shipment: shipment

diff --git a/core/spec/models/spree/line_item_spec.rb b/core/spec/models/spree/line_item_spec.rb
@@ -93,19 +93,36 @@ def copy_price
     end
   end
 
-  describe '.discounted_amount' do
+  # TODO: Remove this spec after the method has been removed.
+  describe '#discounted_amount' do
     it "returns the amount minus any discounts" do
       line_item.price = 10
       line_item.quantity = 2
       line_item.promo_total = -5
-      expect(line_item.discounted_amount).to eq(15)
+      expect(Spree::Deprecation.silence { line_item.discounted_amount }).to eq(15)
     end
   end
 
+  # TODO: Remove this spec after the method has been removed.
   describe "#discounted_money" do
     it "should return a money object with the discounted amount" do
-      expect(line_item.discounted_amount).to eq(10.00)
-      expect(line_item.discounted_money.to_s).to eq "$10.00"
+      expect(Spree::Deprecation.silence { line_item.discounted_amount }).to eq(10.00)
+      expect(Spree::Deprecation.silence { line_item.discounted_money.to_s }).to eq "$10.00"
+    end
+  end
+
+  describe '#total_before_tax' do
+    before do
+      line_item.update!(price: 10, quantity: 2)
+    end
+    let!(:admin_adjustment) { create(:adjustment, adjustable: line_item, order: line_item.order, amount: -1, source: nil) }
+    let!(:promo_adjustment) { create(:adjustment, adjustable: line_item, order: line_item.order, amount: -2, source: promo_action) }
+    let!(:ineligible_promo_adjustment) { create(:adjustment, eligible: false, adjustable: line_item, order: line_item.order, amount: -4, source: promo_action) }
+    let(:promo_action) { promo.actions[0] }
+    let(:promo) { create(:promotion, :with_line_item_adjustment) }
+
+    it 'returns the amount minus any adjustments' do
+      expect(line_item.total_before_tax).to eq(20 - 1 - 2)
     end
   end
 

diff --git a/core/spec/models/spree/shipment_spec.rb b/core/spec/models/spree/shipment_spec.rb
@@ -145,7 +145,22 @@
     shipment = create(:shipment)
     shipment.cost = 10
     shipment.promo_total = -1
-    expect(shipment.discounted_cost).to eq(9)
+    expect(Spree::Deprecation.silence { shipment.discounted_cost }).to eq(9)
+  end
+
+  describe '#total_before_tax' do
+    before do
+      shipment.update_attributes!(cost: 10)
+    end
+    let!(:admin_adjustment) { create(:adjustment, adjustable: shipment, order: shipment.order, amount: -1, source: nil) }
+    let!(:promo_adjustment) { create(:adjustment, adjustable: shipment, order: shipment.order, amount: -2, source: promo_action) }
+    let!(:ineligible_promo_adjustment) { create(:adjustment, eligible: false, adjustable: shipment, order: shipment.order, amount: -4, source: promo_action) }
+    let(:promo_action) { promo.actions[0] }
+    let(:promo) { create(:promotion, :with_line_item_adjustment) }
+
+    it 'returns the amount minus any adjustments' do
+      expect(shipment.total_before_tax).to eq(10 - 1 - 2)
+    end
   end
 
   it "#tax_total with included taxes" do

diff --git a/core/spec/models/spree/tax/taxation_integration_spec.rb b/core/spec/models/spree/tax/taxation_integration_spec.rb
@@ -282,7 +282,7 @@
         end
 
         it 'has a constant amount pre tax' do
-          expect(line_item.discounted_amount - line_item.included_tax_total).to eq(18.69)
+          expect(line_item.total_before_tax - line_item.included_tax_total).to eq(18.69)
         end
       end
 
@@ -320,7 +320,7 @@
         end
 
         it 'has a constant amount pre tax' do
-          expect(line_item.discounted_amount - line_item.included_tax_total).to eq(25.21)
+          expect(line_item.total_before_tax - line_item.included_tax_total).to eq(25.21)
         end
       end
 
@@ -358,7 +358,7 @@
         end
 
         it 'has a constant amount pre tax' do
-          expect(line_item.discounted_amount - line_item.included_tax_total).to eq(8.40)
+          expect(line_item.total_before_tax - line_item.included_tax_total).to eq(8.40)
         end
       end
 
@@ -409,7 +409,7 @@
         end
 
         it 'has a constant amount pre tax' do
-          expect(line_item.discounted_amount - line_item.included_tax_total).to eq(18.69)
+          expect(line_item.total_before_tax - line_item.included_tax_total).to eq(18.69)
         end
       end
     end
@@ -442,7 +442,7 @@
         end
 
         it 'has a constant amount pre tax' do
-          expect(line_item.discounted_amount - line_item.included_tax_total).to eq(18.69)
+          expect(line_item.total_before_tax - line_item.included_tax_total).to eq(18.69)
         end
 
         context 'an order with a book and a shipment' do
@@ -484,7 +484,7 @@
         end
 
         it 'has a constant amount pre tax' do
-          expect(line_item.discounted_amount - line_item.included_tax_total).to eq(25.21)
+          expect(line_item.total_before_tax - line_item.included_tax_total).to eq(25.21)
         end
 
         context 'an order with a sweater and a shipment' do
@@ -526,7 +526,7 @@
         end
 
         it 'has a constant amount pre tax' do
-          expect(line_item.discounted_amount - line_item.included_tax_total).to eq(8.40)
+          expect(line_item.total_before_tax - line_item.included_tax_total).to eq(8.40)
         end
       end
 

diff --git a/frontend/app/views/spree/orders/_adjustments.html.erb b/frontend/app/views/spree/orders/_adjustments.html.erb
@@ -13,7 +13,7 @@
     <tr>
       <td colspan="4" align='right'><h5><%= Spree.t(:shipping) %>: <%= shipment.shipping_method.name %></h5></td>
       <td colspan='2'>
-        <h5><%= shipment.display_discounted_cost %></h5>
+        <h5><%= shipment.display_total_before_tax %></h5>
       </td>
     </tr>
   <% end %>

diff --git a/frontend/app/views/spree/shared/_order_details.html.erb b/frontend/app/views/spree/shared/_order_details.html.erb
@@ -110,7 +110,7 @@
     <% order.shipments.group_by { |s| s.selected_shipping_rate.name }.each do |name, shipments| %>
       <tr class="total" data-hook='shipment-row'>
         <td colspan="4"><%= Spree.t(:shipping) %>: <strong><%= name %></strong></td>
-        <td class="total"><span><%= Spree::Money.new(shipments.sum(&:discounted_cost), currency: order.currency).to_html %></span></td>
+        <td class="total"><span><%= Spree::Money.new(shipments.sum(&:total_before_tax), currency: order.currency).to_html %></span></td>
       </tr>
     <% end %>
   </tfoot>