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 the dirty tracking code's save hook overwriting missing attribute… #29018

Merged
merged 1 commit into from Jan 2, 2018

Conversation

Projects
None yet
7 participants
@willbryant
Contributor

willbryant commented May 9, 2017

Summary

Fixes issue #29017

This was causing subtle errors where an attribute appeared to be loaded on a record that was actually loaded without selecting those columns - but only after the record was saved again.

@rails-bot

This comment has been minimized.

Show comment
Hide comment
@rails-bot

rails-bot May 9, 2017

r? @kaspth

(@rails-bot has picked a reviewer for you, use r? to override)

rails-bot commented May 9, 2017

r? @kaspth

(@rails-bot has picked a reviewer for you, use r? to override)

@rails-bot

This comment has been minimized.

Show comment
Hide comment
@rails-bot

rails-bot May 9, 2017

warning Warning warning

  • Pull requests are usually filed against the master branch for this repo, but this one is against 5-1-stable. Please double check that you specified the right target!

rails-bot commented May 9, 2017

warning Warning warning

  • Pull requests are usually filed against the master branch for this repo, but this one is against 5-1-stable. Please double check that you specified the right target!
@maclover7

This comment has been minimized.

Show comment
Hide comment
@maclover7

maclover7 May 9, 2017

Member

@willbryant Thank you for the PR! Can you please open this up against master, and then we'll backport to 5-1-stable from there? Thanks!

Member

maclover7 commented May 9, 2017

@willbryant Thank you for the PR! Can you please open this up against master, and then we'll backport to 5-1-stable from there? Thanks!

@matthewd matthewd changed the base branch from 5-1-stable to master May 9, 2017

@matthewd matthewd changed the base branch from master to 5-1-stable May 9, 2017

@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd May 9, 2017

Member

Okay, just changing the target branch was less of a good idea than it seemed 😅

But you can rebase the commit onto master, force push the branch, and then change the PR target.

Member

matthewd commented May 9, 2017

Okay, just changing the target branch was less of a good idea than it seemed 😅

But you can rebase the commit onto master, force push the branch, and then change the PR target.

@willbryant willbryant changed the base branch from 5-1-stable to master May 9, 2017

@willbryant

This comment has been minimized.

Show comment
Hide comment
@willbryant

willbryant May 9, 2017

Contributor

Righto. I was putting this forward for 5-1-stable because this is a regression in 5.0 and 5.1 vs. 4.2.

To get it against master without a merge conflict I've cherry-picked it and force-pushed.

Contributor

willbryant commented May 9, 2017

Righto. I was putting this forward for 5-1-stable because this is a regression in 5.0 and 5.1 vs. 4.2.

To get it against master without a merge conflict I've cherry-picked it and force-pushed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment