solidus

solidus copied to clipboard

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 super when overwriting the update_shipped_shipments method.

I also use an update_all call, 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: https://github.com/solidusio/solidus/issues/2947

Checklist:

• [x] I have followed Pull Request guidelines
• [x] I have added a detailed description into each commit message
• [x] I have updated Guides and README accordingly to this change (if needed)
• [ ] I have added tests to cover this change (if needed)
• [x] I have attached screenshots to this PR for visual changes (if needed)

Hey @NewAlexandria, it looks like tests are failing. Would you like to keep working on it? :slightly_smiling_face:

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.