[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
 

app/models/spree/order_contents.rb

@@ -38,6 +38,7 @@ def update_or_create(variant, attributes)
         line_item.price = variant.price
         order.line_items << line_item
       end
+      update_shipment
 
       order.reload
       line_item

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
@@ -175,6 +237,22 @@
         expect(line_item.max_quantity).to eq 3
         expect(line_item.price).to eq variant.price
       end
+
+      it "ensures shipments are updated" do
+        expect(order).to receive(:ensure_updated_shipments)
+
+        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
@@ -186,6 +264,22 @@
         expect(line_item.reload.quantity).to eq 3
         expect(line_item.max_quantity).to eq 4
       end
+
+      it "ensures shipments are updated" do
+        expect(order).to receive(:ensure_updated_shipments)
+
+        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

@@ -1211,76 +1211,73 @@
     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!
+
+        item_num = order.line_items.sum(&:quantity)
+        expect(order.reload.adjustment_total).to eq(item_num * shipping_fee)
+      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 +1293,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