From df5eca254ef830ca5fae8ab909222a29eb38f930 Mon Sep 17 00:00:00 2001 From: Mir Bhatia Date: Thu, 23 Apr 2026 11:17:07 +0200 Subject: [PATCH 01/12] Replace ActionMenu with multi select SelectPanel for role selector --- .../workflows/blankslate_component.html.erb | 3 +- .../workflows/status_form_component.html.erb | 3 +- .../status_matrix_form_component.html.erb | 38 ++++++--- .../workflows/status_matrix_form_component.rb | 16 +++- ...s_removal_danger_dialog_component.html.erb | 3 +- app/controllers/workflows/tabs_controller.rb | 32 +++++--- app/controllers/workflows_controller.rb | 11 ++- app/helpers/workflow_helper.rb | 2 +- app/views/workflows/edit.html.erb | 4 +- app/views/workflows/summaries/show.html.erb | 2 +- app/views/workflows/tabs/edit.html.erb | 2 +- config/locales/en.yml | 4 + .../workflow-checkbox-state.controller.ts | 65 ++++++++++++---- .../admin/workflow-role-select.controller.ts | 78 +++++++++++++++++++ 14 files changed, 214 insertions(+), 49 deletions(-) create mode 100644 frontend/src/stimulus/controllers/dynamic/admin/workflow-role-select.controller.ts diff --git a/app/components/workflows/blankslate_component.html.erb b/app/components/workflows/blankslate_component.html.erb index da7a8b2ae1cb..6947ea1984f6 100644 --- a/app/components/workflows/blankslate_component.html.erb +++ b/app/components/workflows/blankslate_component.html.erb @@ -31,8 +31,9 @@ See COPYRIGHT and LICENSE files for more details. render(Primer::Beta::Blankslate.new(border: true)) do |blankslate| blankslate.with_heading(tag: :h2).with_content(t("admin.workflows.blankslate.title")) blankslate.with_description_content(t("admin.workflows.blankslate.description")) + # TODO: pass all roles once BlankslateComponent accepts roles: and StatusDialogComponent supports multi-role natively. blankslate.with_primary_action( - href: helpers.status_dialog_workflow_tab_path(@type, @tab, role_id: @role.id), + href: helpers.status_dialog_workflow_tab_path(@type, @tab, role_ids: [@role.id]), scheme: :secondary, data: { controller: "async-dialog" } ) do |button| diff --git a/app/components/workflows/status_form_component.html.erb b/app/components/workflows/status_form_component.html.erb index d9bf8ef27b7c..59394c0a330b 100644 --- a/app/components/workflows/status_form_component.html.erb +++ b/app/components/workflows/status_form_component.html.erb @@ -28,8 +28,9 @@ See COPYRIGHT and LICENSE files for more details. ++#%> <%= + # TODO: pass all roles once StatusFormComponent and confirm_statuses support multi-role natively. primer_form_with( - url: helpers.confirm_statuses_workflow_tab_path(@type, @tab, role_id: @role.id), + url: helpers.confirm_statuses_workflow_tab_path(@type, @tab, role_ids: [@role.id]), method: :post, id: FORM_ID, data: { turbo_frame: "workflow-table" } diff --git a/app/components/workflows/status_matrix_form_component.html.erb b/app/components/workflows/status_matrix_form_component.html.erb index 93522276a2fb..c6aa018b334a 100644 --- a/app/components/workflows/status_matrix_form_component.html.erb +++ b/app/components/workflows/status_matrix_form_component.html.erb @@ -32,19 +32,29 @@ See COPYRIGHT and LICENSE files for more details. render Primer::OpenProject::SubHeader.new do |subheader| if @type && @available_roles.any? subheader.with_filter_component do - render(Primer::Alpha::ActionMenu.new(select_variant: :single)) do |menu| - menu.with_show_button(scheme: :secondary) do |button| + render( + Primer::Alpha::SelectPanel.new( + select_variant: :multiple, + fetch_strategy: :local, + title: t("admin.workflows.role_selector.title"), + data: data_attributes + ) + ) do |panel| + panel.with_show_button(scheme: :secondary) do |button| button.with_trailing_visual_icon(icon: :"triangle-down") - @role ? t("admin.workflows.role_selector.label", role: @role.name) : t("admin.workflows.role_selector.no_role") + if @roles.many? + t("admin.workflows.role_selector.roles", count: @roles.size) + elsif @role + t("admin.workflows.role_selector.label", role: @role.name) + else + t("admin.workflows.role_selector.no_role") + end end @available_roles.each do |available_role| - menu.with_item( + panel.with_item( label: available_role.name, - active: available_role == @role, - tag: :a, - href: helpers.edit_workflow_tab_path(@type, @tab, role_id: available_role.id), - content_arguments: { data: { "admin--workflow-checkbox-state-confirmation-trigger": "click", - turbo_action: "advance" } } + active: @roles.include?(available_role), + item_id: available_role.id ) end end @@ -56,7 +66,9 @@ See COPYRIGHT and LICENSE files for more details. scheme: :secondary, leading_icon: :plus, label: t("admin.workflows.status_button"), - href: helpers.status_dialog_workflow_tab_path(@type, @tab, role_id: @role&.id, status_ids: @statuses.pluck(:id).presence), + # TODO: status_dialog and StatusDialogComponent currently work with a single role (@role = @roles.first); + # update when they support multi-role natively. + href: helpers.status_dialog_workflow_tab_path(@type, @tab, role_ids: @roles.map(&:id), status_ids: @statuses.pluck(:id).presence), data: { controller: "async-dialog" } ) do t("admin.workflows.status_button") @@ -76,7 +88,9 @@ See COPYRIGHT and LICENSE files for more details. } ) do %> <%= hidden_field_tag "type_id", @type.id %> - <%= hidden_field_tag "role_id", @role.id %> + <% @roles.each do |role| %> + <%= hidden_field_tag "role_ids[]", role.id %> + <% end %> <%= hidden_field_tag "tab", @tab %> <%= helpers.render_tabs helpers.workflow_tabs(@type) %> @@ -94,7 +108,7 @@ See COPYRIGHT and LICENSE files for more details. Primer::OpenProject::FeedbackDialog.new( title: t("admin.workflows.leave_confirmation.title"), data: { - "admin--workflow-checkbox-state-target": "confirmationDialog", + "admin--workflow-checkbox-state-target": "confirmationDialog" } ) ) do |dialog| diff --git a/app/components/workflows/status_matrix_form_component.rb b/app/components/workflows/status_matrix_form_component.rb index 14c3ade5fdbe..bc3365aaff19 100644 --- a/app/components/workflows/status_matrix_form_component.rb +++ b/app/components/workflows/status_matrix_form_component.rb @@ -33,14 +33,26 @@ class StatusMatrixFormComponent < ApplicationComponent include OpTurbo::Streamable include OpPrimer::ComponentHelpers - def initialize(tab:, role:, type:, available_roles:, statuses:, has_status_changes:) + def initialize(tab:, roles:, type:, available_roles:, statuses:, has_status_changes:) super @tab = tab - @role = role + @roles = roles + @role = @roles.first @type = type @available_roles = available_roles @statuses = statuses @has_status_changes = has_status_changes end + + private + + def data_attributes + { + controller: "admin--workflow-role-select", + "admin--workflow-role-select-base-url-value": helpers.edit_workflow_path(@type), + "admin--workflow-role-select-current-role-ids-value": @roles.map(&:id).join(","), + "admin--workflow-role-select-admin--workflow-checkbox-state-outlet": "#workflow_form" + } + end end end diff --git a/app/components/workflows/status_removal_danger_dialog_component.html.erb b/app/components/workflows/status_removal_danger_dialog_component.html.erb index c2b020ff5b72..8a8ccd8e5783 100644 --- a/app/components/workflows/status_removal_danger_dialog_component.html.erb +++ b/app/components/workflows/status_removal_danger_dialog_component.html.erb @@ -46,7 +46,8 @@ See COPYRIGHT and LICENSE files for more details. # The reason this is done here is because the submit is not a DELETE, and GET form submissions # strip url params dialog.with_additional_details do - concat(hidden_field_tag(:role_id, @role.id)) + # TODO: pass all roles once StatusRemovalDangerDialogComponent supports multi-role natively. + concat(hidden_field_tag("role_ids[]", @role.id)) @status_ids.each { |id| concat(hidden_field_tag("status_ids[]", id)) } end end diff --git a/app/controllers/workflows/tabs_controller.rb b/app/controllers/workflows/tabs_controller.rb index 94c9805c22a8..6c476c890b51 100644 --- a/app/controllers/workflows/tabs_controller.rb +++ b/app/controllers/workflows/tabs_controller.rb @@ -38,11 +38,11 @@ class Workflows::TabsController < ApplicationController before_action :set_type before_action :set_tab before_action :set_eligible_roles - before_action :set_role + before_action :set_roles def edit unless turbo_frame_request? - redirect_to edit_workflow_path(@type, role_id: params[:role_id], tab: @tab) + redirect_to edit_workflow_path(@type, role_ids: params[:role_ids], tab: @tab) return end @@ -53,12 +53,19 @@ def edit end end - def update - call = Workflows::BulkUpdateService - .new(role: @role, type: @type, tab: @tab) - .call(permitted_status_params) + def update # rubocop:disable Metrics/AbcSize + success = false + Workflow.transaction do + success = true + @roles.each do |role| + result = Workflows::BulkUpdateService.new(role:, type: @type, tab: @tab) + .call(permitted_status_params) + success = false unless result.success? + end + raise ActiveRecord::Rollback unless success + end - if call.success? + if success render_flash_message_via_turbo_stream( message: I18n.t(:notice_successful_update), scheme: :success @@ -69,7 +76,7 @@ def update update_via_turbo_stream( component: Workflows::StatusMatrixFormComponent.new( tab: @tab, - role: @role, + roles: @roles, type: @type, available_roles: @eligible_roles, statuses:, @@ -125,7 +132,7 @@ def confirm_statuses # rubocop:disable Metrics/AbcSize update_via_turbo_stream( component: Workflows::StatusMatrixFormComponent.new( tab: @tab, - role: @role, + roles: @roles, type: @type, available_roles: @eligible_roles, statuses:, @@ -150,8 +157,11 @@ def set_eligible_roles @eligible_roles = Workflow.eligible_roles.order(:builtin, :position) end - def set_role - @role = @eligible_roles.find(params[:role_id]) + def set_roles + @roles = @eligible_roles.where(id: params[:role_ids]) + # TODO: remove @role once the matrix form and all dependent components + # (dialogs, status selectors, page headers) work natively with @roles (multi-role). + @role = @roles.first end def statuses_for_form diff --git a/app/controllers/workflows_controller.rb b/app/controllers/workflows_controller.rb index 82b03b151186..fb40fead1934 100644 --- a/app/controllers/workflows_controller.rb +++ b/app/controllers/workflows_controller.rb @@ -38,7 +38,7 @@ class WorkflowsController < ApplicationController before_action :find_types, only: %i[index] before_action :find_type, only: %i[edit] - before_action :find_optional_role, only: %i[edit] + before_action :find_optional_roles, only: %i[edit] def index; end @@ -64,8 +64,13 @@ def find_type @type = ::Type.find(params[:type_id]) end - def find_optional_role - @role = eligible_roles.find_by(id: params[:role_id]) || eligible_roles.order(:builtin, :position).first + def find_optional_roles + ordered = eligible_roles.order(:builtin, :position) + @roles = ordered.where(id: params[:role_ids]) + @roles = [ordered.first] if @roles.empty? + # TODO: remove @role once the matrix form and all dependent components + # (dialogs, status selectors, page headers) work natively with @roles (multi-role). + @role = @roles.first end def eligible_roles diff --git a/app/helpers/workflow_helper.rb b/app/helpers/workflow_helper.rb index a56b291fce3d..61c7007adb09 100644 --- a/app/helpers/workflow_helper.rb +++ b/app/helpers/workflow_helper.rb @@ -37,7 +37,7 @@ def workflow_tabs(type) ].map do |tab| tab.merge( partial: "workflows/form", - path: edit_workflow_tab_path(type, tab[:name], params.permit(:role_id)), + path: edit_workflow_tab_path(type, tab[:name], params.permit(role_ids: [])), data: { "admin--workflow-checkbox-state-confirmation-trigger": "click", turbo_frame: "workflow-table", turbo_action: "advance" } diff --git a/app/views/workflows/edit.html.erb b/app/views/workflows/edit.html.erb index 49db69fe7cce..26a2de62642c 100644 --- a/app/views/workflows/edit.html.erb +++ b/app/views/workflows/edit.html.erb @@ -31,6 +31,6 @@ See COPYRIGHT and LICENSE files for more details. <%= render Workflows::PageHeaders::EditComponent.new(@type, role: @role, tabs: workflow_tabs(@type)) %> <% end %> -<% if @type && @role %> - <%= turbo_frame_tag "workflow-table", src: edit_workflow_tab_path(@type, @current_tab, role_id: @role.id, status_ids: params[:status_ids]) %> +<% if @type && @roles.any? %> + <%= turbo_frame_tag "workflow-table", src: edit_workflow_tab_path(@type, @current_tab, role_ids: @roles.map(&:id), status_ids: params[:status_ids]) %> <% end %> diff --git a/app/views/workflows/summaries/show.html.erb b/app/views/workflows/summaries/show.html.erb index 034123f66d9f..f5ed8bcff9e4 100644 --- a/app/views/workflows/summaries/show.html.erb +++ b/app/views/workflows/summaries/show.html.erb @@ -61,7 +61,7 @@ See COPYRIGHT and LICENSE files for more details. <%= h type %> <% roles.each do |role, count| -%> - <%= link_to((count > 0 ? count : content_tag(:span, "", class: "icon-close icon-context icon-button")), edit_workflow_path(type, role_id: role), title: t(:button_edit)) %> + <%= link_to((count > 0 ? count : content_tag(:span, "", class: "icon-close icon-context icon-button")), edit_workflow_path(type, role_ids: [role]), title: t(:button_edit)) %> <% end -%> diff --git a/app/views/workflows/tabs/edit.html.erb b/app/views/workflows/tabs/edit.html.erb index f665ac5cd3ca..61af4af9c6f4 100644 --- a/app/views/workflows/tabs/edit.html.erb +++ b/app/views/workflows/tabs/edit.html.erb @@ -28,7 +28,7 @@ See COPYRIGHT and LICENSE files for more details. ++#%> <%= turbo_frame_tag "workflow-table", data: { turbo_cache: false } do %> - <%= render Workflows::StatusMatrixFormComponent.new(tab: @tab, role: @role, type: @type, available_roles: @eligible_roles, statuses: @statuses, has_status_changes: @has_status_changes) %> + <%= render Workflows::StatusMatrixFormComponent.new(tab: @tab, roles: @roles, type: @type, available_roles: @eligible_roles, statuses: @statuses, has_status_changes: @has_status_changes) %> <%= turbo_stream.replace(Workflows::PageHeaders::EditComponent.wrapper_key) do %> <%= render Workflows::PageHeaders::EditComponent.new(@type, role: @role, tabs: workflow_tabs(@type)) %> <% end %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 797f428cbfee..dd1b14da815a 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -461,8 +461,12 @@ en: ignore: "Ignore changes" save: "Save changes and continue" role_selector: + title: "Select roles" label: "Role: %{role}" no_role: "Select role" + roles: + one: "%{count} role selected" + other: "%{count} roles selected" blankslate: title: "No status transitions configured" description: "Add statuses to start configuring workflows for this role" diff --git a/frontend/src/stimulus/controllers/dynamic/admin/workflow-checkbox-state.controller.ts b/frontend/src/stimulus/controllers/dynamic/admin/workflow-checkbox-state.controller.ts index 3dd327642bf2..a88031e47c58 100644 --- a/frontend/src/stimulus/controllers/dynamic/admin/workflow-checkbox-state.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/admin/workflow-checkbox-state.controller.ts @@ -113,13 +113,21 @@ export default class WorkflowCheckboxStateController extends Controller(`input[name="${name}"]`)!.value; } + private formValues(name:string):string[] { + return Array.from( + this.element.querySelectorAll(`input[name="${name}"]`), + ).map((el) => el.value); + } + private pushState(key:string, state:CheckboxesState) { const savedState:SavedState = { formKey: this.formKey, checkboxes: state }; sessionStorage.setItem(key, JSON.stringify(savedState)); @@ -155,7 +163,7 @@ export default class WorkflowCheckboxStateController extends Controller { - const onIgnoreCallback = this.onIgnoreChanges(target, event); - this.ignoreButtonTarget.addEventListener('click', onIgnoreCallback); - - const onSaveCallback = this.onSaveChanges(target, event); - this.saveButtonTarget.addEventListener('click', onSaveCallback); - + private openConfirmationDialog(onIgnore:() => void, onSave:() => void) { + this.ignoreButtonTarget.addEventListener('click', onIgnore); + this.saveButtonTarget.addEventListener('click', onSave); this.confirmationDialogTarget.addEventListener('close', () => { - this.ignoreButtonTarget.removeEventListener('click', onIgnoreCallback); - this.saveButtonTarget.removeEventListener('click', onSaveCallback); + this.ignoreButtonTarget.removeEventListener('click', onIgnore); + this.saveButtonTarget.removeEventListener('click', onSave); }); - this.confirmationDialogTarget.showModal(); + } + + private confirmThenResubmit = (target:HTMLElement, event:Event) => { + this.openConfirmationDialog( + this.onIgnoreChanges(target, event), + this.onSaveChanges(target, event), + ); }; private onIgnoreChanges = (originalTarget:HTMLElement, originalEvent:Event) => { @@ -193,7 +203,7 @@ export default class WorkflowCheckboxStateController extends Controller params.append('role_ids[]', id)); url.search = params.toString(); turboFrame.setAttribute('src', url.toString()); @@ -273,4 +283,33 @@ export default class WorkflowCheckboxStateController extends Controller { + this.hasCheckboxChangesValue = false; + this.hasStatusChangesValue = false; + this.confirmationDialogTarget.close(); + setTimeout(() => { Turbo.visit(url); }, 0); + }, + () => { + this.element.requestSubmit(); + this.confirmationDialogTarget.close(); + // Delay to allow the flash message from the form submission to appear. + setTimeout(() => { Turbo.visit(url); }, 1000); + }, + ); + } } diff --git a/frontend/src/stimulus/controllers/dynamic/admin/workflow-role-select.controller.ts b/frontend/src/stimulus/controllers/dynamic/admin/workflow-role-select.controller.ts new file mode 100644 index 000000000000..11c66b3d3832 --- /dev/null +++ b/frontend/src/stimulus/controllers/dynamic/admin/workflow-role-select.controller.ts @@ -0,0 +1,78 @@ +/* + * -- copyright + * OpenProject is an open source project management software. + * Copyright (C) the OpenProject GmbH + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License version 3. + * + * OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: + * Copyright (C) 2006-2013 Jean-Philippe Lang + * Copyright (C) 2010-2013 the ChiliProject Team + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version 2 + * of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + * + * See COPYRIGHT and LICENSE files for more details. + * ++ + */ + +import { Controller } from '@hotwired/stimulus'; +import type { SelectPanelElement } from '@primer/view-components/app/components/primer/alpha/select_panel_element'; +import WorkflowCheckboxStateController from './workflow-checkbox-state.controller'; + +/** + * Mounted on the element for role selection in the workflow quick filters. + * When the panel closes, it navigates to the workflow edit page with the selected role IDs. + * Delegates dirty-state confirmation to the workflow-checkbox-state controller via an outlet. + */ +export default class WorkflowRoleSelectController extends Controller { + static outlets = ['admin--workflow-checkbox-state']; + static values = { baseUrl: String, currentRoleIds: String }; + + declare readonly adminWorkflowCheckboxStateOutlet:WorkflowCheckboxStateController; + declare readonly hasAdminWorkflowCheckboxStateOutlet:boolean; + declare baseUrlValue:string; + declare currentRoleIdsValue:string; + + connect() { + this.element.addEventListener('panelClosed', this.handlePanelClosed); + } + + disconnect() { + this.element.removeEventListener('panelClosed', this.handlePanelClosed); + } + + private handlePanelClosed = () => { + const panel = this.element as HTMLElement as SelectPanelElement; + const selectedIds = panel.items + .filter((item) => panel.isItemChecked(item)) + .map((item) => item.getAttribute('data-item-id')) + .filter(Boolean); + + if (!selectedIds.length) return; + + const currentIds = this.currentRoleIdsValue.split(',').filter(Boolean); + if (selectedIds.slice().sort().join(',') === currentIds.slice().sort().join(',')) return; + + const url = new URL(this.baseUrlValue, window.location.origin); + selectedIds.forEach((id) => url.searchParams.append('role_ids[]', id!)); + + if (this.hasAdminWorkflowCheckboxStateOutlet) { + this.adminWorkflowCheckboxStateOutlet.navigateTo(url.toString()); + } else { + Turbo.visit(url.toString()); + } + }; +} From b8b5574df04d3648b4a2957380e33e36aeaa079b Mon Sep 17 00:00:00 2001 From: Mir Bhatia Date: Thu, 23 Apr 2026 12:09:17 +0200 Subject: [PATCH 02/12] Update specs to work with multiple roles --- .../workflows/tabs_controller_spec.rb | 170 +++++++++++++----- spec/controllers/workflows_controller_spec.rb | 85 ++++++--- spec/features/roles/create_spec.rb | 5 +- spec/features/workflows/edit_spec.rb | 34 ++-- 4 files changed, 198 insertions(+), 96 deletions(-) diff --git a/spec/controllers/workflows/tabs_controller_spec.rb b/spec/controllers/workflows/tabs_controller_spec.rb index c870c3bd936d..483c1a44baed 100644 --- a/spec/controllers/workflows/tabs_controller_spec.rb +++ b/spec/controllers/workflows/tabs_controller_spec.rb @@ -48,6 +48,11 @@ .with(role.id.to_s) .and_return(role) + allow(role_scope) + .to receive(:where) + .with(id: [role.id.to_s]) + .and_return([role]) + role_scope end @@ -68,32 +73,58 @@ describe "#edit" do context "when not a turbo frame request" do - it "redirects to the parent workflow edit path" do - get :edit, - params: { - role_id: role.id.to_s, - workflow_type_id: type.id.to_s, - tab: "always" - } - - expect(response).to redirect_to( - edit_workflow_path(type, role_id: role.id.to_s, tab: "always") - ) + context "with a single role" do + it "redirects to the parent workflow edit path" do + get :edit, + params: { + role_ids: [role.id.to_s], + workflow_type_id: type.id.to_s, + tab: "always" + } + + expect(response).to redirect_to( + edit_workflow_path(type, role_ids: [role.id.to_s], tab: "always") + ) + end + + it "does not forward status_ids to the redirect" do + get :edit, + params: { + role_ids: [role.id.to_s], + workflow_type_id: type.id.to_s, + tab: "always", + status_ids: ["1", "2"] + } + + expect(response).to redirect_to( + edit_workflow_path(type, role_ids: [role.id.to_s], tab: "always") + ) + expect(response.location).not_to include("status_ids") + end end - it "does not forward status_ids to the redirect" do - get :edit, - params: { - role_id: role.id.to_s, - workflow_type_id: type.id.to_s, - tab: "always", - status_ids: ["1", "2"] - } - - expect(response).to redirect_to( - edit_workflow_path(type, role_id: role.id.to_s, tab: "always") - ) - expect(response.location).not_to include("status_ids") + context "with multiple roles" do + let(:role2) { build_stubbed(:project_role) } + + before do + allow(role_scope) + .to receive(:where) + .with(id: [role.id.to_s, role2.id.to_s]) + .and_return([role, role2]) + end + + it "redirects preserving all role ids" do + get :edit, + params: { + role_ids: [role.id.to_s, role2.id.to_s], + workflow_type_id: type.id.to_s, + tab: "always" + } + + expect(response).to redirect_to( + edit_workflow_path(type, role_ids: [role.id.to_s, role2.id.to_s], tab: "always") + ) + end end end end @@ -114,7 +145,7 @@ post :confirm_statuses, params: { - role_id: role.id.to_s, + role_ids: [role.id.to_s], workflow_type_id: type.id.to_s, status_ids: ["1", "2"], original_status_ids: ["1", "2"], @@ -133,7 +164,7 @@ before do post :confirm_statuses, params: { - role_id: role.id.to_s, + role_ids: [role.id.to_s], workflow_type_id: type.id.to_s, status_ids: ["1"], original_status_ids: ["1", "2"], @@ -152,33 +183,74 @@ describe "#update" do let(:status_params) { { "1" => { "2" => ["always"] } } } - let(:service) do - instance_double(Workflows::BulkUpdateService).tap do |dbl| - allow(Workflows::BulkUpdateService) - .to receive(:new) - .with(role: role, type: type, tab: "always") - .and_return(dbl) - end - end let(:call_result) { ServiceResult.success } - let(:params) do - { - role_id: role.id, - workflow_type_id: type.id, - tab: "always", - status: status_params - } - end - before do - allow(service).to receive(:call).with(status_params).and_return(call_result) - allow(controller).to receive(:statuses_for_form).and_return([build_stubbed(:status)]) - post :update, params:, format: :turbo_stream + context "with a single role" do + let(:service) do + instance_double(Workflows::BulkUpdateService).tap do |dbl| + allow(Workflows::BulkUpdateService) + .to receive(:new) + .with(role: role, type: type, tab: "always") + .and_return(dbl) + end + end + + before do + allow(service).to receive(:call).with(status_params).and_return(call_result) + allow(controller).to receive(:statuses_for_form).and_return([build_stubbed(:status)]) + post :update, + params: { role_ids: [role.id.to_s], workflow_type_id: type.id, tab: "always", status: status_params }, + format: :turbo_stream + end + + it "calls the service and renders a flash turbo stream" do + expect(service).to have_received(:call).with(status_params) + expect(response).to have_turbo_stream action: "flash", target: "op-primer-flash-component" + end end - it "renders a flash turbo stream" do - expect(service).to have_received(:call).with(status_params) - expect(response).to have_turbo_stream action: "flash", target: "op-primer-flash-component" + context "with multiple roles" do + let(:role2) { build_stubbed(:project_role) } + let(:service1) do + instance_double(Workflows::BulkUpdateService).tap do |dbl| + allow(Workflows::BulkUpdateService) + .to receive(:new) + .with(role: role, type: type, tab: "always") + .and_return(dbl) + end + end + let(:service2) do + instance_double(Workflows::BulkUpdateService).tap do |dbl| + allow(Workflows::BulkUpdateService) + .to receive(:new) + .with(role: role2, type: type, tab: "always") + .and_return(dbl) + end + end + + before do + allow(role_scope) + .to receive(:where) + .with(id: [role.id.to_s, role2.id.to_s]) + .and_return([role, role2]) + allow(service1).to receive(:call).with(status_params).and_return(call_result) + allow(service2).to receive(:call).with(status_params).and_return(call_result) + allow(controller).to receive(:statuses_for_form).and_return([build_stubbed(:status)]) + post :update, + params: { + role_ids: [role.id.to_s, role2.id.to_s], + workflow_type_id: type.id, + tab: "always", + status: status_params + }, + format: :turbo_stream + end + + it "calls the service for each role and renders a flash turbo stream" do + expect(service1).to have_received(:call).with(status_params) + expect(service2).to have_received(:call).with(status_params) + expect(response).to have_turbo_stream action: "flash", target: "op-primer-flash-component" + end end end end diff --git a/spec/controllers/workflows_controller_spec.rb b/spec/controllers/workflows_controller_spec.rb index 1bfd390f410b..ccddd2983627 100644 --- a/spec/controllers/workflows_controller_spec.rb +++ b/spec/controllers/workflows_controller_spec.rb @@ -40,7 +40,7 @@ .and_return(role_scope) allow(role_scope) - .to receive_messages(order: role_scope, find_by: nil) + .to receive_messages(order: role_scope, find_by: nil, first: role) allow(role_scope) .to receive(:find) @@ -52,6 +52,11 @@ .with(id: role.id.to_s) .and_return(role) + allow(role_scope) + .to receive(:where) + .with(id: nil) + .and_return([]) + role_scope end @@ -90,10 +95,6 @@ allow(Status) .to receive(:all) .and_return [type_status, non_type_status] - - allow(role_scope) - .to receive(:order) - .and_return([role]) end context "without parameters" do @@ -111,9 +112,10 @@ .to render_template :edit end - it "assigns the first role" do - expect(assigns[:role]) - .to eq role + # TODO: @role is a temporary single-role fallback; remove once components support multi-role natively + it "assigns the first role as fallback for @role, with @roles as the canonical collection" do + expect(assigns[:role]).to eq role + expect(assigns[:roles]).to contain_exactly(role) end it "does assign type" do @@ -122,40 +124,65 @@ end end - context "with role and type params" do + context "with a single role param" do before do - get :edit, params: { role_id: role.id.to_s, type_id: type.id.to_s } + allow(role_scope) + .to receive(:where) + .with(id: [role.id.to_s]) + .and_return([role]) + + get :edit, params: { role_ids: [role.id.to_s], type_id: type.id.to_s } end - it "responds with the role type statuses", :aggregate_failures do - expect(response) - .to have_http_status(:ok) - expect(response) - .to render_template :edit - expect(assigns[:role]) - .to eq role - expect(assigns[:type]) - .to eq type + it "is successful" do + expect(response).to have_http_status(:ok) + end + + it "renders the edit template" do + expect(response).to render_template :edit + end + + it "assigns the selected role" do + expect(assigns[:roles]).to contain_exactly(role) + expect(assigns[:role]).to eq role + end + + it "assigns type" do + expect(assigns[:type]).to eq type + end + end + + context "with multiple role params" do + let(:role2) { build_stubbed(:project_role) } + + before do + allow(role_scope) + .to receive(:where) + .with(id: [role.id.to_s, role2.id.to_s]) + .and_return([role, role2]) + + get :edit, params: { role_ids: [role.id.to_s, role2.id.to_s], type_id: type.id.to_s } end it "is successful" do - expect(response) - .to have_http_status(:ok) + expect(response).to have_http_status(:ok) end it "renders the edit template" do - expect(response) - .to render_template :edit + expect(response).to render_template :edit end - it "assigns role" do - expect(assigns[:role]) - .to eq role + it "assigns all selected roles" do + expect(assigns[:roles]).to contain_exactly(role, role2) end - it "assign type" do - expect(assigns[:type]) - .to eq type + # TODO: @role is a temporary single-role fallback; remove once components support multi-role natively + it "assigns the first role as a temporary single-role fallback for @role" do + expect(assigns[:role]).to eq role + end + + it "assigns type" do + expect(assigns[:type]).to eq type end end end diff --git a/spec/features/roles/create_spec.rb b/spec/features/roles/create_spec.rb index fab54f84acc4..f2a09d7cc31b 100644 --- a/spec/features/roles/create_spec.rb +++ b/spec/features/roles/create_spec.rb @@ -111,8 +111,11 @@ click_link type.name end + new_role = Role.find_by!(name: "New role name") click_button existing_role.name - click_link "New role name" + find("[data-item-id='#{new_role.id}']").click + find("[data-item-id='#{existing_role.id}']").click + page.send_keys :escape old_status = existing_workflow.old_status.name new_status = existing_workflow.new_status.name diff --git a/spec/features/workflows/edit_spec.rb b/spec/features/workflows/edit_spec.rb index e214e19b4693..d828c3400c38 100644 --- a/spec/features/workflows/edit_spec.rb +++ b/spec/features/workflows/edit_spec.rb @@ -53,10 +53,17 @@ def workflow_checkbox(from_index, to_index) end def visit_workflow_edit(role: nil, tab: nil) - params = { controller: "/workflows", action: :edit, type_id: type.id } - params[:role_id] = role.id if role + params = {} + params[:role_ids] = [role.id] if role params[:tab] = tab if tab - visit url_for(params) + visit edit_workflow_path(type, **params) + end + + def switch_role_via_panel(from_role, to_role) + click_button from_role.name + find("[data-item-id='#{to_role.id}']").click + find("[data-item-id='#{from_role.id}']").click + page.send_keys :escape end def add_status_via_dialog(status) @@ -322,8 +329,7 @@ def remove_status_via_dialog(status) end it "loads the matrix for a different role after switching" do - click_button role.name - click_link other_role.name + switch_role_via_panel(role, other_role) within "#workflow_form_always" do expect(page).to have_field workflow_checkbox(1, 2) @@ -336,8 +342,7 @@ def remove_status_via_dialog(status) check workflow_checkbox(1, 0) end - click_button role.name - click_link other_role.name + switch_role_via_panel(role, other_role) within_dialog "Save changes before continuing?" do click_button "Ignore changes" @@ -347,8 +352,7 @@ def remove_status_via_dialog(status) expect(page).to have_field workflow_checkbox(1, 2) end - click_button other_role.name - click_link role.name + switch_role_via_panel(other_role, role) within "#workflow_form_always" do expect(page).to have_field workflow_checkbox(1, 0), checked: false @@ -360,8 +364,7 @@ def remove_status_via_dialog(status) check workflow_checkbox(1, 0) end - click_button role.name - click_link other_role.name + switch_role_via_panel(role, other_role) within_dialog "Save changes before continuing?" do click_button "Save changes and continue" @@ -383,8 +386,7 @@ def remove_status_via_dialog(status) check workflow_checkbox(1, 0) end - click_button role.name - click_link other_role.name + switch_role_via_panel(role, other_role) within_dialog "Save changes before continuing?" do find(".close-button").click @@ -402,8 +404,7 @@ def remove_status_via_dialog(status) add_status_via_dialog(statuses[2]) expect(page).to have_field workflow_checkbox(0, 2) - click_button role.name - click_link other_role.name + switch_role_via_panel(role, other_role) expect(page).to have_dialog("Save changes before continuing?") end @@ -417,8 +418,7 @@ def remove_status_via_dialog(status) expect(page).to have_no_field workflow_checkbox(0, 1) - click_button role.name - click_link other_role.name + switch_role_via_panel(role, other_role) expect(page).to have_dialog("Save changes before continuing?") end From f1fc3c17b277f4ffa6a32c65671e957130df6048 Mon Sep 17 00:00:00 2001 From: Mir Bhatia Date: Thu, 23 Apr 2026 18:50:42 +0200 Subject: [PATCH 03/12] Show indeterminate checkboxes in workflows matrix --- app/controllers/workflows/tabs_controller.rb | 17 +++++++++-------- app/views/workflows/_form.html.erb | 10 ++++++---- .../admin/workflow-checkbox-state.controller.ts | 9 +++++++++ 3 files changed, 24 insertions(+), 12 deletions(-) diff --git a/app/controllers/workflows/tabs_controller.rb b/app/controllers/workflows/tabs_controller.rb index 6c476c890b51..ab0efcc764f9 100644 --- a/app/controllers/workflows/tabs_controller.rb +++ b/app/controllers/workflows/tabs_controller.rb @@ -48,7 +48,7 @@ def edit statuses_for_form - if @type && @role && @statuses.any? + if @type && @roles.any? && @statuses.any? workflows_for_form end end @@ -100,7 +100,7 @@ def status_dialog current_statuses = if params[:status_ids].present? Status.where(id: params[:status_ids].map(&:to_i)).order(:position) elsif @type && @role - statuses_for_role_and_type + statuses_for_roles_and_type else Status.none end @@ -169,8 +169,8 @@ def statuses_for_form @has_status_changes = false @statuses = if @type && params[:status_ids].present? statuses_from_params - elsif @type && @role - statuses_for_role_and_type + elsif @type && @roles.any? + statuses_for_roles_and_type elsif @type @type.statuses else @@ -180,18 +180,19 @@ def statuses_for_form def statuses_from_params status_ids = params[:status_ids].map(&:to_i) - saved_ids = statuses_for_role_and_type.pluck(:id) + saved_ids = statuses_for_roles_and_type.pluck(:id) @added_status_ids = status_ids - saved_ids @has_status_changes = @added_status_ids.any? || (saved_ids - status_ids).any? Status.where(id: status_ids).order(:position) end - def statuses_for_role_and_type - @type.statuses(role: @role, tab: @tab) + def statuses_for_roles_and_type + status_ids = @roles.map { |role| @type.statuses(role:, tab: @tab).pluck(:id) }.flatten.uniq + Status.where(id: status_ids) end def workflows_for_form - workflows = Workflow.where(role_id: @role.id, type_id: @type.id) + workflows = Workflow.where(role_id: @roles.map(&:id), type_id: @type.id) @workflows = {} @workflows["always"] = workflows.select { |w| !w.author && !w.assignee } @workflows["author"] = workflows.select(&:author) diff --git a/app/views/workflows/_form.html.erb b/app/views/workflows/_form.html.erb index 95e3a7469a40..390b86a2c0fc 100644 --- a/app/views/workflows/_form.html.erb +++ b/app/views/workflows/_form.html.erb @@ -187,8 +187,9 @@ See COPYRIGHT and LICENSE files for more details. <% end %> <% @statuses.each do |new_status| -%> - <% transition_saved = workflows.any? { it.old_status_id == old_status.id && it.new_status_id == new_status.id } - newly_added_status = @added_status_ids.include?(old_status.id) || @added_status_ids.include?(new_status.id) %> + <% transition_role_ids = workflows.select { it.old_status_id == old_status.id && it.new_status_id == new_status.id }.map(&:role_id).uniq + newly_added_status = @added_status_ids.include?(old_status.id) || @added_status_ids.include?(new_status.id) + some_roles = !transition_role_ids.empty? && transition_role_ids.size < @roles.size && !newly_added_status %> <%= render(Primer::BaseComponent.new(tag: :div, display: :flex, align_items: :center, mx: 1)) do @@ -198,13 +199,14 @@ See COPYRIGHT and LICENSE files for more details. name: "status[#{old_status.id}][#{new_status.id}]", id: "status_#{old_status.id}_#{new_status.id}", # See BUG https://github.com/primer/view_components/issues/3811 value: name, - checked: transition_saved || newly_added_status, + checked: transition_role_ids.any? || newly_added_status, label: t(".matrix_checkbox_label", old_status: old_status.name, new_status: new_status.name), visually_hide_label: true, data: { checkable_target: "checkbox", old_status: old_status.id, - new_status: new_status.id + new_status: new_status.id, + indeterminate: (true if some_roles) } ) ) diff --git a/frontend/src/stimulus/controllers/dynamic/admin/workflow-checkbox-state.controller.ts b/frontend/src/stimulus/controllers/dynamic/admin/workflow-checkbox-state.controller.ts index a88031e47c58..2199f07fa0b0 100644 --- a/frontend/src/stimulus/controllers/dynamic/admin/workflow-checkbox-state.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/admin/workflow-checkbox-state.controller.ts @@ -83,6 +83,9 @@ export default class WorkflowCheckboxStateController extends Controller('input[type="checkbox"][data-indeterminate="true"]').forEach((cb) => { + cb.indeterminate = true; + }); + } + // // Trigger navigation with dirty-state confirmation. // From 225d874101758fac4da52f13b7e23badc150a7ce Mon Sep 17 00:00:00 2001 From: Mir Bhatia Date: Fri, 24 Apr 2026 16:18:15 +0200 Subject: [PATCH 04/12] Add handling for indeterminate checkboxes --- app/controllers/workflows/tabs_controller.rb | 41 +++++++++++++++++-- app/views/workflows/_form.html.erb | 3 +- .../workflow-checkbox-state.controller.ts | 18 ++++++-- 3 files changed, 54 insertions(+), 8 deletions(-) diff --git a/app/controllers/workflows/tabs_controller.rb b/app/controllers/workflows/tabs_controller.rb index ab0efcc764f9..e13f869d3111 100644 --- a/app/controllers/workflows/tabs_controller.rb +++ b/app/controllers/workflows/tabs_controller.rb @@ -57,9 +57,12 @@ def update # rubocop:disable Metrics/AbcSize success = false Workflow.transaction do success = true + base_params = permitted_status_params + indeterminate = permitted_indeterminate_params @roles.each do |role| + role_params = indeterminate.empty? ? base_params : role_specific_params(base_params, indeterminate, role) result = Workflows::BulkUpdateService.new(role:, type: @type, tab: @tab) - .call(permitted_status_params) + .call(role_params) success = false unless result.success? end raise ActiveRecord::Rollback unless success @@ -200,10 +203,40 @@ def workflows_for_form end def permitted_status_params - return {} if params["status"].blank? + status_params("status") + end + + def permitted_indeterminate_params + status_params("indeterminate_status") + end - params["status"] + def status_params(key) + return {} if params[key].blank? + + params[key] .to_unsafe_h - .select { |key, value| /\A\d+\z/.match?(key) && value.keys.all? { /\A\d+\z/.match?(it) } } + .select { |k, value| /\A\d+\z/.match?(k) && value.keys.all? { /\A\d+\z/.match?(it) } } + end + + def role_specific_params(base_params, indeterminate, role) + params = base_params.deep_dup + indeterminate.each do |old_id, new_ids| + new_ids.each_key do |new_id| + # Restore from DB so that it isn't overwritten by indeterminate state (unchecked) + had_transition = Workflow.exists?( + role_id: role.id, + type_id: @type.id, + old_status_id: old_id.to_i, + new_status_id: new_id.to_i, + author: @tab == "author", + assignee: @tab == "assignee" + ) + if had_transition + params[old_id] ||= {} + params[old_id][new_id] = "1" + end + end + end + params end end diff --git a/app/views/workflows/_form.html.erb b/app/views/workflows/_form.html.erb index 390b86a2c0fc..11cbe41d9449 100644 --- a/app/views/workflows/_form.html.erb +++ b/app/views/workflows/_form.html.erb @@ -191,6 +191,7 @@ See COPYRIGHT and LICENSE files for more details. newly_added_status = @added_status_ids.include?(old_status.id) || @added_status_ids.include?(new_status.id) some_roles = !transition_role_ids.empty? && transition_role_ids.size < @roles.size && !newly_added_status %> + <%= hidden_field_tag "indeterminate_status[#{old_status.id}][#{new_status.id}]", "1" if some_roles %> <%= render(Primer::BaseComponent.new(tag: :div, display: :flex, align_items: :center, mx: 1)) do render( @@ -199,7 +200,7 @@ See COPYRIGHT and LICENSE files for more details. name: "status[#{old_status.id}][#{new_status.id}]", id: "status_#{old_status.id}_#{new_status.id}", # See BUG https://github.com/primer/view_components/issues/3811 value: name, - checked: transition_role_ids.any? || newly_added_status, + checked: !some_roles && (transition_role_ids.any? || newly_added_status), label: t(".matrix_checkbox_label", old_status: old_status.name, new_status: new_status.name), visually_hide_label: true, data: { diff --git a/frontend/src/stimulus/controllers/dynamic/admin/workflow-checkbox-state.controller.ts b/frontend/src/stimulus/controllers/dynamic/admin/workflow-checkbox-state.controller.ts index 2199f07fa0b0..181e0f0dee63 100644 --- a/frontend/src/stimulus/controllers/dynamic/admin/workflow-checkbox-state.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/admin/workflow-checkbox-state.controller.ts @@ -82,7 +82,7 @@ export default class WorkflowCheckboxStateController extends Controller { + private onCheckboxChange = (event:Event) => { + this.removeIndeterminateMarker(event.target as HTMLInputElement); + this.recomputeDirtyFlag(); + }; + + private recomputeDirtyFlag() { const current = this.captureState(); const hasChanges = Object.keys(current).some((key) => current[key] !== this.initialCheckboxState[key]); this.hasCheckboxChangesValue = hasChanges; - }; + } + + private removeIndeterminateMarker(checkbox:HTMLInputElement):void { + const { oldStatus, newStatus } = checkbox.dataset; + this.element.querySelector( + `input[name="indeterminate_status[${oldStatus}][${newStatus}]"]`, + )?.remove(); + } private captureState():Record { const checkboxes:Record = {}; From 625a65ac1156cb2dfa5b9fcc0bce4f1ceb93cfc5 Mon Sep 17 00:00:00 2001 From: Mir Bhatia Date: Fri, 24 Apr 2026 17:33:16 +0200 Subject: [PATCH 05/12] Add multi role and indeterminate state specs --- .../workflows/edit_multi_role_spec.rb | 247 ++++++++++++++++++ spec/features/workflows/edit_spec.rb | 75 ++---- spec/support/workflows/edit_helpers.rb | 83 ++++++ 3 files changed, 345 insertions(+), 60 deletions(-) create mode 100644 spec/features/workflows/edit_multi_role_spec.rb create mode 100644 spec/support/workflows/edit_helpers.rb diff --git a/spec/features/workflows/edit_multi_role_spec.rb b/spec/features/workflows/edit_multi_role_spec.rb new file mode 100644 index 000000000000..c384bd4a9339 --- /dev/null +++ b/spec/features/workflows/edit_multi_role_spec.rb @@ -0,0 +1,247 @@ +# frozen_string_literal: true + +# -- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +# ++ + +require "spec_helper" + +RSpec.describe "Workflow edit with multiple roles", :js do + include Toasts::Expectations + include Workflows::EditHelpers + + let(:role) { create(:project_role) } + let(:role2) { create(:project_role) } + let(:type) { create(:type) } + let(:admin) { create(:admin) } + let(:statuses) { (1..3).map { create(:status) } } + + # workflow for 0 -> 1 for 'role' + let!(:role_workflow) do + create(:workflow, role_id: role.id, type_id: type.id, + old_status_id: statuses[0].id, new_status_id: statuses[1].id, + author: false, assignee: false) + end + + current_user { admin } + + context "when displaying checkboxes" do + context "when all selected roles have a transition" do + let!(:role2_workflow) do + create(:workflow, role_id: role2.id, type_id: type.id, + old_status_id: statuses[0].id, new_status_id: statuses[1].id, + author: false, assignee: false) + end + + before { visit_workflow_edit(roles: [role, role2]) } + + it "shows the checkbox as checked" do + expect(page).to have_field workflow_checkbox(0, 1), checked: true + expect(indeterminate?(workflow_checkbox(0, 1))).to be false + end + end + + context "when no selected roles have a transition" do + before { visit_workflow_edit(roles: [role, role2]) } + + it "shows the checkbox as unchecked" do + expect(page).to have_field workflow_checkbox(1, 0), checked: false + expect(indeterminate?(workflow_checkbox(1, 0))).to be false + end + end + + context "when only some selected roles have a transition" do + before { visit_workflow_edit(roles: [role, role2]) } + + it "shows the checkbox as indeterminate" do + expect(page).to have_field workflow_checkbox(0, 1), checked: false + expect(indeterminate?(workflow_checkbox(0, 1))).to be true + end + end + + context "when roles have different statuses in their workflows" do + let!(:role2_workflow) do + create(:workflow, role_id: role2.id, type_id: type.id, + old_status_id: statuses[1].id, new_status_id: statuses[2].id, + author: false, assignee: false) + end + + before { visit_workflow_edit(roles: [role, role2]) } + + it "shows the union of statuses from all selected roles" do + expect(page).to have_field workflow_checkbox(0, 2) + expect(page).to have_field workflow_checkbox(1, 2) + end + end + + context "with a single role selected" do + before { visit_workflow_edit(roles: [role]) } + + it "does not show indeterminate checkboxes" do + expect(page).to have_field workflow_checkbox(0, 1), checked: true + expect(indeterminate?(workflow_checkbox(0, 1))).to be false + end + end + end + + context "when saving" do + before { visit_workflow_edit(roles: [role, role2]) } + + it "adds the transition for all roles when checking an unchecked checkbox" do + expect_transition(role, 1, 0, exist: false) + expect_transition(role2, 1, 0, exist: false) + + check workflow_checkbox(1, 0) + click_button "Save" + expect_flash(message: "Successful update.") + + expect_transition(role, 1, 0, exist: true) + expect_transition(role2, 1, 0, exist: true) + end + + it "preserves state for each role when saving an untouched indeterminate checkbox" do + expect_transition(role, 0, 1, exist: true) + expect_transition(role2, 0, 1, exist: false) + + expect(page).to have_field workflow_checkbox(0, 1), checked: false + expect(indeterminate?(workflow_checkbox(0, 1))).to be true + + click_button "Save" + expect_flash(message: "Successful update.") + + expect_transition(role, 0, 1, exist: true) + expect_transition(role2, 0, 1, exist: false) + + expect(indeterminate?(workflow_checkbox(0, 1))).to be true + end + + it "adds the transition for all roles when checking an indeterminate checkbox" do + expect_transition(role, 0, 1, exist: true) + expect_transition(role2, 0, 1, exist: false) + + expect(page).to have_field workflow_checkbox(0, 1), checked: false + expect(indeterminate?(workflow_checkbox(0, 1))).to be true + + check workflow_checkbox(0, 1) + click_button "Save" + expect_flash(message: "Successful update.") + + expect_transition(role, 0, 1, exist: true) + expect_transition(role2, 0, 1, exist: true) + + expect(indeterminate?(workflow_checkbox(0, 1))).to be false + end + + it "removes the transition from all roles when unchecking an indeterminate checkbox" do + expect_transition(role, 0, 1, exist: true) + expect_transition(role2, 0, 1, exist: false) + + expect(page).to have_field workflow_checkbox(0, 1), checked: false + expect(indeterminate?(workflow_checkbox(0, 1))).to be true + + check workflow_checkbox(0, 1) + uncheck workflow_checkbox(0, 1) + click_button "Save" + expect_flash(message: "Successful update.") + + expect_transition(role, 0, 1, exist: false) + expect_transition(role2, 0, 1, exist: false) + + expect(indeterminate?(workflow_checkbox(0, 1))).to be false + end + + context "when all roles have the transition" do + let!(:role2_workflow) do + create(:workflow, role_id: role2.id, type_id: type.id, + old_status_id: statuses[0].id, new_status_id: statuses[1].id, + author: false, assignee: false) + end + + before { visit_workflow_edit(roles: [role, role2]) } + + it "removes the transition from all roles when unchecking a fully checked checkbox" do + expect_transition(role, 0, 1, exist: true) + expect_transition(role2, 0, 1, exist: true) + + uncheck workflow_checkbox(0, 1) + click_button "Save" + expect_flash(message: "Successful update.") + + expect_transition(role, 0, 1, exist: false) + expect_transition(role2, 0, 1, exist: false) + end + end + + context "with multiple indeterminate checkboxes" do + let!(:role_workflow2) do + create(:workflow, role_id: role.id, type_id: type.id, + old_status_id: statuses[0].id, new_status_id: statuses[2].id, + author: false, assignee: false) + end + + before { visit_workflow_edit(roles: [role, role2]) } + + it "handles touched and untouched indeterminate checkboxes independently" do + # Both 0 -> 1 and 0 -> 2 are indeterminate + expect_transition(role, 0, 1, exist: true) + expect_transition(role2, 0, 1, exist: false) + expect_transition(role, 0, 2, exist: true) + expect_transition(role2, 0, 2, exist: false) + + check workflow_checkbox(0, 1) # explicitly check for all roles + + click_button "Save" + expect_flash(message: "Successful update.") + + # role2 now has this workflow + expect_transition(role2, 0, 1, exist: true) + + # 0 -> 2 stays indeterminate + expect_transition(role, 0, 2, exist: true) + expect_transition(role2, 0, 2, exist: false) + end + end + + it "marks the form dirty when interacting with an indeterminate checkbox" do + expect(page).to have_field workflow_checkbox(0, 1), checked: false + expect(indeterminate?(workflow_checkbox(0, 1))).to be true + + check workflow_checkbox(0, 1) + + click_link "User is author" + expect(page).to have_dialog("Save changes before continuing?") + end + + it "succeeds when saving with no changes to indeterminate checkboxes" do + expect(page).to have_field workflow_checkbox(0, 1), checked: false + expect(indeterminate?(workflow_checkbox(0, 1))).to be true + + click_button "Save" + expect_flash(message: "Successful update.") + end + end +end diff --git a/spec/features/workflows/edit_spec.rb b/spec/features/workflows/edit_spec.rb index d828c3400c38..1845346ce6ff 100644 --- a/spec/features/workflows/edit_spec.rb +++ b/spec/features/workflows/edit_spec.rb @@ -32,6 +32,7 @@ RSpec.describe "Workflow edit", :js do include Toasts::Expectations + include Workflows::EditHelpers let(:role) { create(:project_role) } let(:type) { create(:type) } @@ -48,51 +49,12 @@ current_user { admin } - def workflow_checkbox(from_index, to_index) - "status_#{statuses[from_index].id}_#{statuses[to_index].id}" - end - - def visit_workflow_edit(role: nil, tab: nil) - params = {} - params[:role_ids] = [role.id] if role - params[:tab] = tab if tab - visit edit_workflow_path(type, **params) - end - - def switch_role_via_panel(from_role, to_role) - click_button from_role.name - find("[data-item-id='#{to_role.id}']").click - find("[data-item-id='#{from_role.id}']").click - page.send_keys :escape - end - - def add_status_via_dialog(status) - within "#workflow-table" do # Otherwise, click on "Statuses" menu item - click_link "Status" - end - within_dialog "Statuses" do - find(".ng-arrow-wrapper").click - find(".ng-option", text: status.name).click - click_button "Apply" - end - end - - def remove_status_via_dialog(status) - within "#workflow-table" do # Otherwise, click on "Statuses" menu item - click_link "Status" - end - within_dialog "Statuses" do - find(".ng-value", text: status.name).find(".ng-value-icon").click - click_button "Apply" - end - end - before do visit_workflow_edit end it "allows adding another workflow" do - visit_workflow_edit(role:) + visit_workflow_edit(roles: [role]) check workflow_checkbox(1, 0) @@ -121,7 +83,7 @@ def remove_status_via_dialog(status) old_status_id: statuses[0].id, new_status_id: statuses[1].id, author: true, assignee: false) - visit_workflow_edit(role:, tab: "author") + visit_workflow_edit(roles: [role], tab: "author") within "#workflow_form_author" do check workflow_checkbox(1, 0) @@ -157,7 +119,7 @@ def remove_status_via_dialog(status) old_status_id: statuses[0].id, new_status_id: statuses[1].id, author: false, assignee: true) - visit_workflow_edit(role:, tab: "assignee") + visit_workflow_edit(roles: [role], tab: "assignee") within "#workflow_form_assignee" do check workflow_checkbox(1, 0) @@ -201,7 +163,7 @@ def remove_status_via_dialog(status) end before do - visit_workflow_edit(role:) + visit_workflow_edit(roles: [role]) end it "shows the always tab by default" do @@ -261,9 +223,7 @@ def remove_status_via_dialog(status) expect(page).to have_css("#workflow_form_author") - expect(Workflow.exists?(role_id: role.id, type_id: type.id, - old_status_id: statuses[1].id, new_status_id: statuses[0].id, - author: false, assignee: false)).to be true + expect_transition(role, 1, 0, exist: true) end it "keeps unsaved changes and stays on the same tab when closing the dialog via 'X'" do @@ -318,7 +278,7 @@ def remove_status_via_dialog(status) end before do - visit_workflow_edit(role:) + visit_workflow_edit(roles: [role]) end it "shows the matrix for the first role" do @@ -376,9 +336,7 @@ def remove_status_via_dialog(status) expect(page).to have_field workflow_checkbox(1, 2) end - expect(Workflow.exists?(role_id: role.id, type_id: type.id, - old_status_id: statuses[1].id, new_status_id: statuses[0].id, - author: false, assignee: false)).to be true + expect_transition(role, 1, 0, exist: true) end it "keeps unsaved changes and stays on the same role when closing the dialog via 'X'" do @@ -426,7 +384,7 @@ def remove_status_via_dialog(status) context "when reloading the page with unsaved changes", :js do before do - visit_workflow_edit(role:) + visit_workflow_edit(roles: [role]) end it "shows a browser confirmation when reloading with unsaved checkbox changes" do @@ -466,7 +424,7 @@ def remove_status_via_dialog(status) context "with status dialog", :js do before do - visit_workflow_edit(role:) + visit_workflow_edit(roles: [role]) end it "shows only role-specific statuses in the matrix by default" do @@ -474,7 +432,7 @@ def remove_status_via_dialog(status) create(:workflow, role_id: other_role.id, type_id: type.id, old_status_id: statuses[0].id, new_status_id: statuses[2].id) - visit_workflow_edit(role:) + visit_workflow_edit(roles: [role]) expect(page).to have_field workflow_checkbox(0, 1) expect(page).to have_no_field workflow_checkbox(2, 0) @@ -573,8 +531,7 @@ def remove_status_via_dialog(status) expect_flash(message: "Successful update.") - expect(Workflow.exists?(role_id: role.id, type_id: type.id, - old_status_id: statuses[0].id, new_status_id: statuses[2].id)).to be true + expect_transition(role, 0, 2, exist: true) expect(page).to have_field workflow_checkbox(0, 2) @@ -590,7 +547,7 @@ def remove_status_via_dialog(status) expect(page).to have_field workflow_checkbox(2, 0) - visit_workflow_edit(role:) + visit_workflow_edit(roles: [role]) expect(page).to have_no_field workflow_checkbox(2, 0) end @@ -637,7 +594,7 @@ def remove_status_via_dialog(status) expect_flash(message: "Successful update.") - visit_workflow_edit(role:) + visit_workflow_edit(roles: [role]) expect(page).to have_no_field workflow_checkbox(2, 0) expect(page).to have_no_field workflow_checkbox(0, 2) @@ -698,9 +655,7 @@ def remove_status_via_dialog(status) expect_flash(message: "Successful update.") - expect(Workflow.exists?(role_id: role.id, type_id: type.id, - old_status_id: statuses[1].id, new_status_id: statuses[0].id, - author: false, assignee: false)).to be true + expect_transition(role, 1, 0, exist: true) expect(page).to have_dialog "Copy workflow" end diff --git a/spec/support/workflows/edit_helpers.rb b/spec/support/workflows/edit_helpers.rb new file mode 100644 index 000000000000..441c1f5e5eb7 --- /dev/null +++ b/spec/support/workflows/edit_helpers.rb @@ -0,0 +1,83 @@ +# frozen_string_literal: true + +# -- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +# ++ + +module Workflows + module EditHelpers + def workflow_checkbox(from_index, to_index) + "status_#{statuses[from_index].id}_#{statuses[to_index].id}" + end + + def visit_workflow_edit(roles: [], tab: nil) + params = {} + params[:role_ids] = roles.map(&:id) if roles.any? + params[:tab] = tab if tab + visit edit_workflow_path(type, **params) + end + + def switch_role_via_panel(from_role, to_role) + click_button from_role.name + find("[data-item-id='#{to_role.id}']").click + find("[data-item-id='#{from_role.id}']").click + page.send_keys :escape + end + + def add_status_via_dialog(status) + within "#workflow-table" do # Otherwise, click on "Statuses" menu item + click_link "Status" + end + within_dialog "Statuses" do + find(".ng-arrow-wrapper").click + find(".ng-option", text: status.name).click + click_button "Apply" + end + end + + def remove_status_via_dialog(status) + within "#workflow-table" do # Otherwise, click on "Statuses" menu item + click_link "Status" + end + within_dialog "Statuses" do + find(".ng-value", text: status.name).find(".ng-value-icon").click + click_button "Apply" + end + end + + def indeterminate?(checkbox_id) + page.evaluate_script("document.getElementById('#{checkbox_id}')?.indeterminate ?? false") + end + + def expect_transition(role, from_index, to_index, exist:, author: false, assignee: false) + expect(Workflow.exists?(role_id: role.id, type_id: type.id, + old_status_id: statuses[from_index].id, + new_status_id: statuses[to_index].id, + author:, assignee:)).to be exist + end + end +end From f9edc9eedf525839bb129c34944052dc008e7dc7 Mon Sep 17 00:00:00 2001 From: Mir Bhatia Date: Mon, 27 Apr 2026 12:58:18 +0200 Subject: [PATCH 06/12] Add css override to properly show indeterminate state --- frontend/src/global_styles/primer/_overrides.sass | 10 ++++++++++ spec/features/workflows/edit_multi_role_spec.rb | 7 +++++++ spec/support/workflows/edit_helpers.rb | 10 ++++++++++ 3 files changed, 27 insertions(+) diff --git a/frontend/src/global_styles/primer/_overrides.sass b/frontend/src/global_styles/primer/_overrides.sass index a34390c13e36..f9f3869f6c03 100644 --- a/frontend/src/global_styles/primer/_overrides.sass +++ b/frontend/src/global_styles/primer/_overrides.sass @@ -208,3 +208,13 @@ ul.SegmentedControl, & .Banner-actions margin: var(--base-size-8) 0 0 var(--base-size-8) + +// Bug in @openproject/primer-view-components: .FormControl-checkbox:indeterminate +// has no background rule, and :indeterminate:before doesn't cancel checkmarkOut. +.FormControl-checkbox:indeterminate + background: var(--control-checked-bgColor-rest, var(--color-accent-fg)) + border-color: var(--control-checked-borderColor-rest, var(--color-accent-fg)) + + &:before + animation: none + clip-path: inset(0) diff --git a/spec/features/workflows/edit_multi_role_spec.rb b/spec/features/workflows/edit_multi_role_spec.rb index c384bd4a9339..d4266d186efb 100644 --- a/spec/features/workflows/edit_multi_role_spec.rb +++ b/spec/features/workflows/edit_multi_role_spec.rb @@ -81,6 +81,13 @@ expect(page).to have_field workflow_checkbox(0, 1), checked: false expect(indeterminate?(workflow_checkbox(0, 1))).to be true end + + it "the checkbox is visible as indeterminate" do + expect(page).to have_field workflow_checkbox(0, 1), checked: false + + expect(indeterminate?(workflow_checkbox(0, 1))).to be true + expect(indeterminate_visible?(workflow_checkbox(0, 1))).to be true + end end context "when roles have different statuses in their workflows" do diff --git a/spec/support/workflows/edit_helpers.rb b/spec/support/workflows/edit_helpers.rb index 441c1f5e5eb7..8a80b9708cdb 100644 --- a/spec/support/workflows/edit_helpers.rb +++ b/spec/support/workflows/edit_helpers.rb @@ -73,6 +73,16 @@ def indeterminate?(checkbox_id) page.evaluate_script("document.getElementById('#{checkbox_id}')?.indeterminate ?? false") end + def indeterminate_visible?(checkbox_id) + page.evaluate_script(<<~JS) + (() => { + const el = document.getElementById('#{checkbox_id}'); + const bg = window.getComputedStyle(el).backgroundColor; + return bg !== 'rgba(0, 0, 0, 0)' && bg !== 'rgb(255, 255, 255)'; + })() + JS + end + def expect_transition(role, from_index, to_index, exist:, author: false, assignee: false) expect(Workflow.exists?(role_id: role.id, type_id: type.id, old_status_id: statuses[from_index].id, From e561086d3832226268276729bdf137340d5c6555 Mon Sep 17 00:00:00 2001 From: Mir Bhatia Date: Tue, 28 Apr 2026 12:33:07 +0200 Subject: [PATCH 07/12] Update all remaining components to work with multiple roles --- .../workflows/blankslate_component.html.erb | 3 +-- .../workflows/blankslate_component.rb | 4 ++-- .../workflows/page_headers/edit_component.rb | 4 ++-- .../workflows/status_dialog_component.html.erb | 2 +- .../workflows/status_dialog_component.rb | 4 ++-- .../workflows/status_form_component.html.erb | 4 +--- .../workflows/status_form_component.rb | 4 ++-- .../status_matrix_form_component.html.erb | 8 +++----- .../workflows/status_matrix_form_component.rb | 1 - ...us_removal_danger_dialog_component.html.erb | 3 +-- .../status_removal_danger_dialog_component.rb | 4 ++-- app/controllers/workflows/tabs_controller.rb | 9 +++------ app/controllers/workflows_controller.rb | 3 --- app/forms/workflows/status_select_form.rb | 4 +--- app/views/workflows/edit.html.erb | 2 +- app/views/workflows/tabs/edit.html.erb | 2 +- spec/controllers/workflows_controller_spec.rb | 10 +--------- .../features/workflows/edit_multi_role_spec.rb | 18 ++++++++++++++++++ 18 files changed, 42 insertions(+), 47 deletions(-) diff --git a/app/components/workflows/blankslate_component.html.erb b/app/components/workflows/blankslate_component.html.erb index 6947ea1984f6..48aedf4b9bc5 100644 --- a/app/components/workflows/blankslate_component.html.erb +++ b/app/components/workflows/blankslate_component.html.erb @@ -31,9 +31,8 @@ See COPYRIGHT and LICENSE files for more details. render(Primer::Beta::Blankslate.new(border: true)) do |blankslate| blankslate.with_heading(tag: :h2).with_content(t("admin.workflows.blankslate.title")) blankslate.with_description_content(t("admin.workflows.blankslate.description")) - # TODO: pass all roles once BlankslateComponent accepts roles: and StatusDialogComponent supports multi-role natively. blankslate.with_primary_action( - href: helpers.status_dialog_workflow_tab_path(@type, @tab, role_ids: [@role.id]), + href: helpers.status_dialog_workflow_tab_path(@type, @tab, role_ids: @roles.map(&:id)), scheme: :secondary, data: { controller: "async-dialog" } ) do |button| diff --git a/app/components/workflows/blankslate_component.rb b/app/components/workflows/blankslate_component.rb index ed4c14ef797c..0f92b6ea2f4c 100644 --- a/app/components/workflows/blankslate_component.rb +++ b/app/components/workflows/blankslate_component.rb @@ -32,9 +32,9 @@ module Workflows class BlankslateComponent < ApplicationComponent include OpPrimer::ComponentHelpers - def initialize(role:, type:, tab:) + def initialize(roles:, type:, tab:) super - @role = role + @roles = roles @type = type @tab = tab end diff --git a/app/components/workflows/page_headers/edit_component.rb b/app/components/workflows/page_headers/edit_component.rb index fac6786a992c..794dbed58e8e 100644 --- a/app/components/workflows/page_headers/edit_component.rb +++ b/app/components/workflows/page_headers/edit_component.rb @@ -30,7 +30,7 @@ module Workflows::PageHeaders class EditComponent < BaseComponent - options :tabs, :role + options :tabs, :roles def type = model @@ -49,7 +49,7 @@ def add_action_buttons(header) mobile_icon: :copy, mobile_label: t(:button_copy), size: :medium, - href: new_workflow_copy_path(type, source_role_id: role&.id), + href: new_workflow_copy_path(type, source_role_id: roles&.first&.id), aria: { label: helpers.t(:button_copy) }, title: helpers.t(:button_copy) ) do |button| diff --git a/app/components/workflows/status_dialog_component.html.erb b/app/components/workflows/status_dialog_component.html.erb index 3727681676f1..d9fe0c120653 100644 --- a/app/components/workflows/status_dialog_component.html.erb +++ b/app/components/workflows/status_dialog_component.html.erb @@ -42,7 +42,7 @@ See COPYRIGHT and LICENSE files for more details. Workflows::StatusFormComponent.new( all_statuses: @all_statuses, current_statuses: @current_statuses, - role: @role, + roles: @roles, type: @type, tab: @tab ) diff --git a/app/components/workflows/status_dialog_component.rb b/app/components/workflows/status_dialog_component.rb index 0f0ccde45601..2c724a580024 100644 --- a/app/components/workflows/status_dialog_component.rb +++ b/app/components/workflows/status_dialog_component.rb @@ -35,11 +35,11 @@ class StatusDialogComponent < ApplicationComponent DIALOG_ID = "workflows-status-dialog" - def initialize(all_statuses:, current_statuses:, role:, type:, tab:) + def initialize(all_statuses:, current_statuses:, roles:, type:, tab:) super @all_statuses = all_statuses @current_statuses = current_statuses - @role = role + @roles = roles @type = type @tab = tab end diff --git a/app/components/workflows/status_form_component.html.erb b/app/components/workflows/status_form_component.html.erb index 59394c0a330b..c41534cb44f5 100644 --- a/app/components/workflows/status_form_component.html.erb +++ b/app/components/workflows/status_form_component.html.erb @@ -28,9 +28,8 @@ See COPYRIGHT and LICENSE files for more details. ++#%> <%= - # TODO: pass all roles once StatusFormComponent and confirm_statuses support multi-role natively. primer_form_with( - url: helpers.confirm_statuses_workflow_tab_path(@type, @tab, role_ids: [@role.id]), + url: helpers.confirm_statuses_workflow_tab_path(@type, @tab, role_ids: @roles.map(&:id)), method: :post, id: FORM_ID, data: { turbo_frame: "workflow-table" } @@ -40,7 +39,6 @@ See COPYRIGHT and LICENSE files for more details. f, all_statuses: @all_statuses, current_statuses: @current_statuses, - role: @role, type: @type, tab: @tab, dialog_id: diff --git a/app/components/workflows/status_form_component.rb b/app/components/workflows/status_form_component.rb index 114351c64b86..8b99cfc125e6 100644 --- a/app/components/workflows/status_form_component.rb +++ b/app/components/workflows/status_form_component.rb @@ -32,11 +32,11 @@ module Workflows class StatusFormComponent < ApplicationComponent FORM_ID = "status-selection-form" - def initialize(all_statuses:, current_statuses:, role:, type:, tab:) + def initialize(all_statuses:, current_statuses:, roles:, type:, tab:) super @all_statuses = all_statuses @current_statuses = current_statuses - @role = role + @roles = roles @type = type @tab = tab end diff --git a/app/components/workflows/status_matrix_form_component.html.erb b/app/components/workflows/status_matrix_form_component.html.erb index c6aa018b334a..cfeae1152656 100644 --- a/app/components/workflows/status_matrix_form_component.html.erb +++ b/app/components/workflows/status_matrix_form_component.html.erb @@ -44,8 +44,8 @@ See COPYRIGHT and LICENSE files for more details. button.with_trailing_visual_icon(icon: :"triangle-down") if @roles.many? t("admin.workflows.role_selector.roles", count: @roles.size) - elsif @role - t("admin.workflows.role_selector.label", role: @role.name) + elsif @roles.one? + t("admin.workflows.role_selector.label", role: @roles.first.name) else t("admin.workflows.role_selector.no_role") end @@ -66,8 +66,6 @@ See COPYRIGHT and LICENSE files for more details. scheme: :secondary, leading_icon: :plus, label: t("admin.workflows.status_button"), - # TODO: status_dialog and StatusDialogComponent currently work with a single role (@role = @roles.first); - # update when they support multi-role natively. href: helpers.status_dialog_workflow_tab_path(@type, @tab, role_ids: @roles.map(&:id), status_ids: @statuses.pluck(:id).presence), data: { controller: "async-dialog" } ) do @@ -132,6 +130,6 @@ See COPYRIGHT and LICENSE files for more details. %> <% end %> <% else %> - <%= render Workflows::BlankslateComponent.new(role: @role, type: @type, tab: @tab) %> + <%= render Workflows::BlankslateComponent.new(roles: @roles, type: @type, tab: @tab) %> <% end %> <% end %> diff --git a/app/components/workflows/status_matrix_form_component.rb b/app/components/workflows/status_matrix_form_component.rb index bc3365aaff19..4fc301838e5d 100644 --- a/app/components/workflows/status_matrix_form_component.rb +++ b/app/components/workflows/status_matrix_form_component.rb @@ -37,7 +37,6 @@ def initialize(tab:, roles:, type:, available_roles:, statuses:, has_status_chan super @tab = tab @roles = roles - @role = @roles.first @type = type @available_roles = available_roles @statuses = statuses diff --git a/app/components/workflows/status_removal_danger_dialog_component.html.erb b/app/components/workflows/status_removal_danger_dialog_component.html.erb index 8a8ccd8e5783..9846295eb11e 100644 --- a/app/components/workflows/status_removal_danger_dialog_component.html.erb +++ b/app/components/workflows/status_removal_danger_dialog_component.html.erb @@ -46,8 +46,7 @@ See COPYRIGHT and LICENSE files for more details. # The reason this is done here is because the submit is not a DELETE, and GET form submissions # strip url params dialog.with_additional_details do - # TODO: pass all roles once StatusRemovalDangerDialogComponent supports multi-role natively. - concat(hidden_field_tag("role_ids[]", @role.id)) + @roles.each { |role| concat(hidden_field_tag("role_ids[]", role.id)) } @status_ids.each { |id| concat(hidden_field_tag("status_ids[]", id)) } end end diff --git a/app/components/workflows/status_removal_danger_dialog_component.rb b/app/components/workflows/status_removal_danger_dialog_component.rb index 1753687632a2..d3f926f24c36 100644 --- a/app/components/workflows/status_removal_danger_dialog_component.rb +++ b/app/components/workflows/status_removal_danger_dialog_component.rb @@ -35,9 +35,9 @@ class StatusRemovalDangerDialogComponent < ApplicationComponent DIALOG_ID = "workflows-status-removal-dialog" - def initialize(role:, type:, tab:, status_ids:, removed_count:) + def initialize(roles:, type:, tab:, status_ids:, removed_count:) super - @role = role + @roles = roles @type = type @tab = tab @status_ids = Array(status_ids).flatten.map(&:to_i) diff --git a/app/controllers/workflows/tabs_controller.rb b/app/controllers/workflows/tabs_controller.rb index e13f869d3111..62d5d679d91c 100644 --- a/app/controllers/workflows/tabs_controller.rb +++ b/app/controllers/workflows/tabs_controller.rb @@ -102,7 +102,7 @@ def status_dialog all_statuses = Status.order(:position) current_statuses = if params[:status_ids].present? Status.where(id: params[:status_ids].map(&:to_i)).order(:position) - elsif @type && @role + elsif @type && @roles.any? statuses_for_roles_and_type else Status.none @@ -111,7 +111,7 @@ def status_dialog respond_with_dialog Workflows::StatusDialogComponent.new( all_statuses:, current_statuses:, - role: @role, + roles: @roles, type: @type, tab: @tab ) @@ -124,7 +124,7 @@ def confirm_statuses # rubocop:disable Metrics/AbcSize if removed_count > 0 respond_with_dialog Workflows::StatusRemovalDangerDialogComponent.new( - role: @role, + roles: @roles, type: @type, tab: @tab, status_ids: current_status_ids, @@ -162,9 +162,6 @@ def set_eligible_roles def set_roles @roles = @eligible_roles.where(id: params[:role_ids]) - # TODO: remove @role once the matrix form and all dependent components - # (dialogs, status selectors, page headers) work natively with @roles (multi-role). - @role = @roles.first end def statuses_for_form diff --git a/app/controllers/workflows_controller.rb b/app/controllers/workflows_controller.rb index fb40fead1934..90650d70b8aa 100644 --- a/app/controllers/workflows_controller.rb +++ b/app/controllers/workflows_controller.rb @@ -68,9 +68,6 @@ def find_optional_roles ordered = eligible_roles.order(:builtin, :position) @roles = ordered.where(id: params[:role_ids]) @roles = [ordered.first] if @roles.empty? - # TODO: remove @role once the matrix form and all dependent components - # (dialogs, status selectors, page headers) work natively with @roles (multi-role). - @role = @roles.first end def eligible_roles diff --git a/app/forms/workflows/status_select_form.rb b/app/forms/workflows/status_select_form.rb index cb6c4cf106f7..f650969ea3e4 100644 --- a/app/forms/workflows/status_select_form.rb +++ b/app/forms/workflows/status_select_form.rb @@ -30,18 +30,16 @@ module Workflows class StatusSelectForm < ApplicationForm - def initialize(all_statuses:, current_statuses:, role:, type:, tab:, dialog_id:) + def initialize(all_statuses:, current_statuses:, type:, tab:, dialog_id:) super() @all_statuses = all_statuses @current_statuses = current_statuses - @role = role @type = type @tab = tab @dialog_id = dialog_id end form do |f| - f.hidden(name: :role_id, value: @role.id) f.hidden(name: :type_id, value: @type.id) f.hidden(name: :tab, value: @tab || "always") @current_statuses.each { |status| f.hidden(name: "original_status_ids[]", value: status.id) } diff --git a/app/views/workflows/edit.html.erb b/app/views/workflows/edit.html.erb index 26a2de62642c..b7bdacd0a47e 100644 --- a/app/views/workflows/edit.html.erb +++ b/app/views/workflows/edit.html.erb @@ -28,7 +28,7 @@ See COPYRIGHT and LICENSE files for more details. ++#%> <% html_title t(:label_administration), t(:label_workflow_plural), @type.name -%> <% content_for :content_header do %> - <%= render Workflows::PageHeaders::EditComponent.new(@type, role: @role, tabs: workflow_tabs(@type)) %> + <%= render Workflows::PageHeaders::EditComponent.new(@type, roles: @roles, tabs: workflow_tabs(@type)) %> <% end %> <% if @type && @roles.any? %> diff --git a/app/views/workflows/tabs/edit.html.erb b/app/views/workflows/tabs/edit.html.erb index 61af4af9c6f4..cc5624dbda93 100644 --- a/app/views/workflows/tabs/edit.html.erb +++ b/app/views/workflows/tabs/edit.html.erb @@ -30,6 +30,6 @@ See COPYRIGHT and LICENSE files for more details. <%= turbo_frame_tag "workflow-table", data: { turbo_cache: false } do %> <%= render Workflows::StatusMatrixFormComponent.new(tab: @tab, roles: @roles, type: @type, available_roles: @eligible_roles, statuses: @statuses, has_status_changes: @has_status_changes) %> <%= turbo_stream.replace(Workflows::PageHeaders::EditComponent.wrapper_key) do %> - <%= render Workflows::PageHeaders::EditComponent.new(@type, role: @role, tabs: workflow_tabs(@type)) %> + <%= render Workflows::PageHeaders::EditComponent.new(@type, roles: @roles, tabs: workflow_tabs(@type)) %> <% end %> <% end %> diff --git a/spec/controllers/workflows_controller_spec.rb b/spec/controllers/workflows_controller_spec.rb index ccddd2983627..09696f87db46 100644 --- a/spec/controllers/workflows_controller_spec.rb +++ b/spec/controllers/workflows_controller_spec.rb @@ -112,9 +112,7 @@ .to render_template :edit end - # TODO: @role is a temporary single-role fallback; remove once components support multi-role natively - it "assigns the first role as fallback for @role, with @roles as the canonical collection" do - expect(assigns[:role]).to eq role + it "assigns @roles as the canonical collection" do expect(assigns[:roles]).to contain_exactly(role) end @@ -144,7 +142,6 @@ it "assigns the selected role" do expect(assigns[:roles]).to contain_exactly(role) - expect(assigns[:role]).to eq role end it "assigns type" do @@ -176,11 +173,6 @@ expect(assigns[:roles]).to contain_exactly(role, role2) end - # TODO: @role is a temporary single-role fallback; remove once components support multi-role natively - it "assigns the first role as a temporary single-role fallback for @role" do - expect(assigns[:role]).to eq role - end - it "assigns type" do expect(assigns[:type]).to eq type end diff --git a/spec/features/workflows/edit_multi_role_spec.rb b/spec/features/workflows/edit_multi_role_spec.rb index d4266d186efb..4214e949e2e7 100644 --- a/spec/features/workflows/edit_multi_role_spec.rb +++ b/spec/features/workflows/edit_multi_role_spec.rb @@ -251,4 +251,22 @@ expect_flash(message: "Successful update.") end end + + context "when modifying statuses" do + before { visit_workflow_edit(roles: [role, role2]) } + + it "preserves all selected roles after adding a status" do + add_status_via_dialog(statuses[2]) + + expect(page).to have_text("2 roles selected") + end + + it "preserves all selected roles after removing a status via the danger dialog" do + remove_status_via_dialog(statuses[1]) + + within_dialog("Remove statuses") { click_button "Remove" } + + expect(page).to have_text("2 roles selected") + end + end end From 939592a11d1bf0ce069a0eca50878c20e5a40d0d Mon Sep 17 00:00:00 2001 From: Mir Bhatia Date: Tue, 28 Apr 2026 13:02:27 +0200 Subject: [PATCH 08/12] Add more multi role specs --- .../workflows/edit_multi_role_spec.rb | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/spec/features/workflows/edit_multi_role_spec.rb b/spec/features/workflows/edit_multi_role_spec.rb index 4214e949e2e7..2e00914fcc30 100644 --- a/spec/features/workflows/edit_multi_role_spec.rb +++ b/spec/features/workflows/edit_multi_role_spec.rb @@ -103,6 +103,19 @@ expect(page).to have_field workflow_checkbox(0, 2) expect(page).to have_field workflow_checkbox(1, 2) end + + it "pre-selects the union of statuses from all selected roles in the status dialog" do + within "#workflow-table" do + click_link "Status" + end + + expect(page).to have_dialog("Statuses") + within_dialog "Statuses" do + expect(page).to have_css(".ng-value-label", text: statuses[0].name) + expect(page).to have_css(".ng-value-label", text: statuses[1].name) + expect(page).to have_css(".ng-value-label", text: statuses[2].name) + end + end end context "with a single role selected" do @@ -268,5 +281,39 @@ expect(page).to have_text("2 roles selected") end + + it "checks all new status checkboxes as fully checked, not indeterminate, when adding a status" do + add_status_via_dialog(statuses[2]) + + [workflow_checkbox(2, 0), workflow_checkbox(0, 2), workflow_checkbox(2, 1), + workflow_checkbox(1, 2), workflow_checkbox(2, 2)].each do |cb| + expect(page).to have_field cb, checked: true + expect(indeterminate?(cb)).to be false + end + end + + context "when no statuses are configured" do + let(:empty_role1) { create(:project_role) } + let(:empty_role2) { create(:project_role) } + + before { visit_workflow_edit(roles: [empty_role1, empty_role2]) } + + it "shows a blankslate" do + expect(page).to have_text("No status transitions configured") + end + + it "preserves all selected roles when adding statuses from the blankslate" do + within "#workflow-table" do + all(:link, "Status").last.click + end + within_dialog "Statuses" do + find(".ng-arrow-wrapper").click + find(".ng-option", text: statuses[0].name).click + click_button "Apply" + end + + expect(page).to have_text("2 roles selected") + end + end end end From 3c4d231cc1930c2c481dd1582b13c25cb22e5dc5 Mon Sep 17 00:00:00 2001 From: Mir Bhatia Date: Tue, 28 Apr 2026 13:47:27 +0200 Subject: [PATCH 09/12] Fix buggy case where nothing happens on unchecking all roles --- .../admin/workflow-role-select.controller.ts | 13 +++++++++++-- spec/features/workflows/edit_multi_role_spec.rb | 14 ++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/frontend/src/stimulus/controllers/dynamic/admin/workflow-role-select.controller.ts b/frontend/src/stimulus/controllers/dynamic/admin/workflow-role-select.controller.ts index 11c66b3d3832..2c231485991c 100644 --- a/frontend/src/stimulus/controllers/dynamic/admin/workflow-role-select.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/admin/workflow-role-select.controller.ts @@ -33,7 +33,6 @@ import type { SelectPanelElement } from '@primer/view-components/app/components/ import WorkflowCheckboxStateController from './workflow-checkbox-state.controller'; /** - * Mounted on the element for role selection in the workflow quick filters. * When the panel closes, it navigates to the workflow edit page with the selected role IDs. * Delegates dirty-state confirmation to the workflow-checkbox-state controller via an outlet. */ @@ -61,9 +60,19 @@ export default class WorkflowRoleSelectController extends Controller { .map((item) => item.getAttribute('data-item-id')) .filter(Boolean); - if (!selectedIds.length) return; + // For when all roles are deselected + if (!selectedIds.length) { + const url = new URL(this.baseUrlValue, window.location.origin); + if (this.hasAdminWorkflowCheckboxStateOutlet) { + this.adminWorkflowCheckboxStateOutlet.navigateTo(url.toString()); + } else { + Turbo.visit(url.toString()); + } + return; + } const currentIds = this.currentRoleIdsValue.split(',').filter(Boolean); + if (selectedIds.slice().sort().join(',') === currentIds.slice().sort().join(',')) return; const url = new URL(this.baseUrlValue, window.location.origin); diff --git a/spec/features/workflows/edit_multi_role_spec.rb b/spec/features/workflows/edit_multi_role_spec.rb index 2e00914fcc30..7d46f9a97382 100644 --- a/spec/features/workflows/edit_multi_role_spec.rb +++ b/spec/features/workflows/edit_multi_role_spec.rb @@ -265,6 +265,20 @@ end end + context "when deselecting all roles in the select panel" do + before { visit_workflow_edit(roles: [role, role2]) } + + it "falls back to the first eligible role instead of leaving the page stuck" do + click_button "2 roles selected" + find("[data-item-id='#{role.id}']").click + find("[data-item-id='#{role2.id}']").click + page.send_keys :escape + + expect(page).to have_no_text("2 roles selected") + expect(page).to have_button(role.name) + end + end + context "when modifying statuses" do before { visit_workflow_edit(roles: [role, role2]) } From eea4c36b7c6112df04b51a14500418dd542339e0 Mon Sep 17 00:00:00 2001 From: Mir Bhatia Date: Tue, 28 Apr 2026 15:48:41 +0200 Subject: [PATCH 10/12] Update navigation to ensure tab param isn't reset on updating roles --- .../workflows/status_matrix_form_component.rb | 2 +- app/controllers/workflows/tabs_controller.rb | 1 + .../workflow-checkbox-state.controller.ts | 19 ++++++++++++--- .../admin/workflow-role-select.controller.ts | 23 ++++++++++--------- 4 files changed, 30 insertions(+), 15 deletions(-) diff --git a/app/components/workflows/status_matrix_form_component.rb b/app/components/workflows/status_matrix_form_component.rb index 4fc301838e5d..d9e31dba4ab5 100644 --- a/app/components/workflows/status_matrix_form_component.rb +++ b/app/components/workflows/status_matrix_form_component.rb @@ -48,7 +48,7 @@ def initialize(tab:, roles:, type:, available_roles:, statuses:, has_status_chan def data_attributes { controller: "admin--workflow-role-select", - "admin--workflow-role-select-base-url-value": helpers.edit_workflow_path(@type), + "admin--workflow-role-select-base-url-value": helpers.edit_workflow_tab_path(@type, @tab), "admin--workflow-role-select-current-role-ids-value": @roles.map(&:id).join(","), "admin--workflow-role-select-admin--workflow-checkbox-state-outlet": "#workflow_form" } diff --git a/app/controllers/workflows/tabs_controller.rb b/app/controllers/workflows/tabs_controller.rb index 62d5d679d91c..a6c2c1757870 100644 --- a/app/controllers/workflows/tabs_controller.rb +++ b/app/controllers/workflows/tabs_controller.rb @@ -162,6 +162,7 @@ def set_eligible_roles def set_roles @roles = @eligible_roles.where(id: params[:role_ids]) + @roles = [@eligible_roles.first] if @roles.empty? end def statuses_for_form diff --git a/frontend/src/stimulus/controllers/dynamic/admin/workflow-checkbox-state.controller.ts b/frontend/src/stimulus/controllers/dynamic/admin/workflow-checkbox-state.controller.ts index 181e0f0dee63..e40218e878a9 100644 --- a/frontend/src/stimulus/controllers/dynamic/admin/workflow-checkbox-state.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/admin/workflow-checkbox-state.controller.ts @@ -313,7 +313,7 @@ export default class WorkflowCheckboxStateController extends Controller { Turbo.visit(url); }, 0); + setTimeout(() => { this.frameNavigateTo(url); }, 0); }, () => { this.element.requestSubmit(); this.confirmationDialogTarget.close(); // Delay to allow the flash message from the form submission to appear. - setTimeout(() => { Turbo.visit(url); }, 1000); + setTimeout(() => { this.frameNavigateTo(url); }, 1000); }, ); } + + // This keeps the url in the /tabs/:tab/edit format consistently, + // rather than doing a Turbo.visit which changes the format. + // It also keeps history usable, similar to data-turbo-action="advance". + private frameNavigateTo(url:string) { + const turboFrame = this.element.closest('turbo-frame') as HTMLElement | null; + if (turboFrame) { + turboFrame.setAttribute('src', url); + history.pushState({}, '', url); + } else { + Turbo.visit(url); + } + } } diff --git a/frontend/src/stimulus/controllers/dynamic/admin/workflow-role-select.controller.ts b/frontend/src/stimulus/controllers/dynamic/admin/workflow-role-select.controller.ts index 2c231485991c..75cbdd3dc650 100644 --- a/frontend/src/stimulus/controllers/dynamic/admin/workflow-role-select.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/admin/workflow-role-select.controller.ts @@ -62,26 +62,27 @@ export default class WorkflowRoleSelectController extends Controller { // For when all roles are deselected if (!selectedIds.length) { - const url = new URL(this.baseUrlValue, window.location.origin); - if (this.hasAdminWorkflowCheckboxStateOutlet) { - this.adminWorkflowCheckboxStateOutlet.navigateTo(url.toString()); - } else { - Turbo.visit(url.toString()); - } + this.navigateTo(this.buildUrl([])); return; } const currentIds = this.currentRoleIdsValue.split(',').filter(Boolean); - if (selectedIds.slice().sort().join(',') === currentIds.slice().sort().join(',')) return; + this.navigateTo(this.buildUrl(selectedIds as string[])); + }; + + private buildUrl(roleIds:string[]):string { const url = new URL(this.baseUrlValue, window.location.origin); - selectedIds.forEach((id) => url.searchParams.append('role_ids[]', id!)); + roleIds.forEach((id) => url.searchParams.append('role_ids[]', id)); + return url.toString(); + } + private navigateTo(url:string) { if (this.hasAdminWorkflowCheckboxStateOutlet) { - this.adminWorkflowCheckboxStateOutlet.navigateTo(url.toString()); + this.adminWorkflowCheckboxStateOutlet.navigateTo(url); } else { - Turbo.visit(url.toString()); + Turbo.visit(url); } - }; + } } From 9e0a7367fe020cef45e5446d702c366f321da145 Mon Sep 17 00:00:00 2001 From: Mir Bhatia Date: Tue, 28 Apr 2026 16:18:03 +0200 Subject: [PATCH 11/12] Refactor and clean up --- .../workflows/status_matrix_form_component.html.erb | 2 +- app/components/workflows/status_matrix_form_component.rb | 8 ++++++-- app/controllers/workflows_controller.rb | 4 ---- .../dynamic/admin/workflow-role-select.controller.ts | 7 +++---- 4 files changed, 10 insertions(+), 11 deletions(-) diff --git a/app/components/workflows/status_matrix_form_component.html.erb b/app/components/workflows/status_matrix_form_component.html.erb index cfeae1152656..b84b2e25fae1 100644 --- a/app/components/workflows/status_matrix_form_component.html.erb +++ b/app/components/workflows/status_matrix_form_component.html.erb @@ -77,7 +77,7 @@ See COPYRIGHT and LICENSE files for more details. <% if @statuses.any? %> <%= form_tag( workflow_tab_path(@type), - id: "workflow_form", + id: form_id, method: :patch, autocomplete: "off", data: { diff --git a/app/components/workflows/status_matrix_form_component.rb b/app/components/workflows/status_matrix_form_component.rb index d9e31dba4ab5..5af6570d32ff 100644 --- a/app/components/workflows/status_matrix_form_component.rb +++ b/app/components/workflows/status_matrix_form_component.rb @@ -33,6 +33,8 @@ class StatusMatrixFormComponent < ApplicationComponent include OpTurbo::Streamable include OpPrimer::ComponentHelpers + FORM_ID = "workflow_form" + def initialize(tab:, roles:, type:, available_roles:, statuses:, has_status_changes:) super @tab = tab @@ -45,12 +47,14 @@ def initialize(tab:, roles:, type:, available_roles:, statuses:, has_status_chan private + def form_id = FORM_ID + def data_attributes { controller: "admin--workflow-role-select", "admin--workflow-role-select-base-url-value": helpers.edit_workflow_tab_path(@type, @tab), - "admin--workflow-role-select-current-role-ids-value": @roles.map(&:id).join(","), - "admin--workflow-role-select-admin--workflow-checkbox-state-outlet": "#workflow_form" + "admin--workflow-role-select-current-role-ids-value": @roles.map(&:id), + "admin--workflow-role-select-admin--workflow-checkbox-state-outlet": "##{form_id}" } end end diff --git a/app/controllers/workflows_controller.rb b/app/controllers/workflows_controller.rb index 90650d70b8aa..3de82fd8d54e 100644 --- a/app/controllers/workflows_controller.rb +++ b/app/controllers/workflows_controller.rb @@ -56,10 +56,6 @@ def find_types @types = ::Type.order(:position) end - def find_role - @role = eligible_roles.find(params[:role_id]) - end - def find_type @type = ::Type.find(params[:type_id]) end diff --git a/frontend/src/stimulus/controllers/dynamic/admin/workflow-role-select.controller.ts b/frontend/src/stimulus/controllers/dynamic/admin/workflow-role-select.controller.ts index 75cbdd3dc650..bbf429027fe6 100644 --- a/frontend/src/stimulus/controllers/dynamic/admin/workflow-role-select.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/admin/workflow-role-select.controller.ts @@ -38,12 +38,12 @@ import WorkflowCheckboxStateController from './workflow-checkbox-state.controlle */ export default class WorkflowRoleSelectController extends Controller { static outlets = ['admin--workflow-checkbox-state']; - static values = { baseUrl: String, currentRoleIds: String }; + static values = { baseUrl: String, currentRoleIds: Array }; declare readonly adminWorkflowCheckboxStateOutlet:WorkflowCheckboxStateController; declare readonly hasAdminWorkflowCheckboxStateOutlet:boolean; declare baseUrlValue:string; - declare currentRoleIdsValue:string; + declare currentRoleIdsValue:unknown[]; connect() { this.element.addEventListener('panelClosed', this.handlePanelClosed); @@ -66,8 +66,7 @@ export default class WorkflowRoleSelectController extends Controller { return; } - const currentIds = this.currentRoleIdsValue.split(',').filter(Boolean); - if (selectedIds.slice().sort().join(',') === currentIds.slice().sort().join(',')) return; + if (selectedIds.slice().sort().join(',') === this.currentRoleIdsValue.slice().sort().join(',')) return; this.navigateTo(this.buildUrl(selectedIds as string[])); }; From 9b1303a9406dba3685bf824580bca312b6cb1b28 Mon Sep 17 00:00:00 2001 From: Mir Bhatia Date: Tue, 28 Apr 2026 16:46:51 +0200 Subject: [PATCH 12/12] Update SelectPanel variant --- .../status_matrix_form_component.html.erb | 8 ++++++++ .../admin/workflow-role-select.controller.ts | 14 +++----------- spec/features/workflows/edit_multi_role_spec.rb | 2 +- spec/support/workflows/edit_helpers.rb | 2 +- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/app/components/workflows/status_matrix_form_component.html.erb b/app/components/workflows/status_matrix_form_component.html.erb index b84b2e25fae1..68ca0b219a19 100644 --- a/app/components/workflows/status_matrix_form_component.html.erb +++ b/app/components/workflows/status_matrix_form_component.html.erb @@ -57,6 +57,14 @@ See COPYRIGHT and LICENSE files for more details. item_id: available_role.id ) end + panel.with_footer(show_divider: true) do + render( + Primer::Beta::Button.new( + scheme: :primary, + data: { action: "click->admin--workflow-role-select#apply" } + ) + ) { t(:button_save) } + end end end end diff --git a/frontend/src/stimulus/controllers/dynamic/admin/workflow-role-select.controller.ts b/frontend/src/stimulus/controllers/dynamic/admin/workflow-role-select.controller.ts index bbf429027fe6..d81f1ff94b1b 100644 --- a/frontend/src/stimulus/controllers/dynamic/admin/workflow-role-select.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/admin/workflow-role-select.controller.ts @@ -45,15 +45,7 @@ export default class WorkflowRoleSelectController extends Controller { declare baseUrlValue:string; declare currentRoleIdsValue:unknown[]; - connect() { - this.element.addEventListener('panelClosed', this.handlePanelClosed); - } - - disconnect() { - this.element.removeEventListener('panelClosed', this.handlePanelClosed); - } - - private handlePanelClosed = () => { + apply() { const panel = this.element as HTMLElement as SelectPanelElement; const selectedIds = panel.items .filter((item) => panel.isItemChecked(item)) @@ -69,8 +61,8 @@ export default class WorkflowRoleSelectController extends Controller { if (selectedIds.slice().sort().join(',') === this.currentRoleIdsValue.slice().sort().join(',')) return; this.navigateTo(this.buildUrl(selectedIds as string[])); - }; - + } + private buildUrl(roleIds:string[]):string { const url = new URL(this.baseUrlValue, window.location.origin); roleIds.forEach((id) => url.searchParams.append('role_ids[]', id)); diff --git a/spec/features/workflows/edit_multi_role_spec.rb b/spec/features/workflows/edit_multi_role_spec.rb index 7d46f9a97382..19f7871aa820 100644 --- a/spec/features/workflows/edit_multi_role_spec.rb +++ b/spec/features/workflows/edit_multi_role_spec.rb @@ -272,7 +272,7 @@ click_button "2 roles selected" find("[data-item-id='#{role.id}']").click find("[data-item-id='#{role2.id}']").click - page.send_keys :escape + within("select-panel") { click_button "Save" } expect(page).to have_no_text("2 roles selected") expect(page).to have_button(role.name) diff --git a/spec/support/workflows/edit_helpers.rb b/spec/support/workflows/edit_helpers.rb index 8a80b9708cdb..7b30cd14bc4b 100644 --- a/spec/support/workflows/edit_helpers.rb +++ b/spec/support/workflows/edit_helpers.rb @@ -45,7 +45,7 @@ def switch_role_via_panel(from_role, to_role) click_button from_role.name find("[data-item-id='#{to_role.id}']").click find("[data-item-id='#{from_role.id}']").click - page.send_keys :escape + within("select-panel") { click_button "Save" } end def add_status_via_dialog(status)