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.

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

run update_resource inside a transaction to avoid autosaving relationships through assign_attributes when the record is invalid #7437

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

Conversation

deivid-rodriguez
Copy link
Member

A copy of #6202, to not let it be forgotten.

Fixes #5430

When submitting a form with invalid attributes, some associations are autosaved via assign_attributes before calling save_resource. As a consequence, when updating a resource, the user sees a validation error and is not aware that the changes to the associations got persisted.

This PR runs the code inside the update_resource method in an ActiveRecord::Transaction so that everything is rolled back when the model is not valid.

@javierjulio javierjulio force-pushed the 5430_fix_autosaving_relationships_on_invalid_resource branch from c4bc6c4 to 50a9275 Compare March 12, 2023 02:15
@javierjulio javierjulio self-requested a review March 12, 2023 02:42
Copy link
Member

@javierjulio javierjulio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks David! Not sure if you wanted to get this in as is or with tests.

…ships through assign_attributes when the record is invalid
@javierjulio javierjulio force-pushed the 5430_fix_autosaving_relationships_on_invalid_resource branch from 50a9275 to ab27f84 Compare March 12, 2023 23:46
@codecov
Copy link

codecov bot commented Mar 12, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (0a1521d) 99.47% compared to head (ab27f84) 99.47%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #7437   +/-   ##
=======================================
  Coverage   99.47%   99.47%           
=======================================
  Files         308      308           
  Lines       11306    11310    +4     
=======================================
+ Hits        11247    11251    +4     
  Misses         59       59           
Impacted Files Coverage Δ
...ib/active_admin/resource_controller/data_access.rb 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@matgaw
Copy link

matgaw commented Apr 10, 2023

@deivid-rodriguez @javierjulio is this planned to be merged at some point? it solves an important issue

@thibaudgg
Copy link
Contributor

I also just encountered that issue, this fix is important and should be merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Associations are persisted before validation of main record
4 participants