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.

Sign up for GitHub

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

Jump to bottom

Autosave callbacks shouldn't be after_save #17232

Merged
merged 1 commit into from
Oct 13, 2014
Merged

Autosave callbacks shouldn't be after_save #17232

merged 1 commit into from
Oct 13, 2014

Conversation

agis
Copy link
Contributor

@agis agis commented Oct 10, 2014

Fixes #17209.

@agis agis changed the title Revert 068f092ced8483e557725542dd919ab7c516e567 [wip] Revert 068f092ced8483e557725542dd919ab7c516e567 Oct 10, 2014
@agis agis changed the title [wip] Revert 068f092ced8483e557725542dd919ab7c516e567 Revert 068f092ced8483e557725542dd919ab7c516e567 Oct 10, 2014
@agis agis changed the title Revert 068f092ced8483e557725542dd919ab7c516e567 Revert "Don't save through records twice" Oct 10, 2014
@agis
Copy link
Contributor Author

agis commented Oct 10, 2014

@rafaelfranca Not sure if this deserved add a CHANGELOG entry.

@egilburg
Copy link
Contributor

How come the test introduced in same commit of the code you reverted ( 068f092#diff-79fcb40bc6f590b0fb4ea54f891f3114R1155 ) doesn't start failing now? Did some other change fix it?

@agis
Copy link
Contributor Author

agis commented Oct 12, 2014

@egilburg The test is new and it's failing without the commit revert.

@egilburg
Copy link
Contributor

The test I referred to above isn't the one you added, but the one added with the commit you reverted (and you only reverted the code, not the test, so presumably that test should be failing now).

@rafaelfranca
Copy link
Member

I already reverted the test in a previous fix.
On Oct 11, 2014 11:02 PM, "Eugene Gilburg" notifications@github.com wrote:

The test I referred to above isn't the one you added, but the one added
with the commit you reverted (and you only reverted the code, not the test,
so presumably that test should be failing now).

—
Reply to this email directly or view it on GitHub
#17232 (comment).

@rafaelfranca
Copy link
Member

Could you squash the commits and change the commit message? It is not a plain revert because I already reverted part of that commit.

@agis
Autosave callbacks shouldn't be after_save
719d52d 
068f092 registered autosave callbacks
as `after_save` callbacks. This caused the regression described in #17209.

Autosave callbacks should be registered as `after_update` and
`after_create` callbacks, just like before.

This is a partial revert of 068f092.

Fixes #17209.
@agis agis changed the title Revert "Don't save through records twice" Autosave callbacks shouldn't be after_save callbacks Oct 13, 2014
@agis
Copy link
Contributor Author

agis commented Oct 13, 2014

@rafaelfranca ready!

@sgrif
Copy link
Contributor

sgrif commented Oct 13, 2014

LGTM

rafaelfranca added a commit that referenced this pull request Oct 13, 2014
@rafaelfranca
Merge pull request #17232 from Agis-/issue-17209
3c3a344 
Autosave callbacks shouldn't be `after_save` callbacks
@rafaelfranca rafaelfranca merged commit 3c3a344 into rails:master Oct 13, 2014
@agis agis deleted the issue-17209 branch October 13, 2014 18:15
@agis agis changed the title Autosave callbacks shouldn't be after_save callbacks Autosave callbacks shouldn't be after_save Aug 28, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

after_create on parent is called before associations are saved in rails 4.2.0.beta2
4 participants
@agis @egilburg @rafaelfranca @sgrif