Skip to content

Fix attributes passed to form has_many not being set on new record form items #8550

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

Merged
merged 1 commit into from
Nov 21, 2024
Merged

Fix attributes passed to form has_many not being set on new record form items #8550

merged 1 commit into from
Nov 21, 2024

Conversation

Fs00
Copy link
Contributor

@Fs00 Fs00 commented Nov 19, 2024

This PR fixes a behavior inconsistency that occurs when passing custom attributes to the form builder has_many method, like in the following example:

f.has_many :posts, data: { controller: "custom" } do |p|
  ...
end

which builds the following HTML (I kept only the relevant parts):

<li class="has_many_container posts" data-sortable-start="0">
  <fieldset data-controller="custom" class="inputs has_many_fields">...</fieldset>
   ...
  <fieldset data-controller="custom" class="inputs has_many_fields">...</fieldset>
   ...
</li>

However, when clicking the "New Record" link to add a new item to the association, the <fieldset> generated for the new item didn't have the user-provided attributes on it (it was just <fieldset class="inputs has_many_fields">...</fieldset>).

I've discovered that the HTML used for new items is stored in the data-html attribute of the link and generated by a different method that was ignoring all custom attributes except for class.
So I've changed the method to make sure that custom attributes are not ignored and added tests for the relevant scenarios.

We'd need this fix on a 3.x patch, should I open the backport PR once this is merged or will you take care of it?

Copy link

codecov bot commented Nov 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.11%. Comparing base (ca6ddce) to head (6ba4741).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #8550   +/-   ##
=======================================
  Coverage   99.11%   99.11%           
=======================================
  Files         141      141           
  Lines        4076     4076           
=======================================
  Hits         4040     4040           
  Misses         36       36           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@javierjulio javierjulio changed the title Fix custom attributes passed to has_many not being set on new record form items Fix attributes passed to form has_many not being set on new record form items Nov 20, 2024
@javierjulio
Copy link
Member

@Fs00 this looks great, thank you. Yes, you should open a backport PR on the 3-0-stable branch please. My focus is on v4. Usually another maintainer will pick up doing a v3 release. We have at least another change considered to do on v3 regarding Sass deprecations. Either way I will approve and merge that PR if you create it. In the meantime, I'll merge this. Thank you.

@javierjulio javierjulio changed the title Fix attributes passed to form has_many not being set on new record form items Fix attributes passed to form has_many not being set on new record form items Nov 21, 2024
@javierjulio javierjulio self-assigned this Nov 21, 2024
@javierjulio javierjulio self-requested a review November 21, 2024 00:07
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!

@javierjulio javierjulio merged commit a19afe4 into activeadmin:master Nov 21, 2024
20 checks passed
@Fs00 Fs00 deleted the fix-has-many-custom-attrs branch November 21, 2024 15:56
@Fs00
Copy link
Contributor Author

Fs00 commented Nov 21, 2024

Thank you @javierjulio. I have opened #8551 to backport the change to AA 3.

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

Successfully merging this pull request may close these issues.

2 participants