-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Baseclass becomes! subclass #14971
Baseclass becomes! subclass #14971
Conversation
|
||
@klass.connection.update( | ||
um = base_class.unscoped.where(base_class.arel_table[base_class.primary_key].eq(id_was || id)).arel.compile_update(substitutes, base_class.primary_key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when we assign the expression inside where
to a variable? This line is very hard to read...
condition = base_class.arel_table[base_class.primary_key].eq(id_was || id)
This case prevents against regressions. The change was suggested in a recent PR but the all our tests passed.
Before this change, a record which changed its STI type, could not be found when updated. | ||
Setting update_record to the base class, ensures the record can be found. | ||
|
||
Fixes #14785 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick, missing dot at the end.
@senny Any idea on the status of this? Thanks! |
@VersionControl It's on my todo list. I will take a look in the upcoming days. |
Is there a milestone set for this? Will this be a part of 4.1.5? |
By what I could see it will be only part of Rails 4.2. |
Before this change, a record which changed its STI type, could not be found when updated.
Setting update_record to the base class, ensures the record can be found.
The idea was proposed by @matthewd in #14887
Closes #14785