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 for repair adjustments code #1474

Merged

Conversation

jordan-brough
Copy link
Contributor

Fix for Adjustment#repair_adjustments_associations_on_create

When the item gets deleted before after_commit runs.

I ran into this while working on some changes where some tax
adjustments would get created and destroyed in the same transaction.

Also add specs for this code.
See individual commits.

Is this worth adding to the 2.0 & 1.4 branches as well?

When the item gets deleted before after_commit runs.

I ran into this while working on some changes where some tax
adjustments would get created and destroyed in the same transaction.
Copy link
Contributor

@mamhoff mamhoff 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 to me, extra bonus points btw for an awesome spec.

@mamhoff
Copy link
Contributor

mamhoff commented Sep 29, 2016

Whether this should be on the 1.4 and 2.0 branches is a decision I'd like to leave up to @jhawthorn.

Copy link
Contributor

@cbrunsdon cbrunsdon left a comment

Choose a reason for hiding this comment

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

Looks good to me. I'd say yea should probably be put in 2.0/1.4 and cut a release as it doesn't change behaviour beyond fixing a clear bug.

@jordan-brough
Copy link
Contributor Author

jordan-brough commented Sep 29, 2016

Thanks @cbrunsdon & @mamhoff.

@jhawthorn mentioned offline that it might be nice to avoid the query using destroyed? so I've pushed a commit doing that.

@jhawthorn was also going to look at another way of attacking this problem that might allow using after_save instead of after_commit. @jhawthorn do you want to do that in this PR or separately?

@jordan-brough
Copy link
Contributor Author

Note about 1.4: My specs don't work for 1.4 unfortunately :( since after_commit doesn't work correctly in specs in Rails <= 4. See this readme.

@jordan-brough
Copy link
Contributor Author

@jhawthorn merging this for now so that I can rebase #1479 against master. I assume you'll open another PR if you get the other method working.

@jordan-brough jordan-brough merged commit 2c221b1 into solidusio:master Oct 4, 2016
@jordan-brough jordan-brough deleted the fix-for-repair-adjustments-code branch October 4, 2016 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants