When user deletes a shipping method existing orders are updated with a random shipping method

[lbwright22]

Description

When an enterprise removes shipping method from their Shipping Method list existing orders with this shipping method are changed. It seems the first shipping method available to the enterprise is selected.

This results in:

1. the enterprise manager dispatching products incorrectly (eg. expects customer to collect when the customer asked for delivery)
2. If there is a difference in cost of the two shipping methods the order total is changed.

Example:
Orders: R852656180 (paid by PayPal) and R282147658 (paid by Stripe) placed with enterprise Sail Cargo SE (ID 201099).
Customer opted for 'UK delivery outside Brighton and Hove B1, B2, B3 postcodes' and paid 10% fee associated with this method at check out.
Hub removed this shipping method
Order and reports display 'Collection from Fiveways office...' which has £0.00 fee. Orders display as 'credit owing'.

Expected Behavior

1. Customer places order with a shipping method
   
   order confirmation email shows this is what they have paid for
   
   which is reflected on the Orders page
   

2. Hub removes the shipping method from their shipping method list.
   List at the time order was placed
   
   List some time after order placed when hub has removed this shipping option
   

3. Order (via 'Orders' -> Edit order and via Reports) should the shipping method as originally selected by the shopper.

Actual Behaviour

Steps 1 and 2 as Expected Behaviour

3. View Order (Orders - > Edit) or in Reports, order now displaying a different shipping method. As the fee is different the order total is also different.
   

Further, thange is not reflected under order adjustments

Steps to Reproduce

1. Set up an enterprise with 2 shipping methods. One has a fee associated with it.
2. Open an OC. Place an order as a customer and chose shipping method A.
3. Confirm shipping method in order, reports and in customer confirmation email.
4. Remove shipping method A from Enterprise Shipping methods list
5. See that the shipping method has changed in the original order and reports.

Workaround

Workaround is to recreate the shipping method.

Severity

s3 - Damaging if not noticed.

Your Environment

• Version used: 3.2.7
• Browser name and version:
• Operating System and version (desktop or mobile):

Possible Fix

[abdellani]

Hi

When a shipment method is removed from a distributor, the orders are not updated automatically.

The order is updated when the admin access (using the edit link) the order.

module Spree
  module Admin
    class OrdersControlle
...
      def edit
        @order.shipments.map(&:refresh_rates)
...
      end

The shipment's refresh_rates uses OrderManagement::Stock::Estimator#shipping_rates to recalculate shipping_rate

# class Shipment
    def refresh_rates
...
      original_shipping_method_id = shipping_method.try(:id)
      self.shipping_rates = OrderManagement::Stock::Estimator.new(order).shipping_rates(to_package)
      keep_original_shipping_method_selection(original_shipping_method_id)
...
      shipping_rates
    end

OrderManagement::Stock::Estimator uses OrderManagement::Stock::Package as input to calculate shipping_rates

class Shipment
...
    def to_package
      package = OrderManagement::Stock::Package.new(stock_location, order)
...
      package
    end

OrderManagement::Stock::Package#shipping_methods return the current distributor.shipping_methods

class OrderManagement::Stock::Package
...
      def shipping_methods
...
        order.distributor.shipping_methods.uniq.to_a # This is maybe the source of the problem
      end

And finally OrderManagement::Stock::Estimator returns the distributor shipping methods exclusively

class OrderManagement::Stock::Estimator
      def shipping_rates(package, frontend_only = true)
        shipping_rates = []
        shipping_methods = shipping_methods(package)
...
      end
...
      def shipping_methods(package) # This is maybe the source of the problem
        shipping_methods = package.shipping_methods 
...
        shipping_methods
      end

Questions:

1. Why do we run the refresh_rates when loading the order edit page? I'm wondering if this is the right place to do that. What if the admin doesn't access this page? (the answer to this question doesn't fix the bug)
2. Is the problem in the OrderManagement::Stock::Estimator#shipping_methods ? or is it in the OrderManagement::Stock::Package#shipping_methods ? which one needs to be fixed?

OrderManagement::Stock::Estimator is loading the shipping_methods from the package.
OrderManagement::Stock::Package is loading the shipping_methods from the order.distributor.

Any ideas?

[jibees]

Why do we run the refresh_rates when loading the order edit page?

I've no idea ; and I agree with you: there is an inconsistency between an order accessed by an admin, and an order which is not.

which one needs to be fixed?

Currently, i don't know (sorry, i'm useless)

I'll deep dive into keep_original_shipping_method_selection(original_shipping_method_id) because the name sounds good (07a44cf)

[abdellani]

Hi @jibees

I checked the problem again. I think you're right, the problem is on keep_original_shipping_method_selection.
OrderManagement::Stock::Estimator and OrderManagement::Stock::Package are used in other contexts and should not be updated to solve this issue.

The following diagram illustrates the relationship between the models

In Shipment#refresh_rates:

1. we store original_shipping_method_id
2. we overwrite the existing shipping_rates using the Stock::Estimator,
3. then we call keep_original_shipping_method_selection

    def refresh_rates
      return shipping_rates if shipped?

      # The call to Stock::Estimator below will replace the current shipping_method
      original_shipping_method_id = shipping_method.try(:id)
      self.shipping_rates = OrderManagement::Stock::Estimator.new(order).shipping_rates(to_package)

      keep_original_shipping_method_selection(original_shipping_method_id)

      shipping_rates
    end

The Stock::Estimator retrieves the shipping methods from the distributor, and generates the new list of shipping rates.
Since the old selected shipping method is no longer supported by the distributor, none of the new shipping rates will refer to that shipping method.

Now, the objective of keep_original_shipping_method_selection is to update selected_shipping_rate_id (because the old one is deleted when the shipping rates list was replaced in the previous step.)

To do so, it tries to find among the new shipping rates list, a shipping rate connected to the old shipping method ( rate_for_original_shipping_method.id)

    def keep_original_shipping_method_selection(original_shipping_method_id)
      return if shipping_method&.id == original_shipping_method_id

      rate_for_original_shipping_method = find_shipping_rate_for(original_shipping_method_id) #<<<<<
      if rate_for_original_shipping_method.present?
        self.selected_shipping_rate_id = rate_for_original_shipping_method.id
      else
        # If there's no original ship method to keep, or if it cannot be found on the ship rates
        #   But there's a new ship method selected (first if clause in this method)
        #   We need to save the shipment so that callbacks are triggered
        save!
      end
    end

The find_shipping_rate_for does the search and finds nothing.

    def find_shipping_rate_for(shipping_method_id)
      return unless shipping_method_id

      shipping_rates.detect { |rate|
        rate.shipping_method_id == shipping_method_id
      }
    end

The default behavior will return the first shipping rate as the selected one

    def shipping_method
      method = selected_shipping_rate.try(:shipping_method)
      method ||= shipping_rates.first.try(:shipping_method) unless order.manual_shipping_selection #<<<
      method
    end

As you can see, this scenario is already expected in the comment in keep_original_shipping_method_selection.

        # If there's no original ship method to keep, or if it cannot be found on the ship rates
        #   But there's a new ship method selected (first if clause in this method)
        #   We need to save the shipment so that callbacks are triggered

We can solve the issue by not erasing the shipping rate connected to the old shipping method when the Stock::Estimator is called, but will this solution raise other issues late?

What do you think?

[jibees]

You really did a great research job, thanks for that 🙏

I don't spot any error or something suspicious, but I'm not totally sure ;)

What I'd do:

• prepare a spec that test that specific test
• code your solution
• look at eventually failing specs

+ code review with @dacook I guess it's ok.
If we're not that sure, I'd purpose to prepare the solution and wait for @mkllnk's review.

What do you think?

[abdellani]

@jibees I believe that the error is on overwriting the shipping_rates list and removing the shipping_rate that points to the old shipping_method.

Suppose we start with a distributor that supports the shipping methods m1 and m2

order.shipments will have two shipment rates:

