fix: update_shipped_shipments now cancels properly #4446
Conversation
also refactor method parts for easier overwriting, in a way that also encourages the use of super for the main method
|
Hey @NewAlexandria, it looks like tests are failing. Would you like to keep working on it? 🙂 |
Sure, I'm glad to. As I mentioned in the PR body, I just wanted to make sure it seemed like a reasonable change that would be accepted given tests pass, etc. I'll be slow to respond because we're pushing through to a launch, but I can commit to finishing this PR's tests, so it's mergeable |
|
Hey @NewAlexandria! Thanks for your contribution. Before evaluating the solution proposed (which looks good at a first pass), can you please add a failing spec for this bug to be sure this regression will never come back in the future? Thanks in advance! |
|
I will get back to this PR. We're just in the middle of a release. Thanks. |
| end | ||
| end | ||
| def shipped_shippments_available_states | ||
| %w(shipped canceled) |
There was a problem hiding this comment.
sketch of thought on tests, pending what's there already:
- these states
- more valid states
- no states
- invalid state/s (the
sendfail. rescue it?) - idempotency? (maybe exclude those already in
shipped_shippments_available_states?)
also refactor method parts for easier overwriting,
in a way that also encourages the use of super for the main method
Description
We were having this same issue, and I found the related issue when searching.
This solution may a fait bit more ruby algo gymnastics, but if it's acceptable I think it could make it easier to overwrite this pattern, again while still trying to encourage people to call
superwhen overwriting theupdate_shipped_shipmentsmethod.I also use an
update_allcall, to avoid N+1 queries.I can include other comments, tests, etc, if this code looks like it'd be acceptable, and if anyone has suggestions. Until I confirmed receptiveness I didn't want to setup the test env. This PR is basically WIP, pending feedback.
Ref: #2947
Checklist: