Fix regression setting children record in parent before_save callback. #33673
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(@rails-bot has picked a reviewer for you, use r? to override) |
|
r? @kamipo |
rafaelfranca
requested changes
Aug 21, 2018
| valid = association_valid?(reflection, record, index) | ||
| saved = valid ? association.insert_record(record, false) : false | ||
| association_valid?(reflection, record, index) | ||
| record.errors.delete(reflection.foreign_key) |
There was a problem hiding this comment.
This seems wrong. Why would rails remove an error of a validation added by itself? If there is an error we should not save. If this is the behavior we want we should not add this error at the first place, not remove it after it added.
| association.insert_record(record, false) | ||
| else | ||
| false | ||
| end | ||
| else | ||
| association.insert_record(record) |
There was a problem hiding this comment.
@kamipo should not this set saved?
There was a problem hiding this comment.
At least I think should not set saved here for stable version.
Before #32796, autosaving associated collections always don't care about whether saving collections would be succeeded or not.
The else branch keeps the original behavior when the association validate: false is given explicitly.
tgxworld
force-pushed
the
regression_setting_children_record_in_parent_before_save
branch
2 times, most recently
from
aa87222
to
3b7d032
Compare
|
@rafaelfranca @kamipo Updated :) |
kamipo
reviewed
Aug 30, 2018
| else | ||
| association.insert_record(record) | ||
| if !association_saved | ||
| association_valid?(reflection, record, nil, false) |
There was a problem hiding this comment.
How about just using errors.add(reflection.name)?
The record in this context is, not destroyed, not marked_for_destruction?, not reflection.options[:autosave] given.
diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb
index a405f05e0b..ed71e68ad1 100644
--- a/activerecord/lib/active_record/autosave_association.rb
+++ b/activerecord/lib/active_record/autosave_association.rb
@@ -392,7 +392,7 @@ def save_collection_association(reflection)
records -= records_to_destroy
end
- records.each_with_index do |record, index|
+ records.each do |record|
next if record.destroyed?
saved = true
@@ -402,8 +402,8 @@ def save_collection_association(reflection)
saved = association.insert_record(record, false)
elsif !reflection.nested?
if reflection.validate?
- valid = association_valid?(reflection, record, index)
- saved = valid ? association.insert_record(record, false) : false
+ saved = association.insert_record(record)
+ errors.add(reflection.name) unless saved
else
association.insert_record(record)
end
tgxworld
force-pushed
the
regression_setting_children_record_in_parent_before_save
branch
from
3b7d032
to
bbcca44
Compare
tgxworld
force-pushed
the
regression_setting_children_record_in_parent_before_save
branch
from
bbcca44
to
90240f6
Compare
|
@kamipo Thank you for the review 👍 I've updated the code as per your comments. |
|
Thanks! |
kamipo
added a commit
that referenced
this pull request
Sep 3, 2018
…ecord_in_parent_before_save Fix regression setting children record in parent before_save callback.
tgxworld
deleted the
regression_setting_children_record_in_parent_before_save
branch
|
Thank you again @kamipo 👍 |
|
@kamipo By the way, do you want me to backport this to Rails 5.2 stable? |
|
I've already backported in 3a2e893. |
|
Awesome! |
tbrisker
added a commit
to tbrisker/foreman
that referenced
this pull request
Nov 11, 2019
Due to a change in Rails (rails/rails#33673), when factory bot creates a smart proxy with child hosts it now triggers a save on the hosts, which in this test fails because hosts validate that they have an environment set if they have a puppet proxy assigned.
mmoll
pushed a commit
to theforeman/foreman
that referenced
this pull request
Nov 11, 2019
Due to a change in Rails (rails/rails#33673), when factory bot creates a smart proxy with child hosts it now triggers a save on the hosts, which in this test fails because hosts validate that they have an environment set if they have a puppet proxy assigned.
eLvErDe
pushed a commit
to EarthLab-Luxembourg/foreman
that referenced
this pull request
Nov 20, 2019
Due to a change in Rails (rails/rails#33673), when factory bot creates a smart proxy with child hosts it now triggers a save on the hosts, which in this test fails because hosts validate that they have an environment set if they have a puppet proxy assigned.
eLvErDe
pushed a commit
to EarthLab-Luxembourg/foreman
that referenced
this pull request
Nov 20, 2019
Due to a change in Rails (rails/rails#33673), when factory bot creates a smart proxy with child hosts it now triggers a save on the hosts, which in this test fails because hosts validate that they have an environment set if they have a puppet proxy assigned.
ofedoren
pushed a commit
to ofedoren/foreman
that referenced
this pull request
Nov 20, 2019
Due to a change in Rails (rails/rails#33673), when factory bot creates a smart proxy with child hosts it now triggers a save on the hosts, which in this test fails because hosts validate that they have an environment set if they have a puppet proxy assigned.
Hamms
added a commit
to code-dot-org/code-dot-org
that referenced
this pull request
Nov 4, 2020
It's not 100% clear why this is necessary, but here's what we do know: - rails/rails#33673 changed something about the way child records are persisted when created as a part of their parents. - Regional Partner Mappings should be unique, and this Regional Parter was invalid as created, because a Regional Partner for Alabama already exists in our test database as a result of seeding. - On Rails 5.2, attempting to join sessions to workshops with `regional_partner.pd_workshops.joins(:sessions` fails when `regional_parter` is invalid. Like I said, it's not entirely clear why these things are connected but a `git bisect` indicates that they are. A simple - if uninformed - fix is to simply not use a regional partner mapping here when that's not what we are actually testing.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While upgrading Discourse to Rails 5.2.1, one of our model's spec started to fail and it turns out it was due to d7a3f33 which was merged in #32952.
cc @kamipo @mechanicles