Feature/70292 primerize custom field forms #21567

myabc · 2026-01-07T02:22:03Z

Ticket

https://community.openproject.org/wp/70292

What are you trying to accomplish?

Screenshots

What approach did you choose and why?

Merge checklist

Added/updated tests
Added/updated documentation in Lookbook (patterns, previews, etc)
Tested major browsers (Chrome, Firefox, Edge, ...)

frontend/src/stimulus/controllers/dynamic/admin/custom-fields.controller.ts


    if (cb.checked) {
-      const multi = this.multiSelectTargets[0] as HTMLInputElement|undefined;
+      const multi = undefined; // FIXME this.multiSelectTargets[0] as HTMLInputElement|undefined;


In general, unused variables should be removed when they are not read anywhere, especially if any logic depending on them is already commented out. If future logic is planned, that intent can be captured in comments or TODO/FIXME notes without introducing dead code.

Here, the multi variable on line 153 is never used. The logic that would have referenced it is commented out. To fix this without changing behavior, delete the const multi = ... line, but keep the FIXME comment so the future plan remains documented. No imports or additional methods are needed. Only the uncheckOtherDefaults method in frontend/src/stimulus/controllers/dynamic/admin/custom-fields.controller.ts needs editing, removing that single declaration line.

Copilot

Pull request overview

This PR refactors the custom field forms to use Primer Design System components (ViewComponent-based) instead of the legacy form helpers. The changes replace the old ERB-based forms with modular form components organized under CustomFields::Details::* and introduce new component-based rendering for custom options tables.

Changes:

Replaced legacy form helpers with Primer Forms components for custom field configuration
Refactored custom options table into reusable ViewComponent-based components
Removed CustomFieldFormatDependent#attributes method in favor of simpler visible? method
Updated Stimulus controller to use template-based row cloning instead of manual DOM manipulation

Reviewed changes

Copilot reviewed 37 out of 37 changed files in this pull request and generated 12 comments.

Show a summary per file

File	Description
`spec/lib/open_project/custom_field_format_dependent_spec.rb`	Removed obsolete spec testing the removed `attributes` and `stimulus_config` methods
`spec/components/custom_fields/custom_options_component_spec.rb`	Added component test for the new CustomOptionsComponent
`spec/components/custom_fields/custom_options/table_component_spec.rb`	Added component test for the custom options table
`spec/components/custom_fields/custom_options/row_component_spec.rb`	Added component test for custom option rows
`lib/open_project/custom_field_format_dependent.rb`	Simplified to remove format-dependent targets config and expose only `visible?` method
`frontend/src/stimulus/controllers/dynamic/admin/custom-fields.controller.ts`	Updated to use template-based row addition and removed format-dependent target management
`app/views/custom_fields/new.html.erb`	Converted from `labelled_tabular_form_for` to `settings_primer_form_with`
`app/views/custom_fields/edit.html.erb`	Converted from `labelled_tabular_form_for` to `settings_primer_form_with`
`app/views/custom_fields/_form.html.erb`	Replaced inline form fields with modular Primer Form components
`app/views/custom_fields/_custom_options.html.erb`	Removed legacy partial, replaced by component
`app/views/admin/settings/project_custom_fields/new.html.erb`	Converted to `settings_primer_form_with`
`app/views/admin/settings/project_custom_fields/edit.html.erb`	Converted to `settings_primer_form_with`
`app/forms/custom_fields/save.rb`	New form component for submit button
`app/forms/custom_fields/details/*.rb`	New modular form components for each custom field detail section
`app/components/table_component.rb`	Enhanced to accept `table_arguments` for passing attributes to table element
`app/components/table_component.html.erb`	Updated to use `Primer::BaseComponent` for table rendering
`app/components/custom_fields/custom_options_component.rb`	New component orchestrating custom options display
`app/components/custom_fields/custom_options_component.html.erb`	Template for custom options with table and hidden template row
`app/components/custom_fields/custom_options/table_component.rb`	Table component for custom options
`app/components/custom_fields/custom_options/row_component.rb`	Row component for individual custom option
`app/components/custom_fields/custom_options/base_row_component.rb`	Base class for custom option rows with common functionality

Copilot · 2026-01-16T03:31:42Z

app/forms/custom_fields/details/user_custom_field_form.rb

+        f.check_box(
+          name: :editable,
+          label: attribute_name(:editable),
+          caption: instructions_for(:admin_only)


The caption for the editable checkbox is incorrectly using :admin_only instead of :editable. This will display the wrong help text. Change to instructions_for(:editable).

Suggested change

caption: instructions_for(:admin_only)

caption: instructions_for(:editable)

Copilot · 2026-01-16T03:31:42Z

app/forms/custom_fields/details/searchable_form.rb

+        f.check_box(
+          name: :searchable,
+          label: attribute_name(:searchable),
+          caption: for_project ? instructions_for(:searchable_for_project) : instructions_for(:searchable)


The translation key :searchable_for_project does not exist in the translations. Based on the en.yml file, the correct keys are :searchable with nested keys all and project. Change to instructions_for(for_project ? :searchable_for_project : :searchable) and add the missing translation key, or use the existing structure like I18n.t(for_project ? 'custom_fields.instructions.searchable.project' : 'custom_fields.instructions.searchable.all').

Suggested change

caption: for_project ? instructions_for(:searchable_for_project) : instructions_for(:searchable)

caption: I18n.t(

for_project ? 'custom_fields.instructions.searchable.project' : 'custom_fields.instructions.searchable.all'

)

Copilot · 2026-01-16T03:31:43Z

app/forms/custom_fields/details/required_form.rb

+        f.check_box(
+          name: :is_required,
+          label: attribute_name(:is_required),
+          caption: for_project ? instructions_for(:is_required_for_project) : instructions_for(:is_required)


The translation key :is_required_for_project does not exist. Based on en.yml, the correct structure uses nested keys under is_required with all and project sub-keys. Change to use the existing translation structure like I18n.t(for_project ? 'custom_fields.instructions.is_required.project' : 'custom_fields.instructions.is_required.all').

Suggested change

caption: for_project ? instructions_for(:is_required_for_project) : instructions_for(:is_required)

caption: I18n.t(for_project ? 'custom_fields.instructions.is_required.project' : 'custom_fields.instructions.is_required.all')

Copilot · 2026-01-16T03:31:43Z

app/views/custom_fields/_form.html.erb

  <% when "ProjectCustomField" %>
-    <div class="form--field">
-      <%= f.check_box :is_required %>
-      <div class="form--field-instructions">
-        <p><%= t("custom_fields.instructions.is_required.project") %></p>
-      </div>
-    </div>
-    <div class="form--field">
-      <%= f.check_box :is_for_all %>
-      <div class="form--field-instructions">
-        <p><%= t("custom_fields.instructions.is_for_all.project") %></p>
-      </div>
-    </div>
-    <div class="form--field">
-      <%= f.check_box :admin_only %>
-      <div class="form--field-instructions">
-        <p><%= t("custom_fields.instructions.admin_only.project") %></p>
-      </div>
-    </div>
-    <div class="form--field" <%= format_dependent.attributes(:searchable) %>>
-      <%= f.check_box :searchable %>
-      <div class="form--field-instructions">
-        <p><%= t("custom_fields.instructions.searchable.project") %></p>
-      </div>
-    </div>
-  <% when "TimeEntryCustomField" %>
-    <div class="form--field">
-      <%= f.check_box :is_required %>
-      <div class="form--field-instructions">
-        <p><%= t("custom_fields.instructions.is_required.all") %></p>
-      </div>
-    </div>
+    <%=
+      render(
+        Primer::Forms::FormList.new(
+          CustomFields::Details::RequiredForm.new(f, for_project: true),
+          CustomFields::Details::AdminOnlyForm.new(f),
+          CustomFields::Details::SearchableForm.new(f, for_project: true)
+        )
+      )
+    %>


The is_for_all field is missing for ProjectCustomField. According to the removed old form code, ProjectCustomField should have the following fields: is_required, is_for_all, admin_only, and searchable. The is_for_all field needs to be added. Consider adding a CustomFields::Details::IsForAllForm.new(f) component between RequiredForm and AdminOnlyForm, or including it in the AdminOnlyForm or a ProjectCustomFieldForm component.

Copilot · 2026-01-16T03:31:43Z

frontend/src/stimulus/controllers/dynamic/admin/custom-fields.controller.ts

+      const multi = undefined; // FIXME this.multiSelectTargets[0] as HTMLInputElement|undefined;

-      if (multi?.checked === false) {
-        this.customOptionDefaultsTargets.forEach((el) => (el.checked = false));
-        cb.checked = true;
-      }
+      // if (multi?.checked === false) {
+      //   this.customOptionDefaultsTargets.forEach((el) => (el.checked = false));
+      //   cb.checked = true;
+      // }


The uncheckOtherDefaults functionality is commented out with a FIXME note. This breaks the behavior where checking a default value should uncheck others when multi-select is disabled. The multi-select target needs to be properly identified in the new component structure, or this logic needs to be reimplemented to work with the new form components.

Copilot · 2026-01-16T03:31:44Z

frontend/src/stimulus/controllers/dynamic/admin/custom-fields.controller.ts

-    if (idx < this.customOptionRowTargets.length - 1) {
-      this.customOptionRowTargets[idx + 1].after(row);
+    const idx = this.customOptionRows.indexOf(row);
+


Trailing whitespace on an otherwise empty line should be removed for code cleanliness.

Copilot · 2026-01-16T03:31:44Z

app/views/custom_fields/_form.html.erb

-<section
-  class="form--section"
-  id="custom_field_form">
++#%>


The ERB closing tag has unusual formatting with +++#%> instead of the standard ++#%>. While this might work, it's inconsistent with the opening tag format and could be confusing.

Suggested change

++#%>

%>

Copilot · 2026-01-16T03:31:44Z

app/components/custom_fields/custom_options_component.html.erb

+
+See COPYRIGHT and LICENSE files for more details.
+
++#%>


The ERB closing tag has unusual formatting with +++#%> instead of the standard ++#%>. Ensure consistency with the opening copyright comment format.

Suggested change

++#%>

--%>

Copilot · 2026-01-16T03:31:45Z

frontend/src/stimulus/controllers/dynamic/admin/custom-fields.controller.ts

+
+    event.preventDefault();
+    event.stopImmediatePropagation();
+


Trailing whitespace should be removed from this empty line.

Copilot · 2026-01-16T03:31:45Z

frontend/src/stimulus/controllers/dynamic/admin/custom-fields.controller.ts


    if (cb.checked) {
-      const multi = this.multiSelectTargets[0] as HTMLInputElement|undefined;
+      const multi = undefined; // FIXME this.multiSelectTargets[0] as HTMLInputElement|undefined;


Unused variable multi.

myabc added 3 commits January 6, 2026 23:19

[WIP] Primerize Custom Field forms

be57ff7

Teach TableComponent to handle :table_arguments

3a82639

Possible values table

e2d458c

myabc mentioned this pull request Jan 7, 2026

[#69923] Fix project attribute field caption wording #21430

Closed

3 tasks

github-advanced-security bot found potential problems Jan 7, 2026

View reviewed changes

myabc added DO NOT MERGE feature labels Jan 7, 2026

myabc requested a review from Copilot January 16, 2026 03:27

Copilot started reviewing on behalf of myabc January 16, 2026 03:27 View session

Copilot AI reviewed Jan 16, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/70292 primerize custom field forms #21567

Feature/70292 primerize custom field forms #21567

Uh oh!

myabc commented Jan 7, 2026 •

edited

Loading

Uh oh!

Check notice

Copilot Autofix

Copilot AI left a comment

Uh oh!

Copilot AI Jan 16, 2026

Uh oh!

Copilot AI Jan 16, 2026

Uh oh!

Copilot AI Jan 16, 2026

Uh oh!

Copilot AI Jan 16, 2026

Uh oh!

Copilot AI Jan 16, 2026

Uh oh!

Copilot AI Jan 16, 2026

Uh oh!

Copilot AI Jan 16, 2026

Uh oh!

Copilot AI Jan 16, 2026

Uh oh!

Copilot AI Jan 16, 2026

Uh oh!

Copilot AI Jan 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

	caption: instructions_for(:admin_only)
	caption: instructions_for(:editable)

-          caption: for_project ? instructions_for(:searchable_for_project) : instructions_for(:searchable)
+          caption: I18n.t(
+            for_project ? 'custom_fields.instructions.searchable.project' : 'custom_fields.instructions.searchable.all'
+          )

	caption: for_project ? instructions_for(:is_required_for_project) : instructions_for(:is_required)
	caption: I18n.t(for_project ? 'custom_fields.instructions.is_required.project' : 'custom_fields.instructions.is_required.all')

Feature/70292 primerize custom field forms #21567

Are you sure you want to change the base?

Feature/70292 primerize custom field forms #21567

Uh oh!

Conversation

myabc commented Jan 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ticket

What are you trying to accomplish?

Screenshots

What approach did you choose and why?

Merge checklist

Uh oh!

Check notice

Copilot Autofix

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

myabc commented Jan 7, 2026 •

edited

Loading