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

• I have written a thorough PR description.
• I have kept my commits small and atomic.
• I have used clear, explanatory commit messages.
• 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]

Suggested change

	@order.empty! unless @order.shipped?
	@order.empty! unless @order.completed?

[spaghetticode]

@kennyadsl I think we currently allow emptying a complete order as long as it's not been shipped. I'm thinking it may make sense in some instances, but they're probably just corner cases, so we may prefer to change behavior and privilege what makes generally more sense.

[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.

[fthobe]

@jarednorman merged in different PR and can be closed

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.28%. Comparing base (51f44e1) to head (4a4bf62).
Report is 1807 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (51f44e1) and HEAD (4a4bf62). Click for more details.

HEAD has 3 uploads less than BASE

Flag	BASE (51f44e1)	HEAD (4a4bf62)
	5	2

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #5418       +/-   ##
===========================================
- Coverage   88.84%   70.28%   -18.56%     
===========================================
  Files         608      561       -47     
  Lines       14750    14301      -449     
===========================================
- Hits        13105    10052     -3053     
- Misses       1645     4249     +2604     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.