-
Notifications
You must be signed in to change notification settings - Fork 21.8k
Allow subclasses to redefine autosave callbacks for associated records #33378
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
Allow subclasses to redefine autosave callbacks for associated records #33378
Conversation
Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @rafaelfranca (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review. Please see the contribution instructions for more information. |
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.
Thanks.
Could you please fix rubocop issues in https://codeclimate.com/github/rails/rails/pull/33378?
activerecord/CHANGELOG.md
Outdated
@@ -1,3 +1,9 @@ | |||
* Allow subclasses to redefine autosave callbacks for associated records. | |||
|
|||
Fixes #33305 |
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.
Missing dot after #33305
@@ -1774,3 +1775,29 @@ def test_after_save_callback_with_autosave | |||
assert_equal 1, comment.post_comments_count | |||
end | |||
end | |||
|
|||
|
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.
Extra empty line
@@ -1774,3 +1775,29 @@ def test_after_save_callback_with_autosave | |||
assert_equal 1, comment.post_comments_count | |||
end | |||
end | |||
|
|||
|
|||
class TestAutosaveAssociationOnAHasManyAssociationDefinedINSubclass < ActiveRecord::TestCase |
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.
IN
=> In
. I would also add suffix WithAcceptsNestedAttributes
.
|
||
valid_project.reload | ||
|
||
assert_equal valid_project.name, 'Updated' |
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.
Could you change order of arguments since assert_equal
receive expected value as first argument and actual values as the second.
It would improve error message:
Expected: "Initial"
Actual: "Updated"
|
||
valid_project.reload | ||
|
||
assert_equal valid_project.name, "Updated" |
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.
Could you change the order of arguments since assert_equal
receive an expected value as the first argument and an actual values as the second.
It would improve error message:
Expected: "Initial"
Actual: "Updated"
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.
@bogdanvlviv Sure. Can i squash commits into one after?
Could you squash commits into one? |
c27adf7
to
a6aabf4
Compare
has_many :projects, foreign_key: :firm_id | ||
|
||
accepts_nested_attributes_for :projects | ||
end |
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.
Not sure, but maybe it would be better to move this class definition to activerecord/test/models/company.rb
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.
Actually, I'm not sure too. This specific class used in this specific place only. It should be useless in other cases. Especially when activerecord/test/models/company.rb
full of other classes.
Also maybe i should move this spec to activerecord/test/cases/nested_attributes_test.rb
?
a6aabf4
to
3e271dc
Compare
3e271dc
to
35ee756
Compare
…lbacks Allow subclasses to redefine autosave callbacks for associated records
Allow subclasses to redefine autosave callbacks for associated records rails/rails#33378
Summary
This addresses this issue #33305
AutosaveAssociation
skip creation of callback method if it's already defined (even in parent class).