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

abdellani · 2022-12-10T18:41:36Z

What? Why?

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

Summary

The original issue:

a customer creates an order and selects the shipping method sm1
the distributor decides to no longer support sm1
if the distributor accesses the edit order page, the order's selected shipping method will be replaced automatically with the next available shipping method.

source of the problem: Spree::Shipment#refresh_rates
Fix:
we'll keep the selected shipping method when refresh_rates is called.

Potential issues (not sure if we have to solve it on this PR):
If the distributor hits Update & Recalculate Fees

Will the selected shipping method be replaced?
When I tested it, it was not replaced.
Suppose that the shipping rate is updated, will the shipping cost be recalculated?
When I tested it, it was not recalculated.
I notice that the order sorting has changed.

What should we test?

Described on When user deletes a shipping method existing orders are updated with a random shipping method #6029

Release notes

Changelog Category: Technical changes

Dependencies

Documentation updates

abdellani · 2022-12-10T22:39:17Z

The idea is to store a copy of the original select_shipping_rate before overwriting the shipping_rates of a shipment with the results of OrderManagement::Stock::Estimator.new(order).shipping_rates(to_package)

abdellani · 2023-01-02T12:06:04Z

The two tests fail because the selected shipment method is expected to be removed.
If the solution is validated, I'll fix those tests.

./spec/models/spree/shipment_spec.rb:111
./spec/models/spree/shipment_spec.rb:122

mkllnk

Hello @abdellani and a Happy New Year! 💐

I have a question about the existing code: What is the purpose of #refresh_rates? From a quick glance, it checks if the existing shipping method is still available and assigns a new one if not. But now we want to preserve the existing method even if it has been deleted. So is there any other functionality we have to preserve? Otherwise I'm wondering if we can get rid of the whole thing.

I probably didn't consider the case of multiple rates per shipping method and that the quantity of items could change. Then we need to refresh the rate, guess. I'm not familiar with this code at the moment.

In those cases, when a better rate is available, would your addition prevent the update? Or do we have only one rate per method?

If the solution is validated, I'll fix those tests.

Ideally, we would change specs before fixing the code. I know that sometimes we don't know which specs are affected and can change them only afterwards but it would be good to verify your solution with specs before review.

mkllnk · 2023-01-03T04:49:51Z

app/models/spree/shipment.rb

+      new_shapping_rates = OrderManagement::Stock::Estimator.new(order).shipping_rates(to_package)
+
+      if selected_shipping_rate.present? && new_shapping_rates.none?{ |shipping_rate|
+        shipping_rate.shipping_method_id == selected_shipping_rate.shipping_method_id
+      }
+        new_shapping_rates << selected_shipping_rate
+      end
+      self.shipping_rates = new_shapping_rates


This part could go into its own method new_shipping_rates which makes sure that the current rate is in there.

mkllnk · 2023-01-03T04:55:45Z

app/models/spree/shipment.rb

+      if selected_shipping_rate.present? && new_shapping_rates.none?{ |shipping_rate|
+        shipping_rate.shipping_method_id == selected_shipping_rate.shipping_method_id
+      }
+        new_shapping_rates << selected_shipping_rate
+      end


I find this code difficult to read. You are adding the selected rate if it's not in the collection already, right? The added block within the if is confusing, I think.

Isn't this the same as (new_shapping_rates + [selected_shipping_rate]).uniq?

I like the suggested syntax, but new_shapping_rates will always contain new objects.

how about?

new_shapping_rates.map(&:shipping_method_id).exclude?(selected_shipping_rate.shipping_method_id)

Yes, that's easier to read.

mkllnk · 2023-01-03T04:56:13Z

spec/controllers/api/v0/shipments_controller_spec.rb

@@ -246,6 +246,7 @@

      before do
        order.shipments.first.shipping_methods = [shipping_method1, shipping_method2]
+        order.shipments.each(&:refresh_rates)


The commit message doesn't explain why we need to refresh the rates here.

I think now, we don't need to use

order.shipments.first.shipping_methods = [shipping_method1, shipping_method2]

because this will create shipping rates with cost = 0.

Instead, we can use

order.shipments.each(&:refresh_rates)

refresh_rates will automatically load the shipping methods from the distributor and calculate the cost before creating the shipping_rates.

openfoodnetwork/app/models/spree/shipment.rb

Lines 117 to 121 in f122f4b

# 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)

openfoodnetwork/engines/order_management/app/services/order_management/stock/estimator.rb

Lines 18 to 21 in f122f4b

shipping_methods.each do |shipping_method|

cost = calculate_cost(shipping_method, package)

shipping_rates << shipping_method.shipping_rates.new(cost: cost) unless cost.nil?

end

abdellani · 2023-01-03T07:12:37Z

Happy New Year @mkllnk :)

I have a question about the existing code: What is the purpose of #refresh_rates? From a quick glance, it checks if the existing shipping method is still available and assigns a new one if not. But now we want to preserve the existing method even if it has been deleted. So is there any other functionality we have to preserve? Otherwise I'm wondering if we can get rid of the whole thing.

In my understanding:

refresh_rates fetches the list of the available shipping methods on the distributor.
For every shipping method, it creates one shipping rate in the order

Suppose that the distributor offers two shipping methods "sm1" and "sm2", The problem comes when

The user creates an order and selects the shipping method "sm 1"
The distributor decides to no longer support "sm 1"
refresh_rates will remove sm 1 and mark sm 2 as the selected one.

So, we want to preserve the existing shipping method in the order only if this method is selected. If it's not selected, it's ok to lose it.

I probably didn't consider the case of multiple rates per shipping method and that the quantity of items could change. Then we need to refresh the rate, guess. I'm not familiar with this code at the moment.
In those cases, when a better rate is available, would your addition prevent the update? Or do we have only one rate per method?

Now that I think of it, maybe under some conditions, my solution will create a problem.
For example: if the distributor offers one shipping method and the user decided to update his order. The shipment cost will not be updated.
I'll need to recalculate the cost using the shipping method estimator

openfoodnetwork/engines/order_management/app/services/order_management/stock/estimator.rb

Lines 60 to 62 in f122f4b

    
           def calculate_cost(shipping_method, package) 
        
             shipping_method.calculator.compute(package) 
        
           end

mkllnk · 2023-01-03T22:43:14Z

Ah, thank you. I read a bit more and the existing code is really hard to read. One complicating design fault is the selected field on the shipping rates. A direct association would be easier.

Anyway, I think that your idea is really good. When the selected shipping rate is not available any more, we want to keep it. But that needs to apply only for a selected rate. And the user still needs to be able to select another one. Maybe check if there are test cases for this already or set some up. Then it's easier to check if the code is doing what we think it does.

abdellani · 2023-01-12T08:26:10Z

@mkllnk
I implemented a system test that reproduces the same context described in the issue.

mkllnk

I implemented a system test that reproduces the same context described in the issue.

Yes very good. I was wondering though if we need to also cover other cases which are not covered yet.

For example, if we change the amount of a shipping method then the new amount will be applied, correct? Do we want that? Shouldn't the original rate be kept in all cases?

I'm wondering if it would be useful to override the eql? method here.

mkllnk · 2023-01-16T02:52:10Z

app/models/spree/shipment.rb

+      if selected_shipping_rate.present? && new_shapping_rates.none?{ |shipping_rate|
+        shipping_rate.shipping_method_id == selected_shipping_rate.shipping_method_id
+      }
+        new_shapping_rates << selected_shipping_rate
+      end


Yes, that's easier to read.

spec/system/admin/order_spec.rb

abdellani · 2023-01-19T06:53:01Z

If we want to recalculate the shipment price, we can reuse the estimator.

estimator = OrderManagement::Stock::Estimator.new(order)
selected_shipping_rate.update(cost: estimator.calculate_cost(selected_shipping_rate, to_package)

abdellani · 2023-01-21T18:24:42Z

Shipment#refresh_rates fetches the available shipping methods from the order's distributor and calculates the shipping rates.
if the shipment instance is in the state shipped, the method returns without updating the order's amount.
The order state doesn't matter, the refresh_rates method only cares about the shipment instance.

So, we have three parameters to take into account:
a. order shipping status
b. whether the shipping method is supported or not
c. the shipping rate

Based on those, I suggest testing the following cases:

shipping method removed from the distributor?	shipping method rate update?	Result (order amount will be updated?)
Yes	Yes	Yes
Yes	No	No
No	Yes	Yes
No	No	No

suppose the Order is completed but not shipped.
In all those cases, the selected shipping method should not be removed, replaced, or unselected.

abdellani · 2023-01-22T00:06:51Z

@filipefurtad0
Can you please tell me what you think about the new system tests that I implemented?

At this level, I'm stuck with the refresh_rate method.
After the PR, It's returning the expected shipping_rates, but the shipment rate amount is always 0.

filipefurtad0 · 2023-01-23T16:50:34Z

spec/system/admin/order_spec.rb

@@ -420,7 +420,7 @@ def new_order_with_distribution(distributor, order_cycle)

    describe "viewing the edit page" do
      let!(:shipping_method_for_distributor1) do
-        create(:shipping_method, name: "Normal", distributors: [distributor1])
+        create(:shipping_method_with, :flat_rate, name: "Normal", amount: 12, distributors: [distributor1])
      end
      let!(:order) do
        create(:order_with_taxes, distributor: distributor1, ship_address: create(:address),


I think the issue is that, creating an order_with_taxes (which has completed_order_with_totals as parent) will create a shipment for it too. So, later on, when... (continuing below)

spec/system/admin/order_spec.rb

abdellani · 2023-01-26T09:55:20Z

@filipefurtad0

I debugged the test:
All starts from select_shipping_method

openfoodnetwork/app/models/concerns/order_shipment.rb

Lines 39 to 49 in 1ea7304

    
           def select_shipping_method(shipping_method_id) 
        
             return if shipping_method_id.blank? || shipments.empty? 
        
             shipment = shipments.first 
        
             shipping_rate = shipment.shipping_rates.find_by(shipping_method_id: shipping_method_id) 
        
             return unless shipping_rate 
        
             shipment.selected_shipping_rate_id = shipping_rate.id 
        
             shipment.shipping_method 
        
           end

Order#select_shipping_method works as follow:

retrieves shipment
retrieves the shipping_rate among the available shipping_rates using the selected_shipping_method_id

set the shipping_rate as the selected one using shipment.selected_shipping_rate_id

openfoodnetwork/app/models/spree/shipment.rb

Lines 104 to 108 in 1ea7304

    
           def selected_shipping_rate_id=(id) 
        
             shipping_rates.update_all(selected: false) 
        
             shipping_rates.update(id, selected: true) 
        
             save! 
        
           end

save! will trigger the ensure_correct_adjustment

openfoodnetwork/app/models/spree/shipment.rb

Lines 261 to 267 in 1ea7304

    
           def ensure_correct_adjustment 
        
             if fee_adjustment 
        
               fee_adjustment.originator = shipping_method 
        
               fee_adjustment.label = adjustment_label 
        
               fee_adjustment.amount = selected_shipping_rate.cost if fee_adjustment.open? 
        
               fee_adjustment.save! 
        
               fee_adjustment.reload

Now, the shipping method will be updated, as expected, but there's one thing that I don't understand. The cost will not be updated!

     fee_adjustment.amount = selected_shipping_rate.cost if fee_adjustment.open?

Why do we authorize updating the shipping methods, and check if the fee_adjustment.open? only when it comes to updating the cost?

abdellani · 2023-01-26T10:31:24Z

I fixed the new test by calling 'open' and 'close' on the shipment adjustment.
The only test that's not passing is when the shipping rate of an unsupported shipping method is updated.
it's supposed to update the order's shipment cost, but this is not happening for the same reason: fee_adjustment is closed.

filipefurtad0 · 2023-01-26T12:35:35Z

Why do we authorize updating the shipping methods, and check if the fee_adjustment.open? only when it comes to updating the cost?

I think this might be legacy code, related to a functionality which used to be displayed in the UI, see this comment for reference:

Please note that this functionality was removed - notice the radio button associated adjustment open/closed:

It was removed on this PR.

an unsupported shipping method is updated.

Just to be sure, when you mean unsupported, you mean a shipping method which was deleted by the distributor?

this is not happening for the same reason: fee_adjustment is closed.

What is the state of shipment, in this case? There might be a good reason for the code to be designed in this way. I can imagine this is relevant, when an order is shipped (shipment_state = shipped), for example. In this case, there should be no updates on any values, nor it should not be possible to update the order.

abdellani · 2023-01-26T13:26:32Z

Just to be sure, when you mean unsupported, you mean a shipping method which was deleted by the distributor?

Yes

Please note that this functionality was removed - notice the radio button associated adjustment open/closed:

Do you mean I can remove this condition if fee_adjustment.open? ?
and can we remove fee_adjustment.state field from the database?

What is the state of shipment, in this case?

The shipment state is pending, but the problem is related to the state of the adjustment.

abdellani · 2023-02-15T07:59:26Z

@filipefurtad0
Some tests are failing because the sorting of the orders has changed. Is it ok?

filipefurtad0 · 2023-02-15T14:48:27Z

Hey @abdellani ,

Some tests are failing because the sorting of the orders has changed. Is it ok?

The failure seems consistent, but it does not seem related to this PR. If I pull this branch and run the spec locally, it passes.

The test should look like this:

But for some reason, in the CI, we're getting:

I'll try to debug this. It might be an order related failure. But it's a blocker for merging I guess...

My debugging strategy (please feel free to comment):

spec/system/admin/order_spec.rb is changed in this PR. So my best guess is that leaking from one spec to the other is occurring. I'm running:
bundle exec rspec --bisect spec/system/admin/order_spec.rb spec/system/admin/orders_spec.rb --seed 123456
to find failures... lets see.

abdellani · 2023-02-15T15:21:30Z

@filipefurtad0
Sorry, I meant the tests are failing because the returned orders are sorted differently from what's expected.

     Failure/Error: expect(page).to have_content(/#{order5.number}.*#{order4.number}.*#{order3.number}.*#{order2.number}/m)
expected to find text matching /R445887202.*R518028774.*R884221646.*R424560362/m in "Logged in as : [email protected] Account Logout Store DASHBOARD PRODUCTS ORDER CYCLES ORDERS REPORTS CONFIGURATION ENTERPRISES CUSTOMERS GROUPS USERS ORDERS BULK ORDER MANAGEMENT SUBSCRIPTIONS Listing Orders NEW ORDER SEARCH DATE RANGE STATUS cart address delivery payment confirmation complete cancelled awaiting return returned resumed INVOICE NUMBER: EMAIL FIRST NAME BEGINS WITH LAST NAME BEGINS WITH ONLY SHOW COMPLETE ORDERS SHIPPING METHOD UPS Ground UPS Ground Pick-up at the farm UPS Ground Signed, sealed, delivered UPS Ground UPS Ground UPS Ground UPS Ground DISTRIBUTORS Enterprise 99 Enterprise 100 Enterprise 101 Enterprise 98 Enterprise 102 ORDER CYCLES Four Three Two Five FILTER RESULTS CLEAR FILTERS No order selected ACTIONS 15 per page 4 Results found. Viewing 1 to 4. DISTRIBUTOR COMPLETED AT NUMBER STATE PAYMENT STATE SHIPMENT STATE EMAIL NAME TOTAL ▲ Enterprise 100 February 15, 2023 R884221646 COMPLETE CREDIT OWED ($-10,000.00) READY [email protected] Winston Mayer $50.00 Enterprise 102 February 15, 2023 R445887202 COMPLETE PAID READY [email protected] Ashlee Rolfson $50.00 Enterprise 101 February 15, 2023 R518028774 COMPLETE CREDIT OWED ($-9,980.00) READY [email protected] Meaghan Wilkinson $70.00 Enterprise 99 February 13, 2023 R424560362 COMPLETE BALANCE DUE ($40.00) PENDING [email protected] Fermin Morar $90.00 « FIRST PREVIOUS 1 NEXT LAST »"

order2.number,order3.number,order4.number,order5.number are on the page content but in a different order.
I was wondering if it's ok to reorder the orders in the expectation.

filipefurtad0 · 2023-02-15T19:54:59Z

I was wondering if it's ok to reorder the orders in the expectation.

Sure, I think it is ok to change the spec according to the new sorting.

I'm just wondering whether it is a consistent behavior as I cannot make the spec fail in my local env. It seems to only fail on the CI, so I was tipping on order-dependent failure. But I'm not sure, bisecting the specs did not help, as they always pass (for me).

So I don't have a clear answer, I don't know what's causing this failure - I mean, I don't know why the order totals changes, if this PR does not change this spec.

abdellani · 2023-02-16T06:52:17Z

Everything is green Yay !! 🎉

I think that the problem is fixed now.
I'm just not sure 100% if this will not create another problem with the recalculation of the order's amount (I tested some simple scenarios and it looks fine to me).

app/models/spree/shipment.rb

spec/system/admin/order_spec.rb

mkllnk · 2023-02-17T04:28:16Z

Moved back to In Dev because changes were requested by Filipe. I will review after those have been addressed. 😄

…d if distributor no longer support it

Co-authored-by: Filipe <[email protected]>

filipefurtad0

This looks good from the specs, as far as I could gather. I think in the manual testing phase we need a good look at states, and to verify if we keep a consistent behavior when the admin wishes to change the shipping method of a placed (complete, but not shipped) order.

Thanks for all you work here @abdellani!

mkllnk

Great. This is looking good. 👍

dacook

I have to admit I'm not familiar with this part of the system so it's hard to comment.

At a high level, I would have hoped to simply relax a condition somewhere to allow a shipping method to be used, despite being removed from distributors_shipping_methods. Maybe that's what you're doing here.

So looking at this implementation, it's calling a private method which I think is a bad idea. Can you re-arrange it?

dacook · 2023-02-23T23:36:59Z

app/models/spree/shipment.rb

+      if original_shipping_method_id.present? &&
+         distributor_shipping_rates.map(&:shipping_method_id)
+             .exclude?(original_shipping_method_id)
+        cost = estimator.__send__(:calculate_cost, shipping_method, to_package)
+        unless cost.nil?
+          original_shipping_rate = shipping_method.shipping_rates.new(cost: cost)
+          self.shipping_rates = distributor_shipping_rates + [original_shipping_rate]
+          self.selected_shipping_rate_id = original_shipping_rate.id
+        end
+      else
+        self.shipping_rates = distributor_shipping_rates
+      end


I noticed a call to a private method with __send__. This was a hint for me: I think the purpose of this code is to re-implement OrderManagement::Stock::Estimator in a specific way.

I'm wondering if the Estimator class should be updated. Would this implementation be safe to apply directly in Estimator? I see that it is called from perhaps one other place: Order#create_proposed_shipments (though I didn't search extensively).

Otherwise, perhaps this should be an alternate method next to Estimator#shipping_rates. Or at the very least, calculate_cost could be promoted to a public method.

I didn't want to copy-and-paste the code from the estimator to avoid potential issues in case the logic is updated.
I'll promote calculate_cost to a public method.

Okay, I'm still concerned that this code belongs in Estimator, but let's go ahead like this for now.

the Estimator gets the list of the shipping methods from the Package.
Based on the returned list, the Estimator will create the shipping rates

The Package retrieves the list of the shipping methods from the order's distributor.

openfoodnetwork/engines/order_management/app/services/order_management/stock/package.rb

Lines 81 to 85 in c3fe399

def shipping_methods

return [] unless order.distributor.present?

order.distributor.shipping_methods.uniq.to_a

end

I agree that the Estimator is a better place to fix the issue.

we can add an extra parameter like keep_original_shipping_rate = false

openfoodnetwork/engines/order_management/app/services/order_management/stock/estimator.rb

Lines 13 to 30 in c3fe399

def shipping_rates(package, frontend_only = true)

shipping_rates = []

shipping_methods = shipping_methods(package)

return [] unless shipping_methods

shipping_methods.each do |shipping_method|

cost = calculate_cost(shipping_method, package)

shipping_rates << shipping_method.shipping_rates.new(cost: cost) unless cost.nil?

end

shipping_rates.sort_by! { |r| r.cost || 0 }

unless shipping_rates.empty? || order.manual_shipping_selection

select_first_shipping_method(shipping_rates, frontend_only)

end

shipping_rates

end

I guess we should have moved back to 'In Dev' while under discussion ;)

I think this is still a good idea. Would you be confident to try it in a new PR?

I'll do that in a different PR :)

Sorry, I've missed these new messages - went ahead, tested and merged 😬

…method

filipefurtad0 · 2023-03-02T20:01:21Z

Hey @abdellani

Starting from an order with a given shipping method with a fee:

Then, deactivating the shipping method:

Is displayed on the reports:

So in summary, deactivating a shipping method:

removes it from the shipping methods list
removes it from the OC
keeps it in the orders where it was chosen - clicking update and recalculate fees keeps the fee and the shipping method
the used shipping method is kept on Orders and Distributors report

This looks great to me.

We should still find out though if we can still edit orders, change shipping methods and update fees (regression testing). I found this to be very robust -> because we now keep it after deleting, let's say we wish to remove that shipping method from an order:

After selecting and updating fees, we can see that the amounts are correct:

And that the correct shipping fee appears on the invoice - 10 EUR instead of 20 EUR:

I can't think of anything else to test, so...
I'd say this is good to go!! 🎉 Congrats for your work and patience on this one 👏 💪

abdellani force-pushed the fix_when_user_deletes_a_shipping_method_existing_orders_are_updated_with_a_random_shipping_method branch 2 times, most recently from 2068e4d to 6f1f3fb Compare December 10, 2022 22:36

abdellani force-pushed the fix_when_user_deletes_a_shipping_method_existing_orders_are_updated_with_a_random_shipping_method branch from 6f1f3fb to 513d3fa Compare December 12, 2022 12:21

abdellani force-pushed the fix_when_user_deletes_a_shipping_method_existing_orders_are_updated_with_a_random_shipping_method branch 2 times, most recently from 3386876 to 4e82a34 Compare January 2, 2023 11:28

abdellani marked this pull request as ready for review January 2, 2023 12:06

abdellani self-assigned this Jan 2, 2023

mkllnk requested changes Jan 3, 2023

View reviewed changes

abdellani force-pushed the fix_when_user_deletes_a_shipping_method_existing_orders_are_updated_with_a_random_shipping_method branch from 4e82a34 to 11c0481 Compare January 12, 2023 08:24

abdellani requested a review from mkllnk January 12, 2023 08:26

mkllnk reviewed Jan 16, 2023

View reviewed changes

abdellani force-pushed the fix_when_user_deletes_a_shipping_method_existing_orders_are_updated_with_a_random_shipping_method branch from 11c0481 to b09ab89 Compare January 17, 2023 10:24

abdellani force-pushed the fix_when_user_deletes_a_shipping_method_existing_orders_are_updated_with_a_random_shipping_method branch from b09ab89 to 729c4e2 Compare January 19, 2023 07:00

abdellani force-pushed the fix_when_user_deletes_a_shipping_method_existing_orders_are_updated_with_a_random_shipping_method branch from 729c4e2 to fad7fe3 Compare January 22, 2023 00:03

abdellani force-pushed the fix_when_user_deletes_a_shipping_method_existing_orders_are_updated_with_a_random_shipping_method branch from fad7fe3 to 7d61638 Compare January 22, 2023 00:09

filipefurtad0 reviewed Jan 23, 2023

View reviewed changes

spec/system/admin/order_spec.rb Show resolved Hide resolved

abdellani force-pushed the fix_when_user_deletes_a_shipping_method_existing_orders_are_updated_with_a_random_shipping_method branch from 7d61638 to 7636adf Compare January 26, 2023 10:23

abdellani force-pushed the fix_when_user_deletes_a_shipping_method_existing_orders_are_updated_with_a_random_shipping_method branch from 44490dd to c0051cd Compare February 15, 2023 07:09

abdellani force-pushed the fix_when_user_deletes_a_shipping_method_existing_orders_are_updated_with_a_random_shipping_method branch 4 times, most recently from 175d2eb to e8c0689 Compare February 16, 2023 06:41

abdellani requested review from filipefurtad0, mkllnk and dacook February 16, 2023 06:52

filipefurtad0 requested changes Feb 16, 2023

View reviewed changes

app/models/spree/shipment.rb Outdated Show resolved Hide resolved

spec/system/admin/order_spec.rb Outdated Show resolved Hide resolved

spec/system/admin/order_spec.rb Show resolved Hide resolved

spec/system/admin/order_spec.rb Show resolved Hide resolved

filipefurtad0 reviewed Feb 16, 2023

View reviewed changes

spec/system/admin/order_spec.rb Show resolved Hide resolved

abdellani force-pushed the fix_when_user_deletes_a_shipping_method_existing_orders_are_updated_with_a_random_shipping_method branch from da26ccf to 28e98b4 Compare February 20, 2023 08:40

abdellani and others added 2 commits February 20, 2023 09:55

recreate the shipping rate that refers to the original shipping metho…

15c2412

…d if distributor no longer support it

Update spec/system/admin/order_spec.rb

0738133

Co-authored-by: Filipe <[email protected]>

abdellani force-pushed the fix_when_user_deletes_a_shipping_method_existing_orders_are_updated_with_a_random_shipping_method branch from 28e98b4 to 0738133 Compare February 20, 2023 08:55

abdellani requested a review from filipefurtad0 February 20, 2023 09:08

filipefurtad0 approved these changes Feb 20, 2023

View reviewed changes

mkllnk approved these changes Feb 22, 2023

View reviewed changes

dacook requested changes Feb 23, 2023

View reviewed changes

promote OrderManagement::Stock::Estimator.calculate_cost to a public …

066c7bb

…method

abdellani requested a review from dacook February 28, 2023 07:09

dacook approved these changes Feb 28, 2023

View reviewed changes

filipefurtad0 merged commit 60bea89 into openfoodfoundation:master Mar 2, 2023

abdellani mentioned this pull request Jun 7, 2023

fix Viewing an order in state "confirmation" changes the state to "complete" #10936

Merged

	# 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_methods.each do \|shipping_method\|
	cost = calculate_cost(shipping_method, package)
	shipping_rates << shipping_method.shipping_rates.new(cost: cost) unless cost.nil?
	end

	def shipping_methods
	return [] unless order.distributor.present?

	order.distributor.shipping_methods.uniq.to_a
	end

	def shipping_rates(package, frontend_only = true)
	shipping_rates = []
	shipping_methods = shipping_methods(package)
	return [] unless shipping_methods

	shipping_methods.each do \|shipping_method\|
	cost = calculate_cost(shipping_method, package)
	shipping_rates << shipping_method.shipping_rates.new(cost: cost) unless cost.nil?
	end

	shipping_rates.sort_by! { \|r\| r.cost \|\| 0 }

	unless shipping_rates.empty? \|\| order.manual_shipping_selection
	select_first_shipping_method(shipping_rates, frontend_only)
	end

	shipping_rates
	end

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

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

Conversation

abdellani commented Dec 10, 2022 • edited Loading

What? Why?

Summary

What should we test?

Release notes

Dependencies

Documentation updates

abdellani commented Dec 10, 2022

abdellani commented Jan 2, 2023

mkllnk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abdellani commented Jan 3, 2023

mkllnk commented Jan 3, 2023

abdellani commented Jan 12, 2023

mkllnk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abdellani commented Jan 19, 2023

abdellani commented Jan 21, 2023 • edited Loading

abdellani commented Jan 22, 2023

Choose a reason for hiding this comment

abdellani commented Jan 26, 2023

abdellani commented Jan 26, 2023

filipefurtad0 commented Jan 26, 2023

abdellani commented Jan 26, 2023

abdellani commented Feb 15, 2023

filipefurtad0 commented Feb 15, 2023 • edited Loading

abdellani commented Feb 15, 2023

filipefurtad0 commented Feb 15, 2023 • edited Loading

abdellani commented Feb 16, 2023 • edited Loading

mkllnk commented Feb 17, 2023

filipefurtad0 left a comment

Choose a reason for hiding this comment

mkllnk left a comment

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipefurtad0 commented Mar 2, 2023

abdellani commented Dec 10, 2022 •

edited

Loading

abdellani commented Jan 21, 2023 •

edited

Loading

filipefurtad0 commented Feb 15, 2023 •

edited

Loading

filipefurtad0 commented Feb 15, 2023 •

edited

Loading

abdellani commented Feb 16, 2023 •

edited

Loading