-
Notifications
You must be signed in to change notification settings - Fork 3.1k
[#69436] Primerize Webhooks forms #21227
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
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR migrates the webhook management forms from legacy Rails form helpers to Primer-based ViewComponent forms, modernizing the UI with better accessibility and consistency with the rest of the application's design system.
Key Changes:
- Replaces legacy
labelled_tabular_form_forwithsettings_primer_form_withand Primer form components - Introduces a new
SelectPanelcomponent for project selection with remote data fetching - Adds a new Stimulus controller (
disable-when-value-selected) to handle dynamic form field disabling based on radio button selection
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/webhooks/app/views/webhooks/outgoing/admin/new.html.erb | Converted to use settings_primer_form_with with Primer form components instead of legacy partial |
| modules/webhooks/app/views/webhooks/outgoing/admin/edit.html.erb | Converted to use settings_primer_form_with with Primer form components instead of legacy partial |
| modules/webhooks/app/views/webhooks/outgoing/admin/_form.html.erb | Deleted legacy form partial (121 lines removed) |
| modules/webhooks/app/views/webhooks/outgoing/admin/enabled_projects_menu.html_fragment.erb | New template for rendering project selection menu items via AJAX |
| modules/webhooks/app/forms/webhooks/save_or_cancel.rb | New form component for submit/cancel buttons with customizable labels |
| modules/webhooks/app/forms/webhooks/outgoing/webhooks_form.rb | New form component for webhook basic fields (name, URL, description, secret) |
| modules/webhooks/app/forms/webhooks/outgoing/enabled_events_form.rb | New form component for selecting webhook events with checkboxes |
| modules/webhooks/app/forms/webhooks/outgoing/enabled_events_form/events_caption.html.erb | Caption template for events checkboxes with Check All/Uncheck All links |
| modules/webhooks/app/forms/webhooks/outgoing/enabled_projects_form.rb | New form component for project selection with radio buttons and nested SelectPanel |
| modules/webhooks/app/controllers/webhooks/outgoing/admin_controller.rb | Added enabled_projects_menu action to serve project list for SelectPanel |
| modules/webhooks/config/routes.rb | Added route for enabled_projects_menu endpoint |
| lib/primer/open_project/forms/select_panel.rb | New component wrapping Primer's Alpha::SelectPanel |
| lib/primer/open_project/forms/select_panel.html.erb | Template for SelectPanel component |
| lib/primer/open_project/forms/dsl/select_panel_input.rb | Input class for SelectPanel integration with Primer forms DSL |
| lib/primer/open_project/forms/dsl/input_methods.rb | Added select_panel method to form DSL |
| frontend/src/stimulus/controllers/disable-when-value-selected.controller.ts | New Stimulus controller for conditional field disabling based on radio button values |
| frontend/src/stimulus/setup.ts | Registered new disable-when-value-selected controller |
| frontend/src/app/shared/helpers/dom-helpers.ts | Added toggleEnabled, enableElement, and disableElement helper functions |
| config/initializers/mime_types.rb | Registered html_fragment MIME type for partial rendering |
| modules/webhooks/spec/features/manage_webhooks_spec.rb | Updated feature tests to use accessible selectors and new form structure |
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| # available_projects.each do |label, value| | ||
| # menu.with_item( | ||
| # label:, | ||
| # content_arguments: { data: { value: } }, | ||
| # active: !webhook.all_projects? && webhook.project_ids.include?(value) | ||
| # ) | ||
| # end | ||
| end | ||
|
|
||
| # Primer, unlike Rails' check_box helper, does not render this auxilary hidden field for us. | ||
| # f.hidden name: "webhook[selected_project_ids][]", value: "", scope_name_to_model: false | ||
| # | ||
| # f.check_box_group( | ||
| # name: :selected_project_ids, | ||
| # label: I18n.t(:"webhooks.outgoing.form.selected_project_ids.title"), | ||
| # visually_hide_label: true, | ||
| # disabled: webhook.all_projects?, | ||
| # data: { | ||
| # disable_when_value_selected_target: "effect", | ||
| # value: "selection" | ||
| # } | ||
| # ) do |group| | ||
| # available_projects.each do |label, value| | ||
| # group.check_box( | ||
| # label:, | ||
| # value:, | ||
| # checked: !webhook.all_projects? && webhook.project_ids.include?(value) | ||
| # ) | ||
| # end | ||
| end | ||
|
|
Copilot
AI
Nov 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This commented-out code should be removed. It appears to be leftover from development and clutters the codebase. If this is intended as documentation for an alternative implementation, it should be moved to a separate documentation file or removed entirely.
| # available_projects.each do |label, value| | |
| # menu.with_item( | |
| # label:, | |
| # content_arguments: { data: { value: } }, | |
| # active: !webhook.all_projects? && webhook.project_ids.include?(value) | |
| # ) | |
| # end | |
| end | |
| # Primer, unlike Rails' check_box helper, does not render this auxilary hidden field for us. | |
| # f.hidden name: "webhook[selected_project_ids][]", value: "", scope_name_to_model: false | |
| # | |
| # f.check_box_group( | |
| # name: :selected_project_ids, | |
| # label: I18n.t(:"webhooks.outgoing.form.selected_project_ids.title"), | |
| # visually_hide_label: true, | |
| # disabled: webhook.all_projects?, | |
| # data: { | |
| # disable_when_value_selected_target: "effect", | |
| # value: "selection" | |
| # } | |
| # ) do |group| | |
| # available_projects.each do |label, value| | |
| # group.check_box( | |
| # label:, | |
| # value:, | |
| # checked: !webhook.all_projects? && webhook.project_ids.include?(value) | |
| # ) | |
| # end | |
| end | |
| end |
| select_variant: :multiple, | ||
| selected_project_ids: webhook.all_projects? ? [] : webhook.project_ids | ||
| ), | ||
| fetch_strategy: :remote |
Copilot
AI
Nov 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The select_panel is missing data attributes needed for the disable-when-value-selected Stimulus controller to function correctly. Based on the controller implementation (lines 49-60 in disable-when-value-selected.controller.ts) and the commented-out code below (lines 71-74), the select_panel should include data attributes like:
data: {
disable_when_value_selected_target: "effect",
target_name: "webhook_project_ids",
value: "selection"
}These attributes are necessary for the panel to be disabled when "All projects" is selected.
| fetch_strategy: :remote | |
| fetch_strategy: :remote, | |
| data: { | |
| disable_when_value_selected_target: "effect", | |
| target_name: "webhook_project_ids", | |
| value: "selection" | |
| } |
| group.radio_button( | ||
| value: "all", | ||
| label: I18n.t(:"webhooks.outgoing.form.project_ids.all"), | ||
| checked: webhook.all_projects?, | ||
| data: { | ||
| disable_when_value_selected_target: "cause" | ||
| } | ||
| ) | ||
|
|
||
| group.radio_button( | ||
| value: "selection", | ||
| label: I18n.t(:"webhooks.outgoing.form.project_ids.selected"), | ||
| checked: !webhook.all_projects?, | ||
| data: { | ||
| disable_when_value_selected_target: "cause" | ||
| } |
Copilot
AI
Nov 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The radio buttons are missing the data-target-name attribute needed for the disable-when-value-selected Stimulus controller to function correctly. According to the controller implementation (line 51 in disable-when-value-selected.controller.ts), it reads input.dataset.targetName to match cause and effect targets. Both radio buttons should include:
data: {
disable_when_value_selected_target: "cause",
target_name: "webhook_project_ids"
}Without this attribute, the controller cannot properly disable/enable the nested select panel.
|
|
||
| module Webhooks | ||
| class SaveOrCancel < ApplicationForm | ||
| extend Dry::Initializer |
Copilot
AI
Nov 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The SaveOrCancel form uses Dry::Initializer without specifying the undefined: false option. Other forms in the codebase that use Dry::Initializer with optional parameters (e.g., app/forms/admin/settings/general_settings_form.rb:34, app/forms/projects/step_form.rb:33) specify Dry::Initializer[undefined: false] to handle nil values correctly. Consider using the same pattern here for consistency:
extend Dry::Initializer[undefined: false]| extend Dry::Initializer | |
| extend Dry::Initializer[undefined: false] |
| label: attribute_name(:secret), | ||
| caption: I18n.t("webhooks.outgoing.form.secret.description"), | ||
| input_width: :large | ||
| ) |
Copilot
AI
Nov 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The enabled checkbox field is missing from this form. The old form partial included a checkbox for the enabled attribute (line 28 in the deleted _form.html.erb), and the controller still permits the :enabled parameter (line 77 in admin_controller.rb). The webhook model also has an enabled attribute that defaults to true (line 27 in webhook.rb). This field should be added to allow users to enable/disable webhooks.
| ) | |
| ) | |
| f.checkbox( | |
| name: :enabled, | |
| label: attribute_name(:enabled) | |
| ) |
9d4a152 to
5bf8e3c
Compare
fdd9fad to
e197da3
Compare
5bf8e3c to
fb90096
Compare
fb90096 to
2654737
Compare
2654737 to
5fb57f7
Compare
This PR is based off #21134. Please review/merge that first.Ticket
https://community.openproject.org/wp/69436
What are you trying to accomplish?
Screenshots
What approach did you choose and why?
Merge checklist