From 6580addcde4c186f922a84087dc376d55d70a41b Mon Sep 17 00:00:00 2001 From: Mir Bhatia Date: Mon, 27 Jul 2026 13:50:02 +0200 Subject: [PATCH 01/20] Hook up workflows submit to wizard continue button --- .../wizard/footer_component.html.erb | 5 ++- .../wizard/footer_component.rb | 21 +++++++++++ .../wizard/page_component.html.erb | 16 ++++---- .../wizard/page_component.rb | 2 + .../wizard/page_component.sass | 4 ++ .../status_matrix_form_component.html.erb | 1 + app/controllers/workflows/tabs_controller.rb | 37 +++++++++++++------ spec/features/types/creation_wizard_spec.rb | 2 + .../types/creation_wizard_workflows_spec.rb | 11 ++---- 9 files changed, 71 insertions(+), 28 deletions(-) diff --git a/app/components/work_package_types/wizard/footer_component.html.erb b/app/components/work_package_types/wizard/footer_component.html.erb index 7b85e9678a12..7f731b2c33d2 100644 --- a/app/components/work_package_types/wizard/footer_component.html.erb +++ b/app/components/work_package_types/wizard/footer_component.html.erb @@ -30,8 +30,9 @@ <%= render( Primer::Beta::Button.new( type: :submit, - form: WorkPackageTypes::Wizard::FooterComponent::FORM_IDENTIFIER, - scheme: :primary + form: primary_action_form, + scheme: :primary, + **primary_action_arguments ) ) { primary_action_label } %> diff --git a/app/components/work_package_types/wizard/footer_component.rb b/app/components/work_package_types/wizard/footer_component.rb index c95422a8a0d9..e075464d890b 100644 --- a/app/components/work_package_types/wizard/footer_component.rb +++ b/app/components/work_package_types/wizard/footer_component.rb @@ -55,6 +55,27 @@ def first_step? = current_step == Steps.first def last_step? = current_step == Steps.last + # The workflows step embeds the shared matrix form, which persists in place through + # its own endpoint. Continue submits that form with the next step, so the endpoint + # persists and then redirects to advance the wizard (see Workflows::TabsController). + # Escaping the matrix's turbo frame turns that redirect into a full navigation. + # Other steps submit the wizard form directly. + def primary_action_form + workflows_step? ? Workflows::StatusMatrixFormComponent::FORM_ID : FORM_IDENTIFIER + end + + def primary_action_arguments + return {} unless workflows_step? + + { + name: "advance_to_step", + value: Steps.next_after(current_step), + data: { turbo_frame: "_top" } + } + end + + def workflows_step? = current_step == :workflows + def current_number = Steps.index(current_step) + 1 def total_steps = Steps.all.size diff --git a/app/components/work_package_types/wizard/page_component.html.erb b/app/components/work_package_types/wizard/page_component.html.erb index 5879793508b2..8371598bfd20 100644 --- a/app/components/work_package_types/wizard/page_component.html.erb +++ b/app/components/work_package_types/wizard/page_component.html.erb @@ -31,14 +31,16 @@ <% end %> <% end %> <% else %> - <%# Self-persisting editor: this form only advances to the next step on submit. %> <%= render(step_body) %> - <%= primer_form_with( - url: step_url, - method: :patch, - html: { id: WorkPackageTypes::Wizard::FooterComponent::FORM_IDENTIFIER } - ) do %> - <%= tag.input(type: :hidden, name: :step, value: current_step) %> + <% unless step_submits_own_form? %> + <%# Self-persisting editor: this form only advances to the next step on submit. %> + <%= primer_form_with( + url: step_url, + method: :patch, + html: { id: WorkPackageTypes::Wizard::FooterComponent::FORM_IDENTIFIER } + ) do %> + <%= tag.input(type: :hidden, name: :step, value: current_step) %> + <% end %> <% end %> <% end %> <% end %> diff --git a/app/components/work_package_types/wizard/page_component.rb b/app/components/work_package_types/wizard/page_component.rb index 157fd3bb4754..12c3c399c1f6 100644 --- a/app/components/work_package_types/wizard/page_component.rb +++ b/app/components/work_package_types/wizard/page_component.rb @@ -121,6 +121,8 @@ def reuse_mode_banner render(WorkPackageTypes::ReuseModeBannerComponent.new(type:, aspect: step_editor.aspect)) end + def step_submits_own_form? = current_step == :workflows + # Editors that self-persist through their own turbo endpoints. def step_body case current_step diff --git a/app/components/work_package_types/wizard/page_component.sass b/app/components/work_package_types/wizard/page_component.sass index 8e370c9a6f41..2e0cac5efc28 100644 --- a/app/components/work_package_types/wizard/page_component.sass +++ b/app/components/work_package_types/wizard/page_component.sass @@ -54,3 +54,7 @@ &--sidebar display: none + + // The wizard footer's Continue button persists the workflow matrix + .workflow-save-bar + display: none diff --git a/app/components/workflows/status_matrix_form_component.html.erb b/app/components/workflows/status_matrix_form_component.html.erb index 613b1138013b..e27920e63fa5 100644 --- a/app/components/workflows/status_matrix_form_component.html.erb +++ b/app/components/workflows/status_matrix_form_component.html.erb @@ -186,5 +186,6 @@ See COPYRIGHT and LICENSE files for more details. <% end %> <% else %> <%= render Workflows::BlankslateComponent.new(roles: @roles, type: @type, tab: @tab) %> + <%= form_tag(type_creation_wizard_path(@type, step: :workflows), id: form_id, method: :patch) do %><% end %> <% end %> <% end %> diff --git a/app/controllers/workflows/tabs_controller.rb b/app/controllers/workflows/tabs_controller.rb index 596f425d9874..2c87e8e4c891 100644 --- a/app/controllers/workflows/tabs_controller.rb +++ b/app/controllers/workflows/tabs_controller.rb @@ -54,18 +54,10 @@ def edit end 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(role_params) - success = false unless result.success? - end - raise ActiveRecord::Rollback unless success + success = persist_matrix + + if success && params[:advance_to_step].present? + return redirect_to type_creation_wizard_path(@type, step: params[:advance_to_step]), status: :see_other end if success @@ -199,6 +191,27 @@ def workflows_for_form @workflows["assignee"] = workflows.select(&:assignee) end + # A linked type reuses its source's transitions and must never have its own + # rewritten, so it persists nothing and simply reports success. + def persist_matrix + return true if @type.linked?(Type::ConfigurationLink::WORKFLOWS) + + 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(role_params) + success = false unless result.success? + end + raise ActiveRecord::Rollback unless success + end + success + end + def permitted_status_params status_params("status") end diff --git a/spec/features/types/creation_wizard_spec.rb b/spec/features/types/creation_wizard_spec.rb index 01338c2e8068..b2c286e7d449 100644 --- a/spec/features/types/creation_wizard_spec.rb +++ b/spec/features/types/creation_wizard_spec.rb @@ -33,6 +33,7 @@ RSpec.describe "Variant creation wizard", :js, with_flag: { type_variants: true } do shared_let(:admin) { create(:admin) } shared_let(:bug_type) { create(:type, name: "Bug", color: create(:color)) } + shared_let(:project_role) { create(:project_role) } before { login_as(admin) } @@ -98,6 +99,7 @@ def complete_details_step(name) expect_step_saved(:project_attributes) # Step 5 - Workflow + expect(page).to have_css("#workflow_form", visible: :all) click_on I18n.t(:button_continue) expect_step_saved(:workflows) diff --git a/spec/features/types/creation_wizard_workflows_spec.rb b/spec/features/types/creation_wizard_workflows_spec.rb index 2cba7e0ab8d3..59cc9e9e7748 100644 --- a/spec/features/types/creation_wizard_workflows_spec.rb +++ b/spec/features/types/creation_wizard_workflows_spec.rb @@ -63,7 +63,7 @@ def visit_workflow_wizard(roles: [], tab: nil) visit type_creation_wizard_path(type, **params) end - it "allows adding another workflow" do + it "persists the matrix and advances when clicking 'Continue'" do visit_workflow_wizard(roles: [role]) expect(page).to have_field workflow_checkbox(1, 0), checked: false @@ -71,13 +71,10 @@ def visit_workflow_wizard(roles: [], tab: nil) check workflow_checkbox(1, 0) - click_button "Save" - - expect_flash(message: "Successful update.") - - expect(page).to have_field workflow_checkbox(0, 1), checked: true - expect(page).to have_field workflow_checkbox(1, 0), checked: true + expect(page).to have_no_button "Save" + click_on I18n.t(:button_continue) + expect(page).to have_current_path(type_creation_wizard_path(type, step: :projects)) expect(Workflow.where(type_id: type.id, role_id: role.id).count).to be 2 end From 99618df3eb1cf76367d4e0a51339a3b05933bb43 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 28 Jul 2026 12:01:18 +0200 Subject: [PATCH 02/20] Extract persisting of the Matrix into a service --- app/controllers/workflows/tabs_controller.rb | 61 +----- .../workflows/matrix_update_service.rb | 122 ++++++++++++ .../workflows/tabs_controller_spec.rb | 99 +++++----- .../workflows/matrix_update_service_spec.rb | 179 ++++++++++++++++++ 4 files changed, 353 insertions(+), 108 deletions(-) create mode 100644 app/services/workflows/matrix_update_service.rb create mode 100644 spec/services/workflows/matrix_update_service_spec.rb diff --git a/app/controllers/workflows/tabs_controller.rb b/app/controllers/workflows/tabs_controller.rb index 2c87e8e4c891..888fcf719131 100644 --- a/app/controllers/workflows/tabs_controller.rb +++ b/app/controllers/workflows/tabs_controller.rb @@ -54,7 +54,7 @@ def edit end def update # rubocop:disable Metrics/AbcSize - success = persist_matrix + success = persist_matrix.success? if success && params[:advance_to_step].present? return redirect_to type_creation_wizard_path(@type, step: params[:advance_to_step]), status: :see_other @@ -191,62 +191,9 @@ def workflows_for_form @workflows["assignee"] = workflows.select(&:assignee) end - # A linked type reuses its source's transitions and must never have its own - # rewritten, so it persists nothing and simply reports success. def persist_matrix - return true if @type.linked?(Type::ConfigurationLink::WORKFLOWS) - - 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(role_params) - success = false unless result.success? - end - raise ActiveRecord::Rollback unless success - end - success - end - - def permitted_status_params - status_params("status") - end - - def permitted_indeterminate_params - status_params("indeterminate_status") - end - - def status_params(key) - return {} if params[key].blank? - - params[key] - .to_unsafe_h - .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 + Workflows::MatrixUpdateService + .new(type: @type, roles: @roles, tab: @tab) + .call(status: params[:status], indeterminate_status: params[:indeterminate_status]) end end diff --git a/app/services/workflows/matrix_update_service.rb b/app/services/workflows/matrix_update_service.rb new file mode 100644 index 000000000000..326b883d7f9a --- /dev/null +++ b/app/services/workflows/matrix_update_service.rb @@ -0,0 +1,122 @@ +# 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 + # Rewrites one tab of a type's transition matrix for every selected role, delegating + # the per-role write to BulkUpdateService so that either all roles are updated or none. + # + # Accepts the matrix straight from the submitted form, so that every context showing + # the matrix — the workflow tab, the variant creation wizard — persists it identically. + class MatrixUpdateService + def initialize(type:, roles:, tab:) + @type = type + @roles = roles + @tab = tab + end + + # A linked type reuses its source's transitions and must never have its own + # rewritten, so it persists nothing and simply reports success. + def call(status: nil, indeterminate_status: nil) + return ServiceResult.success if type.linked?(Type::ConfigurationLink::WORKFLOWS) + + persist(transitions: matrix(status), indeterminate: matrix(indeterminate_status)) + end + + private + + attr_reader :type, :roles, :tab + + def persist(transitions:, indeterminate:) + results = [] + + Workflow.transaction do + results = roles.map { update_role(it, transitions, indeterminate) } + raise ActiveRecord::Rollback unless results.all?(&:success?) + end + + results.find(&:failure?) || ServiceResult.success + end + + def update_role(role, transitions, indeterminate) + role_transitions = if indeterminate.empty? + transitions + else + restore_indeterminate(transitions, indeterminate, role) + end + + BulkUpdateService.new(role:, type:, tab:).call(role_transitions) + end + + # With several roles selected, a transition only some of them share renders as an + # indeterminate checkbox, which submits as unchecked. Writing that back would drop the + # transition for the roles that had it, so those pairs are restored from the database. + def restore_indeterminate(transitions, indeterminate, role) + transitions.deep_dup.tap do |restored| + indeterminate.each do |old_status_id, new_status_ids| + new_status_ids.each_key do |new_status_id| + next unless transition_exists?(role, old_status_id, new_status_id) + + restored[old_status_id] ||= {} + restored[old_status_id][new_status_id] = "1" + end + end + end + end + + def transition_exists?(role, old_status_id, new_status_id) + Workflow.exists?(role_id: role.id, + type_id: type.id, + old_status_id: old_status_id.to_i, + new_status_id: new_status_id.to_i, + author: author?, + assignee: assignee?) + end + + # The matrix names its checkboxes status[old_status_id][new_status_id], so anything + # that is not nested under a pair of numeric keys did not come from the rendered form. + def matrix(source) + return {} if source.blank? + + to_hash(source).select do |old_status_id, new_status_ids| + numeric?(old_status_id) && new_status_ids.respond_to?(:keys) && new_status_ids.keys.all? { numeric?(it) } + end + end + + def to_hash(source) + source.respond_to?(:to_unsafe_h) ? source.to_unsafe_h : source.to_h + end + + def numeric?(key) = /\A\d+\z/.match?(key.to_s) + + def author? = tab == "author" + + def assignee? = tab == "assignee" + end +end diff --git a/spec/controllers/workflows/tabs_controller_spec.rb b/spec/controllers/workflows/tabs_controller_spec.rb index 9a1d046e0225..7a83e3cb4b36 100644 --- a/spec/controllers/workflows/tabs_controller_spec.rb +++ b/spec/controllers/workflows/tabs_controller_spec.rb @@ -184,71 +184,68 @@ describe "#update" do let(:status_params) { { "1" => { "2" => ["always"] } } } let(:call_result) { ServiceResult.success } - - 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 + let(:roles) { [role] } + + let(:service) do + instance_double(Workflows::MatrixUpdateService, call: call_result).tap do |dbl| + allow(Workflows::MatrixUpdateService) + .to receive(:new) + .with(type: type, roles: roles, 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], type_id: type.id, tab: "always", status: status_params }, - format: :turbo_stream - end + def submit_matrix + service + allow(controller).to receive(:statuses_for_form).and_return([build_stubbed(:status)]) + + post :update, + params: { + role_ids: roles.map { it.id.to_s }, + 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 + it "hands the submitted matrix to the service and renders a flash turbo stream" do + submit_matrix + + expect(service).to have_received(:call).with( + status: satisfy { it.to_unsafe_h == status_params }, + indeterminate_status: nil + ) + expect(response).to have_turbo_stream action: "flash", target: "op-primer-flash-component" end 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 + let(:roles) { [role, role2] } 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], - type_id: type.id, - tab: "always", - status: status_params - }, - format: :turbo_stream + .and_return(roles) 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) + it "passes every selected role to the service" do + submit_matrix + + expect(Workflows::MatrixUpdateService) + .to have_received(:new).with(type: type, roles: roles, tab: "always") + expect(response).to have_turbo_stream action: "flash", target: "op-primer-flash-component" + end + end + + context "when the service fails" do + let(:call_result) { ServiceResult.failure } + + it "responds unprocessable with a danger flash" do + submit_matrix + + expect(response).to have_http_status(:unprocessable_entity) expect(response).to have_turbo_stream action: "flash", target: "op-primer-flash-component" end end diff --git a/spec/services/workflows/matrix_update_service_spec.rb b/spec/services/workflows/matrix_update_service_spec.rb new file mode 100644 index 000000000000..803955ac4a7b --- /dev/null +++ b/spec/services/workflows/matrix_update_service_spec.rb @@ -0,0 +1,179 @@ +# 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 Workflows::MatrixUpdateService, type: :model do + shared_let(:type) { create(:type) } + shared_let(:role) { create(:project_role) } + shared_let(:other_role) { create(:project_role) } + shared_let(:status_a) { create(:status) } + shared_let(:status_b) { create(:status) } + shared_let(:status_c) { create(:status) } + + subject(:service_call) { instance.call(**call_params) } + + let(:instance) { described_class.new(type:, roles:, tab:) } + let(:roles) { [role] } + let(:tab) { "always" } + let(:call_params) { { status: } } + let(:status) { { status_a.id.to_s => { status_b.id.to_s => ["always"] } } } + + def transitions_for(a_role, author: false, assignee: false) + Workflow + .where(type_id: type.id, role_id: a_role.id, author:, assignee:) + .pluck(:old_status_id, :new_status_id) + end + + it "persists the submitted transitions" do + expect(service_call).to be_success + expect(transitions_for(role)).to contain_exactly([status_a.id, status_b.id]) + end + + it "replaces transitions that are no longer submitted" do + create(:workflow, role_id: role.id, type_id: type.id, + old_status_id: status_b.id, new_status_id: status_c.id) + + expect(service_call).to be_success + expect(transitions_for(role)).to contain_exactly([status_a.id, status_b.id]) + end + + context "with several roles selected" do + let(:roles) { [role, other_role] } + + it "persists the same transitions for each role" do + expect(service_call).to be_success + + expect(transitions_for(role)).to contain_exactly([status_a.id, status_b.id]) + expect(transitions_for(other_role)).to contain_exactly([status_a.id, status_b.id]) + end + + context "when a transition is indeterminate because only one role has it" do + let(:call_params) do + { status:, indeterminate_status: { status_b.id.to_s => { status_c.id.to_s => "1" } } } + end + + before do + create(:workflow, role_id: role.id, type_id: type.id, + old_status_id: status_b.id, new_status_id: status_c.id) + end + + it "keeps the transition for the role that had it and does not add it to the others" do + expect(service_call).to be_success + + expect(transitions_for(role)) + .to contain_exactly([status_a.id, status_b.id], [status_b.id, status_c.id]) + expect(transitions_for(other_role)) + .to contain_exactly([status_a.id, status_b.id]) + end + end + + context "when one role fails to persist" do + before do + allow(Workflows::BulkUpdateService) + .to receive(:new).and_call_original + + allow(Workflows::BulkUpdateService) + .to receive(:new).with(role: other_role, type:, tab:) + .and_return(instance_double(Workflows::BulkUpdateService, call: ServiceResult.failure)) + end + + it "rolls back the roles that did succeed" do + expect(service_call).to be_failure + expect(transitions_for(role)).to be_empty + end + end + end + + describe "tabs" do + let(:tab) { "author" } + + it "writes the transitions against the tab's flag only" do + expect(service_call).to be_success + + expect(transitions_for(role, author: true)).to contain_exactly([status_a.id, status_b.id]) + expect(transitions_for(role)).to be_empty + end + end + + describe "when the workflows aspect is linked to a source type" do + shared_let(:source_type) { create(:type) } + + before { type.link!(Type::ConfigurationLink::WORKFLOWS, source: source_type) } + after { type.configuration_links.destroy_all } + + it "persists nothing and reports success" do + expect(service_call).to be_success + expect(transitions_for(role)).to be_empty + end + end + + describe "matrix sanitizing" do + context "with ActionController::Parameters" do + let(:status) do + ActionController::Parameters.new(status_a.id.to_s => { status_b.id.to_s => ["always"] }) + end + + it "persists the transitions" do + expect(service_call).to be_success + expect(transitions_for(role)).to contain_exactly([status_a.id, status_b.id]) + end + end + + context "with non-numeric keys" do + let(:status) do + { + status_a.id.to_s => { status_b.id.to_s => ["always"] }, + "utf8" => { "✓" => "1" }, + status_b.id.to_s => "not-a-nested-hash" + } + end + + it "ignores everything that did not come from the rendered matrix" do + expect(service_call).to be_success + expect(transitions_for(role)).to contain_exactly([status_a.id, status_b.id]) + end + end + + context "with no matrix submitted at all" do + let(:call_params) { {} } + + before do + create(:workflow, role_id: role.id, type_id: type.id, + old_status_id: status_a.id, new_status_id: status_b.id) + end + + it "clears the tab's transitions" do + expect(service_call).to be_success + expect(transitions_for(role)).to be_empty + end + end + end +end From b496c7817796b01fa8f4b4b364bbf9aa1d6e1dff Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 28 Jul 2026 12:25:00 +0200 Subject: [PATCH 03/20] Extract the workflows matrix template into a view component --- .../matrix_table_component.html.erb} | 70 ++++---- .../workflows/matrix_table_component.rb | 114 +++++++++++++ .../status_matrix_form_component.html.erb | 4 +- .../workflows/status_matrix_form_component.rb | 22 ++- app/controllers/workflows/tabs_controller.rb | 8 +- app/helpers/workflow_helper.rb | 1 - app/views/workflows/tabs/edit.html.erb | 2 +- .../workflows/matrix_table_component_spec.rb | 157 ++++++++++++++++++ 8 files changed, 328 insertions(+), 50 deletions(-) rename app/{views/workflows/_form.html.erb => components/workflows/matrix_table_component.html.erb} (71%) create mode 100644 app/components/workflows/matrix_table_component.rb create mode 100644 spec/components/workflows/matrix_table_component_spec.rb diff --git a/app/views/workflows/_form.html.erb b/app/components/workflows/matrix_table_component.html.erb similarity index 71% rename from app/views/workflows/_form.html.erb rename to app/components/workflows/matrix_table_component.html.erb index 0b32ff07bb20..70ae85a136b4 100644 --- a/app/views/workflows/_form.html.erb +++ b/app/components/workflows/matrix_table_component.html.erb @@ -27,36 +27,28 @@ See COPYRIGHT and LICENSE files for more details. ++#%> -<% name = tab[:name] %> -<% workflows = @workflows[name] %> -<% read_only = workflow_linked?(@type) %> +<% if heading %> + <%= render(Primer::OpenProject::Heading.new(tag: :h3, my: 3)) { heading } %> +<% end %> -<%= - if name == "assignee" - render(Primer::OpenProject::Heading.new(tag: :h3, my: 3)) { t(:label_additional_workflow_transitions_for_assignee) } - elsif name == "author" - render(Primer::OpenProject::Heading.new(tag: :h3, my: 3)) { t(:label_additional_workflow_transitions_for_author) } - end -%> - -
+
- <% @statuses.size.times do |role| %> + <% statuses.size.times do %> <% end %> - - <% @statuses.each do |new_status| %> + <% statuses.each do |new_status| %> - <% @statuses.each do |old_status| %> + <% statuses.each do |old_status| %> - <% @statuses.each do |new_status| -%> - <% 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 %> + <% statuses.each do |new_status| -%> + <% indeterminate = indeterminate?(old_status, new_status) %> <% end %> - diff --git a/app/components/workflows/matrix_table_component.rb b/app/components/workflows/matrix_table_component.rb new file mode 100644 index 000000000000..13ecc0e76697 --- /dev/null +++ b/app/components/workflows/matrix_table_component.rb @@ -0,0 +1,114 @@ +# 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 + # One transition tab's matrix: a checkbox per (current status, new status) pair, + # named status[old_status_id][new_status_id] so that MatrixUpdateService can rewrite + # the tab from the submitted form. + # + # Renders the inputs only — the surrounding form belongs to whoever embeds the matrix. + class MatrixTableComponent < ApplicationComponent + include OpPrimer::ComponentHelpers + + def initialize(tab:, statuses:, workflows:, roles:, added_status_ids: [], readonly: false) + super() + + @tab = tab + @statuses = statuses + @workflows = workflows + @roles = roles + @added_status_ids = added_status_ids + @readonly = readonly + end + + private + + attr_reader :tab, :statuses, :roles, :added_status_ids + + def readonly? = @readonly + + def dom_id = "workflow_form_#{tab}" + + def heading + case tab + when "assignee" then t(:label_additional_workflow_transitions_for_assignee) + when "author" then t(:label_additional_workflow_transitions_for_author) + end + end + + def caption + t("workflows.form.matrix_caption_#{tab}", default: t("workflows.form.matrix_caption")) + end + + def checkbox_label(old_status, new_status) + t("workflows.form.matrix_checkbox_label", old_status: old_status.name, new_status: new_status.name) + end + + # The `_html` keys carry an around the status name, so they must go through the + # ActionView translate helper that marks them safe — I18n.t alone would escape it into + # the button's accessible name. + def column_toggle_label(new_status) + t("workflows.form.matrix_check_uncheck_all_in_col_label_html", new_status: new_status.name) + end + + def row_toggle_label(old_status) + t("workflows.form.matrix_check_uncheck_all_in_row_label_html", old_status: old_status.name) + end + + def checked?(old_status, new_status) + return false if indeterminate?(old_status, new_status) + + allowed_role_ids(old_status, new_status).any? || newly_added?(old_status, new_status) + end + + # Only some of the selected roles allow this transition, so a plain checkbox cannot + # represent it. A status that was only just added is exempt: no role can have it yet, + # so it starts out checked for all of them instead. + def indeterminate?(old_status, new_status) + role_ids = allowed_role_ids(old_status, new_status) + + role_ids.any? && role_ids.size < roles.size && !newly_added?(old_status, new_status) + end + + def allowed_role_ids(old_status, new_status) + role_ids_by_transition.fetch([old_status.id, new_status.id], []) + end + + def role_ids_by_transition + @role_ids_by_transition ||= @workflows + .group_by { [it.old_status_id, it.new_status_id] } + .transform_values { it.map(&:role_id).uniq } + end + + def newly_added?(old_status, new_status) + added_status_ids.include?(old_status.id) || added_status_ids.include?(new_status.id) + end + end +end diff --git a/app/components/workflows/status_matrix_form_component.html.erb b/app/components/workflows/status_matrix_form_component.html.erb index e27920e63fa5..d5c79517520f 100644 --- a/app/components/workflows/status_matrix_form_component.html.erb +++ b/app/components/workflows/status_matrix_form_component.html.erb @@ -28,8 +28,6 @@ See COPYRIGHT and LICENSE files for more details. ++#%> <%= component_wrapper do %> - <% transition_tabs = helpers.workflow_tabs(@type) %> - <% current_transition_tab = helpers.selected_tab(transition_tabs) %> <%= render Primer::OpenProject::SubHeader.new do |subheader| subheader.with_filter_component do @@ -144,7 +142,7 @@ See COPYRIGHT and LICENSE files for more details. <% end %> <%= hidden_field_tag "tab", @tab %> - <%= helpers.render_tabs helpers.workflow_tabs(@type) %> + <%= render(matrix_table) %> <% unless read_only? %>
diff --git a/app/components/workflows/status_matrix_form_component.rb b/app/components/workflows/status_matrix_form_component.rb index b451b3d726cc..4f3ab7bd6230 100644 --- a/app/components/workflows/status_matrix_form_component.rb +++ b/app/components/workflows/status_matrix_form_component.rb @@ -35,7 +35,8 @@ class StatusMatrixFormComponent < ApplicationComponent FORM_ID = "workflow_form" - def initialize(tab:, roles:, type:, available_roles:, statuses:, has_status_changes:) + def initialize(tab:, roles:, type:, available_roles:, statuses:, has_status_changes:, + workflows: {}, added_status_ids: []) super @tab = tab @roles = roles @@ -43,6 +44,8 @@ def initialize(tab:, roles:, type:, available_roles:, statuses:, has_status_chan @available_roles = available_roles @statuses = statuses @has_status_changes = has_status_changes + @workflows = workflows || {} + @added_status_ids = added_status_ids || [] end private @@ -51,6 +54,23 @@ def form_id = FORM_ID def read_only? = helpers.workflow_linked?(@type) + def transition_tabs = @transition_tabs ||= helpers.workflow_tabs(@type) + + # The transition tab that is actually on screen. Not necessarily @tab, which is the + # raw request param and is blank when the matrix is opened without one. + def current_transition_tab = @current_transition_tab ||= helpers.selected_tab(transition_tabs) + + def matrix_table + MatrixTableComponent.new( + tab: current_transition_tab[:name], + statuses: @statuses, + workflows: @workflows.fetch(current_transition_tab[:name], []), + roles: @roles, + added_status_ids: @added_status_ids, + readonly: read_only? + ) + end + def data_attributes { controller: "admin--workflow-role-select", diff --git a/app/controllers/workflows/tabs_controller.rb b/app/controllers/workflows/tabs_controller.rb index 888fcf719131..67d9ecabc46b 100644 --- a/app/controllers/workflows/tabs_controller.rb +++ b/app/controllers/workflows/tabs_controller.rb @@ -75,7 +75,9 @@ def update # rubocop:disable Metrics/AbcSize type: @type, available_roles: @eligible_roles, statuses:, - has_status_changes: @has_status_changes + has_status_changes: @has_status_changes, + workflows: @workflows, + added_status_ids: @added_status_ids ) ) end @@ -131,7 +133,9 @@ def confirm_statuses # rubocop:disable Metrics/AbcSize type: @type, available_roles: @eligible_roles, statuses:, - has_status_changes: @has_status_changes + has_status_changes: @has_status_changes, + workflows: @workflows, + added_status_ids: @added_status_ids ) ) respond_with_turbo_streams diff --git a/app/helpers/workflow_helper.rb b/app/helpers/workflow_helper.rb index b424238b62fb..6d37dfb25528 100644 --- a/app/helpers/workflow_helper.rb +++ b/app/helpers/workflow_helper.rb @@ -46,7 +46,6 @@ def workflow_tabs(type) description: I18n.t(:"admin.workflows.tabs.descriptions.user_assignee") } ].map do |tab| tab.merge( - partial: "workflows/form", path: edit_type_workflow_tab_path(type, tab[:name], params.permit(role_ids: [])), data: { controller: "admin--workflow-tab-select", action: "click->admin--workflow-tab-select#select", diff --git a/app/views/workflows/tabs/edit.html.erb b/app/views/workflows/tabs/edit.html.erb index ae7e0450f4e9..ba37f550d396 100644 --- a/app/views/workflows/tabs/edit.html.erb +++ b/app/views/workflows/tabs/edit.html.erb @@ -28,5 +28,5 @@ 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) %> + <%= render Workflows::StatusMatrixFormComponent.new(tab: @tab, roles: @roles, type: @type, available_roles: @eligible_roles, statuses: @statuses, has_status_changes: @has_status_changes, workflows: @workflows, added_status_ids: @added_status_ids) %> <% end %> diff --git a/spec/components/workflows/matrix_table_component_spec.rb b/spec/components/workflows/matrix_table_component_spec.rb new file mode 100644 index 000000000000..96a0eb75f63f --- /dev/null +++ b/spec/components/workflows/matrix_table_component_spec.rb @@ -0,0 +1,157 @@ +# 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 "rails_helper" + +RSpec.describe Workflows::MatrixTableComponent, type: :component do + let(:status_a) { build_stubbed(:status, name: "New") } + let(:status_b) { build_stubbed(:status, name: "In progress") } + let(:statuses) { [status_a, status_b] } + + let(:role) { build_stubbed(:project_role) } + let(:other_role) { build_stubbed(:project_role) } + let(:roles) { [role] } + + let(:workflows) { [] } + let(:added_status_ids) { [] } + let(:readonly) { false } + let(:tab) { "always" } + + subject(:rendered_component) do + render_inline( + described_class.new(tab:, statuses:, workflows:, roles:, added_status_ids:, readonly:) + ) + end + + def transition(old_status:, new_status:, for_role: role) + build_stubbed(:workflow, role_id: for_role.id, old_status_id: old_status.id, new_status_id: new_status.id) + end + + # Primer renders array-scheme checkboxes with a trailing [] on the name + def checkbox_for(old_status:, new_status:) + rendered_component.at_css("input[name='status[#{old_status.id}][#{new_status.id}][]'][type=checkbox]") + end + + def indeterminate_marker_for(old_status:, new_status:) + rendered_component.at_css("input[name='indeterminate_status[#{old_status.id}][#{new_status.id}]'][type=hidden]") + end + + it "renders a checkbox for every ordered pair of statuses" do + expect(rendered_component.css("input[type=checkbox]").size).to eq(statuses.size**2) + expect(checkbox_for(old_status: status_a, new_status: status_b)).to be_present + end + + it "keys the table by tab so several tabs can coexist in one form" do + expect(rendered_component.at_css("#workflow_form_always")).to be_present + expect(rendered_component.at_css("table")["class"]).to include("transitions-always") + end + + describe "checked state" do + let(:workflows) { [transition(old_status: status_a, new_status: status_b)] } + + it "checks the pairs the role allows and leaves the others unchecked" do + expect(checkbox_for(old_status: status_a, new_status: status_b)["checked"]).to be_present + expect(checkbox_for(old_status: status_b, new_status: status_a)["checked"]).to be_nil + end + + it "submits no indeterminate marker" do + expect(indeterminate_marker_for(old_status: status_a, new_status: status_b)).to be_nil + end + end + + describe "with several roles selected" do + let(:roles) { [role, other_role] } + + context "when only some of them allow a transition" do + let(:workflows) { [transition(old_status: status_a, new_status: status_b, for_role: role)] } + + it "renders the pair indeterminate and marks it so saving preserves it" do + checkbox = checkbox_for(old_status: status_a, new_status: status_b) + + expect(checkbox["checked"]).to be_nil + expect(checkbox["data-indeterminate"]).to eq("true") + expect(indeterminate_marker_for(old_status: status_a, new_status: status_b)).to be_present + end + end + + context "when all of them allow a transition" do + let(:workflows) do + [ + transition(old_status: status_a, new_status: status_b, for_role: role), + transition(old_status: status_a, new_status: status_b, for_role: other_role) + ] + end + + it "renders the pair plainly checked" do + expect(checkbox_for(old_status: status_a, new_status: status_b)["checked"]).to be_present + expect(indeterminate_marker_for(old_status: status_a, new_status: status_b)).to be_nil + end + end + + context "when a status was only just added" do + let(:workflows) { [transition(old_status: status_a, new_status: status_b, for_role: role)] } + let(:added_status_ids) { [status_b.id] } + + it "pre-checks its pairs for every role rather than showing them indeterminate" do + checkbox = checkbox_for(old_status: status_a, new_status: status_b) + + expect(checkbox["checked"]).to be_present + expect(checkbox["data-indeterminate"]).to be_nil + expect(indeterminate_marker_for(old_status: status_a, new_status: status_b)).to be_nil + end + end + end + + describe "readonly" do + let(:readonly) { true } + + it "disables every checkbox and drops the bulk toggles" do + expect(rendered_component.css("input[type=checkbox]:not([disabled])")).to be_empty + expect(rendered_component).to have_no_button("Check all") + expect(rendered_component).to have_no_button("Uncheck all") + end + end + + describe "tab headings" do + context "with the author tab" do + let(:tab) { "author" } + + it "names the transitions as additional to the default ones" do + expect(rendered_component).to have_css("h3", text: I18n.t(:label_additional_workflow_transitions_for_author)) + end + end + + context "with the always tab" do + it "renders no heading" do + expect(rendered_component).to have_no_css("h3") + end + end + end +end From f86022c5bd49bbb697dccd09ee450deb9117cf22 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 28 Jul 2026 12:43:43 +0200 Subject: [PATCH 04/20] Introduce a MatrixContext to wrap the setup for the workflows editor --- .../status_matrix_form_component.html.erb | 50 ++--- .../workflows/status_matrix_form_component.rb | 40 ++-- app/controllers/workflows/tabs_controller.rb | 196 +++++++----------- app/models/workflows/matrix_context.rb | 126 +++++++++++ app/views/workflows/tabs/edit.html.erb | 2 +- .../workflows/tabs_controller_spec.rb | 10 +- spec/models/workflows/matrix_context_spec.rb | 163 +++++++++++++++ 7 files changed, 413 insertions(+), 174 deletions(-) create mode 100644 app/models/workflows/matrix_context.rb create mode 100644 spec/models/workflows/matrix_context_spec.rb diff --git a/app/components/workflows/status_matrix_form_component.html.erb b/app/components/workflows/status_matrix_form_component.html.erb index d5c79517520f..05976b5a277f 100644 --- a/app/components/workflows/status_matrix_form_component.html.erb +++ b/app/components/workflows/status_matrix_form_component.html.erb @@ -42,21 +42,21 @@ See COPYRIGHT and LICENSE files for more details. helpers.tab_label(current_transition_tab) end - transition_tabs.each do |tab| + transition_tabs.each do |transition_tab| menu.with_item( - label: helpers.tab_label(tab), + label: helpers.tab_label(transition_tab), tag: :a, - href: tab[:path], - active: current_transition_tab == tab, - content_arguments: { data: tab[:data] } + href: transition_tab[:path], + active: transition_tab[:name] == tab, + content_arguments: { data: transition_tab[:data] } ) do |item| - item.with_description { tab[:description] } + item.with_description { transition_tab[:description] } end end end end - if @type && @available_roles.any? + if eligible_roles.any? flex.with_column(ml: 2) do render( Primer::Alpha::SelectPanel.new( @@ -68,18 +68,18 @@ See COPYRIGHT and LICENSE files for more details. ) do |panel| panel.with_show_button(scheme: :secondary) do |button| button.with_trailing_visual_icon(icon: :"triangle-down") - if @roles.many? - t("admin.workflows.role_selector.roles", count: @roles.size) - elsif @roles.one? - t("admin.workflows.role_selector.label", role: @roles.first.name) + if roles.many? + t("admin.workflows.role_selector.roles", count: roles.size) + elsif roles.one? + t("admin.workflows.role_selector.label", role: roles.first.name) else t("admin.workflows.role_selector.no_role") end end - @available_roles.each do |available_role| + eligible_roles.each do |available_role| panel.with_item( label: available_role.name, - active: @roles.include?(available_role), + active: roles.include?(available_role), item_id: available_role.id ) end @@ -97,13 +97,13 @@ See COPYRIGHT and LICENSE files for more details. end end - unless read_only? + unless readonly? subheader.with_action_button( tag: :a, scheme: :secondary, leading_icon: :plus, label: t("admin.workflows.status_button"), - href: helpers.status_dialog_type_workflow_tab_path(@type, @tab, role_ids: @roles.map(&:id), status_ids: @statuses.pluck(:id).presence), + href: helpers.status_dialog_type_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") @@ -114,7 +114,7 @@ See COPYRIGHT and LICENSE files for more details. scheme: :secondary, leading_icon: :copy, label: t(:button_copy), - href: helpers.new_type_workflow_copy_path(@type, source_role_id: @roles.first&.id), + href: helpers.new_type_workflow_copy_path(type, source_role_id: roles.first&.id), aria: { label: t(:button_copy) }, title: t(:button_copy), data: { controller: "async-dialog", "admin--workflow-checkbox-state-confirmation-trigger": "click" } @@ -125,26 +125,26 @@ See COPYRIGHT and LICENSE files for more details. end %> - <% if @statuses.any? %> + <% if statuses.any? %> <%= form_tag( - type_workflow_tab_path(@type), + type_workflow_tab_path(type, tab), id: form_id, method: :patch, autocomplete: "off", data: { controller: "admin--workflow-checkbox-state", - "admin--workflow-checkbox-state-has-status-changes-value": @has_status_changes + "admin--workflow-checkbox-state-has-status-changes-value": context.status_changes? } ) do %> - <%= hidden_field_tag "type_id", @type.id %> - <% @roles.each do |role| %> + <%= hidden_field_tag "type_id", type.id %> + <% roles.each do |role| %> <%= hidden_field_tag "role_ids[]", role.id %> <% end %> - <%= hidden_field_tag "tab", @tab %> + <%= hidden_field_tag "tab", tab %> <%= render(matrix_table) %> - <% unless read_only? %> + <% unless readonly? %>
<%= render Primer::Beta::Button.new(scheme: :primary, type: :submit, mt: 3, ml: 3, mb: 4) do @@ -183,7 +183,7 @@ See COPYRIGHT and LICENSE files for more details. <% end %> <% end %> <% else %> - <%= render Workflows::BlankslateComponent.new(roles: @roles, type: @type, tab: @tab) %> - <%= form_tag(type_creation_wizard_path(@type, step: :workflows), id: form_id, method: :patch) do %><% end %> + <%= render Workflows::BlankslateComponent.new(roles:, type:, tab:) %> + <%= form_tag(type_creation_wizard_path(type, step: :workflows), id: form_id, method: :patch) do %><% end %> <% end %> <% end %> diff --git a/app/components/workflows/status_matrix_form_component.rb b/app/components/workflows/status_matrix_form_component.rb index 4f3ab7bd6230..e6e3014f5f5c 100644 --- a/app/components/workflows/status_matrix_form_component.rb +++ b/app/components/workflows/status_matrix_form_component.rb @@ -35,47 +35,39 @@ class StatusMatrixFormComponent < ApplicationComponent FORM_ID = "workflow_form" - def initialize(tab:, roles:, type:, available_roles:, statuses:, has_status_changes:, - workflows: {}, added_status_ids: []) + def initialize(context:) super - @tab = tab - @roles = roles - @type = type - @available_roles = available_roles - @statuses = statuses - @has_status_changes = has_status_changes - @workflows = workflows || {} - @added_status_ids = added_status_ids || [] + @context = context end private - def form_id = FORM_ID + attr_reader :context + + delegate :type, :tab, :roles, :eligible_roles, :statuses, :readonly?, to: :context - def read_only? = helpers.workflow_linked?(@type) + def form_id = FORM_ID - def transition_tabs = @transition_tabs ||= helpers.workflow_tabs(@type) + def transition_tabs = @transition_tabs ||= helpers.workflow_tabs(type) - # The transition tab that is actually on screen. Not necessarily @tab, which is the - # raw request param and is blank when the matrix is opened without one. - def current_transition_tab = @current_transition_tab ||= helpers.selected_tab(transition_tabs) + def current_transition_tab = transition_tabs.find { it[:name] == tab } def matrix_table MatrixTableComponent.new( - tab: current_transition_tab[:name], - statuses: @statuses, - workflows: @workflows.fetch(current_transition_tab[:name], []), - roles: @roles, - added_status_ids: @added_status_ids, - readonly: read_only? + tab:, + statuses:, + workflows: context.workflows, + roles:, + added_status_ids: context.added_status_ids, + readonly: readonly? ) end def data_attributes { controller: "admin--workflow-role-select", - "admin--workflow-role-select-base-url-value": helpers.edit_type_workflow_tab_path(@type, @tab), - "admin--workflow-role-select-current-role-ids-value": @roles.map(&:id), + "admin--workflow-role-select-base-url-value": helpers.edit_type_workflow_tab_path(type, tab), + "admin--workflow-role-select-current-role-ids-value": roles.map(&:id), "admin--workflow-role-select-admin--workflow-checkbox-state-outlet": "##{form_id}" } end diff --git a/app/controllers/workflows/tabs_controller.rb b/app/controllers/workflows/tabs_controller.rb index 67d9ecabc46b..6cea7eb69524 100644 --- a/app/controllers/workflows/tabs_controller.rb +++ b/app/controllers/workflows/tabs_controller.rb @@ -35,169 +35,123 @@ class Workflows::TabsController < ApplicationController before_action :require_admin - before_action :set_type - before_action :set_tab - before_action :set_eligible_roles - before_action :set_roles + helper_method :matrix_context def edit unless turbo_frame_request? - redirect_to edit_type_workflow_path(@type, role_ids: params[:role_ids], tab: @tab) - return - end - - statuses_for_form - - if @type && @roles.any? && @statuses.any? - workflows_for_form + redirect_to edit_type_workflow_path(type, role_ids: params[:role_ids], tab: matrix_context.tab) end end - def update # rubocop:disable Metrics/AbcSize - success = persist_matrix.success? - - if success && params[:advance_to_step].present? - return redirect_to type_creation_wizard_path(@type, step: params[:advance_to_step]), status: :see_other - end + def update + if persist_matrix.success? + return redirect_to_wizard_step if advance_to_wizard_step? - if success - render_flash_message_via_turbo_stream( - message: I18n.t(:notice_successful_update), - scheme: :success - ) - statuses = statuses_for_form - if statuses.empty? - # Need to replace with the blankslate. - update_via_turbo_stream( - component: Workflows::StatusMatrixFormComponent.new( - tab: @tab, - roles: @roles, - type: @type, - available_roles: @eligible_roles, - statuses:, - has_status_changes: @has_status_changes, - workflows: @workflows, - added_status_ids: @added_status_ids - ) - ) - end + render_matrix_saved else - render_flash_message_via_turbo_stream( - message: I18n.t(:notice_unsuccessful_update), - scheme: :danger - ) - @turbo_status = :unprocessable_entity + render_matrix_not_saved end respond_with_turbo_streams end 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 && @roles.any? - statuses_for_roles_and_type - else - Status.none - end - respond_with_dialog Workflows::StatusDialogComponent.new( - all_statuses:, - current_statuses:, - roles: @roles, - type: @type, - tab: @tab + all_statuses: Status.order(:position), + current_statuses: dialog_statuses, + roles: matrix_context.roles, + type:, + tab: matrix_context.tab ) end - def confirm_statuses # rubocop:disable Metrics/AbcSize - current_status_ids = Array(params[:status_ids]).flatten.map(&:to_i) - original_ids = Array(params[:original_status_ids]).flatten.map(&:to_i) - removed_count = (original_ids - current_status_ids).size - - if removed_count > 0 + def confirm_statuses + if removed_status_count.positive? respond_with_dialog Workflows::StatusRemovalDangerDialogComponent.new( - roles: @roles, - type: @type, - tab: @tab, - status_ids: current_status_ids, - removed_count: removed_count + roles: matrix_context.roles, + type:, + tab: matrix_context.tab, + status_ids: requested_status_ids, + removed_count: removed_status_count ) else - statuses = statuses_for_form.tap { workflows_for_form } - update_via_turbo_stream( - component: Workflows::StatusMatrixFormComponent.new( - tab: @tab, - roles: @roles, - type: @type, - available_roles: @eligible_roles, - statuses:, - has_status_changes: @has_status_changes, - workflows: @workflows, - added_status_ids: @added_status_ids - ) - ) + update_via_turbo_stream(component: matrix_form_component) respond_with_turbo_streams end end private - def set_type - @type = ::Type.find(params.expect(:type_id)) + def type + @type ||= ::Type.find(params.expect(:type_id)) end - def set_tab - @tab = params[:tab] + def matrix_context + @matrix_context ||= build_matrix_context end - def set_eligible_roles - @eligible_roles = Workflow.ordered_eligible_roles + def build_matrix_context + Workflows::MatrixContext.new( + type:, + tab: params[:tab], + role_ids: params[:role_ids], + status_ids: params[:status_ids] + ) end - def set_roles - @roles = Workflow.selected_roles(params[:role_ids]) + def matrix_form_component(context = matrix_context) + Workflows::StatusMatrixFormComponent.new(context:) end - def statuses_for_form - @added_status_ids = [] - @has_status_changes = false - @statuses = if @type && params[:status_ids].present? - statuses_from_params - elsif @type && @roles.any? - statuses_for_roles_and_type - elsif @type - @type.statuses - else - Status.all - end + def persist_matrix + Workflows::MatrixUpdateService + .new(type:, roles: matrix_context.roles, tab: matrix_context.tab) + .call(status: params[:status], indeterminate_status: params[:indeterminate_status]) end - def statuses_from_params - status_ids = params[:status_ids].map(&:to_i) - 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) + def render_matrix_saved + render_flash_message_via_turbo_stream( + message: I18n.t(:notice_successful_update), + scheme: :success + ) + + # Resolved afresh: the write just changed which statuses have transitions, and with + # none left the matrix has to give way to the blankslate. + saved = build_matrix_context + update_via_turbo_stream(component: matrix_form_component(saved)) if saved.statuses.empty? end - 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) + def render_matrix_not_saved + render_flash_message_via_turbo_stream( + message: I18n.t(:notice_unsuccessful_update), + scheme: :danger + ) + @turbo_status = :unprocessable_entity end - def workflows_for_form - workflows = @type.workflows.where(role_id: @roles.map(&:id)) - @workflows = {} - @workflows["always"] = workflows.select { |w| !w.author && !w.assignee } - @workflows["author"] = workflows.select(&:author) - @workflows["assignee"] = workflows.select(&:assignee) + # Temporary: the wizard still submits the matrix through this endpoint and asks it to + # advance afterwards. Goes away once the wizard persists the step itself. + def advance_to_wizard_step? = params[:advance_to_step].present? + + def redirect_to_wizard_step + redirect_to type_creation_wizard_path(type, step: params[:advance_to_step]), status: :see_other end - def persist_matrix - Workflows::MatrixUpdateService - .new(type: @type, roles: @roles, tab: @tab) - .call(status: params[:status], indeterminate_status: params[:indeterminate_status]) + def requested_status_ids + @requested_status_ids ||= Array(params[:status_ids]).flatten.map(&:to_i) + end + + def removed_status_count + original_ids = Array(params[:original_status_ids]).flatten.map(&:to_i) + + (original_ids - requested_status_ids).size + end + + # The dialog opens on the pending selection when there is one, otherwise on what the + # selected roles have saved — and on nothing at all while no role is selected. + def dialog_statuses + return Status.none if requested_status_ids.blank? && matrix_context.roles.empty? + + matrix_context.statuses end end diff --git a/app/models/workflows/matrix_context.rb b/app/models/workflows/matrix_context.rb new file mode 100644 index 000000000000..bce51212b2f6 --- /dev/null +++ b/app/models/workflows/matrix_context.rb @@ -0,0 +1,126 @@ +# 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 + # Resolves everything the transition matrix needs in order to render: which transition + # tab is on screen, the roles it covers, the statuses forming its axes and the + # transitions that already exist. + # + # Every host of the matrix builds one of these from its own request — the type's + # workflow tab, the variant creation wizard — so that none of them has to reproduce + # the resolution rules, and all of them agree on what is being edited. + class MatrixContext + TABS = %w[always author assignee].freeze + DEFAULT_TAB = "always" + + attr_reader :type + + def initialize(type:, tab: nil, role_ids: nil, status_ids: nil) + @type = type + @requested_tab = tab + @requested_role_ids = role_ids + @status_ids = status_ids + end + + # Normalised, so that rendering, the status query and persistence cannot disagree + # about which tab is being edited. The raw value is absent whenever the matrix is + # opened without one. + def tab + @tab ||= TABS.include?(@requested_tab.to_s) ? @requested_tab.to_s : DEFAULT_TAB + end + + # A linked type shows its source's transitions and cannot edit them. + def readonly? = type.linked?(Type::ConfigurationLink::WORKFLOWS) + + def eligible_roles + @eligible_roles ||= Workflow.ordered_eligible_roles + end + + def roles + @roles ||= Workflow.selected_roles(@requested_role_ids) + end + + # The axes of the matrix: a pending selection if the status dialog submitted one, + # otherwise whatever the selected roles already have transitions for. + def statuses + @statuses ||= if requested_status_ids.present? + Status.where(id: requested_status_ids).order(:position) + elsif roles.any? + Status.where(id: saved_status_ids) + else + type.statuses + end + end + + # Statuses the dialog added but which have no transitions yet, so the matrix can + # pre-check their pairs instead of rendering them indeterminate. + def added_status_ids + return [] if requested_status_ids.blank? + + @added_status_ids ||= requested_status_ids - saved_status_ids + end + + # Whether the pending selection differs from what is saved, which the matrix uses to + # warn before navigating away. + def status_changes? + return false if requested_status_ids.blank? + + added_status_ids.any? || (saved_status_ids - requested_status_ids).any? + end + + # The existing transitions of the selected roles, narrowed to the tab on screen. + def workflows + @workflows ||= type + .workflows + .where(role_id: roles.map(&:id)) + .select { belongs_to_tab?(it) } + end + + private + + def belongs_to_tab?(workflow) + case tab + when "author" then workflow.author + when "assignee" then workflow.assignee + else !workflow.author && !workflow.assignee + end + end + + def requested_status_ids + @requested_status_ids ||= Array(@status_ids).flatten.map(&:to_i) + end + + # The baseline a pending selection is compared against: always what the selected roles + # have saved, never the pending selection itself. + def saved_status_ids + @saved_status_ids ||= roles.flat_map { type.statuses(role: it, tab:).pluck(:id) }.uniq + end + end +end diff --git a/app/views/workflows/tabs/edit.html.erb b/app/views/workflows/tabs/edit.html.erb index ba37f550d396..4c5872888dac 100644 --- a/app/views/workflows/tabs/edit.html.erb +++ b/app/views/workflows/tabs/edit.html.erb @@ -28,5 +28,5 @@ 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, workflows: @workflows, added_status_ids: @added_status_ids) %> + <%= render Workflows::StatusMatrixFormComponent.new(context: matrix_context) %> <% end %> diff --git a/spec/controllers/workflows/tabs_controller_spec.rb b/spec/controllers/workflows/tabs_controller_spec.rb index 7a83e3cb4b36..0dcc5fee0b85 100644 --- a/spec/controllers/workflows/tabs_controller_spec.rb +++ b/spec/controllers/workflows/tabs_controller_spec.rb @@ -138,8 +138,6 @@ context "when no statuses were removed" do before do - allow(controller).to receive(:statuses_for_form).and_return([]) - allow(controller).to receive(:workflows_for_form) allow(controller).to receive(:update_via_turbo_stream) allow(controller).to receive(:respond_with_turbo_streams) @@ -195,9 +193,15 @@ end end + # Statuses remain, so the response is the flash alone — the blankslate replacement is + # covered by the feature specs. + let(:matrix_context) do + instance_double(Workflows::MatrixContext, roles:, tab: "always", statuses: [build_stubbed(:status)]) + end + def submit_matrix service - allow(controller).to receive(:statuses_for_form).and_return([build_stubbed(:status)]) + allow(controller).to receive(:build_matrix_context).and_return(matrix_context) post :update, params: { diff --git a/spec/models/workflows/matrix_context_spec.rb b/spec/models/workflows/matrix_context_spec.rb new file mode 100644 index 000000000000..2523f5592d50 --- /dev/null +++ b/spec/models/workflows/matrix_context_spec.rb @@ -0,0 +1,163 @@ +# 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 Workflows::MatrixContext do + shared_let(:type) { create(:type) } + shared_let(:role) { create(:project_role) } + shared_let(:other_role) { create(:project_role) } + shared_let(:status_a) { create(:status) } + shared_let(:status_b) { create(:status) } + shared_let(:status_c) { create(:status) } + + subject(:context) { described_class.new(type:, **options) } + + let(:options) { { role_ids: [role.id] } } + + def create_transition(old_status:, new_status:, for_role: role, **flags) + create(:workflow, role_id: for_role.id, type_id: type.id, + old_status_id: old_status.id, new_status_id: new_status.id, **flags) + end + + describe "#tab" do + it "defaults to the always tab when none was requested" do + expect(context.tab).to eq("always") + end + + it "keeps a known tab" do + expect(described_class.new(type:, tab: "author").tab).to eq("author") + expect(described_class.new(type:, tab: "assignee").tab).to eq("assignee") + end + + it "falls back to the always tab for anything unrecognised" do + expect(described_class.new(type:, tab: "bogus").tab).to eq("always") + end + end + + describe "#roles" do + it "resolves the requested roles" do + expect(context.roles).to contain_exactly(role) + end + + it "falls back to the first eligible role when none was requested" do + expect(described_class.new(type:).roles).to contain_exactly(Workflow.ordered_eligible_roles.first) + end + end + + describe "#statuses" do + before { create_transition(old_status: status_a, new_status: status_b) } + + it "spans the statuses the selected roles have transitions for" do + expect(context.statuses).to contain_exactly(status_a, status_b) + end + + it "is narrowed to the tab on screen" do + author_context = described_class.new(type:, tab: "author", role_ids: [role.id]) + + expect(author_context.statuses).to be_empty + end + + context "when the status dialog submitted a selection" do + let(:options) { { role_ids: [role.id], status_ids: [status_a.id, status_c.id] } } + + it "uses the submitted selection instead" do + expect(context.statuses).to contain_exactly(status_a, status_c) + end + + it "reports the ones without transitions as newly added" do + expect(context.added_status_ids).to contain_exactly(status_c.id) + end + + it "reports that the selection differs from what is saved" do + expect(context).to be_status_changes + end + end + + context "when the submitted selection matches what is saved" do + let(:options) { { role_ids: [role.id], status_ids: [status_a.id, status_b.id] } } + + it "reports no additions and no changes" do + expect(context.added_status_ids).to be_empty + expect(context).not_to be_status_changes + end + end + + context "with no selection submitted" do + it "reports no additions and no changes" do + expect(context.added_status_ids).to be_empty + expect(context).not_to be_status_changes + end + end + end + + describe "#workflows" do + before do + create_transition(old_status: status_a, new_status: status_b) + create_transition(old_status: status_b, new_status: status_c, author: true) + create_transition(old_status: status_a, new_status: status_c, for_role: other_role) + end + + it "returns only the selected roles' transitions for the tab on screen" do + expect(context.workflows.map { [it.old_status_id, it.new_status_id] }) + .to contain_exactly([status_a.id, status_b.id]) + end + + it "returns the author transitions on the author tab" do + author_context = described_class.new(type:, tab: "author", role_ids: [role.id]) + + expect(author_context.workflows.map { [it.old_status_id, it.new_status_id] }) + .to contain_exactly([status_b.id, status_c.id]) + end + + it "covers every selected role" do + both = described_class.new(type:, role_ids: [role.id, other_role.id]) + + expect(both.workflows.map(&:role_id)).to contain_exactly(role.id, other_role.id) + end + end + + describe "#readonly?" do + it "is false while the type owns its workflows" do + expect(context).not_to be_readonly + end + + context "when the workflows aspect is linked to a source type" do + shared_let(:source_type) { create(:type) } + + before { type.link!(Type::ConfigurationLink::WORKFLOWS, source: source_type) } + after { type.configuration_links.destroy_all } + + it "is true" do + expect(context).to be_readonly + end + end + end +end From 1030a133d43897b2a5510d4dd5ca6180eb8be5a9 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 28 Jul 2026 12:57:30 +0200 Subject: [PATCH 05/20] Add a Matrix controller that independently from the rendered context modifies the matrix editor --- .../wizard/footer_component.rb | 2 +- .../wizard/workflows_step_component.html.erb | 2 +- .../workflows/blankslate_component.html.erb | 2 +- .../workflows/status_form_component.html.erb | 2 +- .../status_matrix_form_component.html.erb | 4 ++-- .../workflows/status_matrix_form_component.rb | 2 +- ...s_removal_danger_dialog_component.html.erb | 2 +- .../workflows/copies/from_roles_controller.rb | 2 +- ...abs_controller.rb => matrix_controller.rb} | 8 +++++-- app/helpers/workflow_helper.rb | 2 +- .../workflow_tab/edit.html.erb | 2 +- .../edit.html.erb => matrix/show.html.erb} | 0 config/routes.rb | 11 +++++----- .../workflow-checkbox-state.controller.ts | 7 +++++-- ...ller_spec.rb => matrix_controller_spec.rb} | 10 ++++----- ...ts_spec.rb => matrix_mount_points_spec.rb} | 4 ++-- spec/routing/types_spec.rb | 21 ++++++++++++------- 17 files changed, 48 insertions(+), 35 deletions(-) rename app/controllers/workflows/{tabs_controller.rb => matrix_controller.rb} (92%) rename app/views/workflows/{tabs/edit.html.erb => matrix/show.html.erb} (100%) rename spec/controllers/workflows/{tabs_controller_spec.rb => matrix_controller_spec.rb} (98%) rename spec/requests/workflows/{tabs_mount_points_spec.rb => matrix_mount_points_spec.rb} (92%) diff --git a/app/components/work_package_types/wizard/footer_component.rb b/app/components/work_package_types/wizard/footer_component.rb index e075464d890b..5a5fa883888a 100644 --- a/app/components/work_package_types/wizard/footer_component.rb +++ b/app/components/work_package_types/wizard/footer_component.rb @@ -57,7 +57,7 @@ def last_step? = current_step == Steps.last # The workflows step embeds the shared matrix form, which persists in place through # its own endpoint. Continue submits that form with the next step, so the endpoint - # persists and then redirects to advance the wizard (see Workflows::TabsController). + # persists and then redirects to advance the wizard (see Workflows::MatrixController). # Escaping the matrix's turbo frame turns that redirect into a full navigation. # Other steps submit the wizard form directly. def primary_action_form diff --git a/app/components/work_package_types/wizard/workflows_step_component.html.erb b/app/components/work_package_types/wizard/workflows_step_component.html.erb index c4a317a4735b..446e95d2ea3a 100644 --- a/app/components/work_package_types/wizard/workflows_step_component.html.erb +++ b/app/components/work_package_types/wizard/workflows_step_component.html.erb @@ -31,6 +31,6 @@ See COPYRIGHT and LICENSE files for more details. <%= render(WorkPackageTypes::ReuseModeBannerComponent.new(type:, aspect: Type::ConfigurationLink::WORKFLOWS)) %> <% if roles.any? %> - <%= helpers.turbo_frame_tag "workflow-table", src: helpers.edit_type_workflow_tab_path(type, current_tab, role_ids: roles.map(&:id)) %> + <%= helpers.turbo_frame_tag "workflow-table", src: helpers.type_workflow_matrix_path(type, tab: current_tab, role_ids: roles.map(&:id), status_ids: helpers.params[:status_ids]) %> <% end %> <% end %> diff --git a/app/components/workflows/blankslate_component.html.erb b/app/components/workflows/blankslate_component.html.erb index c96914eb9cbb..2915fa9864e3 100644 --- a/app/components/workflows/blankslate_component.html.erb +++ b/app/components/workflows/blankslate_component.html.erb @@ -33,7 +33,7 @@ See COPYRIGHT and LICENSE files for more details. blankslate.with_description_content(t(description_key)) unless read_only? blankslate.with_primary_action( - href: helpers.status_dialog_type_workflow_tab_path(@type, @tab, role_ids: @roles.map(&:id)), + href: helpers.status_dialog_type_workflow_matrix_path(@type, tab: @tab, role_ids: @roles.map(&: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 fc7294164b5b..271eb4daec09 100644 --- a/app/components/workflows/status_form_component.html.erb +++ b/app/components/workflows/status_form_component.html.erb @@ -29,7 +29,7 @@ See COPYRIGHT and LICENSE files for more details. <%= primer_form_with( - url: helpers.confirm_statuses_type_workflow_tab_path(@type, @tab, role_ids: @roles.map(&:id)), + url: helpers.confirm_statuses_type_workflow_matrix_path(@type, tab: @tab, role_ids: @roles.map(&: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 05976b5a277f..0c6ea570ddcf 100644 --- a/app/components/workflows/status_matrix_form_component.html.erb +++ b/app/components/workflows/status_matrix_form_component.html.erb @@ -103,7 +103,7 @@ See COPYRIGHT and LICENSE files for more details. scheme: :secondary, leading_icon: :plus, label: t("admin.workflows.status_button"), - href: helpers.status_dialog_type_workflow_tab_path(type, tab, role_ids: roles.map(&:id), status_ids: statuses.pluck(:id).presence), + href: helpers.status_dialog_type_workflow_matrix_path(type, tab:, role_ids: roles.map(&:id), status_ids: statuses.pluck(:id).presence), data: { controller: "async-dialog" } ) do t("admin.workflows.status_button") @@ -127,7 +127,7 @@ See COPYRIGHT and LICENSE files for more details. <% if statuses.any? %> <%= form_tag( - type_workflow_tab_path(type, tab), + type_workflow_matrix_path(type, tab:), id: form_id, method: :patch, autocomplete: "off", diff --git a/app/components/workflows/status_matrix_form_component.rb b/app/components/workflows/status_matrix_form_component.rb index e6e3014f5f5c..09840de40796 100644 --- a/app/components/workflows/status_matrix_form_component.rb +++ b/app/components/workflows/status_matrix_form_component.rb @@ -66,7 +66,7 @@ def matrix_table def data_attributes { controller: "admin--workflow-role-select", - "admin--workflow-role-select-base-url-value": helpers.edit_type_workflow_tab_path(type, tab), + "admin--workflow-role-select-base-url-value": helpers.type_workflow_matrix_path(type, tab:), "admin--workflow-role-select-current-role-ids-value": roles.map(&:id), "admin--workflow-role-select-admin--workflow-checkbox-state-outlet": "##{form_id}" } 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 3e587660e380..ba5075334c92 100644 --- a/app/components/workflows/status_removal_danger_dialog_component.html.erb +++ b/app/components/workflows/status_removal_danger_dialog_component.html.erb @@ -33,7 +33,7 @@ See COPYRIGHT and LICENSE files for more details. id: DIALOG_ID, title: t("admin.workflows.statuses_removal_dialog.title"), confirm_button_text: t("admin.workflows.statuses_removal_dialog.confirm"), - form_arguments: { action: helpers.edit_type_workflow_tab_path(@type, @tab), method: :get, data: { turbo_frame: "workflow-table" } } + form_arguments: { action: helpers.type_workflow_matrix_path(@type, tab: @tab), method: :get, data: { turbo_frame: "workflow-table" } } ) ) do |dialog| dialog.with_confirmation_message do |message| diff --git a/app/controllers/workflows/copies/from_roles_controller.rb b/app/controllers/workflows/copies/from_roles_controller.rb index d297ff4deddc..2bae35ded556 100644 --- a/app/controllers/workflows/copies/from_roles_controller.rb +++ b/app/controllers/workflows/copies/from_roles_controller.rb @@ -61,7 +61,7 @@ def create # rubocop:disable Metrics/AbcSize ) set_frame_src_via_turbo_stream( "workflow-table", - edit_type_workflow_tab_path(@source_type, params[:tab].presence || "always", role_ids: @target_roles.map(&:id)) + type_workflow_matrix_path(@source_type, tab: params[:tab], role_ids: @target_roles.map(&:id)) ) end diff --git a/app/controllers/workflows/tabs_controller.rb b/app/controllers/workflows/matrix_controller.rb similarity index 92% rename from app/controllers/workflows/tabs_controller.rb rename to app/controllers/workflows/matrix_controller.rb index 6cea7eb69524..134b5aa654a7 100644 --- a/app/controllers/workflows/tabs_controller.rb +++ b/app/controllers/workflows/matrix_controller.rb @@ -28,7 +28,11 @@ # See COPYRIGHT and LICENSE files for more details. #++ -class Workflows::TabsController < ApplicationController +# The transition matrix editor, scoped to the workflows aspect rather than to any one +# page that shows it. Every action responds with the editor's own turbo frame, a turbo +# stream patching one of its components, or a dialog — never a full page — so the +# workflow tab and the variant creation wizard can both drive it unchanged. +class Workflows::MatrixController < ApplicationController include OpTurbo::ComponentStream layout false @@ -37,7 +41,7 @@ class Workflows::TabsController < ApplicationController helper_method :matrix_context - def edit + def show unless turbo_frame_request? redirect_to edit_type_workflow_path(type, role_ids: params[:role_ids], tab: matrix_context.tab) end diff --git a/app/helpers/workflow_helper.rb b/app/helpers/workflow_helper.rb index 6d37dfb25528..1ee6340f9d0e 100644 --- a/app/helpers/workflow_helper.rb +++ b/app/helpers/workflow_helper.rb @@ -46,7 +46,7 @@ def workflow_tabs(type) description: I18n.t(:"admin.workflows.tabs.descriptions.user_assignee") } ].map do |tab| tab.merge( - path: edit_type_workflow_tab_path(type, tab[:name], params.permit(role_ids: [])), + path: type_workflow_matrix_path(type, tab: tab[:name], **params.permit(role_ids: []).to_h.symbolize_keys), data: { controller: "admin--workflow-tab-select", action: "click->admin--workflow-tab-select#select", "admin--workflow-tab-select-tab-value": tab[:name], diff --git a/app/views/work_package_types/workflow_tab/edit.html.erb b/app/views/work_package_types/workflow_tab/edit.html.erb index febc44689f39..280ea400de98 100644 --- a/app/views/work_package_types/workflow_tab/edit.html.erb +++ b/app/views/work_package_types/workflow_tab/edit.html.erb @@ -38,6 +38,6 @@ See COPYRIGHT and LICENSE files for more details. <%= render(WorkPackageTypes::ReuseModeBannerComponent.new(type: @type, aspect: Type::ConfigurationLink::WORKFLOWS)) %> <% if @type && @roles.any? %> - <%= turbo_frame_tag "workflow-table", class: "workflow-table--pinned-save-bar", src: edit_type_workflow_tab_path(@type, @current_tab, role_ids: @roles.map(&:id), status_ids: params[:status_ids]) %> + <%= turbo_frame_tag "workflow-table", class: "workflow-table--pinned-save-bar", src: type_workflow_matrix_path(@type, tab: @current_tab, role_ids: @roles.map(&:id), status_ids: params[:status_ids]) %> <% end %> <% end %> diff --git a/app/views/workflows/tabs/edit.html.erb b/app/views/workflows/matrix/show.html.erb similarity index 100% rename from app/views/workflows/tabs/edit.html.erb rename to app/views/workflows/matrix/show.html.erb diff --git a/config/routes.rb b/config/routes.rb index e60c10f458a8..e2a721c53523 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -209,11 +209,12 @@ resource :creation_wizard, controller: "creation_wizard", only: %i[show update] resource :workflow, controller: "workflow_tab", only: %i[edit] do - resources :tabs, only: %i[edit update], param: :tab, controller: "/workflows/tabs" do - member do - get :status_dialog - post :confirm_statuses - end + # The transition matrix editor, shared by the workflow tab and the creation wizard. + # Which transition tab it shows is a filter like the roles and statuses, so it + # travels as a query param rather than being part of the route. + resource :matrix, only: %i[show update], controller: "/workflows/matrix" do + get :status_dialog + post :confirm_statuses end resource :copy, only: %i[new], controller: "/workflows/copies" do resource :from_type, only: %i[create], controller: "/workflows/copies/from_types" 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 b3458938af78..44b79278f163 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 @@ -203,9 +203,12 @@ export default class WorkflowCheckboxStateController extends Controller params.append('role_ids[]', id)); url.search = params.toString(); turboFrame.setAttribute('src', url.toString()); diff --git a/spec/controllers/workflows/tabs_controller_spec.rb b/spec/controllers/workflows/matrix_controller_spec.rb similarity index 98% rename from spec/controllers/workflows/tabs_controller_spec.rb rename to spec/controllers/workflows/matrix_controller_spec.rb index 0dcc5fee0b85..7b54f1160f44 100644 --- a/spec/controllers/workflows/tabs_controller_spec.rb +++ b/spec/controllers/workflows/matrix_controller_spec.rb @@ -30,7 +30,7 @@ require "spec_helper" -RSpec.describe Workflows::TabsController do +RSpec.describe Workflows::MatrixController do let!(:role_scope) do role_scope = instance_double(ActiveRecord::Relation) @@ -71,11 +71,11 @@ current_user { build_stubbed(:admin) } - describe "#edit" do + describe "#show" do context "when not a turbo frame request" do context "with a single role" do it "redirects to the parent workflow edit path" do - get :edit, + get :show, params: { role_ids: [role.id.to_s], type_id: type.id.to_s, @@ -88,7 +88,7 @@ end it "does not forward status_ids to the redirect" do - get :edit, + get :show, params: { role_ids: [role.id.to_s], type_id: type.id.to_s, @@ -114,7 +114,7 @@ end it "redirects preserving all role ids" do - get :edit, + get :show, params: { role_ids: [role.id.to_s, role2.id.to_s], type_id: type.id.to_s, diff --git a/spec/requests/workflows/tabs_mount_points_spec.rb b/spec/requests/workflows/matrix_mount_points_spec.rb similarity index 92% rename from spec/requests/workflows/tabs_mount_points_spec.rb rename to spec/requests/workflows/matrix_mount_points_spec.rb index f811f58a4514..77ef85720e7e 100644 --- a/spec/requests/workflows/tabs_mount_points_spec.rb +++ b/spec/requests/workflows/matrix_mount_points_spec.rb @@ -45,12 +45,12 @@ before { login_as admin } it "renders the matrix frame with the transition menu and posts to the type-nested path" do - get edit_type_workflow_tab_path(type, "always", role_ids: [role.id]), + get type_workflow_matrix_path(type, tab: "always", role_ids: [role.id]), headers: { "Turbo-Frame" => "workflow-table" } expect(response).to have_http_status(:ok) expect(response.body).to include("Default transitions") - expect(response.body).to include("action=\"#{type_workflow_tab_path(type)}\"") + expect(response.body).to include("action=\"#{type_workflow_matrix_path(type, tab: 'always')}\"") end it "renders the type edit page shell with the lazy workflow frame" do diff --git a/spec/routing/types_spec.rb b/spec/routing/types_spec.rb index afeef6a77f64..f86941656957 100644 --- a/spec/routing/types_spec.rb +++ b/spec/routing/types_spec.rb @@ -44,23 +44,28 @@ end it do - expect(get("/types/42/workflow/tabs/always/edit")) - .to route_to("workflows/tabs#edit", type_id: "42", tab: "always") + expect(get("/types/42/workflow/matrix")) + .to route_to("workflows/matrix#show", type_id: "42") end it do - expect(patch("/types/42/workflow/tabs/always")) - .to route_to("workflows/tabs#update", type_id: "42", tab: "always") + expect(patch("/types/42/workflow/matrix")) + .to route_to("workflows/matrix#update", type_id: "42") end it do - expect(get("/types/42/workflow/tabs/always/status_dialog")) - .to route_to("workflows/tabs#status_dialog", type_id: "42", tab: "always") + expect(get("/types/42/workflow/matrix/status_dialog")) + .to route_to("workflows/matrix#status_dialog", type_id: "42") end it do - expect(post("/types/42/workflow/tabs/always/confirm_statuses")) - .to route_to("workflows/tabs#confirm_statuses", type_id: "42", tab: "always") + expect(post("/types/42/workflow/matrix/confirm_statuses")) + .to route_to("workflows/matrix#confirm_statuses", type_id: "42") + end + + it "carries the transition tab as a query param rather than a path segment" do + expect(get("/types/42/workflow/matrix?tab=author")) + .to route_to("workflows/matrix#show", type_id: "42", tab: "author") end end From 0b3c872969efa73fa0a799dceff40fe7153f9c0d Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 28 Jul 2026 13:13:59 +0200 Subject: [PATCH 06/20] Remove special behavior for workflows and integrate them like the other step editors --- .../wizard/footer_component.html.erb | 5 +- .../wizard/footer_component.rb | 21 ----- .../wizard/page_component.html.erb | 19 ++-- .../wizard/page_component.rb | 12 +-- .../wizard/page_component.sass | 4 - .../work_package_types/wizard/step_editors.rb | 25 ++++++ .../wizard/workflows_step_component.html.erb | 8 +- .../wizard/workflows_step_component.rb | 14 ++- ...l.erb => matrix_editor_component.html.erb} | 88 ++++++++----------- ...omponent.rb => matrix_editor_component.rb} | 23 ++++- .../creation_wizard_controller.rb | 22 +++++ .../workflows/matrix_controller.rb | 18 +--- app/helpers/workflow_helper.rb | 2 +- .../workflow_tab/edit.html.erb | 19 +++- app/views/workflows/matrix/show.html.erb | 2 +- .../workflow-checkbox-state.controller.ts | 18 ++-- spec/features/types/creation_wizard_spec.rb | 5 +- .../workflows/matrix_mount_points_spec.rb | 25 ++++-- 18 files changed, 192 insertions(+), 138 deletions(-) rename app/components/workflows/{status_matrix_form_component.html.erb => matrix_editor_component.html.erb} (69%) rename app/components/workflows/{status_matrix_form_component.rb => matrix_editor_component.rb} (71%) diff --git a/app/components/work_package_types/wizard/footer_component.html.erb b/app/components/work_package_types/wizard/footer_component.html.erb index 7f731b2c33d2..7b85e9678a12 100644 --- a/app/components/work_package_types/wizard/footer_component.html.erb +++ b/app/components/work_package_types/wizard/footer_component.html.erb @@ -30,9 +30,8 @@ <%= render( Primer::Beta::Button.new( type: :submit, - form: primary_action_form, - scheme: :primary, - **primary_action_arguments + form: WorkPackageTypes::Wizard::FooterComponent::FORM_IDENTIFIER, + scheme: :primary ) ) { primary_action_label } %>
diff --git a/app/components/work_package_types/wizard/footer_component.rb b/app/components/work_package_types/wizard/footer_component.rb index 5a5fa883888a..c95422a8a0d9 100644 --- a/app/components/work_package_types/wizard/footer_component.rb +++ b/app/components/work_package_types/wizard/footer_component.rb @@ -55,27 +55,6 @@ def first_step? = current_step == Steps.first def last_step? = current_step == Steps.last - # The workflows step embeds the shared matrix form, which persists in place through - # its own endpoint. Continue submits that form with the next step, so the endpoint - # persists and then redirects to advance the wizard (see Workflows::MatrixController). - # Escaping the matrix's turbo frame turns that redirect into a full navigation. - # Other steps submit the wizard form directly. - def primary_action_form - workflows_step? ? Workflows::StatusMatrixFormComponent::FORM_ID : FORM_IDENTIFIER - end - - def primary_action_arguments - return {} unless workflows_step? - - { - name: "advance_to_step", - value: Steps.next_after(current_step), - data: { turbo_frame: "_top" } - } - end - - def workflows_step? = current_step == :workflows - def current_number = Steps.index(current_step) + 1 def total_steps = Steps.all.size diff --git a/app/components/work_package_types/wizard/page_component.html.erb b/app/components/work_package_types/wizard/page_component.html.erb index 8371598bfd20..5dcb8d4336c5 100644 --- a/app/components/work_package_types/wizard/page_component.html.erb +++ b/app/components/work_package_types/wizard/page_component.html.erb @@ -27,20 +27,19 @@ <%= within_step_frame do %> <%= reuse_mode_banner %> <%= primer_form_with(**step_form_options) do |f| %> - <%= render(step_editor.form_class.new(f)) %> + <%= render(step_editor.form_class.new(f)) if step_editor.form_class %> + <%= render(step_editor.body) if step_editor.body %> <% end %> <% end %> <% else %> + <%# Self-persisting editor: this form only advances to the next step on submit. %> <%= render(step_body) %> - <% unless step_submits_own_form? %> - <%# Self-persisting editor: this form only advances to the next step on submit. %> - <%= primer_form_with( - url: step_url, - method: :patch, - html: { id: WorkPackageTypes::Wizard::FooterComponent::FORM_IDENTIFIER } - ) do %> - <%= tag.input(type: :hidden, name: :step, value: current_step) %> - <% end %> + <%= primer_form_with( + url: step_url, + method: :patch, + html: { id: WorkPackageTypes::Wizard::FooterComponent::FORM_IDENTIFIER } + ) do %> + <%= tag.input(type: :hidden, name: :step, value: current_step) %> <% end %> <% end %> <% end %> diff --git a/app/components/work_package_types/wizard/page_component.rb b/app/components/work_package_types/wizard/page_component.rb index 12c3c399c1f6..a5451db595b7 100644 --- a/app/components/work_package_types/wizard/page_component.rb +++ b/app/components/work_package_types/wizard/page_component.rb @@ -112,7 +112,13 @@ def step_form_options def within_step_frame(&) return capture(&) unless step_editor.linkable_aspect? - render(WorkPackageTypes::ReloadableConfigurationFrameComponent.new(reload_url: step_url), &) + render( + WorkPackageTypes::ReloadableConfigurationFrameComponent.new( + reload_url: step_url, + reload_from_location: step_editor.reload_from_location? + ), + & + ) end def reuse_mode_banner @@ -121,15 +127,11 @@ def reuse_mode_banner render(WorkPackageTypes::ReuseModeBannerComponent.new(type:, aspect: step_editor.aspect)) end - def step_submits_own_form? = current_step == :workflows - # Editors that self-persist through their own turbo endpoints. def step_body case current_step when :form_configuration FormConfigurationStepComponent.new(type:) - when :workflows - WorkflowsStepComponent.new(type:) when :project_attributes ProjectAttributesStepComponent.new(type:) when :projects diff --git a/app/components/work_package_types/wizard/page_component.sass b/app/components/work_package_types/wizard/page_component.sass index 2e0cac5efc28..8e370c9a6f41 100644 --- a/app/components/work_package_types/wizard/page_component.sass +++ b/app/components/work_package_types/wizard/page_component.sass @@ -54,7 +54,3 @@ &--sidebar display: none - - // The wizard footer's Continue button persists the workflow matrix - .workflow-save-bar - display: none diff --git a/app/components/work_package_types/wizard/step_editors.rb b/app/components/work_package_types/wizard/step_editors.rb index bcda42ebbb6f..3bcba22bdd6d 100644 --- a/app/components/work_package_types/wizard/step_editors.rb +++ b/app/components/work_package_types/wizard/step_editors.rb @@ -39,6 +39,7 @@ def self.for(step, type) case step when :details then Details.new(type) when :defaults then Defaults.new(type) + when :workflows then Workflows.new(type) end end @@ -55,9 +56,21 @@ def linkable_aspect? = aspect.present? def model = type + # A Primer form class whose fields the wizard form submits, or nil for editors + # that render their inputs themselves. + def form_class = nil + + # A component rendered inside the wizard form, for editors that are not + # expressible as a Primer form. + def body = nil + # Data attributes for the form element, e.g. Stimulus wiring. def form_data = {} + # Whether the step's frame reloads from the current location rather than from the + # step's own path, so that in-frame selections survive an out-of-band reload. + def reload_from_location? = false + def readonly? = linkable_aspect? && type.linked?(aspect) end @@ -77,6 +90,18 @@ def model # Without this the pattern input cannot be toggled as the subject mode changes. def form_data = readonly? ? {} : model.stimulus_data end + + # The transition matrix renders its own inputs rather than a Primer form, and it + # supplies no save control of its own — the wizard's Continue submits it. + class Workflows < Base + def aspect = Type::ConfigurationLink::WORKFLOWS + + def body = WorkflowsStepComponent.new(type:) + + # The matrix keeps the selected roles and transition tab in the page URL, which a + # reload from the step path would discard. + def reload_from_location? = true + end end end end diff --git a/app/components/work_package_types/wizard/workflows_step_component.html.erb b/app/components/work_package_types/wizard/workflows_step_component.html.erb index 446e95d2ea3a..c1a19af9eec0 100644 --- a/app/components/work_package_types/wizard/workflows_step_component.html.erb +++ b/app/components/work_package_types/wizard/workflows_step_component.html.erb @@ -27,10 +27,6 @@ See COPYRIGHT and LICENSE files for more details. ++#%> -<%= render(WorkPackageTypes::ReloadableConfigurationFrameComponent.new(reload_url:, reload_from_location: true)) do %> - <%= render(WorkPackageTypes::ReuseModeBannerComponent.new(type:, aspect: Type::ConfigurationLink::WORKFLOWS)) %> - - <% if roles.any? %> - <%= helpers.turbo_frame_tag "workflow-table", src: helpers.type_workflow_matrix_path(type, tab: current_tab, role_ids: roles.map(&:id), status_ids: helpers.params[:status_ids]) %> - <% end %> +<% if roles.any? %> + <%= helpers.turbo_frame_tag "workflow-table", src: matrix_url %> <% end %> diff --git a/app/components/work_package_types/wizard/workflows_step_component.rb b/app/components/work_package_types/wizard/workflows_step_component.rb index d43d944e433c..ad7bdae16fe6 100644 --- a/app/components/work_package_types/wizard/workflows_step_component.rb +++ b/app/components/work_package_types/wizard/workflows_step_component.rb @@ -30,6 +30,9 @@ module WorkPackageTypes module Wizard + # The workflows wizard step: the shared transition matrix in its frame. The matrix + # renders no form of its own, so its inputs are submitted by the wizard form that + # PageComponent wraps around this, and persisted by CreationWizardController. class WorkflowsStepComponent < ApplicationComponent include OpPrimer::ComponentHelpers @@ -41,12 +44,15 @@ def initialize(type:) def type = model - def reload_url - helpers.type_creation_wizard_path(model, step: :workflows) + def matrix_url + helpers.type_workflow_matrix_path( + type, + tab: helpers.params[:tab], + role_ids: roles.map(&:id), + status_ids: helpers.params[:status_ids] + ) end - def current_tab = helpers.params[:tab].presence || "always" - def roles Workflow.selected_roles(helpers.params[:role_ids]) end diff --git a/app/components/workflows/status_matrix_form_component.html.erb b/app/components/workflows/matrix_editor_component.html.erb similarity index 69% rename from app/components/workflows/status_matrix_form_component.html.erb rename to app/components/workflows/matrix_editor_component.html.erb index 0c6ea570ddcf..c9aa761782db 100644 --- a/app/components/workflows/status_matrix_form_component.html.erb +++ b/app/components/workflows/matrix_editor_component.html.erb @@ -126,64 +126,48 @@ See COPYRIGHT and LICENSE files for more details. %> <% if statuses.any? %> - <%= form_tag( - type_workflow_matrix_path(type, tab:), - id: form_id, - method: :patch, - autocomplete: "off", - data: { - controller: "admin--workflow-checkbox-state", - "admin--workflow-checkbox-state-has-status-changes-value": context.status_changes? - } - ) do %> - <%= hidden_field_tag "type_id", type.id %> - <% roles.each do |role| %> - <%= hidden_field_tag "role_ids[]", role.id %> - <% end %> - <%= hidden_field_tag "tab", tab %> - - <%= render(matrix_table) %> - - <% unless readonly? %> -
- <%= - render Primer::Beta::Button.new(scheme: :primary, type: :submit, mt: 3, ml: 3, mb: 4) do - t(:button_save) - end - %> -
- - <%= - render( - Primer::OpenProject::FeedbackDialog.new( - title: t("admin.workflows.leave_confirmation.title"), - data: { - "admin--workflow-checkbox-state-target": "confirmationDialog" - } - ) - ) do |dialog| - dialog.with_feedback_message(icon_arguments: { icon: :none }) do |message| - message.with_heading(tag: :h2) { t("admin.workflows.leave_confirmation.title") } - message.with_description_content(t("admin.workflows.leave_confirmation.description")) - end + <%# The matrix inputs and the state they are compared against. The enclosing form + belongs to the host page; this only groups what the dirty-state controller needs. %> + <%= tag.div(id: state_id, data: state_data) do %> + <%= hidden_field_tag "type_id", type.id %> + <% roles.each do |role| %> + <%= hidden_field_tag "role_ids[]", role.id %> + <% end %> + <%= hidden_field_tag "tab", tab %> + + <%= render(matrix_table) %> + + <% unless readonly? %> + <%= + render( + Primer::OpenProject::FeedbackDialog.new( + title: t("admin.workflows.leave_confirmation.title"), + data: { + "admin--workflow-checkbox-state-target": "confirmationDialog" + } + ) + ) do |dialog| + dialog.with_feedback_message(icon_arguments: { icon: :none }) do |message| + message.with_heading(tag: :h2) { t("admin.workflows.leave_confirmation.title") } + message.with_description_content(t("admin.workflows.leave_confirmation.description")) + end - dialog.with_footer do - component_collection do |footer| - footer.with_component( - Primer::Beta::Button.new(scheme: :danger, data: { "admin--workflow-checkbox-state-target": "ignoreButton" }) - ) { t("admin.workflows.leave_confirmation.ignore") } + dialog.with_footer do + component_collection do |footer| + footer.with_component( + Primer::Beta::Button.new(scheme: :danger, data: { "admin--workflow-checkbox-state-target": "ignoreButton" }) + ) { t("admin.workflows.leave_confirmation.ignore") } - footer.with_component( - Primer::Beta::Button.new(scheme: :primary, data: { "admin--workflow-checkbox-state-target": "saveButton" }) - ) { t("admin.workflows.leave_confirmation.save") } - end + footer.with_component( + Primer::Beta::Button.new(scheme: :primary, data: { "admin--workflow-checkbox-state-target": "saveButton" }) + ) { t("admin.workflows.leave_confirmation.save") } end end - %> - <% end %> + end + %> <% end %> + <% end %> <% else %> <%= render Workflows::BlankslateComponent.new(roles:, type:, tab:) %> - <%= form_tag(type_creation_wizard_path(type, step: :workflows), id: form_id, method: :patch) do %><% end %> <% end %> <% end %> diff --git a/app/components/workflows/status_matrix_form_component.rb b/app/components/workflows/matrix_editor_component.rb similarity index 71% rename from app/components/workflows/status_matrix_form_component.rb rename to app/components/workflows/matrix_editor_component.rb index 09840de40796..0c7f434d6fcb 100644 --- a/app/components/workflows/status_matrix_form_component.rb +++ b/app/components/workflows/matrix_editor_component.rb @@ -29,11 +29,19 @@ # ++ module Workflows - class StatusMatrixFormComponent < ApplicationComponent + # The transition matrix editor: the tab and role pickers, the matrix itself, and the + # dirty-state machinery that warns before navigating away from unsaved changes. + # + # Renders no form of its own and no save button. Whoever embeds the editor wraps it in + # a form and provides the control that submits it — the workflow tab has a pinned Save + # bar, the creation wizard has Continue in its footer — so the editor stays identical + # in both and needs nothing injected. + class MatrixEditorComponent < ApplicationComponent include OpTurbo::Streamable include OpPrimer::ComponentHelpers - FORM_ID = "workflow_form" + # The dirty-state controller's root. Other controllers reach it as a Stimulus outlet. + STATE_ID = "workflow_matrix" def initialize(context:) super @@ -46,7 +54,14 @@ def initialize(context:) delegate :type, :tab, :roles, :eligible_roles, :statuses, :readonly?, to: :context - def form_id = FORM_ID + def state_id = STATE_ID + + def state_data + { + controller: "admin--workflow-checkbox-state", + "admin--workflow-checkbox-state-has-status-changes-value": context.status_changes? + } + end def transition_tabs = @transition_tabs ||= helpers.workflow_tabs(type) @@ -68,7 +83,7 @@ def data_attributes controller: "admin--workflow-role-select", "admin--workflow-role-select-base-url-value": helpers.type_workflow_matrix_path(type, tab:), "admin--workflow-role-select-current-role-ids-value": roles.map(&:id), - "admin--workflow-role-select-admin--workflow-checkbox-state-outlet": "##{form_id}" + "admin--workflow-role-select-admin--workflow-checkbox-state-outlet": "##{STATE_ID}" } end end diff --git a/app/controllers/work_package_types/creation_wizard_controller.rb b/app/controllers/work_package_types/creation_wizard_controller.rb index c65695e91217..00a30c5f427e 100644 --- a/app/controllers/work_package_types/creation_wizard_controller.rb +++ b/app/controllers/work_package_types/creation_wizard_controller.rb @@ -71,6 +71,8 @@ def update update_details when :defaults update_defaults + when :workflows + update_workflows else advance end @@ -107,6 +109,26 @@ def update_defaults end end + # The matrix submits its inputs with the wizard form, along with the roles and + # transition tab it was showing, so that only that slice is rewritten. + def update_workflows + matrix_context = ::Workflows::MatrixContext.new( + type: @type, + tab: params[:tab], + role_ids: params[:role_ids] + ) + + service_call = ::Workflows::MatrixUpdateService + .new(type: @type, roles: matrix_context.roles, tab: matrix_context.tab) + .call(status: params[:status], indeterminate_status: params[:indeterminate_status]) + + if service_call.success? + advance + else + render :show, status: :unprocessable_entity + end + end + def advance redirect_to_step Wizard::Steps.next_after(@current_step) end diff --git a/app/controllers/workflows/matrix_controller.rb b/app/controllers/workflows/matrix_controller.rb index 134b5aa654a7..a5f61aa689d1 100644 --- a/app/controllers/workflows/matrix_controller.rb +++ b/app/controllers/workflows/matrix_controller.rb @@ -49,8 +49,6 @@ def show def update if persist_matrix.success? - return redirect_to_wizard_step if advance_to_wizard_step? - render_matrix_saved else render_matrix_not_saved @@ -79,7 +77,7 @@ def confirm_statuses removed_count: removed_status_count ) else - update_via_turbo_stream(component: matrix_form_component) + update_via_turbo_stream(component: matrix_editor_component) respond_with_turbo_streams end end @@ -103,8 +101,8 @@ def build_matrix_context ) end - def matrix_form_component(context = matrix_context) - Workflows::StatusMatrixFormComponent.new(context:) + def matrix_editor_component(context = matrix_context) + Workflows::MatrixEditorComponent.new(context:) end def persist_matrix @@ -122,7 +120,7 @@ def render_matrix_saved # Resolved afresh: the write just changed which statuses have transitions, and with # none left the matrix has to give way to the blankslate. saved = build_matrix_context - update_via_turbo_stream(component: matrix_form_component(saved)) if saved.statuses.empty? + update_via_turbo_stream(component: matrix_editor_component(saved)) if saved.statuses.empty? end def render_matrix_not_saved @@ -133,14 +131,6 @@ def render_matrix_not_saved @turbo_status = :unprocessable_entity end - # Temporary: the wizard still submits the matrix through this endpoint and asks it to - # advance afterwards. Goes away once the wizard persists the step itself. - def advance_to_wizard_step? = params[:advance_to_step].present? - - def redirect_to_wizard_step - redirect_to type_creation_wizard_path(type, step: params[:advance_to_step]), status: :see_other - end - def requested_status_ids @requested_status_ids ||= Array(params[:status_ids]).flatten.map(&:to_i) end diff --git a/app/helpers/workflow_helper.rb b/app/helpers/workflow_helper.rb index 1ee6340f9d0e..3dafb618765f 100644 --- a/app/helpers/workflow_helper.rb +++ b/app/helpers/workflow_helper.rb @@ -51,7 +51,7 @@ def workflow_tabs(type) action: "click->admin--workflow-tab-select#select", "admin--workflow-tab-select-tab-value": tab[:name], "admin--workflow-tab-select-admin--workflow-checkbox-state-outlet": - "##{Workflows::StatusMatrixFormComponent::FORM_ID}" } + "##{Workflows::MatrixEditorComponent::STATE_ID}" } ) end end diff --git a/app/views/work_package_types/workflow_tab/edit.html.erb b/app/views/work_package_types/workflow_tab/edit.html.erb index 280ea400de98..6241f8836d85 100644 --- a/app/views/work_package_types/workflow_tab/edit.html.erb +++ b/app/views/work_package_types/workflow_tab/edit.html.erb @@ -38,6 +38,23 @@ See COPYRIGHT and LICENSE files for more details. <%= render(WorkPackageTypes::ReuseModeBannerComponent.new(type: @type, aspect: Type::ConfigurationLink::WORKFLOWS)) %> <% if @type && @roles.any? %> - <%= turbo_frame_tag "workflow-table", class: "workflow-table--pinned-save-bar", src: type_workflow_matrix_path(@type, tab: @current_tab, role_ids: @roles.map(&:id), status_ids: params[:status_ids]) %> + <%= form_tag( + type_workflow_matrix_path(@type, tab: @current_tab), + method: :patch, + autocomplete: "off", + class: "workflow-table--pinned-save-bar" + ) do %> + <%= turbo_frame_tag "workflow-table", src: type_workflow_matrix_path(@type, tab: @current_tab, role_ids: @roles.map(&:id), status_ids: params[:status_ids]) %> + + <% unless workflow_linked?(@type) %> +
+ <%= + render Primer::Beta::Button.new(scheme: :primary, type: :submit, mt: 3, ml: 3, mb: 4) do + t(:button_save) + end + %> +
+ <% end %> + <% end %> <% end %> <% end %> diff --git a/app/views/workflows/matrix/show.html.erb b/app/views/workflows/matrix/show.html.erb index 4c5872888dac..fdf7b740b4c8 100644 --- a/app/views/workflows/matrix/show.html.erb +++ b/app/views/workflows/matrix/show.html.erb @@ -28,5 +28,5 @@ See COPYRIGHT and LICENSE files for more details. ++#%> <%= turbo_frame_tag "workflow-table", data: { turbo_cache: false } do %> - <%= render Workflows::StatusMatrixFormComponent.new(context: matrix_context) %> + <%= render Workflows::MatrixEditorComponent.new(context: matrix_context) %> <% end %> 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 44b79278f163..2ae8c198c053 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 @@ -53,7 +53,7 @@ interface SavedState { * via document-level event delegation, so it works even after the EditComponent * is replaced by a turbo-stream after the frame content loads. */ -export default class WorkflowCheckboxStateController extends Controller { +export default class WorkflowCheckboxStateController extends Controller { static targets = [ 'confirmationDialog', 'ignoreButton', 'saveButton' ]; declare readonly confirmationDialogTarget:HTMLDialogElement; declare readonly ignoreButtonTarget:HTMLButtonElement; @@ -71,9 +71,16 @@ export default class WorkflowCheckboxStateController extends Controller { @@ -222,7 +230,7 @@ export default class WorkflowCheckboxStateController extends Controller { return () => { - this.element.requestSubmit(); + this.form?.requestSubmit(); this.closeAndProceed(originalTarget, originalEvent); }; @@ -326,7 +334,7 @@ export default class WorkflowCheckboxStateController extends Controller { - this.element.requestSubmit(); + this.form?.requestSubmit(); this.confirmationDialogTarget.close(); // Delay to allow the flash message from the form submission to appear. setTimeout(navigate, 1000); diff --git a/spec/features/types/creation_wizard_spec.rb b/spec/features/types/creation_wizard_spec.rb index b2c286e7d449..a91376922d4a 100644 --- a/spec/features/types/creation_wizard_spec.rb +++ b/spec/features/types/creation_wizard_spec.rb @@ -98,8 +98,9 @@ def complete_details_step(name) click_on I18n.t(:button_continue) expect_step_saved(:project_attributes) - # Step 5 - Workflow - expect(page).to have_css("#workflow_form", visible: :all) + # Step 5 - Workflow: the matrix has no Save of its own, Continue persists it. + expect(page).to have_text("Linked mode") + expect(page).to have_no_button "Save" click_on I18n.t(:button_continue) expect_step_saved(:workflows) diff --git a/spec/requests/workflows/matrix_mount_points_spec.rb b/spec/requests/workflows/matrix_mount_points_spec.rb index 77ef85720e7e..ac0cb247bb9b 100644 --- a/spec/requests/workflows/matrix_mount_points_spec.rb +++ b/spec/requests/workflows/matrix_mount_points_spec.rb @@ -30,8 +30,10 @@ require "spec_helper" -# The workflow matrix lives only under the type edit page now. This exercises the -# frame body end-to-end, ensuring every route helper resolves and the matrix renders. +# Pins the split between the shared matrix editor and the page hosting it: the editor's +# frame carries the inputs and no form of its own, and the host supplies the form that +# submits them. Also exercises the frame body end-to-end, so every route helper the +# editor builds has to resolve. RSpec.describe "Workflow matrix on the type tab", type: :rails_request do shared_let(:admin) { create(:admin) } shared_let(:role) { create(:project_role) } @@ -44,19 +46,32 @@ before { login_as admin } - it "renders the matrix frame with the transition menu and posts to the type-nested path" do + it "renders the matrix frame with the transition menu and no form of its own" do get type_workflow_matrix_path(type, tab: "always", role_ids: [role.id]), headers: { "Turbo-Frame" => "workflow-table" } expect(response).to have_http_status(:ok) expect(response.body).to include("Default transitions") - expect(response.body).to include("action=\"#{type_workflow_matrix_path(type, tab: 'always')}\"") + expect(response.body).to include("status[#{status_a.id}][#{status_b.id}]") + expect(response.body).to include(Workflows::MatrixEditorComponent::STATE_ID) + expect(response.body).not_to include(" Date: Tue, 28 Jul 2026 13:40:07 +0200 Subject: [PATCH 07/20] Ensure that the save unchanged dialog does not submit the external form but saves internally --- .../workflows/matrix_editor_component.rb | 6 +- .../workflow-checkbox-state.controller.ts | 81 ++++++++++++++++--- .../types/creation_wizard_workflows_spec.rb | 31 +++++++ 3 files changed, 108 insertions(+), 10 deletions(-) diff --git a/app/components/workflows/matrix_editor_component.rb b/app/components/workflows/matrix_editor_component.rb index 0c7f434d6fcb..68f6d91131ae 100644 --- a/app/components/workflows/matrix_editor_component.rb +++ b/app/components/workflows/matrix_editor_component.rb @@ -59,7 +59,11 @@ def state_id = STATE_ID def state_data { controller: "admin--workflow-checkbox-state", - "admin--workflow-checkbox-state-has-status-changes-value": context.status_changes? + "admin--workflow-checkbox-state-has-status-changes-value": context.status_changes?, + # Saving from the "unsaved changes" dialog has to persist without navigating, which + # the host's form cannot do — the wizard's would advance a step. So the editor + # offers its own endpoint for that. + "admin--workflow-checkbox-state-save-url-value": helpers.type_workflow_matrix_path(type, tab:) } end 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 2ae8c198c053..112dc3424bfa 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 @@ -29,6 +29,8 @@ */ import { Controller } from '@hotwired/stimulus'; +import { renderStreamMessage } from '@hotwired/turbo'; +import { useMeta } from 'stimulus-use'; const PRISTINE_STATE_KEY = 'workflow-pristine-state'; const STATUS_STATE_KEY = 'workflow-status-state'; @@ -62,12 +64,17 @@ export default class WorkflowCheckboxStateController extends Controller { + this.markSaved(); + }; + + private markSaved() { this.popState(STATUS_STATE_KEY); this.popState(PRISTINE_STATE_KEY); this.initialCheckboxState = this.captureState(); this.hasCheckboxChangesValue = false; this.hasStatusChangesValue = false; - }; + } + + /** + * Persists the matrix against its own endpoint without navigating anywhere. + * + * Submitting the enclosing form is not an option here: it belongs to the host page, so + * it would advance the creation wizard instead of leaving the user on the editor they + * are still working in. Resolves to whether the matrix was stored. + */ + private async save():Promise { + if (!this.saveUrlValue) return false; + + const response = await fetch(this.saveUrlValue, { + method: 'PATCH', + body: this.matrixFormData(), + headers: { + Accept: 'text/vnd.turbo-stream.html', + 'X-CSRF-Token': this.csrfToken, + }, + }); + + const html = await response.text(); + if (html) renderStreamMessage(html); + + if (!response.ok) return false; + + this.markSaved(); + return true; + } + + // Mirrors what a real submit of these inputs would send: unchecked boxes are simply + // absent, which is how the server reads "transition not allowed". + private matrixFormData():FormData { + const data = new FormData(); + + this.element.querySelectorAll('input[name]').forEach((input) => { + if (input.type === 'checkbox' && !input.checked) return; + + data.append(input.name, input.value); + }); + + return data; + } private get formKey():string { const typeId = this.formValue('type_id'); @@ -230,9 +287,11 @@ export default class WorkflowCheckboxStateController extends Controller { return () => { - this.form?.requestSubmit(); - - this.closeAndProceed(originalTarget, originalEvent); + void this.save().then((stored) => { + // Leave the dialog open on failure: the flash explains why, and the pending + // changes are still there to retry with. + if (stored) this.closeAndProceed(originalTarget, originalEvent); + }); }; }; @@ -334,10 +393,14 @@ export default class WorkflowCheckboxStateController extends Controller { - this.form?.requestSubmit(); - this.confirmationDialogTarget.close(); - // Delay to allow the flash message from the form submission to appear. - setTimeout(navigate, 1000); + // Navigate only once the matrix is actually stored, so that switching tab or role + // cannot race the write and discard it. On failure the dialog stays open. + void this.save().then((stored) => { + if (!stored) return; + + this.confirmationDialogTarget.close(); + navigate(); + }); }, ); } diff --git a/spec/features/types/creation_wizard_workflows_spec.rb b/spec/features/types/creation_wizard_workflows_spec.rb index 59cc9e9e7748..a87dc7aa4968 100644 --- a/spec/features/types/creation_wizard_workflows_spec.rb +++ b/spec/features/types/creation_wizard_workflows_spec.rb @@ -97,6 +97,37 @@ def visit_workflow_wizard(roles: [], tab: nil) expect(page).to have_no_field workflow_checkbox(0, 1) end end + + # The wizard form advances a step, so saving from the dialog has to go through the + # matrix's own endpoint instead — otherwise the change is dropped or the user is + # thrown off the step (see MatrixEditorComponent). + it "stores pending changes and stays on the step when confirming the save" do + check workflow_checkbox(1, 0) + + switch_transition_tab "User is author" + + within_dialog "Save changes before continuing?" do + click_button "Save changes and continue" + end + + expect(page).to have_css "#workflow_form_author" + expect(page).to have_current_path(/step=workflows/) + + expect_transition(role, 1, 0, exist: true) + end + + it "keeps the step and discards the change when ignoring" do + check workflow_checkbox(1, 0) + + switch_transition_tab "User is author" + + within_dialog "Save changes before continuing?" do + click_button "Ignore changes" + end + + expect(page).to have_css "#workflow_form_author" + expect_transition(role, 1, 0, exist: false) + end end context "when switching roles" do From 509b44fc6db6e881c5e1a877fe200261ee445422 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 28 Jul 2026 14:00:20 +0200 Subject: [PATCH 08/20] Do not hard code a random id for tests, but instead derive it from current id --- .../work_packages/create_contract_spec.rb | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/spec/contracts/work_packages/create_contract_spec.rb b/spec/contracts/work_packages/create_contract_spec.rb index 3c98050ff681..1d20f06b5bbd 100644 --- a/spec/contracts/work_packages/create_contract_spec.rb +++ b/spec/contracts/work_packages/create_contract_spec.rb @@ -95,7 +95,7 @@ it "can write #{attribute}", :aggregate_failures do expect(contract.writable_attributes).to include(attribute.to_s) - work_package.send(:"#{attribute}=", value) + work_package.send(:"#{attribute}=", resolve_written_value(value)) expect(validated_contract.errors[attribute]).to be_empty end end @@ -104,18 +104,25 @@ it "can not write #{attribute}", :aggregate_failures do expect(contract.writable_attributes).not_to include(attribute.to_s) - work_package.send(:"#{attribute}=", value) + work_package.send(:"#{attribute}=", resolve_written_value(value)) expect(validated_contract).not_to be_valid expect(validated_contract.errors[attribute]).to include "was attempted to be written but is not writable." end end + # Values may be given as a callable so they can be derived from the example's own + # state. Writing a literal id risks matching what the attribute already holds, in + # which case nothing changes and the contract has nothing to object to. + def resolve_written_value(value) + value.respond_to?(:call) ? instance_exec(&value) : value + end + context "when enabled for admin", with_settings: { apiv3_write_readonly_attributes: true } do let(:user) { build_stubbed(:admin) } it_behaves_like "can write", :created_at, 1.day.ago it_behaves_like "can not write", :updated_at, 1.day.ago - it_behaves_like "can write", :author_id, 1234 + it_behaves_like "can write", :author_id, -> { user.id + 1 } end context "when disabled for admin", with_settings: { apiv3_write_readonly_attributes: false } do @@ -123,19 +130,19 @@ it_behaves_like "can not write", :created_at, 1.day.ago it_behaves_like "can not write", :updated_at, 1.day.ago - it_behaves_like "can not write", :author_id, 1234 + it_behaves_like "can not write", :author_id, -> { user.id + 1 } end context "when enabled for regular user", with_settings: { apiv3_write_readonly_attributes: true } do it_behaves_like "can not write", :created_at, 1.day.ago it_behaves_like "can not write", :updated_at, 1.day.ago - it_behaves_like "can not write", :author_id, 1234 + it_behaves_like "can not write", :author_id, -> { user.id + 1 } end context "when disabled for regular user", with_settings: { apiv3_write_readonly_attributes: false } do it_behaves_like "can not write", :created_at, 1.day.ago it_behaves_like "can not write", :updated_at, 1.day.ago - it_behaves_like "can not write", :author_id, 1234 + it_behaves_like "can not write", :author_id, -> { user.id + 1 } end end From 4fd7e513971c46422e32bdacb89e212067b4c2e8 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 28 Jul 2026 14:56:35 +0200 Subject: [PATCH 09/20] Simplify PageComponent and put all logic in the StepEditor --- .../wizard/page_component.html.erb | 3 +-- .../work_package_types/wizard/step_editors.rb | 22 +++++++++---------- 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/app/components/work_package_types/wizard/page_component.html.erb b/app/components/work_package_types/wizard/page_component.html.erb index 5dcb8d4336c5..2b1fb9a872cd 100644 --- a/app/components/work_package_types/wizard/page_component.html.erb +++ b/app/components/work_package_types/wizard/page_component.html.erb @@ -27,8 +27,7 @@ <%= within_step_frame do %> <%= reuse_mode_banner %> <%= primer_form_with(**step_form_options) do |f| %> - <%= render(step_editor.form_class.new(f)) if step_editor.form_class %> - <%= render(step_editor.body) if step_editor.body %> + <%= render(step_editor.editor(f)) %> <% end %> <% end %> <% else %> diff --git a/app/components/work_package_types/wizard/step_editors.rb b/app/components/work_package_types/wizard/step_editors.rb index 3bcba22bdd6d..42159b9932e9 100644 --- a/app/components/work_package_types/wizard/step_editors.rb +++ b/app/components/work_package_types/wizard/step_editors.rb @@ -56,30 +56,28 @@ def linkable_aspect? = aspect.present? def model = type - # A Primer form class whose fields the wizard form submits, or nil for editors - # that render their inputs themselves. - def form_class = nil - - # A component rendered inside the wizard form, for editors that are not - # expressible as a Primer form. - def body = nil + # The component rendered inside the wizard form. Receives the form builder for + # editors expressible as a Primer form; ignored by those rendering their own inputs. + def editor(_builder) + raise SubclassResponsibilityError + end # Data attributes for the form element, e.g. Stimulus wiring. def form_data = {} - # Whether the step's frame reloads from the current location rather than from the - # step's own path, so that in-frame selections survive an out-of-band reload. + # Steps whose in-frame state lives in the page URL reload from the current + # location instead of the step's own path. def reload_from_location? = false def readonly? = linkable_aspect? && type.linked?(aspect) end class Details < Base - def form_class = WorkPackageTypes::DetailsForm + def editor(builder) = WorkPackageTypes::DetailsForm.new(builder) end class Defaults < Base - def form_class = WorkPackageTypes::DefaultsForm + def editor(builder) = WorkPackageTypes::DefaultsForm.new(builder) def aspect = Type::ConfigurationLink::DEFAULTS @@ -96,7 +94,7 @@ def form_data = readonly? ? {} : model.stimulus_data class Workflows < Base def aspect = Type::ConfigurationLink::WORKFLOWS - def body = WorkflowsStepComponent.new(type:) + def editor(_builder) = WorkflowsStepComponent.new(type:) # The matrix keeps the selected roles and transition tab in the page URL, which a # reload from the step path would discard. From 74ff556131d6c0aae36c9ad08befb1aa7c202fbf Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 28 Jul 2026 15:03:11 +0200 Subject: [PATCH 10/20] Improve comment for arguing what we do in the Workflows form --- app/components/work_package_types/wizard/step_editors.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/app/components/work_package_types/wizard/step_editors.rb b/app/components/work_package_types/wizard/step_editors.rb index 42159b9932e9..0ff54d925c18 100644 --- a/app/components/work_package_types/wizard/step_editors.rb +++ b/app/components/work_package_types/wizard/step_editors.rb @@ -89,11 +89,12 @@ def model def form_data = readonly? ? {} : model.stimulus_data end - # The transition matrix renders its own inputs rather than a Primer form, and it - # supplies no save control of its own — the wizard's Continue submits it. class Workflows < Base def aspect = Type::ConfigurationLink::WORKFLOWS + # The workflow matrix editor is not using a primer form, thus it does not consume the builder + # It can internally switch what tab it is editing, those trigger a submit to its own controller action. + # The submit of the final page happens through the wizard's continue button def editor(_builder) = WorkflowsStepComponent.new(type:) # The matrix keeps the selected roles and transition tab in the page URL, which a From af3fc1b3492b932e1c0e9ea1559810856df1140f Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 28 Jul 2026 15:28:22 +0200 Subject: [PATCH 11/20] Inline tab data generation into the component --- .../workflows/matrix_editor_component.rb | 23 +++++++++++++++++-- app/helpers/workflow_helper.rb | 23 ------------------- config/locales/en.yml | 12 +++++----- 3 files changed, 27 insertions(+), 31 deletions(-) diff --git a/app/components/workflows/matrix_editor_component.rb b/app/components/workflows/matrix_editor_component.rb index 68f6d91131ae..c5f8162a1207 100644 --- a/app/components/workflows/matrix_editor_component.rb +++ b/app/components/workflows/matrix_editor_component.rb @@ -67,9 +67,28 @@ def state_data } end - def transition_tabs = @transition_tabs ||= helpers.workflow_tabs(type) + def transition_tabs + @transition_tabs ||= MatrixContext::TABS.map { transition_tab(it) } + end + + def transition_tab(name) + { + name:, + label: I18n.t(:"admin.workflows.tabs.#{name}"), + description: I18n.t(:"admin.workflows.tabs.descriptions.#{name}"), + path: helpers.type_workflow_matrix_path(type, tab: name, role_ids: roles.map(&:id)), + data: { + controller: "admin--workflow-tab-select", + action: "click->admin--workflow-tab-select#select", + "admin--workflow-tab-select-tab-value": name, + "admin--workflow-tab-select-admin--workflow-checkbox-state-outlet": "##{STATE_ID}" + } + } + end - def current_transition_tab = transition_tabs.find { it[:name] == tab } + def current_transition_tab + transition_tabs.find { it[:name] == tab } + end def matrix_table MatrixTableComponent.new( diff --git a/app/helpers/workflow_helper.rb b/app/helpers/workflow_helper.rb index 3dafb618765f..073812b3dbc4 100644 --- a/app/helpers/workflow_helper.rb +++ b/app/helpers/workflow_helper.rb @@ -32,27 +32,4 @@ module WorkflowHelper def workflow_linked?(type) type&.linked?(Type::ConfigurationLink::WORKFLOWS) end - - def workflow_tabs(type) - [ - { name: "always", - label: I18n.t(:"admin.workflows.tabs.default_transitions"), - description: I18n.t(:"admin.workflows.tabs.descriptions.default_transitions") }, - { name: "author", - label: I18n.t(:"admin.workflows.tabs.user_author"), - description: I18n.t(:"admin.workflows.tabs.descriptions.user_author") }, - { name: "assignee", - label: I18n.t(:"admin.workflows.tabs.user_assignee"), - description: I18n.t(:"admin.workflows.tabs.descriptions.user_assignee") } - ].map do |tab| - tab.merge( - path: type_workflow_matrix_path(type, tab: tab[:name], **params.permit(role_ids: []).to_h.symbolize_keys), - data: { controller: "admin--workflow-tab-select", - action: "click->admin--workflow-tab-select#select", - "admin--workflow-tab-select-tab-value": tab[:name], - "admin--workflow-tab-select-admin--workflow-checkbox-state-outlet": - "##{Workflows::MatrixEditorComponent::STATE_ID}" } - ) - end - end end diff --git a/config/locales/en.yml b/config/locales/en.yml index 5186dd2ab155..3106cb3aca26 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1582,15 +1582,15 @@ en: other: "Remove %{count} statuses?" title: "Remove statuses" tabs: + always: "Default transitions" aria_label: "Workflow transitions" - default_transitions: "Default transitions" + assignee: "User is assignee" + author: "User is author" descriptions: - default_transitions: "Transitions available to the selected role(s) in all cases." - user_assignee: "Additional transitions available to the work package's assignee." - user_author: "Additional transitions available to the work package's author." + always: "Transitions available to the selected role(s) in all cases." + assignee: "Additional transitions available to the work package's assignee." + author: "Additional transitions available to the work package's author." menu_label: "Transitions shown" - user_assignee: "User is assignee" - user_author: "User is author" announcements: is_active: currently displayed is_inactive: currently not displayed From 42895557eff24afda56eb336201bfa2f0faf17fd Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 28 Jul 2026 15:45:18 +0200 Subject: [PATCH 12/20] Get rid of WorkflowHelper and inline the linked? check --- .../workflows/blankslate_component.html.erb | 4 +-- .../workflows/blankslate_component.rb | 12 +++---- .../matrix_editor_component.html.erb | 2 +- app/helpers/workflow_helper.rb | 35 ------------------- .../workflow_tab/edit.html.erb | 2 +- 5 files changed, 10 insertions(+), 45 deletions(-) delete mode 100644 app/helpers/workflow_helper.rb diff --git a/app/components/workflows/blankslate_component.html.erb b/app/components/workflows/blankslate_component.html.erb index 2915fa9864e3..4c7fafbef27b 100644 --- a/app/components/workflows/blankslate_component.html.erb +++ b/app/components/workflows/blankslate_component.html.erb @@ -31,9 +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(description_key)) - unless read_only? + unless readonly? blankslate.with_primary_action( - href: helpers.status_dialog_type_workflow_matrix_path(@type, tab: @tab, role_ids: @roles.map(&:id)), + href: helpers.status_dialog_type_workflow_matrix_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 a19e6e7ad6de..5c7b7ec69553 100644 --- a/app/components/workflows/blankslate_component.rb +++ b/app/components/workflows/blankslate_component.rb @@ -32,19 +32,19 @@ module Workflows class BlankslateComponent < ApplicationComponent include OpPrimer::ComponentHelpers - def initialize(roles:, type:, tab:) + def initialize(context:) super - @roles = roles - @type = type - @tab = tab + @context = context end private - def read_only? = helpers.workflow_linked?(@type) + attr_reader :context + + delegate :type, :tab, :roles, :readonly?, to: :context def description_key - read_only? ? "admin.workflows.blankslate.linked_description" : "admin.workflows.blankslate.description" + readonly? ? "admin.workflows.blankslate.linked_description" : "admin.workflows.blankslate.description" end end end diff --git a/app/components/workflows/matrix_editor_component.html.erb b/app/components/workflows/matrix_editor_component.html.erb index c9aa761782db..b500a0880dd0 100644 --- a/app/components/workflows/matrix_editor_component.html.erb +++ b/app/components/workflows/matrix_editor_component.html.erb @@ -168,6 +168,6 @@ See COPYRIGHT and LICENSE files for more details. <% end %> <% end %> <% else %> - <%= render Workflows::BlankslateComponent.new(roles:, type:, tab:) %> + <%= render Workflows::BlankslateComponent.new(context:) %> <% end %> <% end %> diff --git a/app/helpers/workflow_helper.rb b/app/helpers/workflow_helper.rb deleted file mode 100644 index 073812b3dbc4..000000000000 --- a/app/helpers/workflow_helper.rb +++ /dev/null @@ -1,35 +0,0 @@ -# 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 WorkflowHelper - def workflow_linked?(type) - type&.linked?(Type::ConfigurationLink::WORKFLOWS) - end -end diff --git a/app/views/work_package_types/workflow_tab/edit.html.erb b/app/views/work_package_types/workflow_tab/edit.html.erb index 6241f8836d85..74c23a1c0c45 100644 --- a/app/views/work_package_types/workflow_tab/edit.html.erb +++ b/app/views/work_package_types/workflow_tab/edit.html.erb @@ -46,7 +46,7 @@ See COPYRIGHT and LICENSE files for more details. ) do %> <%= turbo_frame_tag "workflow-table", src: type_workflow_matrix_path(@type, tab: @current_tab, role_ids: @roles.map(&:id), status_ids: params[:status_ids]) %> - <% unless workflow_linked?(@type) %> + <% unless @type.linked?(Type::ConfigurationLink::WORKFLOWS) %>
<%= render Primer::Beta::Button.new(scheme: :primary, type: :submit, mt: 3, ml: 3, mb: 4) do From 7bc15f884a8bba04ff2ad1a0c2cf30112656c7d2 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 28 Jul 2026 15:49:46 +0200 Subject: [PATCH 13/20] Pass the `MatrixContext` into the MatrixTableComponent --- .../workflows/matrix_editor_component.html.erb | 2 +- .../workflows/matrix_editor_component.rb | 11 ----------- .../workflows/matrix_table_component.rb | 18 +++++------------- .../workflows/matrix_table_component_spec.rb | 10 ++++++---- 4 files changed, 12 insertions(+), 29 deletions(-) diff --git a/app/components/workflows/matrix_editor_component.html.erb b/app/components/workflows/matrix_editor_component.html.erb index b500a0880dd0..18516f467f4f 100644 --- a/app/components/workflows/matrix_editor_component.html.erb +++ b/app/components/workflows/matrix_editor_component.html.erb @@ -135,7 +135,7 @@ See COPYRIGHT and LICENSE files for more details. <% end %> <%= hidden_field_tag "tab", tab %> - <%= render(matrix_table) %> + <%= render(Workflows::MatrixTableComponent.new(context:)) %> <% unless readonly? %> <%= diff --git a/app/components/workflows/matrix_editor_component.rb b/app/components/workflows/matrix_editor_component.rb index c5f8162a1207..4edaaf7b8f94 100644 --- a/app/components/workflows/matrix_editor_component.rb +++ b/app/components/workflows/matrix_editor_component.rb @@ -90,17 +90,6 @@ def current_transition_tab transition_tabs.find { it[:name] == tab } end - def matrix_table - MatrixTableComponent.new( - tab:, - statuses:, - workflows: context.workflows, - roles:, - added_status_ids: context.added_status_ids, - readonly: readonly? - ) - end - def data_attributes { controller: "admin--workflow-role-select", diff --git a/app/components/workflows/matrix_table_component.rb b/app/components/workflows/matrix_table_component.rb index 13ecc0e76697..671e56f4e8e2 100644 --- a/app/components/workflows/matrix_table_component.rb +++ b/app/components/workflows/matrix_table_component.rb @@ -37,22 +37,17 @@ module Workflows class MatrixTableComponent < ApplicationComponent include OpPrimer::ComponentHelpers - def initialize(tab:, statuses:, workflows:, roles:, added_status_ids: [], readonly: false) + def initialize(context:) super() - @tab = tab - @statuses = statuses - @workflows = workflows - @roles = roles - @added_status_ids = added_status_ids - @readonly = readonly + @context = context end private - attr_reader :tab, :statuses, :roles, :added_status_ids + attr_reader :context - def readonly? = @readonly + delegate :tab, :statuses, :workflows, :roles, :added_status_ids, :readonly?, to: :context def dom_id = "workflow_form_#{tab}" @@ -71,9 +66,6 @@ def checkbox_label(old_status, new_status) t("workflows.form.matrix_checkbox_label", old_status: old_status.name, new_status: new_status.name) end - # The `_html` keys carry an around the status name, so they must go through the - # ActionView translate helper that marks them safe — I18n.t alone would escape it into - # the button's accessible name. def column_toggle_label(new_status) t("workflows.form.matrix_check_uncheck_all_in_col_label_html", new_status: new_status.name) end @@ -102,7 +94,7 @@ def allowed_role_ids(old_status, new_status) end def role_ids_by_transition - @role_ids_by_transition ||= @workflows + @role_ids_by_transition ||= workflows .group_by { [it.old_status_id, it.new_status_id] } .transform_values { it.map(&:role_id).uniq } end diff --git a/spec/components/workflows/matrix_table_component_spec.rb b/spec/components/workflows/matrix_table_component_spec.rb index 96a0eb75f63f..e4a9fddb5157 100644 --- a/spec/components/workflows/matrix_table_component_spec.rb +++ b/spec/components/workflows/matrix_table_component_spec.rb @@ -44,12 +44,14 @@ let(:readonly) { false } let(:tab) { "always" } - subject(:rendered_component) do - render_inline( - described_class.new(tab:, statuses:, workflows:, roles:, added_status_ids:, readonly:) - ) + # Stubbed rather than resolved from a persisted type: the matrix is a pure function of + # these five values, and MatrixContext's own resolution rules are specced separately. + let(:context) do + instance_double(Workflows::MatrixContext, tab:, statuses:, workflows:, roles:, added_status_ids:, readonly?: readonly) end + subject(:rendered_component) { render_inline(described_class.new(context:)) } + def transition(old_status:, new_status:, for_role: role) build_stubbed(:workflow, role_id: for_role.id, old_status_id: old_status.id, new_status_id: new_status.id) end From 1b91b41452b3047d11b2de02aa2a22030446175a Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 28 Jul 2026 15:53:47 +0200 Subject: [PATCH 14/20] Better comment for MatrixController --- app/controllers/workflows/matrix_controller.rb | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/app/controllers/workflows/matrix_controller.rb b/app/controllers/workflows/matrix_controller.rb index a5f61aa689d1..ec015f3eb4e3 100644 --- a/app/controllers/workflows/matrix_controller.rb +++ b/app/controllers/workflows/matrix_controller.rb @@ -28,10 +28,8 @@ # See COPYRIGHT and LICENSE files for more details. #++ -# The transition matrix editor, scoped to the workflows aspect rather than to any one -# page that shows it. Every action responds with the editor's own turbo frame, a turbo -# stream patching one of its components, or a dialog — never a full page — so the -# workflow tab and the variant creation wizard can both drive it unchanged. +# All actions either update the workflow matrix editor's internal representation or render dialogs to ensure +# that the editor can be modified regardless of the context it is used in class Workflows::MatrixController < ApplicationController include OpTurbo::ComponentStream From bd314765bb39fc1c885e5463bda09b3f44434342 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 28 Jul 2026 16:14:20 +0200 Subject: [PATCH 15/20] Hand over the `MatrixContext` to the dialogs and do all calculations in there --- .../status_dialog_component.html.erb | 10 +--- .../workflows/status_dialog_component.rb | 12 ++-- .../workflows/status_form_component.html.erb | 13 +--- .../workflows/status_form_component.rb | 14 +++-- ...s_removal_danger_dialog_component.html.erb | 8 +-- .../status_removal_danger_dialog_component.rb | 16 +++-- .../workflows/matrix_controller.rb | 35 +---------- app/forms/workflows/status_select_form.rb | 33 +++++++---- app/models/workflows/matrix_context.rb | 30 +++++----- .../workflows/matrix_controller_spec.rb | 59 +++++++++++-------- 10 files changed, 106 insertions(+), 124 deletions(-) diff --git a/app/components/workflows/status_dialog_component.html.erb b/app/components/workflows/status_dialog_component.html.erb index d9fe0c120653..b15b2642ae6d 100644 --- a/app/components/workflows/status_dialog_component.html.erb +++ b/app/components/workflows/status_dialog_component.html.erb @@ -38,15 +38,7 @@ See COPYRIGHT and LICENSE files for more details. dialog.with_header(variant: :large) dialog.with_body(classes: "workflow-status-dialog-body") do - render( - Workflows::StatusFormComponent.new( - all_statuses: @all_statuses, - current_statuses: @current_statuses, - roles: @roles, - type: @type, - tab: @tab - ) - ) + render(Workflows::StatusFormComponent.new(context:)) end dialog.with_footer do diff --git a/app/components/workflows/status_dialog_component.rb b/app/components/workflows/status_dialog_component.rb index 2c724a580024..b403ccc9c72a 100644 --- a/app/components/workflows/status_dialog_component.rb +++ b/app/components/workflows/status_dialog_component.rb @@ -35,13 +35,13 @@ class StatusDialogComponent < ApplicationComponent DIALOG_ID = "workflows-status-dialog" - def initialize(all_statuses:, current_statuses:, roles:, type:, tab:) + def initialize(context:) super - @all_statuses = all_statuses - @current_statuses = current_statuses - @roles = roles - @type = type - @tab = tab + @context = context end + + private + + attr_reader :context end end diff --git a/app/components/workflows/status_form_component.html.erb b/app/components/workflows/status_form_component.html.erb index 271eb4daec09..9ffaf2216936 100644 --- a/app/components/workflows/status_form_component.html.erb +++ b/app/components/workflows/status_form_component.html.erb @@ -29,20 +29,11 @@ See COPYRIGHT and LICENSE files for more details. <%= primer_form_with( - url: helpers.confirm_statuses_type_workflow_matrix_path(@type, tab: @tab, role_ids: @roles.map(&:id)), + url: helpers.confirm_statuses_type_workflow_matrix_path(type, tab:, role_ids: roles.map(&:id)), method: :post, id: FORM_ID, data: { turbo_frame: "workflow-table" } ) do |f| - render( - Workflows::StatusSelectForm.new( - f, - all_statuses: @all_statuses, - current_statuses: @current_statuses, - type: @type, - tab: @tab, - dialog_id: - ) - ) + render(Workflows::StatusSelectForm.new(f, context:, dialog_id:)) end %> diff --git a/app/components/workflows/status_form_component.rb b/app/components/workflows/status_form_component.rb index 8b99cfc125e6..a956bd299bed 100644 --- a/app/components/workflows/status_form_component.rb +++ b/app/components/workflows/status_form_component.rb @@ -32,17 +32,19 @@ module Workflows class StatusFormComponent < ApplicationComponent FORM_ID = "status-selection-form" - def initialize(all_statuses:, current_statuses:, roles:, type:, tab:) + def initialize(context:) super - @all_statuses = all_statuses - @current_statuses = current_statuses - @roles = roles - @type = type - @tab = tab + @context = context end def dialog_id StatusDialogComponent::DIALOG_ID end + + private + + attr_reader :context + + delegate :type, :tab, :roles, to: :context 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 ba5075334c92..fb00eb049e66 100644 --- a/app/components/workflows/status_removal_danger_dialog_component.html.erb +++ b/app/components/workflows/status_removal_danger_dialog_component.html.erb @@ -33,11 +33,11 @@ See COPYRIGHT and LICENSE files for more details. id: DIALOG_ID, title: t("admin.workflows.statuses_removal_dialog.title"), confirm_button_text: t("admin.workflows.statuses_removal_dialog.confirm"), - form_arguments: { action: helpers.type_workflow_matrix_path(@type, tab: @tab), method: :get, data: { turbo_frame: "workflow-table" } } + form_arguments: { action: helpers.type_workflow_matrix_path(type, tab:), method: :get, data: { turbo_frame: "workflow-table" } } ) ) do |dialog| dialog.with_confirmation_message do |message| - message.with_heading(tag: :h2) { t("admin.workflows.statuses_removal_dialog.heading", count: @removed_count) } + message.with_heading(tag: :h2) { t("admin.workflows.statuses_removal_dialog.heading", count: removed_count) } message.with_description_content(t("admin.workflows.statuses_removal_dialog.description")) end @@ -46,8 +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 - @roles.each { |role| concat(hidden_field_tag("role_ids[]", role.id)) } - @status_ids.each { |id| concat(hidden_field_tag("status_ids[]", id)) } + roles.each { |role| concat(hidden_field_tag("role_ids[]", role.id)) } + requested_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 d3f926f24c36..3f4d1ad3c27a 100644 --- a/app/components/workflows/status_removal_danger_dialog_component.rb +++ b/app/components/workflows/status_removal_danger_dialog_component.rb @@ -35,13 +35,17 @@ class StatusRemovalDangerDialogComponent < ApplicationComponent DIALOG_ID = "workflows-status-removal-dialog" - def initialize(roles:, type:, tab:, status_ids:, removed_count:) + def initialize(context:) super - @roles = roles - @type = type - @tab = tab - @status_ids = Array(status_ids).flatten.map(&:to_i) - @removed_count = removed_count + @context = context end + + private + + attr_reader :context + + delegate :type, :tab, :roles, :requested_status_ids, :removed_status_ids, to: :context + + def removed_count = removed_status_ids.size end end diff --git a/app/controllers/workflows/matrix_controller.rb b/app/controllers/workflows/matrix_controller.rb index ec015f3eb4e3..791dd0eac709 100644 --- a/app/controllers/workflows/matrix_controller.rb +++ b/app/controllers/workflows/matrix_controller.rb @@ -56,24 +56,12 @@ def update end def status_dialog - respond_with_dialog Workflows::StatusDialogComponent.new( - all_statuses: Status.order(:position), - current_statuses: dialog_statuses, - roles: matrix_context.roles, - type:, - tab: matrix_context.tab - ) + respond_with_dialog Workflows::StatusDialogComponent.new(context: matrix_context) end def confirm_statuses - if removed_status_count.positive? - respond_with_dialog Workflows::StatusRemovalDangerDialogComponent.new( - roles: matrix_context.roles, - type:, - tab: matrix_context.tab, - status_ids: requested_status_ids, - removed_count: removed_status_count - ) + if matrix_context.removed_status_ids.any? + respond_with_dialog Workflows::StatusRemovalDangerDialogComponent.new(context: matrix_context) else update_via_turbo_stream(component: matrix_editor_component) respond_with_turbo_streams @@ -129,21 +117,4 @@ def render_matrix_not_saved @turbo_status = :unprocessable_entity end - def requested_status_ids - @requested_status_ids ||= Array(params[:status_ids]).flatten.map(&:to_i) - end - - def removed_status_count - original_ids = Array(params[:original_status_ids]).flatten.map(&:to_i) - - (original_ids - requested_status_ids).size - end - - # The dialog opens on the pending selection when there is one, otherwise on what the - # selected roles have saved — and on nothing at all while no role is selected. - def dialog_statuses - return Status.none if requested_status_ids.blank? && matrix_context.roles.empty? - - matrix_context.statuses - end end diff --git a/app/forms/workflows/status_select_form.rb b/app/forms/workflows/status_select_form.rb index f650969ea3e4..06abf593ea84 100644 --- a/app/forms/workflows/status_select_form.rb +++ b/app/forms/workflows/status_select_form.rb @@ -30,19 +30,15 @@ module Workflows class StatusSelectForm < ApplicationForm - def initialize(all_statuses:, current_statuses:, type:, tab:, dialog_id:) + def initialize(context:, dialog_id:) super() - @all_statuses = all_statuses - @current_statuses = current_statuses - @type = type - @tab = tab + @context = context @dialog_id = dialog_id end form do |f| - 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) } + f.hidden(name: :type_id, value: context.type.id) + f.hidden(name: :tab, value: context.tab) f.autocompleter( name: :status_ids, @@ -56,14 +52,31 @@ def initialize(all_statuses:, current_statuses:, type:, tab:, dialog_id:) appendTo: "##{@dialog_id}" } ) do |list| - @all_statuses.each do |status| + all_statuses.each do |status| list.option( label: status.name, value: status.id, - selected: @current_statuses.include?(status) + selected: current_statuses.include?(status) ) end end end + + private + + attr_reader :context + + def all_statuses + @all_statuses ||= Status.order(:position) + end + + # The dialog opens on the pending selection when there is one, otherwise on what the + # selected roles have saved — and on nothing at all while no role is selected, where + # the matrix itself would fall back to every status of the type. + def current_statuses + return Status.none if context.requested_status_ids.blank? && context.roles.empty? + + context.statuses + end end end diff --git a/app/models/workflows/matrix_context.rb b/app/models/workflows/matrix_context.rb index bce51212b2f6..b78b8e6fbf97 100644 --- a/app/models/workflows/matrix_context.rb +++ b/app/models/workflows/matrix_context.rb @@ -29,13 +29,6 @@ #++ module Workflows - # Resolves everything the transition matrix needs in order to render: which transition - # tab is on screen, the roles it covers, the statuses forming its axes and the - # transitions that already exist. - # - # Every host of the matrix builds one of these from its own request — the type's - # workflow tab, the variant creation wizard — so that none of them has to reproduce - # the resolution rules, and all of them agree on what is being edited. class MatrixContext TABS = %w[always author assignee].freeze DEFAULT_TAB = "always" @@ -67,6 +60,13 @@ def roles @roles ||= Workflow.selected_roles(@requested_role_ids) end + # The selection the status dialog submitted, empty while the matrix shows what is saved. + # The dialogs forward these verbatim, so they must stay the raw request and never fall + # back to the saved statuses the way #statuses does. + def requested_status_ids + @requested_status_ids ||= Array(@status_ids).flatten.map(&:to_i) + end + # The axes of the matrix: a pending selection if the status dialog submitted one, # otherwise whatever the selected roles already have transitions for. def statuses @@ -87,12 +87,18 @@ def added_status_ids @added_status_ids ||= requested_status_ids - saved_status_ids end + # Statuses that saving the pending selection would drop from the type, deleting their + # transitions with them — which is what the removal dialog warns about. + def removed_status_ids + return [] if requested_status_ids.blank? + + @removed_status_ids ||= saved_status_ids - requested_status_ids + end + # Whether the pending selection differs from what is saved, which the matrix uses to # warn before navigating away. def status_changes? - return false if requested_status_ids.blank? - - added_status_ids.any? || (saved_status_ids - requested_status_ids).any? + added_status_ids.any? || removed_status_ids.any? end # The existing transitions of the selected roles, narrowed to the tab on screen. @@ -113,10 +119,6 @@ def belongs_to_tab?(workflow) end end - def requested_status_ids - @requested_status_ids ||= Array(@status_ids).flatten.map(&:to_i) - end - # The baseline a pending selection is compared against: always what the selected roles # have saved, never the pending selection itself. def saved_status_ids diff --git a/spec/controllers/workflows/matrix_controller_spec.rb b/spec/controllers/workflows/matrix_controller_spec.rb index 7b54f1160f44..b20647265fab 100644 --- a/spec/controllers/workflows/matrix_controller_spec.rb +++ b/spec/controllers/workflows/matrix_controller_spec.rb @@ -130,26 +130,41 @@ end describe "#confirm_statuses" do - before do - allow(controller) - .to receive(:respond_with_dialog) - .and_call_original + let(:status) { build_stubbed(:status) } + + # What the pending selection would drop is the context's call, so the branch is driven + # through it rather than through a status_ids/saved-statuses fixture. + let(:matrix_context) do + instance_double(Workflows::MatrixContext, + type:, + tab: "always", + roles: [role], + requested_status_ids: [status.id], + removed_status_ids:) + end + + def submit_statuses + allow(controller).to receive(:respond_with_dialog).and_call_original + allow(controller).to receive(:build_matrix_context).and_return(matrix_context) + + post :confirm_statuses, + params: { + role_ids: [role.id.to_s], + type_id: type.id.to_s, + status_ids: [status.id.to_s], + tab: "always" + }, + as: :turbo_stream end - context "when no statuses were removed" do + context "when the pending selection drops no saved status" do + let(:removed_status_ids) { [] } + before do allow(controller).to receive(:update_via_turbo_stream) allow(controller).to receive(:respond_with_turbo_streams) - post :confirm_statuses, - params: { - role_ids: [role.id.to_s], - type_id: type.id.to_s, - status_ids: ["1", "2"], - original_status_ids: ["1", "2"], - tab: "always" - }, - as: :turbo_stream + submit_statuses end it "updates the status matrix via turbo stream" do @@ -158,18 +173,10 @@ end end - context "when statuses were removed" do - before do - post :confirm_statuses, - params: { - role_ids: [role.id.to_s], - type_id: type.id.to_s, - status_ids: ["1"], - original_status_ids: ["1", "2"], - tab: "always" - }, - as: :turbo_stream - end + context "when the pending selection drops a saved status" do + let(:removed_status_ids) { [build_stubbed(:status).id] } + + before { submit_statuses } it "responds with the danger dialog" do expect(controller) From 6d1934ec40999f599add38d02cc6df164ba005ad Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 28 Jul 2026 16:28:40 +0200 Subject: [PATCH 16/20] Cleanup routes --- app/components/workflows/matrix_editor_component.rb | 11 +---------- app/controllers/workflows/matrix_controller.rb | 1 - config/routes.rb | 5 ++--- 3 files changed, 3 insertions(+), 14 deletions(-) diff --git a/app/components/workflows/matrix_editor_component.rb b/app/components/workflows/matrix_editor_component.rb index 4edaaf7b8f94..f9f602dfd986 100644 --- a/app/components/workflows/matrix_editor_component.rb +++ b/app/components/workflows/matrix_editor_component.rb @@ -29,13 +29,6 @@ # ++ module Workflows - # The transition matrix editor: the tab and role pickers, the matrix itself, and the - # dirty-state machinery that warns before navigating away from unsaved changes. - # - # Renders no form of its own and no save button. Whoever embeds the editor wraps it in - # a form and provides the control that submits it — the workflow tab has a pinned Save - # bar, the creation wizard has Continue in its footer — so the editor stays identical - # in both and needs nothing injected. class MatrixEditorComponent < ApplicationComponent include OpTurbo::Streamable include OpPrimer::ComponentHelpers @@ -60,9 +53,7 @@ def state_data { controller: "admin--workflow-checkbox-state", "admin--workflow-checkbox-state-has-status-changes-value": context.status_changes?, - # Saving from the "unsaved changes" dialog has to persist without navigating, which - # the host's form cannot do — the wizard's would advance a step. So the editor - # offers its own endpoint for that. + # for saving the workflow when switching tabs "admin--workflow-checkbox-state-save-url-value": helpers.type_workflow_matrix_path(type, tab:) } end diff --git a/app/controllers/workflows/matrix_controller.rb b/app/controllers/workflows/matrix_controller.rb index 791dd0eac709..7b47964cee2c 100644 --- a/app/controllers/workflows/matrix_controller.rb +++ b/app/controllers/workflows/matrix_controller.rb @@ -116,5 +116,4 @@ def render_matrix_not_saved ) @turbo_status = :unprocessable_entity end - end diff --git a/config/routes.rb b/config/routes.rb index e2a721c53523..fff76e4c829d 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -208,14 +208,13 @@ end resource :creation_wizard, controller: "creation_wizard", only: %i[show update] + resource :workflow, controller: "workflow_tab", only: %i[edit] do - # The transition matrix editor, shared by the workflow tab and the creation wizard. - # Which transition tab it shows is a filter like the roles and statuses, so it - # travels as a query param rather than being part of the route. resource :matrix, only: %i[show update], controller: "/workflows/matrix" do get :status_dialog post :confirm_statuses end + resource :copy, only: %i[new], controller: "/workflows/copies" do resource :from_type, only: %i[create], controller: "/workflows/copies/from_types" resource :from_role, only: %i[create], controller: "/workflows/copies/from_roles" From c7b1e81de90acd26383528d3424e64d812073543 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 28 Jul 2026 16:39:09 +0200 Subject: [PATCH 17/20] Reintroduce concept of currently displayed statuses, with a better wording of the variables --- .../status_removal_danger_dialog_component.rb | 4 +-- .../workflows/matrix_controller.rb | 5 ++-- app/forms/workflows/status_select_form.rb | 3 +++ app/models/workflows/matrix_context.rb | 26 ++++++++++++++----- .../workflows/matrix_controller_spec.rb | 14 +++++----- 5 files changed, 35 insertions(+), 17 deletions(-) diff --git a/app/components/workflows/status_removal_danger_dialog_component.rb b/app/components/workflows/status_removal_danger_dialog_component.rb index 3f4d1ad3c27a..a2b4b6b4a618 100644 --- a/app/components/workflows/status_removal_danger_dialog_component.rb +++ b/app/components/workflows/status_removal_danger_dialog_component.rb @@ -44,8 +44,8 @@ def initialize(context:) attr_reader :context - delegate :type, :tab, :roles, :requested_status_ids, :removed_status_ids, to: :context + delegate :type, :tab, :roles, :requested_status_ids, :removed_displayed_status_ids, to: :context - def removed_count = removed_status_ids.size + def removed_count = removed_displayed_status_ids.size end end diff --git a/app/controllers/workflows/matrix_controller.rb b/app/controllers/workflows/matrix_controller.rb index 7b47964cee2c..509c8ab7ddc2 100644 --- a/app/controllers/workflows/matrix_controller.rb +++ b/app/controllers/workflows/matrix_controller.rb @@ -60,7 +60,7 @@ def status_dialog end def confirm_statuses - if matrix_context.removed_status_ids.any? + if matrix_context.removed_displayed_status_ids.any? respond_with_dialog Workflows::StatusRemovalDangerDialogComponent.new(context: matrix_context) else update_via_turbo_stream(component: matrix_editor_component) @@ -83,7 +83,8 @@ def build_matrix_context type:, tab: params[:tab], role_ids: params[:role_ids], - status_ids: params[:status_ids] + status_ids: params[:status_ids], + displayed_status_ids: params[:displayed_status_ids] ) end diff --git a/app/forms/workflows/status_select_form.rb b/app/forms/workflows/status_select_form.rb index 06abf593ea84..7673908045e1 100644 --- a/app/forms/workflows/status_select_form.rb +++ b/app/forms/workflows/status_select_form.rb @@ -39,6 +39,9 @@ def initialize(context:, dialog_id:) form do |f| f.hidden(name: :type_id, value: context.type.id) f.hidden(name: :tab, value: context.tab) + # The baseline the submit measures its removals against, echoed back because the + # dialog is rendered fresh on every open and the POST cannot see the page's state. + current_statuses.each { |status| f.hidden(name: "displayed_status_ids[]", value: status.id) } f.autocompleter( name: :status_ids, diff --git a/app/models/workflows/matrix_context.rb b/app/models/workflows/matrix_context.rb index b78b8e6fbf97..51f5a1640e5f 100644 --- a/app/models/workflows/matrix_context.rb +++ b/app/models/workflows/matrix_context.rb @@ -35,11 +35,12 @@ class MatrixContext attr_reader :type - def initialize(type:, tab: nil, role_ids: nil, status_ids: nil) + def initialize(type:, tab: nil, role_ids: nil, status_ids: nil, displayed_status_ids: nil) @type = type @requested_tab = tab @requested_role_ids = role_ids - @status_ids = status_ids + @requested_status_ids = status_ids_from(status_ids) + @displayed_status_ids = status_ids_from(displayed_status_ids) end # Normalised, so that rendering, the status query and persistence cannot disagree @@ -63,9 +64,7 @@ def roles # The selection the status dialog submitted, empty while the matrix shows what is saved. # The dialogs forward these verbatim, so they must stay the raw request and never fall # back to the saved statuses the way #statuses does. - def requested_status_ids - @requested_status_ids ||= Array(@status_ids).flatten.map(&:to_i) - end + attr_reader :requested_status_ids # The axes of the matrix: a pending selection if the status dialog submitted one, # otherwise whatever the selected roles already have transitions for. @@ -88,13 +87,24 @@ def added_status_ids end # Statuses that saving the pending selection would drop from the type, deleting their - # transitions with them — which is what the removal dialog warns about. + # transitions along with them. def removed_status_ids return [] if requested_status_ids.blank? @removed_status_ids ||= saved_status_ids - requested_status_ids end + # The removals of a single dialog turn, measured against the statuses the dialog was + # rendered with instead of against the database. This is what the removal dialog asks + # about: taking out a status that is not saved yet still counts, and a removal already + # confirmed is not counted a second time. + # + # Only the dialog's own submit carries that baseline; every other request leaves this + # empty, which is why #status_changes? compares against what is saved instead. + def removed_displayed_status_ids + @removed_displayed_status_ids ||= @displayed_status_ids - requested_status_ids + end + # Whether the pending selection differs from what is saved, which the matrix uses to # warn before navigating away. def status_changes? @@ -111,6 +121,10 @@ def workflows private + def status_ids_from(ids) + Array(ids).flatten.map(&:to_i) + end + def belongs_to_tab?(workflow) case tab when "author" then workflow.author diff --git a/spec/controllers/workflows/matrix_controller_spec.rb b/spec/controllers/workflows/matrix_controller_spec.rb index b20647265fab..e6843a5a74e3 100644 --- a/spec/controllers/workflows/matrix_controller_spec.rb +++ b/spec/controllers/workflows/matrix_controller_spec.rb @@ -132,15 +132,15 @@ describe "#confirm_statuses" do let(:status) { build_stubbed(:status) } - # What the pending selection would drop is the context's call, so the branch is driven - # through it rather than through a status_ids/saved-statuses fixture. + # Which statuses the submit drops is the context's call, so the branch is driven through + # it rather than through a status_ids/displayed_status_ids fixture. let(:matrix_context) do instance_double(Workflows::MatrixContext, type:, tab: "always", roles: [role], requested_status_ids: [status.id], - removed_status_ids:) + removed_displayed_status_ids:) end def submit_statuses @@ -157,8 +157,8 @@ def submit_statuses as: :turbo_stream end - context "when the pending selection drops no saved status" do - let(:removed_status_ids) { [] } + context "when the pending selection drops nothing the dialog was showing" do + let(:removed_displayed_status_ids) { [] } before do allow(controller).to receive(:update_via_turbo_stream) @@ -173,8 +173,8 @@ def submit_statuses end end - context "when the pending selection drops a saved status" do - let(:removed_status_ids) { [build_stubbed(:status).id] } + context "when the pending selection drops a status the dialog was showing" do + let(:removed_displayed_status_ids) { [build_stubbed(:status).id] } before { submit_statuses } From d77ba912d35855500a936308fb16d73310b382e8 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 28 Jul 2026 17:05:44 +0200 Subject: [PATCH 18/20] Fix styling issues --- .../global_styles/content/work_packages/_workflows.sass | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/frontend/src/global_styles/content/work_packages/_workflows.sass b/frontend/src/global_styles/content/work_packages/_workflows.sass index ebc797dcac5b..635906a287c9 100644 --- a/frontend/src/global_styles/content/work_packages/_workflows.sass +++ b/frontend/src/global_styles/content/work_packages/_workflows.sass @@ -8,7 +8,7 @@ turbo-frame#workflow-table width: 100% overflow-x: auto - #workflows-status-matrix-form-component + #workflows-matrix-editor-component display: contents sub-header @@ -16,7 +16,9 @@ turbo-frame#workflow-table left: 0 background: var(--body-background) -#workflow_form +// Mirrors Workflows::MatrixEditorComponent::STATE_ID, the element wrapping the matrix in +// every host of the editor +#workflow_matrix .generic-table--results-container position: relative overflow: visible @@ -79,6 +81,9 @@ turbo-frame#workflow-table // Edit tab pins the save button; wizard step keeps it together to not mess with the footer .workflow-table--pinned-save-bar + // This form computes to display: inline, where the padding below would paint but reserve + // no space, leaving the last rows stuck behind the pinned bar with nothing left to scroll + display: block // Clear the pinned save bar so the last rows aren't hidden behind it padding-bottom: 72px // 16px (mt: 3) + 32px (button) + 24px (mb: 4) From 6934be6534fa36d8f6b8939a7a70530db2f38a46 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 28 Jul 2026 17:11:46 +0200 Subject: [PATCH 19/20] Get rid of superfluous comments --- app/components/work_package_types/wizard/step_editors.rb | 5 ----- .../work_package_types/wizard/workflows_step_component.rb | 5 ++--- app/components/workflows/matrix_editor_component.html.erb | 4 ++-- app/models/workflows/matrix_context.rb | 6 +----- app/services/workflows/matrix_update_service.rb | 3 +-- spec/components/workflows/matrix_table_component_spec.rb | 2 +- 6 files changed, 7 insertions(+), 18 deletions(-) diff --git a/app/components/work_package_types/wizard/step_editors.rb b/app/components/work_package_types/wizard/step_editors.rb index 0ff54d925c18..399231d824b5 100644 --- a/app/components/work_package_types/wizard/step_editors.rb +++ b/app/components/work_package_types/wizard/step_editors.rb @@ -56,17 +56,12 @@ def linkable_aspect? = aspect.present? def model = type - # The component rendered inside the wizard form. Receives the form builder for - # editors expressible as a Primer form; ignored by those rendering their own inputs. def editor(_builder) raise SubclassResponsibilityError end - # Data attributes for the form element, e.g. Stimulus wiring. def form_data = {} - # Steps whose in-frame state lives in the page URL reload from the current - # location instead of the step's own path. def reload_from_location? = false def readonly? = linkable_aspect? && type.linked?(aspect) diff --git a/app/components/work_package_types/wizard/workflows_step_component.rb b/app/components/work_package_types/wizard/workflows_step_component.rb index ad7bdae16fe6..41879b96574b 100644 --- a/app/components/work_package_types/wizard/workflows_step_component.rb +++ b/app/components/work_package_types/wizard/workflows_step_component.rb @@ -30,9 +30,8 @@ module WorkPackageTypes module Wizard - # The workflows wizard step: the shared transition matrix in its frame. The matrix - # renders no form of its own, so its inputs are submitted by the wizard form that - # PageComponent wraps around this, and persisted by CreationWizardController. + # The matrix renders no form of its own, so its inputs are submitted by the wizard form + # that PageComponent wraps around this, and persisted by CreationWizardController. class WorkflowsStepComponent < ApplicationComponent include OpPrimer::ComponentHelpers diff --git a/app/components/workflows/matrix_editor_component.html.erb b/app/components/workflows/matrix_editor_component.html.erb index 18516f467f4f..b1ada0f79020 100644 --- a/app/components/workflows/matrix_editor_component.html.erb +++ b/app/components/workflows/matrix_editor_component.html.erb @@ -126,8 +126,8 @@ See COPYRIGHT and LICENSE files for more details. %> <% if statuses.any? %> - <%# The matrix inputs and the state they are compared against. The enclosing form - belongs to the host page; this only groups what the dirty-state controller needs. %> + <%# The enclosing form belongs to the host page; this only groups what the dirty-state + controller needs. %> <%= tag.div(id: state_id, data: state_data) do %> <%= hidden_field_tag "type_id", type.id %> <% roles.each do |role| %> diff --git a/app/models/workflows/matrix_context.rb b/app/models/workflows/matrix_context.rb index 51f5a1640e5f..672061ba9a66 100644 --- a/app/models/workflows/matrix_context.rb +++ b/app/models/workflows/matrix_context.rb @@ -50,7 +50,6 @@ def tab @tab ||= TABS.include?(@requested_tab.to_s) ? @requested_tab.to_s : DEFAULT_TAB end - # A linked type shows its source's transitions and cannot edit them. def readonly? = type.linked?(Type::ConfigurationLink::WORKFLOWS) def eligible_roles @@ -61,13 +60,11 @@ def roles @roles ||= Workflow.selected_roles(@requested_role_ids) end - # The selection the status dialog submitted, empty while the matrix shows what is saved. # The dialogs forward these verbatim, so they must stay the raw request and never fall # back to the saved statuses the way #statuses does. attr_reader :requested_status_ids - # The axes of the matrix: a pending selection if the status dialog submitted one, - # otherwise whatever the selected roles already have transitions for. + # The axes of the matrix. def statuses @statuses ||= if requested_status_ids.present? Status.where(id: requested_status_ids).order(:position) @@ -111,7 +108,6 @@ def status_changes? added_status_ids.any? || removed_status_ids.any? end - # The existing transitions of the selected roles, narrowed to the tab on screen. def workflows @workflows ||= type .workflows diff --git a/app/services/workflows/matrix_update_service.rb b/app/services/workflows/matrix_update_service.rb index 326b883d7f9a..3efd0d3520e1 100644 --- a/app/services/workflows/matrix_update_service.rb +++ b/app/services/workflows/matrix_update_service.rb @@ -41,8 +41,7 @@ def initialize(type:, roles:, tab:) @tab = tab end - # A linked type reuses its source's transitions and must never have its own - # rewritten, so it persists nothing and simply reports success. + # A linked type reuses its source's transitions and must never have its own rewritten. def call(status: nil, indeterminate_status: nil) return ServiceResult.success if type.linked?(Type::ConfigurationLink::WORKFLOWS) diff --git a/spec/components/workflows/matrix_table_component_spec.rb b/spec/components/workflows/matrix_table_component_spec.rb index e4a9fddb5157..682c8053ead2 100644 --- a/spec/components/workflows/matrix_table_component_spec.rb +++ b/spec/components/workflows/matrix_table_component_spec.rb @@ -45,7 +45,7 @@ let(:tab) { "always" } # Stubbed rather than resolved from a persisted type: the matrix is a pure function of - # these five values, and MatrixContext's own resolution rules are specced separately. + # these values, and MatrixContext's own resolution rules are specced separately. let(:context) do instance_double(Workflows::MatrixContext, tab:, statuses:, workflows:, roles:, added_status_ids:, readonly?: readonly) end From 63bc514ff423ea7a5b64351f2c567cb9e5a47005 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 28 Jul 2026 17:17:21 +0200 Subject: [PATCH 20/20] Optimize lookup if transisiton exists --- app/services/workflows/matrix_update_service.rb | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/app/services/workflows/matrix_update_service.rb b/app/services/workflows/matrix_update_service.rb index 3efd0d3520e1..27789d896461 100644 --- a/app/services/workflows/matrix_update_service.rb +++ b/app/services/workflows/matrix_update_service.rb @@ -90,12 +90,17 @@ def restore_indeterminate(transitions, indeterminate, role) end def transition_exists?(role, old_status_id, new_status_id) - Workflow.exists?(role_id: role.id, - type_id: type.id, - old_status_id: old_status_id.to_i, - new_status_id: new_status_id.to_i, - author: author?, - assignee: assignee?) + saved_transitions.include?([role.id, old_status_id.to_i, new_status_id.to_i]) + end + + # Every indeterminate cell of every selected role is looked up against this, so it is + # read once up front rather than per cell. Safe to reuse across roles even as they are + # written: a role's write only ever touches its own rows. + def saved_transitions + @saved_transitions ||= Workflow + .where(type_id: type.id, role_id: roles.map(&:id), author: author?, assignee: assignee?) + .pluck(:role_id, :old_status_id, :new_status_id) + .to_set end # The matrix names its checkboxes status[old_status_id][new_status_id], so anything
- <%= t(".matrix_caption_#{name}", default: t(".matrix_caption")) %> + <%= caption %>
+
<%= @@ -73,16 +65,16 @@ See COPYRIGHT and LICENSE files for more details. <%= t(:label_new_statuses_allowed) %> - <% unless read_only %> + <% unless readonly? %> <%= - check_all_links "workflow_form_#{name}" do |links| + render(OpenProject::Common::CheckAllComponent.new(checkable_id: dom_id)) do |links| links.with_check_all_button do |button| - button.with_tooltip(text: t(".matrix_check_all_label")) + button.with_tooltip(text: t("workflows.form.matrix_check_all_label")) t(:button_check_all) end links.with_uncheck_all_button do |button| - button.with_tooltip(text: t(".matrix_uncheck_all_label")) + button.with_tooltip(text: t("workflows.form.matrix_uncheck_all_label")) t(:button_uncheck_all) end end @@ -95,7 +87,7 @@ See COPYRIGHT and LICENSE files for more details.
<%= render( @@ -115,11 +107,9 @@ See COPYRIGHT and LICENSE files for more details. scheme: :invisible, size: :small, icon: :check, - disabled: read_only, + disabled: readonly?, tooltip_direction: :sw, - aria: { - label: t(".matrix_check_uncheck_all_in_col_label_html", new_status: new_status.name) - }, + aria: { label: column_toggle_label(new_status) }, data: { action: "checkable#toggleSelection", checkable_key_param: "newStatus", @@ -135,7 +125,7 @@ See COPYRIGHT and LICENSE files for more details.
<%= @@ -156,11 +146,9 @@ See COPYRIGHT and LICENSE files for more details. scheme: :invisible, size: :small, icon: :check, - disabled: read_only, + disabled: readonly?, tooltip_direction: :sw, - aria: { - label: t(".matrix_check_uncheck_all_in_row_label_html", old_status: old_status.name) - }, + aria: { label: row_toggle_label(old_status) }, data: { action: "checkable#toggleSelection", checkable_key_param: "oldStatus", @@ -174,12 +162,10 @@ See COPYRIGHT and LICENSE files for more details. <% end %> <% end %> - <%= hidden_field_tag "indeterminate_status[#{old_status.id}][#{new_status.id}]", "1" if some_roles %> + <%= hidden_field_tag "indeterminate_status[#{old_status.id}][#{new_status.id}]", "1" if indeterminate %> <%= render(Primer::BaseComponent.new(tag: :div, display: :flex, align_items: :center, mx: 1)) do render( @@ -187,16 +173,16 @@ See COPYRIGHT and LICENSE files for more details. scheme: :array, 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, - disabled: read_only, - 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), + value: tab, + disabled: readonly?, + checked: checked?(old_status, new_status), + label: checkbox_label(old_status, new_status), visually_hide_label: true, data: { checkable_target: "checkbox", old_status: old_status.id, new_status: new_status.id, - indeterminate: (true if some_roles) + indeterminate: (true if indeterminate) } ) ) @@ -207,8 +193,8 @@ See COPYRIGHT and LICENSE files for more details.
- + + <%= t(:label_current_status) %>