Openfoodnetwork: Error when deleting an item from order #step6

[enricostano]

This issue has been found by both @sstead and @sigmundpetersen while testing https://github.com/openfoodfoundation/openfoodnetwork/pull/1524, more info here.

In admin/orders/R245886616/edit you can delete specific line items by clicking on the trash bin icon located at the right end of each line and on Ok on the confirmation dialog.

As a result the line item has been deleted but since we get the HTML response from the controller for some weird reason it ends up rendered as a huge .error flash bar.

The issue has been introduced by https://github.com/openfoodfoundation/openfoodnetwork/commit/710c94b8f03313ee46a2ea1a19cc782e7cb39f62 and https://github.com/openfoodfoundation/openfoodnetwork/commit/a4bd8538f7161333280abfb27f66cee4496e4ea3

The questions here are:

• do we still need HTML responses for LineItemsController?
• is there any reason why we're not using the official API /api/orders/:order_id/line_items/:id for both PUT and DELETE?

As a context please look at a this discussion about the future of APIs in OFN.

cc/ @RohanM @oeoeaio @sauloperez

[enricostano]

I created a patch that solves this issue by always returning a JSON response... but we need to find an answer to the questions in the descritpion first.

[RohanM]

In response to 2 (why are we not using the official API), I believe the answer is that the Spree API is intended for external use, and so we're not using it on our own backend/frontend. I could be wrong though, it's a long time since I've worked on this code.

An alternative solution would be to modify the JS code to request .json endpoints, which would take us through the JS pathway already present in the controllers. I imagine that modifying the JS would be more difficult than the controllers, though, so up to you which solution to go with.

[kirstenalarsen]

just an outside question . . if we're making decisions about use of API etc, should we have a discussion about where we want to actually go with API (which will become more and more important) to shape how these changes are made? e.g. can we be doing it in a way that is improving API accessibility as we go? @RohanM @oeoeaio @mkllnk

[sauloperez]

I found the fix for this: https://github.com/Em-AK/openfoodnetwork/pull/10. As you will see in depth there, we should have added a JS response and not a JSON one.

I suggest we move forward with it while we figure out later on whether we need to override that controller in the first place.

[enricostano]

I merged https://github.com/Em-AK/openfoodnetwork/issues/10 since it seems to us the quickest way to fix this issue, but we'll need to come back to that "multi format AJAX" problem in the future. Open issue here https://github.com/openfoodfoundation/openfoodnetwork/issues/1691

Now we can test this again to see if it has been actually fixed. Maybe @sstead or @sigmundpetersen can help with that once we deploy step 6 somewhere. Will ping you ASAP, thanks!

[enricostano]

@sstead @sigmundpetersen could you please test this again? Thanks!

It is deployed at https://staging1.openfood.com.au

cc/ @OliverUK

[sstead]

I'm still experiencing problems deleting items from orders, but the sequence of events has changed...

[sauloperez]

The problem is due to the fact that Spree only hides the deleted row once it's been deleted in the backend.

The hidden input id field for that line item it's still in the DOM and thus sent to the backend (among the other line items) when udpating the fees. The backend then simply fails because it can't find that same line item that it just removed.

[sauloperez]

It gets fixed by https://github.com/coopdevs/spree/pull/1. Once that gets merged we can upgrade to the latest spree fork commit.

[enricostano]

@sstead @sigmundpetersen could you please test this again? We deployed a fix in staging1. Thanks!

[sstead]

@enricostano This issue seems to be fixed :) I can successfully delete line items.

[enricostano]

Thanks @sstead for the help 🙏