Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Spree Upgrade] Handle shipping_method_id in checkout #2683

Closed
luisramos0 opened this issue Sep 11, 2018 · 5 comments
Closed

[Spree Upgrade] Handle shipping_method_id in checkout #2683

luisramos0 opened this issue Sep 11, 2018 · 5 comments
Assignees

Comments

@luisramos0
Copy link
Contributor

luisramos0 commented Sep 11, 2018

We need to look at both

  • app/assets/javascripts/darkswarm/controllers/checkout/checkout_controller.js.coffee
  • app/assets/javascripts/darkswarm/services/checkout.js.coffee
    and
  • app/views/checkout/_shipping.html.haml

and review/replace shipping_method_id usage.

For reference, other files that may need review/changes:

  • app/controllers/checkout_controller.rb
  • spec/requests/checkout/failed_checkout_spec.rb
  • spec/requests/checkout/stripe_connect_spec.rb
  • spec/javascripts/unit/darkswarm/services/checkout_spec.js.coffee
  • spec/controllers/checkout_controller_spec.rb

See this commit for line numbers.

@luisramos0 luisramos0 self-assigned this Sep 11, 2018
@luisramos0 luisramos0 changed the title [Spree Upgrade] Handle shipping_method_id on checkout JS code [Spree Upgrade] Handle shipping_method_id on checkout view and js code Sep 11, 2018
@luisramos0 luisramos0 changed the title [Spree Upgrade] Handle shipping_method_id on checkout view and js code [Spree Upgrade] Handle shipping_method_id in checkout Sep 12, 2018
@luisramos0 luisramos0 removed their assignment Sep 19, 2018
@luisramos0
Copy link
Contributor Author

luisramos0 commented Sep 22, 2018

From what I understand right now...

The checkout workflow moves to the delivery state and order.created_proposed_shipments is executed (this rebuilds packages, shipments and shipping_rates from line_items and shipping_methods). In spree, this prepares everything for the user to select one shipping_rate (which is a shipping_method assigned to a shipment in the order).

BUT we have a single step checkout, SO we need to have shipping_method selection before all this happens. I believe the simple solution is to keep doing what we are doing: presenting the user with a list of shipping_methods.

To do this:

  • we may need to repeat some of the shipping_rates logic before-hand.
  • we will need to keep order.shipping_method_id (the user selection!) in the frontend and in the controller. We will use shipping_method_id to select a specific shipping_rate before moving forward from state "delivery" (this will be done in the checkout controller). The user selected shipping_method_id will be stored as order.shipment.first.selected_shipping_rate.shipping_method_id.

With this I think we will be able to drop the order.shipping_method_id column (which is a non-sense column in the multi shipments world anyway, not totally nonsense in OFN2 because we force 1 order to have 1 shipment).

So, this is my current plan. I'll investigate a bit further and create a PR for this change.

ping @sauloperez @mkllnk @HugsDaniel
thoughts?

@sauloperez
Copy link
Contributor

I agree with your plan. I don't really understand what you mean by

With this I think we will be able to drop the order.shipping_method_id column (which is a non-sense column in the multi shipments world anyway, not totally nonsense in OFN2 because we force 1 order to have 1 shipment).

We'll be able to remove the column in OFN2 or not?

@luisramos0
Copy link
Contributor Author

luisramos0 commented Oct 1, 2018

yes, we will drop the column.

I was just re-iterating the difference between spree v2 model and ofn v2 model. the column is non-sense in spree 2 because 1 order - n shipments. BUT, although we WILL drop it, the column would still make sense in ofn v2 because in ofn v2 1 order - 1 shipment. Sorry, I could have omitted that, it just creates unnecessary complexity.

@luisramos0
Copy link
Contributor Author

I think this will take me a bit of time to get finished so I'll wait for @mkllnk feedback before I start the work to be sure we are all aligned.

@luisramos0
Copy link
Contributor Author

luisramos0 commented Dec 12, 2018

fixed in #2775

luisramos0 added a commit to luisramos0/openfoodnetwork that referenced this issue Dec 21, 2018
…is now enough as order.shipping_method_id is not longer used and will be dropped. See openfoodfoundation#2683 for more details
luisramos0 added a commit to luisramos0/openfoodnetwork that referenced this issue Dec 21, 2018
…is now enough as order.shipping_method_id is not longer used and will be dropped. See openfoodfoundation#2683 for more details
luisramos0 added a commit to luisramos0/openfoodnetwork that referenced this issue Dec 21, 2018
…is now enough as order.shipping_method_id is no longer used and will be dropped. See openfoodfoundation#2683 for more details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants