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

Fix users removing last item of confirmed order on /cart page #6528

Conversation

mprieger
Copy link
Contributor

What? Why?

Closes #5546

This change prevents users from removing the last item of a confirmed order on the /cart page. This was allowed when shops had the option to edit confirmed orders for the same OC.

The functionality is similar to what happens when an order is edited from the /orders page. The main difference is that it displays the error without a page reload. The remove button is disabled when a user presses it for 10s (the time the flash error message is displayed for) to prevent multiple error messages appearing with each button press.

What should we test?

When a shop allows edits to confirmed orders, ensure that user cannot remove last item of a confirmed order in the /cart page.
Confirm that attempting to remove an item multiple times displays at most one error message.

Release notes

Customers can no longer remove the last item of a confirmed order on the /cart page.

Changelog Category: User facing changes

Dependencies

This fix is related to #5505 and should prevent one possible cause for this bug of getting a 500 error when viewing an order from which all items were removed.

@mprieger mprieger force-pushed the 5546-cart-page-last-item-deletion branch from 2b18b94 to 6c82bdb Compare December 14, 2020 18:29
Copy link
Contributor

@andrewpbrett andrewpbrett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good! I think it can go to testing after a second review.

One way to potentially go further: the error message suggests that the customer should cancel the order instead of removing the last line item. Is that easy for them to do? Could we make it easier?

@openfoodfoundation openfoodfoundation deleted a comment from hound bot Jan 8, 2021
@andrewpbrett
Copy link
Contributor

@mprieger Do you mind if I commit these suggestions to your branch?

@sauloperez
Copy link
Contributor

sauloperez commented Jan 11, 2021

I would go ahead @andrewpbrett . I hope @mprieger doesn't mind because it'd be cool to have this included in the next release.

@andrewpbrett
Copy link
Contributor

@sauloperez done! Needs a second review and then I think it can be moved to Test Ready.

@openfoodfoundation openfoodfoundation deleted a comment from hound bot Jan 11, 2021
@andrewpbrett andrewpbrett force-pushed the 5546-cart-page-last-item-deletion branch from aab004c to 6d6ddee Compare January 11, 2021 20:51
@sigmundpetersen
Copy link
Contributor

Build is failing

@andrewpbrett
Copy link
Contributor

Thanks @sigmundpetersen! Let's see if this fixes it.

@filipefurtad0 filipefurtad0 self-assigned this Jan 13, 2021
@filipefurtad0 filipefurtad0 added the pr-staged-uk staging.openfoodnetwork.org.uk label Jan 13, 2021
@filipefurtad0
Copy link
Contributor

Hey @mprieger ,

Thank you for this contribution!

I staged your PR, but somehow could not verify the underlying issue is fixed. Following the steps described on #5546, I was still able to remove the last item, while on the /cart page, thus breaking the order. See for example, this one:

image

Would you like to have a second look at it? I'm moving to In Dev, for now.

@filipefurtad0 filipefurtad0 removed the pr-staged-uk staging.openfoodnetwork.org.uk label Jan 13, 2021
@RachL
Copy link
Contributor

RachL commented Feb 1, 2021

hello @mprieger Are you still available to make the required changes? Thank you for your contribution :-)

@mprieger
Copy link
Contributor Author

mprieger commented Feb 4, 2021

Hello @RachL and @filipefurtad0

My apologies for the delayed response! Unfortunately, I will not be able to continue work on this at this time, but hopefully my changes (and the revisions from the reviewers) serve as a good starting point for anyone looking to pick this up.

@andrewpbrett andrewpbrett force-pushed the 5546-cart-page-last-item-deletion branch from 9e9c890 to ad8973d Compare March 2, 2021 05:01
@andrewpbrett andrewpbrett self-assigned this Mar 2, 2021
@andrewpbrett
Copy link
Contributor

Hi @filipefurtad0 - I rebased and made a small change and I'm now seeing the message show up on the cart page; I think this is ready for a re-test. One thing I did notice is that if there are multiple open orders, it still allows you to delete the last item from all the open orders except the last one. So this doesn't completely fix the problem, but I think it would be fairly uncommon for there to be multiple orders that someone is placing in an order cycle, so I think this would definitely be an improvement (if you do in fact verify it's working).

@filipefurtad0
Copy link
Contributor

Hey @mprieger,

Thanks for this contribution!

After staging your changes it's possible to verify @andrewpbrett findings - your notes really made testing this much easier, thanks Andy 👍

  • attempting to remove the last item from the cart, with one previous order only now displays a warning 🎉

Peek 2021-03-04 11-31

  • if more orders are placed, we can see below, that it is still possible to remove the last item (from the same order as above):

Peek 2021-03-04 11-33

So, agree this is an improvement, let's merge this 👍

Instead of closing, I'd propose to downgrade #5546. Surely this PR will make it easier to address the missing scenario.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants