2696,2788 [Spree Upgrade] Fix use of shipping method ID for subscriptions #3560

kristinalim · 2019-02-27T01:44:28Z

Fix use of shipping method ID for subscriptions.

What? Why?

Closes [Spree Upgrade] Fix subscriptions.shipping_method_id (13 broken specs) #2696
Relates to [Spree Upgrade] Fix pending specs in Order Syncer and Proxy Order (Subscriptions) #2788 (does not close issue)

The shipping method in Spree 2.0 is not associated directly to orders, but through shipment shipping rates.

Before Spree 2, OrderFactory keeps subscription orders in cart state but already sets associations and creates associated records (like payments). But in Spree 2, transitioning an order to "delivery" state removes and regenerates shipments. Considering above, this means that the selected shipping method could be forgotten.

To work around this, the code that regenerates shipping rates for a shipment has been updated to remember the previously selected shipping method first and select it again after creating the new shipping rates.

What should we test?

Not worth testing as there are still some broken functionalities. But some unit and feature tests have been uncommented and now pass.

luisramos0

I am really happy you are looking at this @kristinalim :-)

I try to add info about these methods so you can evaluate what's the best approach for subscriptions.

it's not an obvious decision but I think we should close the order api with order.shipment: single shipment per order (it's a fact in v2 although it may change in the future). we can make subscriptions work on that assumption. what do you think?

luisramos0 · 2019-02-27T23:42:01Z

app/services/order_syncer.rb

      order.update_attribute(:shipping_method_id, shipping_method_id)
    else
-      unless order.shipments.with_state('pending').where(shipping_method_id: shipping_method_id).any?
+      unless pending_shipment_using_shipping_method(order, shipping_method_id).present?
        order_update_issues.add(order, I18n.t('admin.shipping_method'))
      end
    end


see order_shipment.rb, in ofn v2 we have one very useful rule (it makes everything much easier): one order can only have one shipment, there will never be 2 shipments in an order. So you can use order.shipment.

also, spree::shipment.selected_shipping_rate is kept to single selection in spree::shipment.selected_shipping_rate_id=
so you can always use order.shipment.selected_shipping_rate

also, you can simply use order_shipment.shipping_method that redirects to spree::shipment.shipping_method that redirects to spree::shipment.selected_shipping_rate :-)

I think these facts will help simplify this code, right?

luisramos0 · 2019-02-27T23:42:34Z

app/services/order_syncer.rb

+
+  def select_shipping_method_for_shipment(shipment, shipping_method)
+    shipping_rate = find_or_create_shipping_rate_for_shipping_method(shipment, shipping_method)
+    shipment.update_attributes(selected_shipping_rate_id: shipping_rate.id)


should be shipment.selected_shipping_rate_id=(shipping_rate.id)

@luisramos0 Using update_attributes also works because selected_shipping_rate_id is attr_accessible: https://github.com/openfoodfoundation/spree/blob/2-0-4-stable/core/app/models/spree/shipment.rb#L19 And specs would fail if in the future it won't allow mass assignment anymore.

luisramos0 · 2019-02-27T23:45:10Z

app/services/order_syncer.rb

+  end
+
+  def find_or_create_shipping_rate_for_shipping_method(shipment, shipping_method)
+    shipping_rate = shipment.shipping_rates.where(shipping_method_id: shipping_method.id).last


as in order_shipment.select_shipping_method you can do:
shipping_rate = shipment.shipping_rates.find_by_shipping_method_id(shipping_method_id)

luisramos0 · 2019-02-27T23:47:07Z

app/services/order_syncer.rb

@@ -66,12 +66,16 @@ def update_payment_for(order)
  end

  def update_shipment_for(order)
-    shipment = order.shipments.with_state('pending').where(shipping_method_id: shipping_method_id_was).last
+    shipment = pending_shipment_using_shipping_method(order, shipping_method_id_was)
+
    if shipment


maybe this can simply be
if order.shipment.shipping_method == shipping_method_id_was?

luisramos0 · 2019-02-27T23:49:32Z

spec/services/order_syncer_spec.rb

-          order.update_attributes(shipping_method_id: changed_shipping_method.id)
+          shipment = order.shipments.first
+          shipping_rate = shipment.shipping_rates.where(shipping_method_id: changed_shipping_method.id).first
+          shipment.update_attributes(selected_shipping_rate_id: shipping_rate.id)


this is probably order.shipment.select_shipping_method(changed_shipping_method.id)

luisramos0 · 2019-02-27T23:50:11Z

spec/services/order_syncer_spec.rb

+          shipment = order.shipments.first
+          shipping_rate = shipment.shipping_rates.where(shipping_method_id: changed_shipping_method.id).first
+          shipment.update_attributes(selected_shipping_rate_id: shipping_rate.id)
+          order.update_attributes!(shipping_method_id: changed_shipping_method.id)


see below regarding the column order.shipping_method_id

luisramos0 · 2019-02-27T23:56:16Z

app/services/order_factory.rb

+
+    # Transition to :payment state.
+    @order.next!
+


you are assuming a specific order workflow here...

also, do you know what happens if one of these steps fails for some reason?
you could run @order.next! only if order.state is the expected state and you could do something if @order.next! fails

I am sorry I dont have a specific suggestion for this one...

@luisramos0 What do you think about this approach: 7206cd8 dd2f7ec

With these changes, shipment records are now created without changing the order status (similar to what is done for payments), and we now remember the previously selected shipping method and select it again after the new shipping rates have been created.

luisramos0 · 2019-02-27T23:57:47Z

app/services/order_factory.rb

@@ -65,6 +79,10 @@ def set_addresses
    @order.update_attributes(attrs.slice(:bill_address_attributes, :ship_address_attributes))
  end

+  def set_shipping_method
+    @order.update_attributes!(shipping_method_id: attrs[:shipping_method_id])


the column order.shipping_method_id is deprecated and will be deleted after v2 is live. we cannot rely on it.
this is how you do it in v2:
@order.shipment.select_shipping_method(attrs[:shipping_method_id])

kristinalim · 2019-03-01T11:07:26Z

Thanks for the info, @luisramos0 - that's very helpful info and indeed makes this simpler.

kristinalim · 2019-03-06T20:13:53Z

@luisramos0 Thanks for that feedback.

I have already been working on this, but then noticed this code in Spree admin/orders#edit which transitions orders as far as it can. 🤕

while @order.next; end

So that was why I kept seeing in the UI that subscription orders were completed... I suppose we're keeping this behaviour to keep the workflow similar to Spree? This behaviour wasn't in the previous version.

And then I checked Spree 2.4, and it looks like it remembers the shipping method when regenerating the shipping rates:

      # StockEstimator.new assigment below will replace the current shipping_method
      original_shipping_method_id = shipping_method.try(:id)

      self.shipping_rates = Stock::Estimator.new(order).shipping_rates(to_package)

      if shipping_method
        selected_rate = shipping_rates.detect { |rate|
          rate.shipping_method_id == original_shipping_method_id
        }
        self.selected_shipping_rate_id = selected_rate.id if selected_rate
      end

Considering above, I think that overriding the Spree::Order method would be the best approach for now.

Let me know what you think. Moving this back to Code Review for further critique.

luisramos0 · 2019-03-06T22:57:35Z

yes, that while order.next is so painful!

I see you prefer this version. I still prefer the previous version or some form of service that handles this logic outside of spree models. The main reason is that this overriding is not necessary and by creating a service with a clear API we would be able to improve the messy spree checkout workflow code.

Let's see what other reviewers say.

mkllnk

I agree that modifying Spree's model is not a great option. But I haven't see the previous code to compare. Do you have a link @kristinalim?

From what I read in the comments, it seemed to use the subscription to choose the shipping method. What if the method got changed before the end of the order cycle? Would that change get lost in the old implementation?

How does Spree deal with this? Does Spree only run the code when a shipping method is selected and available as params?

luisramos0 · 2019-03-08T09:13:33Z

yes, the order workflow will fail (it will be stuck in delivery) if no shipping methods are available. another case is that the selection gets lost if the selected shipping method becomes unavailable. But that will be a problem in both implementations: in or out of the spree model.

kristinalim · 2019-03-12T04:09:59Z

I was wrong in my previous statement, where I said future versions of Spree remember the shipping method. This remembering only happens in Spree::Shopment#refresh_rates, which happens separately from transitioning an order (I got confused). This is called in admin/orders#edit and admin/orders/customer_details#update, separate from while @order.next; end. I think the goal mostly is to make sure that Spree::ShippingRate records exist and are calculated where needed.

When transitioning, it still calls Spree::Order#create_proposed_shipments which continues to remove shipments, until 2-4-stable.

What if the method got changed before the end of the order cycle? Would that change get lost in the old implementation?

