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

Propagate save(validate:) option for :has_many associations on au… #43525

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Propagate save(validate:) option for :has_many associations on au… #43525

wants to merge 1 commit into from
+55 −5

Conversation

intrip
Copy link
Contributor

@intrip intrip commented Oct 25, 2021 •
edited

Summary

save(validate:) option is propagated to associated records during autosave when autosave option is nil.
Affects only :has_many associations.

Fixes #43400

@intrip intrip force-pushed the fix-43400 branch 2 times, most recently from d9c6010 to 95407ef Compare October 25, 2021 13:38
@intrip intrip marked this pull request as ready for review October 25, 2021 13:40
@intrip intrip mentioned this pull request Oct 25, 2021
Open
@ghiculescu
Copy link
Member

Did you confirm this passes all the tests here ?

@intrip
Copy link
Contributor Author

intrip commented Oct 26, 2021 •
edited

Did you confirm this passes all the tests here ?

@ghiculescu Yes all tests in #43400 (comment) passes with this change.

In this PR I've only added a test for when autosave is nil but If you think we should add more tests I can port part of #43400 (comment) such that we test when autosave is true|nil with save(validate: false)|save.

@ghiculescu
Copy link
Member

In this PR I've only added a test for when autosave is nil

I think it's worth adding at least one more test, because autosave effectively has 3 possible values: nil, false, and true. https://github.com/rails/rails/blob/main/activerecord/lib/active_record/autosave_association.rb#L21..L23

@intrip intrip force-pushed the fix-43400 branch 2 times, most recently from 299d8ab to 9f8e276 Compare October 27, 2021 08:20
@intrip
Propagate save(validate:) option for :has_many associations on au…
4a31955 
…tosave.

`save(validate:)` option is propagated to associated records during `autosave` when `autosave`
option is `nil`. Affects only `:has_many` associations.

Fixes rails#43400
@intrip
Copy link
Contributor Author

intrip commented Oct 27, 2021

@ghiculescu I've added tests for all the 3 possible values of autosave.

Can you please do another check? Thanks!

@ghiculescu
Copy link
Member

Thanks. This looks great to me.

@intrip intrip mentioned this pull request Oct 28, 2021
Open
@rails-bot
Copy link

rails-bot bot commented Jan 25, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Jan 25, 2022
@intrip
Copy link
Contributor Author

intrip commented Jan 26, 2022

ping

@rails-bot rails-bot bot removed the stale label Jan 26, 2022
@rails-bot
Copy link

rails-bot bot commented Apr 26, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Apr 26, 2022
@intrip
Copy link
Contributor Author

intrip commented Apr 26, 2022

ping

@rails-bot rails-bot bot removed the stale label Apr 26, 2022
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
activerecord activesupport
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

save(validate: false) still runs before_validations on associated records
2 participants
@intrip @ghiculescu