Call empty only on unshipped orders

[nirnaeth]

Summary

Fixes #4438.

The change triggers @order.empty! only when the shipping status is false. The 500 is not triggered from the FE, but only from the BE. Nevertheless I tested manually for both cases.

Notes:

1. I was not able to redirect to the Shipping action (admin/order/xxxxxx/edit), directions are welcome.
2. The test is failing on my local environment on interaction with the alert for confirming the emptying action. I am also not sure if that is the best location for the test itself. Please advise :)

Checklist

• [x] I have written a thorough PR description.
• [x] I have kept my commits small and atomic.
• [x] I have used clear, explanatory commit messages.
• [x] II have added tests

[kennyadsl]

Thanks a lot, @nirnaeth! ❤️

Maybe unrelated to this fix: is emptying a completed order (whether it's shipped or not) something that could make sense in some scenarios? I can't figure out why someone would want to do that, so maybe we should just avoid it if the order is completed, instead of checking if it's shipped only.

cc @spaghetticode

[nirnaeth]

by looking at the issue, I think this is more a case of an accident. You happen to have 2 tabs open and you complete the order in one. I don't think it's something really intentional.

happy to close the PR if you feel this is a non-problem or change the PR according to new specs :)

[kennyadsl]

@nirnaeth my comment was a bit twisted actually, this is what I meant: https://github.com/solidusio/solidus/pull/5418/files#r1348457754.

I don't think anyone wants to empty a completed order, as they don't want to empty a shipped order. Does it make sense?

[spaghetticode]

@nirnaeth thanks for working on this! ❤️

I've checked out the code, now the API call succeeds with a 200 and the order is not emptied, so no 500 error anymore 🎉. Unfortunately, there's no error message shown and cart items disappear from the page, just as if it was emptied for real, so this may be confusing for users. For this reason, I'm thinking a more clear approach may be to fail with a 422 HTTP error and show an error message to the user, what do you think?

[nirnaeth]

@nirnaeth my comment was a bit twisted actually, this is what I meant: https://github.com/solidusio/solidus/pull/5418/files#r1348457754.

I don't think anyone wants to empty a completed order, as they don't want to empty a shipped order. Does it make sense?

gotcha, let me see if the change you suggested fix both cases.

[nirnaeth]

@nirnaeth thanks for working on this! ❤️

I've checked out the code, now the API call succeeds with a 200 and the order is not emptied, so no 500 error anymore 🎉. Unfortunately, there's no error message shown and cart items disappear from the page, just as if it was emptied for real, so this may be confusing for users. For this reason, I'm thinking a more clear approach may be to fail with a 422 HTTP error and show an error message to the user, what do you think?

makes perfect sense! mine was the minimum to get to the goal of not having a 500, but I do agree it makes more sense for UX to have a proper message explaining what's going on. let me see what I can do :)

[nvandoorn]

Hey @nirnaeth thanks for the contribution. I pulled down your changes and made a few changes, and then pushed them back up here: https://github.com/nvandoorn/solidus/tree/empty_cart_error_with_shipped_order

Changes are as follows:

1. Call order.incomplete? instead of order.shipped? as suggested by @kennyadsl
2. Updated the new feature spec to make it pass.

You're welcome to pull my branch and push here, or I can open a new pull request if you would prefer.