[API for shopfronts] Refactor OFN shop list to use API for add line items to cart

[luisramos0]

Description

Spree 2 has changed order.add_variant and a few more things in Order and OrderPopulator. See the checkout customizations wiki page for context.

We propose to move away from extending Spree code order_decorator and order_populator_decorator and re-implement this logic in OFN.
So, there should be a new API endpoint for /cart that will take (to be confrmed): order_id, line_item_id and quantity. This will be implemented in Api::CartController. This controller should depend on new OFN service class CartService. This service will implement what's currently in order and order_populator and will depend, but not extend, Spree classes. Basically, what's in orderpopulator.populate and order.add_variant needs to be moved and adapted.

This change must be done on OFN master branch (so that we can validate and test easily) and then merged back to spree branch.

Additionally, and to support the upgrade to spree 2.1, we should move away from order.add_variant and use add_contents instead. This can be done later. If we make the code depend on order.add_variant we can then simply move from add_variant to add_contents.

Additionally, if not too difficult we would change the frontend to stop making bulk calls with groups of line items and make one call per line item. We can keep current API with bulk to make it easier for now.

Testing

It's a technical change, there should be no UX change in the add to cart process.

[mkllnk]

Excellent, @luisramos0, that's exactly what we discussed. Looking at it now, I'm just wondering if it's too much in one step. Maybe we can break it into smaller pull requests.

1. Create app/controllers/cart_controller.rb that is basically doing the same as the current code. The frontend code just needs to change the URL to post updates to. As at the moment, it would always modify the current cart (Spree's current order).
2. Create an API endpoint app/controllers/api/carts_controller.rb. Here we expect a cart id (order id) and modify one line item at a time. Actually, we can call it LineItemsController. Uh, in fact, we already have a LineItemsController in app/controllers. It's just for deleting from a finalised order. We can probably build on that.

What do you think?

[luisramos0]

Yes, agree, excellent.

Before I start changing things I am documenting my study of the code here:
https://github.com/openfoodfoundation/openfoodnetwork/wiki/Tech-Doc:-Customer-shopping-flow-(Orders,-Line-Items-and-Checkout)

The controllers structure is quite amusing!!!
For example, CartController.add_variant was written in 2013 :-D
I am sure we have some very healthy quantities of dead code here...

[luisramos0]

The PR with the first change is openfoodfoundation/openfoodnetwork#2542, it's ready for review.

[luisramos0]

Hey @mkllnk , after evaluating what to do next, these are my conclusions:

I understand that refactoring of the /cart/populate endpoint: Instead of having the UI sending the full list of variants in the cart, we would have the UI send individual variants to be added to the order using a new API endpoint. We could do this and it would be good.

There are other things we could do, for example, move the cart page (orders#edit and orders#update, both controllers and views) to become completely independent of spree code. It would not be too difficult to do and it would be a very good improvement.

But the main conclusion is: these improvements are not necessary for the spree upgrade.

For the spree upgrade, all we need is to review the order.add_variant method and start to use the new order.contents.add.
order.contents.add is just a refactored version of order.add_variant so it should be a straight forward change.

So, I'd create a new "Spree Upgrade" issue to adapt add_variant and I'd leave this issue open but in a different epic: [OFN API evolution]. What do you think?

[luisramos0]

Update:
After analysing add_variant and remove_variant in detail I realize there's nothing we need to do in terms of spree upgrade here.
order.contents.add is just the result of extracting add_variant and remove_variant to orderContents.

The only new detail here is related to shipments: in spree 2 , when removing line items from a completed/finalised order, the shipments/stock_locations are updated in the order.contents.remove_variant. In case a finalised line_item is deleted from the order, I am not sure how is the stock being updated right now in OFN.
Anyway, I don't think this deserves an issue in itself (at least now) as this is a detail in the larger scope of openfoodfoundation/openfoodnetwork#2009 . So, I'd not create a new issue, I'd just mention this point in the shipments investigation.

So, my suggestion is to:

• leave this issue open with new epic tag [OFN API evolution]
• reference this point above in [Spree 2 Upgrade] Replace Order#shipping_method with Order#shipments  openfoodnetwork#2009

[mkllnk]

Yes, from memory, we don't need to do anything else for the Spree upgrade. So I'm happy with your solution. Let's focus on the Spree upgrade.

[luisramos0]

ok, I have referenced this issue in openfoodfoundation/openfoodnetwork#2009. I'll leave this issue as it is.

Note: both existing PRs openfoodfoundation/openfoodnetwork#2539 and openfoodfoundation/openfoodnetwork#2542 are still valid and will go forward. The work described in this issue is just not complete yet.

I want to move this issue to Dev Ready but I cant decouple this issue state (Test Ready) from PR openfoodfoundation/openfoodnetwork#2542...

[daniellemoorhead]

I disconnected the PR @luisramos0 (you can do this via the PR, down the bottom near the comment field there is a connect/Disconnect zenhub section) and have moved it back to Dev Ready 😃

[luisramos0]

ah, now I see it. thanks!

[sigmundpetersen]

Should this be removed from Spree Upgrade epic @luisramos0 ? (Since you changed the title from [Spree Upgrade] to [OFN API Evolution])

[luisramos0]

Yes, done, thanks @sigmundpetersen

[mkllnk]

This issue was related to a Spree upgrade which is finished. The code changed a lot since and I think that this issue is obsolete. Closing.