Merge pull request openfoodfoundation#13023 from rioug/12907-fix-chec…

…kout-shipping-fee

[Checkout] Shipping fees update, remove order callback

app/models/spree/order.rb

@@ -106,7 +106,6 @@ def states
     before_validation :clone_billing_address, if: :use_billing?
     before_validation :ensure_customer
 
-    before_save :update_shipping_fees!, if: :complete?
     before_save :update_payment_fees!, if: :complete?
     before_create :link_by_email
 
@@ -672,10 +671,6 @@ def process_each_payment
         break if payment_total >= total
 
         yield payment
-
-        if payment.completed?
-          self.payment_total += payment.amount
-        end
       end
     end
 

app/views/spree/orders/_totals_footer.html.haml

@@ -29,7 +29,7 @@
       %td.text-right{colspan: "3"}
         %strong
           = t :order_amount_paid
-      %td.text-right.total
+      %td.text-right.total{id: "amount-paid"}
         %strong
           = order.display_payment_total.to_html
   - if order.outstanding_balance.positive?

spec/controllers/admin/customers_controller_spec.rb

@@ -83,13 +83,9 @@ module Admin
               let!(:variant) { create(:variant, price: 10.0) }
 
               before do
-                allow_any_instance_of(Spree::Payment).to receive(:completed?).and_return(true)
-
                 order.contents.add(variant)
-                order.payments << create(:payment, order:, amount: order.total)
-                order.reload
+                order.payments << create(:payment, :completed, order:, amount: order.total)
 
-                order.process_payments!
                 order.update_attribute(:state, 'canceled')
               end
 

spec/models/spree/order/payment_spec.rb

@@ -15,13 +15,13 @@ module Spree
       create(:credit_card)
     }
     let(:payment1) {
-      create(:payment, amount: 50, payment_method:, source:, response_code: "12345")
+      create(:payment, order:, amount: 50, payment_method:, source:, response_code: "12345")
     }
     let(:payment2) {
-      create(:payment, amount: 50, payment_method:, source:, response_code: "12345")
+      create(:payment, order:, amount: 50, payment_method:, source:, response_code: "12345")
     }
     let(:payment3) {
-      create(:payment, amount: 50, payment_method:, source:, response_code: "12345")
+      create(:payment, order:, amount: 50, payment_method:, source:, response_code: "12345")
     }
     let(:failed_payment) {
       create(:payment, amount: 50, state: 'failed', payment_method:, source:,

spec/models/spree/order_contents_spec.rb

@@ -38,6 +38,27 @@
       expect(order.item_total.to_f).to eq 19.99
       expect(order.total.to_f).to eq 19.99
     end
+
+    context "with a completed order" do
+      let!(:order) { create(:completed_order_with_totals) }
+
+      it "updates shipping fees" do
+        expect(order).to receive(:update_shipping_fees!)
+
+        subject.add(variant, 1)
+      end
+    end
+
+    context "when passing a shipment" do
+      let!(:order) { create(:order_with_line_items) }
+
+      it "updates shipping fees" do
+        shipment = order.shipments.first
+        expect(order).to receive(:update_shipping_fees!)
+
+        subject.add(variant, 1, shipment)
+      end
+    end
   end
 
   context "#remove" do
@@ -86,6 +107,27 @@
       expect(order.item_total.to_f).to eq 19.99
       expect(order.total.to_f).to eq 19.99
     end
+
+    context "with a completed order" do
+      let!(:order) { create(:completed_order_with_totals) }
+
+      it "updates shipping fees" do
+        expect(order).to receive(:update_shipping_fees!)
+
+        subject.remove(order.line_items.first.variant, 1)
+      end
+    end
+
+    context "when passing a shipment" do
+      let!(:order) { create(:order_with_line_items) }
+
+      it "updates shipping fees" do
+        shipment = order.reload.shipments.first
+        expect(order).to receive(:update_shipping_fees!)
+
+        subject.remove(order.line_items.first.variant, 1, shipment)
+      end
+    end
   end
 
   context "#update_cart" do
@@ -126,6 +168,16 @@
       expect(subject.order).to receive(:ensure_updated_shipments)
       subject.update_cart params
     end
+
+    context "with a completed order" do
+      let!(:order) { create(:completed_order_with_totals) }
+
+      it "updates shipping fees" do
+        expect(order).to receive(:update_shipping_fees!)
+
+        subject.update_cart params
+      end
+    end
   end
 
   describe "#update_item" do
@@ -163,6 +215,16 @@
 
       subject.update_item(line_item, { quantity: 3 })
     end
+
+    context "with a completed order" do
+      let!(:order) { create(:completed_order_with_totals) }
+
+      it "updates shipping fees" do
+        expect(order).to receive(:update_shipping_fees!)
+
+        subject.update_item(order.line_items.first, { quantity: 3 })
+      end
+    end
   end
 
   describe "#update_or_create" do
@@ -181,6 +243,16 @@
 
         subject.update_or_create(variant, { quantity: 2, max_quantity: 3 })
       end
+
+      context "with completed order" do
+        let!(:order) { create(:completed_order_with_totals) }
+
+        it "updates shipping fees" do
+          expect(order).to receive(:update_shipping_fees!)
+
+          subject.update_or_create(variant, { quantity: 2, max_quantity: 3 })
+        end
+      end
     end
 
     describe "updating" do
@@ -198,6 +270,16 @@
 
         subject.update_or_create(variant, { quantity: 3, max_quantity: 4 })
       end
+
+      context "with completed order" do
+        let!(:order) { create(:completed_order_with_totals) }
+
+        it "updates shipping fees" do
+          expect(order).to receive(:update_shipping_fees!)
+
+          subject.update_or_create(variant, { quantity: 3, max_quantity: 4 })
+        end
+      end
     end
   end
 end

spec/models/spree/order_spec.rb

@@ -280,40 +280,31 @@
     end
   end
 
-  context "#process_payments!" do
+  describe "#process_payments!" do
     let(:payment) { build(:payment) }
     before { allow(order).to receive_messages pending_payments: [payment], total: 10 }
 
-    it "should return false if no pending_payments available" do
+    it "returns false if no pending_payments available" do
       allow(order).to receive_messages pending_payments: []
       expect(order.process_payments!).to be_falsy
     end
 
     context "when the processing is sucessful" do
-      it "should process the payments" do
+      it "processes the payments" do
         expect(payment).to receive(:process!)
         expect(order.process_payments!).to be_truthy
       end
-
-      it "stores the payment total on the order" do
-        allow(payment).to receive(:process!)
-        allow(payment).to receive(:completed?).and_return(true)
-
-        order.process_payments!
-
-        expect(order.payment_total).to eq(payment.amount)
-      end
     end
 
     context "when a payment raises a GatewayError" do
       before { expect(payment).to receive(:process!).and_raise(Spree::Core::GatewayError) }
 
-      it "should return true when configured to allow checkout on gateway failures" do
+      it "returns true when configured to allow checkout on gateway failures" do
         Spree::Config.set allow_checkout_on_gateway_error: true
         expect(order.process_payments!).to be_truthy
       end
 
-      it "should return false when not configured to allow checkout on gateway failures" do
+      it "returns false when not configured to allow checkout on gateway failures" do
         Spree::Config.set allow_checkout_on_gateway_error: false
         expect(order.process_payments!).to be_falsy
       end
@@ -1211,76 +1202,72 @@
     end
   end
 
-  describe "a completed order with shipping and transaction fees" do
+  describe "#update_shipping_fees!" do
+    let(:distributor) { create(:distributor_enterprise) }
+    let(:order) {
+      create(:completed_order_with_fees, distributor:, shipping_fee:, payment_fee: 0)
+    }
+    let(:shipping_fee) { 5 }
+
+    it "does nothing if shipment is shipped" do
+      # An order need to be paid before we can ship a shipment
+      create(:payment, amount: order.total, order:, state: "completed")
+
+      shipment = order.shipments.first
+      shipment.ship
+
+      expect(shipment).not_to receive(:save)
+
+      order.update_shipping_fees!
+    end
+
+    it "saves the each shipment" do
+      order.shipments << create(:shipment, order:)
+      order.shipments.each do |shipment|
+        expect(shipment).to receive(:save)
+      end
+
+      order.update_shipping_fees!
+    end
+
+    context "with shipment including a shipping fee" do
+      it "updates shipping fee" do
+        # Manually udate line item quantity, in a normal scenario we would use
+        # order.contents method, which takes care of updating shipments
+        order.line_items.first.update(quantity: 2)
+
+        order.update_shipping_fees!
+
+        expect(order.reload.adjustment_total).to eq(15) # 3 items * 5
+      end
+    end
+  end
+
+  describe "a completed order with transaction fees" do
     let(:distributor) { create(:distributor_enterprise_with_tax) }
-    let(:zone) { create(:zone_with_member) }
-    let(:shipping_tax_rate) { create(:tax_rate, amount: 0.25, included_in_price: true, zone:) }
-    let(:shipping_tax_category) { create(:tax_category, tax_rates: [shipping_tax_rate]) }
     let(:order) {
-      create(:completed_order_with_fees, distributor:, shipping_fee:,
-                                         payment_fee:,
-                                         shipping_tax_category:)
+      create(:completed_order_with_fees, distributor:, shipping_fee: 0, payment_fee:)
     }
-    let(:shipping_fee) { 3 }
     let(:payment_fee) { 5 }
     let(:item_num) { order.line_items.length }
-    let(:expected_fees) { item_num * (shipping_fee + payment_fee) }
+    let(:expected_fees) { item_num * payment_fee }
 
     before do
       order.reload
       order.create_tax_charge!
 
       # Sanity check the fees
-      expect(order.all_adjustments.length).to eq 3
-      expect(order.shipment_adjustments.length).to eq 2
+      expect(order.all_adjustments.length).to eq 2
       expect(item_num).to eq 2
       expect(order.adjustment_total).to eq expected_fees
-      expect(order.shipment.adjustments.tax.inclusive.sum(:amount)).to eq 1.2
-      expect(order.shipment.included_tax_total).to eq 1.2
     end
 
     context "removing line_items" do
-      it "updates shipping and transaction fees" do
+      it "updates transaction fees" do
         order.line_items.first.update_attribute(:quantity, 0)
         order.save
 
-        expect(order.adjustment_total).to eq expected_fees - shipping_fee - payment_fee
-        expect(order.shipment.adjustments.tax.inclusive.sum(:amount)).to eq 0.6
-        expect(order.shipment.included_tax_total).to eq 0.6
-      end
-
-      context "when finalized fee adjustments exist on the order" do
-        before do
-          order.all_adjustments.each(&:finalize!)
-          order.reload
-        end
-
-        it "does not attempt to update such adjustments" do
-          order.update(line_items_attributes: [{ id: order.line_items.first.id, quantity: 0 }])
-
-          # Check if fees got updated
-          order.reload
-
-          expect(order.adjustment_total).to eq expected_fees
-          expect(order.shipment.adjustments.tax.inclusive.sum(:amount)).to eq 1.2
-          expect(order.shipment.included_tax_total).to eq 1.2
-        end
-      end
-    end
-
-    context "changing the shipping method to one without fees" do
-      let(:shipping_method) {
-        create(:shipping_method, calculator: Calculator::FlatRate.new(preferred_amount: 0))
-      }
-
-      it "updates shipping fees" do
-        order.shipments = [create(:shipment_with, :shipping_method,
-                                  shipping_method:)]
-        order.save
-
-        expect(order.adjustment_total).to eq expected_fees - (item_num * shipping_fee)
-        expect(order.shipment.adjustments.tax.inclusive.sum(:amount)).to eq 0
-        expect(order.shipment.included_tax_total).to eq 0
+        expect(order.adjustment_total).to eq expected_fees - payment_fee
       end
     end
 
@@ -1296,6 +1283,7 @@
 
         # Check if fees got updated
         order.reload
+
         expect(order.adjustment_total).to eq expected_fees - (item_num * payment_fee)
       end
     end

spec/system/admin/orders_spec.rb

@@ -452,14 +452,11 @@
 
       context "orders with different order totals" do
         before do
-          Spree::LineItem.where(order_id: order2.id).first.update!(quantity: 5)
-          Spree::LineItem.where(order_id: order3.id).first.update!(quantity: 4)
-          Spree::LineItem.where(order_id: order4.id).first.update!(quantity: 3)
-          Spree::LineItem.where(order_id: order5.id).first.update!(quantity: 2)
-          order2.save
-          order3.save
-          order4.save
-          order5.save
+          order2.contents.update_item(Spree::LineItem.find_by(order_id: order2.id), { quantity: 5 })
+          order3.contents.update_item(Spree::LineItem.find_by(order_id: order3.id), { quantity: 4 })
+          order4.contents.update_item(Spree::LineItem.find_by(order_id: order4.id), { quantity: 3 })
+          order5.contents.update_item(Spree::LineItem.find_by(order_id: order5.id), { quantity: 2 })
+
           login_as_admin
           visit spree.admin_orders_path
         end

spec/system/consumer/shopping/checkout_paypal_spec.rb

@@ -69,6 +69,7 @@
 
         click_on "Complete order"
         expect(page).to have_content "Your order has been processed successfully"
+        expect(page.find("#amount-paid").text).to have_content "$19.99"
 
         expect(order.reload.state).to eq "complete"
         expect(order.payments.count).to eq 1

spec/system/consumer/shopping/checkout_stripe_spec.rb

@@ -71,6 +71,8 @@
           checkout_with_stripe
 
           expect(page).to have_content "Confirmed"
+          expect(page.find("#amount-paid").text).to have_content "$19.99"
+
           expect(order.reload.completed?).to eq true
           expect(order.payments.first.state).to eq "completed"
         end