@mkllnk When the shipping method in the subscription is changed before the OC closes, OrderSyncer updates the shipping method in future orders IF the shipping method in the order is still the same as that in the subscription (meaning it wasn't customized in the order). This behaviour is described in specs here.

luisramos0 · 2019-03-12T09:10:07Z

@kristinalim fyi as it is related to this PR, as I was playing with live data in v2, I realized that refresh rates should only be called if order is not complete: https://github.com/spree/spree/blob/3c46d141c5cb2bedced63596178a0a79df3f0c9e/backend/app/controllers/spree/admin/orders_controller.rb#L54-L60

I think we'll need to bring this change to our spree 2-0-4 branch otherwise, editing an old order may just break it as shipping methods may not be setup appropriately for older orders.

kristinalim · 2019-03-12T10:30:15Z

I think we'll need to bring this change to our spree 2-0-4 branch otherwise, editing an old order may just break it as shipping methods may not be setup appropriately for older orders.

@luisramos0 I'll do it if you're not working on it yet. 🙂

The ship address of the order is properly set in ProxyOrder#initialise_order! through OrderFactory. The source of these spec failures seem to be a matter of how the records are set up for the tests.

A spec has been added to check that the attributes for the order states after "cart" ("address", "delivery", "payment") are retained if the order is transitioned to completion, BUT currently this is passing because it is the only shipping method available.

This method needs to be updated to use custom order advance logic in service.

luisramos0 · 2019-03-12T10:52:53Z

no, I am not working on it. please go ahead :-)

I think we need to keep the workflow .next loop in these methods, because otherwise the page flow (moving automatically to customer details) will not work.

mkllnk · 2019-03-12T21:49:46Z

@kristinalim

When the shipping method in the subscription is changed before the OC closes, OrderSyncer updates the shipping method in future orders

That's not what I meant. I was thinking of the scenario that a single order is changed without touching the subscription. From the description of your previous implementation it sounded like that change would get lost because you would use the proxy order as reference for the shipping method. If the change is done in the proxy order as well (I don't know) then it's no problem and your previous implementation was correct.

I'll wait for your next version and review again. :-)

kristinalim · 2019-03-13T00:57:15Z

Thanks, @mkllnk!

That's not what I meant. I was thinking of the scenario that a single order is changed without touching the subscription. From the description of your previous implementation it sounded like that change would get lost because you would use the proxy order as reference for the shipping method.

The implementation was using the previous shipping method of the order before the transition, whether or not this was created for a subscription, so it wasn't losing the data. Does this answer your concern?

Anyway, I've already moved the order advancement code to a service (pretty okay with the idea now). 🙂 I'm just making a last change of having other places advancing the order state to use this service as well, if the initial order state is not specific.

This copies the following: * admin/orders/customer_details#update * admin/payments#create

This separates logic for bang and non-bang versions of Spree::Order#next. The different conditions used in both methods (state == "completed" vs order.completed?) have implications in whether a transition is attempted or not.

kristinalim · 2019-03-13T07:04:52Z

This is now back in Code Review. Thanks for your feedback, @luisramos0 and @mkllnk!

luisramos0

Awesome work @kristinalim
I really like it!

I'd just make the conditions in update_shipment_for a bit simpler.

luisramos0 · 2019-03-13T11:01:39Z

app/controllers/spree/admin/orders_controller_decorator.rb

@@ -27,6 +27,17 @@ def index
    # within the page then fetches the data it needs from Api::OrdersController
  end

+  def edit
+    @order.shipments.map &:refresh_rates


there are two points about this method:

there's a unless statement that we will need before the refresh shipping rates (out of scope for this PR)
https://github.com/spree/spree/blob/3c46d141c5cb2bedced63596178a0a79df3f0c9e/backend/app/controllers/spree/admin/orders_controller.rb#L54

and also, the order advancement logic was removed in later versions of spree 2-0:
spree/spree@8208b4b#diff-1e2ba31309f1f1abd2a4b626036d316f

these are both out of scope of this PR.

luisramos0 · 2019-03-13T11:15:50Z

app/controllers/spree/admin/payments_controller_decorator.rb

+      flash[:error] = "#{e.message}"
+      redirect_to new_admin_order_payment_path(@order)
+    end
+  end


It's not good that we need to copy spree code to our code base but I am actually very happy that we are reclaiming this payments code to OFN, because the spree code is not good in v2 and it's a big mess in v3.7 here.

this kind of logic must go to a service, it's not the responsibility of the controller.
So I see this as the first step to improve it on our side.

It's out of scope for this PR.

app/services/advance_order_service.rb

app/services/order_syncer.rb

luisramos0 · 2019-03-14T10:38:34Z

app/services/advance_order_service.rb

@@ -0,0 +1,35 @@
+class AdvanceOrderService


you went for option 4 here
https://community.openfoodnetwork.org/t/how-to-name-a-new-service-in-ofn/1530