order:
  shipment:
    shipment rates:
      - shipping rate 1 (connected to m1) [Selected]
      - shipping rate 2 (connected to m2)
  distributor:
    shipment methods:
      - m1
      - m 2

Suppose now that the distributor removed m1 from his list, the order.shipments will not change

order:
  shipments: 
    shipping rates: <- need to refresh
      - shipping rate 1 [Selected]
      - shipping rate 2
  distributor:
    shipment methods:
      - m 2

when we access the order as admin, @order.shipments.map(&:refresh_rates) will run to refresh the shipments on the order.
=> the shipping rates will be replaced with a new list built based on the shipping methods available on the distributor.

order:
  shipments: 
    shipping rates: <- we're refreshing this
      - shipping rate 2 (connected to m2)  (Not selected, but will be used by default since it is the first shipping rate in the list) 
  distributor:
    shipment methods:
      - m 2

In the final step, we lose the shipment rate 1 that's connected to the m1.

Now, if you try to access order.shipments.shipping_method you'll get m2 instead of m1.
This is because shipping_method is accessed through the shipments rates, and we erased the shipment_rate that points to m1.

What do you think?

[abdellani]

@RachL @jibees @audez

I was thinking about the solution of preventing the removal of a shipping method if it's used in an order cycle.

Honestly, I was confused about the meaning of 'removing'.

Case 1: if removing means unselecting a shipping method from a distributor (as described in the initial issue).

Denying this operation will not solve the problem, it'll just postpone it to the end of the order cycle (I explained in my previous comments).

I already have a solution for this case (and I'll implement it).

Case 2: if removing means erasing the shipping method from the database.

This is out of the scope. By default, when the user decides to erase a shipping method, it will be softly deleted from the database (acts_as_paranoid is used for that purpose). I don't know what will happen to the completed orders that refer to the deleted shipping methods.

openfoodnetwork/app/models/spree/shipping_method.rb

Lines 3 to 12 in 5ed6e55

	module Spree
	class ShippingMethod < ApplicationRecord
	include CalculatedAdjustments
	DISPLAY_ON_OPTIONS = {
	both: "",
	back_end: "back_end"
	}.freeze
	acts_as_paranoid
	acts_as_taggable

[RachL]

@abdellani thanks for the update. There are two options I think:

1. improve soft delete: it's surprising we do have soft delete in place, but I guess it's good news. However it's not working properly: soft delete should delete the shipping or payment method from the UI of the user - apart from previous orders. The method must stay in database indeed. If we can make that work, it's the cleanest solution.

if this takes too much work (our aim here is to fix a bug, not to implement a feature) we need to:

2. remove the ability for the user to delete a shipping or payment method. In ALL cases, let's forget the order cycle rule.
   If users want to hide the method from the shopfront, they will still have the ability to switch it to "backoffice only". They will still keep all their shipping and payment method ever created, but that's a minor problem.

What do you think?

[abdellani]

Thank you @RachL

However it's not working properly: soft delete should delete the shipping or payment method from the UI of the user - **apart from previous orders**.

I agree and I believe that the source of the problem is the method Shipment#refresh_rates.
It's removing the selected shipping methods from the existing orders.

openfoodnetwork/app/models/spree/shipment.rb

Lines 114 to 124 in 5ed6e55

	def refresh_rates
	return shipping_rates if shipped?
	# The call to Stock::Estimator below will replace the current shipping_method
	original_shipping_method_id = shipping_method.try(:id)
	self.shipping_rates = OrderManagement::Stock::Estimator.new(order).shipping_rates(to_package)
	keep_original_shipping_method_selection(original_shipping_method_id)
	shipping_rates
	end

I tried to prevent that in my PR, but It seems that my solution is causing another problem with the total amount calculation.

More precisely, in the following test, order.fee_adjustment.amount is not calculated properly. I think I'll need some help to fix this.

https://github.com/openfoodfoundation/openfoodnetwork/actions/runs/3675702122/jobs/6215446956#step:10:29