Conversation
There was a problem hiding this comment.
Pull request overview
Converts the “Print labels” form on the species list downloads page from an ERB partial to a Phlex/Superform component, aligning it with the codebase’s ongoing ERB → Phlex form migration (issue #3871).
Changes:
- Replaces the ERB partial render with
Components::PrintLabelsFormin the species list downloads view. - Adds
Components::PrintLabelsForm(Phlex/Superform form component) and a minimalFormObject::PrintLabels. - Adds a component test covering the generated form structure/action and submit button.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/components/print_labels_form_test.rb | Adds structural assertions for the new PrintLabels form component output. |
| app/views/controllers/species_lists/downloads/new.html.erb | Switches from rendering an ERB partial to rendering the new Phlex component. |
| app/views/controllers/species_lists/downloads/_form_print_labels.html.erb | Removes the old ERB partial implementation of the print labels form. |
| app/components/print_labels_form.rb | Introduces the Phlex/Superform implementation of the print labels form. |
| app/classes/form_object/print_labels.rb | Adds a submit-only form object backing the new component. |
You can also share your feedback on Copilot code review. Take the survey.
|
@nimmolo Did I misunderstand the issue? I think your old Phlex status tracker listed it as high-priority. |
| id: "species_list_download_print_labels", **) | ||
| end | ||
|
|
||
| def view_template |
There was a problem hiding this comment.
I noticed that this is not calling super do per out standards. I used CC to determine if this is necessary in this case, and the short answer appears to be "no". I do note that in this case, super is never called, so I asked CC to figure out if that's significant. Here's what it came up with:
⏺ There it is. The key is on line 71-73 of superform/rails/form.rb:
def view_template(&block)
yield self if block_given?
end
And around_template (lines 59-65):
def around_template(&)
form_tag do
authenticity_token_field
_method_field
super
end
end
Here's how it works:
1. around_template always runs — it wraps everything in the <form> tag and renders the CSRF token and _method hidden field. This happens regardless of
whether you call super.
2. view_template(&block) in Superform::Rails::Form does yield self if block_given?. The &block comes from ERB usage — when you render a Superform inline
in ERB with <%= render(MyForm.new(model)) do |f| ... end %>, the block is passed here so the ERB can call f.Field(:name), etc.
3. When you define view_template in a subclass without calling super, you skip that yield self if block_given?. For a standalone form class (not rendered
inline from ERB with a block), there's no block to yield — so nothing is lost.
But there's a subtle case: if someone ever renders this form from ERB with a block:
<%= render(Components::PrintLabelsForm.new(query_param: q)) do |f|
f.some_extra_content
end %>
That block would be silently ignored because super is never called. Without super do...end, the form is closed to external content injection.
For this specific PR — a submit-only form with no fields — that's completely fine. Nobody will ever pass a block to it.
The real risk is with forms that have fields. Let me check if any of the forms that skip super are actually rendered with blocks.
⏺ Searched for 2 patterns (ctrl+o to expand)
⏺ No form component in the codebase is rendered with a block. So in practice, yield self if block_given? is dead code — nobody passes blocks to these forms.
Summary:
- around_template handles the <form> tag, CSRF token, and _method field — this runs regardless of super.
- Superform::Rails::Form#view_template only does yield self if block_given? — it enables ERB-inline usage where you pass a block to the form. No MO form
is used this way.
- So for MO's codebase, skipping super has no functional consequence. The 22 forms that do call super do...end are being defensive/conventional, but
nothing breaks without it.
For this PR specifically: a submit-only form with no fields and no possibility of block rendering — it's completely safe without super. The style guide
recommendation is worth keeping as a defensive convention (in case someone later tries to render a form with a block), but it's not a correctness issue
here.
I haven't experienced the actual consequences of not using super do so I'm not sure how accurate that is. Since there's no harm it, I would probably add the super do around the content of view_template until we can get @nimmolo to weigh in on the above analysis. If in fact super do is no longer serving any purpose, then we should get rid of it. If it is, we need to know when it really matters since it appears that currently only 22 out of 60 cases in the current code base use super do (counts from CC).
There was a problem hiding this comment.
@mo-nathan Great catch!
Seems we should add super everywhere (unless it noticeably degrades performance).
I'll follow your suggestion to add it here.
All seems like if we tell CC to do something, it should do it and not create exceptions on its own. At least it should flag exceptions and add a comment to the code.
There was a problem hiding this comment.
Yeah, this has been a persistent issue with CC.
Responds to [@mo-nathan's suggestion](#4030 (comment))
|
@nimmolo I'm planning to merge this Sat. a.m. unless you'd like more time to review it. |
Resolves #3871