mkllnk

Great work, Kristina! There is just one style issue I'm wondering if you agree with.

app/services/advance_order_service.rb

sauloperez

Great work @kristinalim ! these controllers look neat 👌

sauloperez · 2019-03-20T08:38:38Z

app/controllers/spree/admin/orders/customer_details_controller_decorator.rb

@@ -1,6 +1,22 @@
 Spree::Admin::Orders::CustomerDetailsController.class_eval do
  before_filter :set_guest_checkout_status, only: :update

+  def update
+    if @order.update_attributes(params[:order])
+      if params[:guest_checkout] == "false"


I'm a bit surprised to compare a boolean value to a string. Rails does parse JSON to Ruby primitives. How is it possible that we end up with a String and not with a FalseClass instance 🤔 ? I would have expected to read this as if !params[:guest_checkout]

@sauloperez This isn't the case when the string is sent to the server as HTTP form data. However, this string would still be converted to a boolean as we expect when assigned to an ActiveRecord boolean attribute. 🙂

sauloperez · 2019-03-20T08:42:32Z

app/services/advance_order_service.rb

+  private
+
+  def advance_order(options)
+    until order.state == "complete"


It'd be great to stick to the approach in line 24 for consistency IMO

I gave this a try just now, @sauloperez, and confirmed that a completed but later cancelled order would attach an error here (ActiveRecord::Base#errors) whereas using order.completed? would not. Spree::Order#completed? looks at completed_at, not the order state.

We can actually remove advance_order! in Spree 2.2. (Discussion here: heere) Are you okay with waiting until then so we avoid doing a more comprehensive check of different scenarios?

sauloperez · 2019-03-22T10:08:12Z

Merging as suggested by @kristinalim . It seems to be passing those 13 specs.

kristinalim self-assigned this Feb 27, 2019

kristinalim changed the title ~~2696 [Spree Upgrade] Fix use of shipping method ID for subscriptions~~ 2696 [WIP] [Spree Upgrade] Fix use of shipping method ID for subscriptions Feb 27, 2019

kristinalim changed the title ~~2696 [WIP] [Spree Upgrade] Fix use of shipping method ID for subscriptions~~ 2696,2788 [Spree Upgrade] Fix use of shipping method ID for subscriptions Feb 27, 2019

kristinalim force-pushed the fix/2788-shipping_method_id_in_shipments branch from 43eb89f to cc061ab Compare February 27, 2019 15:50

luisramos0 suggested changes Feb 27, 2019

View reviewed changes

kristinalim force-pushed the fix/2788-shipping_method_id_in_shipments branch 4 times, most recently from 7b7e39f to aad0bbc Compare March 1, 2019 19:42

kristinalim mentioned this pull request Mar 7, 2019

2788 [Subscriptions] Fix address issues when syncing subscriptions and orders #3589

Merged

mkllnk reviewed Mar 8, 2019

View reviewed changes

kristinalim force-pushed the fix/2788-shipping_method_id_in_shipments branch 2 times, most recently from d5f9d0d to 74f4c6a Compare March 12, 2019 09:09

kristinalim added 4 commits March 12, 2019 18:31

Set required ship address in order in spec

5f571aa

The ship address of the order is properly set in ProxyOrder#initialise_order! through OrderFactory. The source of these spec failures seem to be a matter of how the records are set up for the tests.

Set shipping method when creating order for subscription

0306cc7

Copy admin/orders#edit from Spree controller

ce5691d

This method needs to be updated to use custom order advance logic in service.

kristinalim force-pushed the fix/2788-shipping_method_id_in_shipments branch from 74f4c6a to c21d169 Compare March 12, 2019 10:33

kristinalim added 2 commits March 13, 2019 12:45

Move transitioning of order in admin/orders#edit to service

7a8bf76

Retain shipping method when transitioning order

54991f8

kristinalim added 9 commits March 13, 2019 12:45

Fix queries for pending order shipment in OrderSyncer

5b5d196

Fix updating of the shipping method for an order

9316096

Simplify order sync logic for shipping methods

a261d91

Uncomment now passing tests related to subscriptions

182fde5

Fix setup records for OrderFactory specs

7c34334

Fix setup records for SubscriptionPlacementJob specs

0cfee37

Fix specs for OrderSyncer

56e2ee5

Copy more actions advancing order from Spree

abaf90f

This copies the following: * admin/orders/customer_details#update * admin/payments#create

Use service in admin/orders/customer_details#update

0e69181

kristinalim force-pushed the fix/2788-shipping_method_id_in_shipments branch 2 times, most recently from 75d74ea to 2377a7b Compare March 13, 2019 05:57

Use service in admin/payments#create

8782f20

This separates logic for bang and non-bang versions of Spree::Order#next. The different conditions used in both methods (state == "completed" vs order.completed?) have implications in whether a transition is attempted or not.

kristinalim force-pushed the fix/2788-shipping_method_id_in_shipments branch from 2377a7b to 8782f20 Compare March 13, 2019 06:00

luisramos0 approved these changes Mar 13, 2019

View reviewed changes

luisramos0 reviewed Mar 14, 2019

View reviewed changes

Make order sync for shipping methods readable

7505dd0

mkllnk reviewed Mar 18, 2019

View reviewed changes

app/services/advance_order_service.rb Outdated Show resolved Hide resolved

sauloperez reviewed Mar 20, 2019

View reviewed changes

Separate call and call! for AdvanceOrderService

9460602

sauloperez approved these changes Mar 22, 2019

View reviewed changes

sauloperez merged commit 846e6fd into openfoodfoundation:2-0-stable Mar 22, 2019

kristinalim mentioned this pull request Mar 22, 2019

[Spree Upgrade] Fix features/admin/subscriptions_spec (2 broken specs) #3396

Closed

luisramos0 mentioned this pull request Mar 26, 2019

[Spree Upgrade] Override default spree splitters config #3604

Merged

kristinalim mentioned this pull request Aug 22, 2019

[Subs] Wrong Shipping Method Applied to Subscriptions #4176

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2696,2788 [Spree Upgrade] Fix use of shipping method ID for subscriptions #3560

2696,2788 [Spree Upgrade] Fix use of shipping method ID for subscriptions #3560

kristinalim commented Feb 27, 2019 •

edited

Loading

luisramos0 left a comment

luisramos0 Feb 27, 2019

luisramos0 Feb 27, 2019

kristinalim Mar 1, 2019

luisramos0 Feb 27, 2019

luisramos0 Feb 27, 2019

luisramos0 Feb 27, 2019

luisramos0 Feb 27, 2019 •

edited

Loading

luisramos0 Feb 27, 2019

kristinalim Mar 1, 2019

luisramos0 Feb 27, 2019

kristinalim commented Mar 1, 2019

kristinalim commented Mar 6, 2019

luisramos0 commented Mar 6, 2019

mkllnk left a comment

luisramos0 commented Mar 8, 2019

kristinalim commented Mar 12, 2019

luisramos0 commented Mar 12, 2019

kristinalim commented Mar 12, 2019

luisramos0 commented Mar 12, 2019

mkllnk commented Mar 12, 2019

kristinalim commented Mar 13, 2019

kristinalim commented Mar 13, 2019

luisramos0 left a comment

luisramos0 Mar 13, 2019

luisramos0 Mar 13, 2019

luisramos0 Mar 14, 2019 •

edited

Loading

mkllnk left a comment

sauloperez left a comment

sauloperez Mar 20, 2019

kristinalim Mar 21, 2019

sauloperez Mar 20, 2019

kristinalim Mar 21, 2019 •

edited

Loading

sauloperez commented Mar 22, 2019

2696,2788 [Spree Upgrade] Fix use of shipping method ID for subscriptions #3560

2696,2788 [Spree Upgrade] Fix use of shipping method ID for subscriptions #3560

Conversation

kristinalim commented Feb 27, 2019 • edited Loading

What? Why?

What should we test?

luisramos0 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

luisramos0 Feb 27, 2019 • edited Loading

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

kristinalim commented Mar 1, 2019

kristinalim commented Mar 6, 2019

luisramos0 commented Mar 6, 2019

mkllnk left a comment

Choose a reason for hiding this comment

luisramos0 commented Mar 8, 2019

kristinalim commented Mar 12, 2019

luisramos0 commented Mar 12, 2019

kristinalim commented Mar 12, 2019

luisramos0 commented Mar 12, 2019

mkllnk commented Mar 12, 2019

kristinalim commented Mar 13, 2019

kristinalim commented Mar 13, 2019

luisramos0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luisramos0 Mar 14, 2019 • edited Loading

Choose a reason for hiding this comment

mkllnk left a comment

Choose a reason for hiding this comment

sauloperez 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

kristinalim Mar 21, 2019 • edited Loading

Choose a reason for hiding this comment

sauloperez commented Mar 22, 2019

kristinalim commented Feb 27, 2019 •

edited

Loading

luisramos0 Feb 27, 2019 •

edited

Loading

luisramos0 Mar 14, 2019 •

edited

Loading

kristinalim Mar 21, 2019 •

edited

Loading