From 0149a532cbcfc5119eb21d6371dc97fa0645749a Mon Sep 17 00:00:00 2001 From: Deep Sidhu Date: Wed, 8 Jan 2025 12:55:57 -0600 Subject: [PATCH 1/5] Updated back-end (policy, services, concern) for editing workflow execution names post launch --- .../concerns/workflow_execution_actions.rb | 20 +++++- .../workflow_executions_controller.rb | 4 ++ app/policies/workflow_execution_policy.rb | 8 +++ .../workflow_executions/update_service.rb | 16 +++++ .../update.turbo_stream.erb | 0 .../update.turbo_stream.erb | 0 config/locales/en.yml | 1 + config/locales/fr.yml | 1 + config/routes/workflow_executions.rb | 2 +- .../workflow_executions_controller_test.rb | 22 +++++++ .../update_service_test.rb | 62 +++++++++++++++++++ 11 files changed, 134 insertions(+), 2 deletions(-) create mode 100644 app/services/workflow_executions/update_service.rb create mode 100644 app/views/projects/workflow_executions/update.turbo_stream.erb create mode 100644 app/views/workflow_executions/update.turbo_stream.erb create mode 100644 test/services/workflow_executions/update_service_test.rb diff --git a/app/controllers/concerns/workflow_execution_actions.rb b/app/controllers/concerns/workflow_execution_actions.rb index 00c87dc559..667e061035 100644 --- a/app/controllers/concerns/workflow_execution_actions.rb +++ b/app/controllers/concerns/workflow_execution_actions.rb @@ -7,7 +7,7 @@ module WorkflowExecutionActions # rubocop:disable Metrics/ModuleLength included do before_action :set_default_tab, only: :show before_action :current_page, only: %i[show index] - before_action :workflow_execution, only: %i[show cancel destroy] + before_action :workflow_execution, only: %i[show cancel destroy update edit] end TABS = %w[summary params samplesheet files].freeze @@ -22,6 +22,24 @@ def index @pagy, @workflow_executions = pagy_with_metadata_sort(@q.result) end + def edit + authorize! @namespace + end + + def update + respond_to do |format| + format.turbo_stream do + @updated = WorkflowExecutions::UpdateService.new(@workflow_execution, current_user, + workflow_execution_update_params).execute + if @updated + render status: :ok + else + render status: :unprocessable_entity + end + end + end + end + def show authorize! @namespace, to: :view_workflow_executions? unless @namespace.nil? diff --git a/app/controllers/workflow_executions_controller.rb b/app/controllers/workflow_executions_controller.rb index 788276951b..83362f1914 100644 --- a/app/controllers/workflow_executions_controller.rb +++ b/app/controllers/workflow_executions_controller.rb @@ -30,6 +30,10 @@ def workflow_execution_params params.require(:workflow_execution).permit(workflow_execution_params_attributes) end + def workflow_execution_update_params + params.require(:workflow_execution).permit(:name) + end + def workflow_execution_params_attributes [ :name, diff --git a/app/policies/workflow_execution_policy.rb b/app/policies/workflow_execution_policy.rb index 52aa0db719..3adb434c00 100644 --- a/app/policies/workflow_execution_policy.rb +++ b/app/policies/workflow_execution_policy.rb @@ -82,6 +82,14 @@ def cancel? # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity false end + def update? + return true if record.submitter.id == user.id + return true if effective_access_level >= Member::AccessLevel::ANALYST + + details[:id] = record.id + false + end + scope_for :relation, :automated do |relation, options| project = options[:project] diff --git a/app/services/workflow_executions/update_service.rb b/app/services/workflow_executions/update_service.rb new file mode 100644 index 0000000000..bae4c8e76c --- /dev/null +++ b/app/services/workflow_executions/update_service.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +module WorkflowExecutions + # Service used to Update a WorkflowExecution + class UpdateService < BaseService + def initialize(workflow_execution, user = nil, params = {}) + super(user, params) + @workflow_execution = workflow_execution + end + + def execute + authorize! @workflow_execution, to: :update? + @workflow_execution.update!(params) + end + end +end diff --git a/app/views/projects/workflow_executions/update.turbo_stream.erb b/app/views/projects/workflow_executions/update.turbo_stream.erb new file mode 100644 index 0000000000..e69de29bb2 diff --git a/app/views/workflow_executions/update.turbo_stream.erb b/app/views/workflow_executions/update.turbo_stream.erb new file mode 100644 index 0000000000..e69de29bb2 diff --git a/config/locales/en.yml b/config/locales/en.yml index 83af042ea1..26af7d1fd7 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -97,6 +97,7 @@ en: destroy?: You are not authorized to destroy workflow executions for %{namespace_type} %{name} index?: You are not authorized to view workflow executions for %{namespace_type} %{name} read?: You are not authorized to view workflow execution %{id} + update?: You are not authorized to update workflow execution %{id} unauthorized: You are not authorized to perform this action activerecord: attributes: diff --git a/config/locales/fr.yml b/config/locales/fr.yml index d1744de172..3df04efbb1 100644 --- a/config/locales/fr.yml +++ b/config/locales/fr.yml @@ -97,6 +97,7 @@ fr: destroy?: You are not authorized to destroy workflow executions for %{namespace_type} %{name} index?: You are not authorized to view workflow executions for %{namespace_type} %{name} read?: You are not authorized to view workflow execution %{id} + update?: You are not authorized to update workflow execution %{id} unauthorized: You are not authorized to perform this action activerecord: attributes: diff --git a/config/routes/workflow_executions.rb b/config/routes/workflow_executions.rb index 3d69df9d2d..b4cf7f8fa4 100644 --- a/config/routes/workflow_executions.rb +++ b/config/routes/workflow_executions.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -resources :workflow_executions, only: %i[index create show destroy] do +resources :workflow_executions, only: %i[index create show destroy edit update] do member do put :cancel end diff --git a/test/controllers/workflow_executions_controller_test.rb b/test/controllers/workflow_executions_controller_test.rb index 4832330fd8..e372901192 100644 --- a/test/controllers/workflow_executions_controller_test.rb +++ b/test/controllers/workflow_executions_controller_test.rb @@ -212,4 +212,26 @@ class WorkflowExecutionsControllerTest < ActionDispatch::IntegrationTest assert_redirected_to workflow_executions_path end + + test 'Submitter can update workflow execution name post launch' do + workflow_execution = workflow_executions(:irida_next_example_new) + + update_params = { workflow_execution: { name: 'New Name' } } + + put workflow_execution_path(workflow_execution, format: :turbo_stream), params: update_params + + assert_response :success + end + + test 'Cannot update another user\'s personal workflow execution name' do + sign_in users(:ryan_doe) + + workflow_execution = workflow_executions(:irida_next_example_new) + + update_params = { workflow_execution: { name: 'New Name' } } + + put workflow_execution_path(workflow_execution, format: :turbo_stream), params: update_params + + assert_response :unprocessable_entity + end end diff --git a/test/services/workflow_executions/update_service_test.rb b/test/services/workflow_executions/update_service_test.rb new file mode 100644 index 0000000000..39f84c80c9 --- /dev/null +++ b/test/services/workflow_executions/update_service_test.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +require 'test_helper' + +module WorkflowExecutions + class UpdateServiceTest < ActiveSupport::TestCase + def setup + @user = users(:john_doe) + @workflow_execution = workflow_executions(:irida_next_example_new) + end + + test 'submitter can update name of workflow execution post launch' do + valid_params = { name: 'New Name' } + + assert_changes -> { @workflow_execution.name }, to: 'New Name' do + WorkflowExecutions::UpdateService.new(@workflow_execution, @user, valid_params).execute + end + end + + test 'user cannot update workflow execution name for another user\'s personal workflow execution' do + valid_params = { name: 'New Name' } + + exception = assert_raises(ActionPolicy::Unauthorized) do + WorkflowExecutions::UpdateService.new(@workflow_execution, users(:michelle_doe), + valid_params).execute + end + + assert_equal WorkflowExecutionPolicy, exception.policy + assert_equal :update?, exception.rule + assert exception.result.reasons.is_a?(::ActionPolicy::Policy::FailureReasons) + assert_equal I18n.t(:'action_policy.policy.workflow_execution.update?', + id: @workflow_execution.id), + exception.result.message + end + + test 'analyst or higher can update workflow execution name for automated workflow execution' do + workflow_execution = workflow_executions(:automated_workflow_execution) + valid_params = { name: 'New Name Automated' } + + assert_changes -> { workflow_execution.name }, to: 'New Name Automated' do + WorkflowExecutions::UpdateService.new(workflow_execution, users(:james_doe), valid_params).execute + end + end + + test 'access level below analyst cannot update workflow execution name for automated workflow execution' do + workflow_execution = workflow_executions(:automated_workflow_execution) + valid_params = { name: 'New Name' } + + exception = assert_raises(ActionPolicy::Unauthorized) do + WorkflowExecutions::UpdateService.new(workflow_execution, users(:ryan_doe), + valid_params).execute + end + + assert_equal WorkflowExecutionPolicy, exception.policy + assert_equal :update?, exception.rule + assert exception.result.reasons.is_a?(::ActionPolicy::Policy::FailureReasons) + assert_equal I18n.t(:'action_policy.policy.workflow_execution.update?', + id: workflow_execution.id), + exception.result.message + end + end +end From 8c8f4104c16898a3a84171cc683f6a930ed24603 Mon Sep 17 00:00:00 2001 From: Deep Sidhu Date: Wed, 8 Jan 2025 13:30:57 -0600 Subject: [PATCH 2/5] Updated assert_response --- test/controllers/workflow_executions_controller_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/controllers/workflow_executions_controller_test.rb b/test/controllers/workflow_executions_controller_test.rb index e372901192..17388440b7 100644 --- a/test/controllers/workflow_executions_controller_test.rb +++ b/test/controllers/workflow_executions_controller_test.rb @@ -232,6 +232,6 @@ class WorkflowExecutionsControllerTest < ActionDispatch::IntegrationTest put workflow_execution_path(workflow_execution, format: :turbo_stream), params: update_params - assert_response :unprocessable_entity + assert_response :not_found end end From f5d7650e09479c4829ef92cdb7550fe6d3f7f388 Mon Sep 17 00:00:00 2001 From: Deep Sidhu Date: Wed, 8 Jan 2025 15:31:51 -0600 Subject: [PATCH 3/5] Fixed rubocop warnings --- test/services/workflow_executions/update_service_test.rb | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/test/services/workflow_executions/update_service_test.rb b/test/services/workflow_executions/update_service_test.rb index 39f84c80c9..0327849b41 100644 --- a/test/services/workflow_executions/update_service_test.rb +++ b/test/services/workflow_executions/update_service_test.rb @@ -21,8 +21,7 @@ def setup valid_params = { name: 'New Name' } exception = assert_raises(ActionPolicy::Unauthorized) do - WorkflowExecutions::UpdateService.new(@workflow_execution, users(:michelle_doe), - valid_params).execute + WorkflowExecutions::UpdateService.new(@workflow_execution, users(:michelle_doe), valid_params).execute end assert_equal WorkflowExecutionPolicy, exception.policy @@ -47,8 +46,7 @@ def setup valid_params = { name: 'New Name' } exception = assert_raises(ActionPolicy::Unauthorized) do - WorkflowExecutions::UpdateService.new(workflow_execution, users(:ryan_doe), - valid_params).execute + WorkflowExecutions::UpdateService.new(workflow_execution, users(:ryan_doe), valid_params).execute end assert_equal WorkflowExecutionPolicy, exception.policy From 86ca610faf85cfdb22b3f77494046422ae548750 Mon Sep 17 00:00:00 2001 From: Deep Sidhu Date: Thu, 9 Jan 2025 11:24:43 -0600 Subject: [PATCH 4/5] Added routes for project workflow executions and associated tests --- .../workflow_executions_controller.rb | 4 ++++ config/routes/project.rb | 2 +- .../workflow_executions_controller_test.rb | 20 +++++++++++++++++++ 3 files changed, 25 insertions(+), 1 deletion(-) diff --git a/app/controllers/projects/workflow_executions_controller.rb b/app/controllers/projects/workflow_executions_controller.rb index 58207291d1..b6d255ace9 100644 --- a/app/controllers/projects/workflow_executions_controller.rb +++ b/app/controllers/projects/workflow_executions_controller.rb @@ -19,6 +19,10 @@ def workflow_execution @workflow_execution = WorkflowExecution.find_by!(id: params[:id], submitter: @project.namespace.automation_bot) end + def workflow_execution_update_params + params.require(:workflow_execution).permit(:name) + end + def load_workflows authorized_scope(WorkflowExecution, type: :relation, as: :automated, scope_options: { project: @project }) end diff --git a/config/routes/project.rb b/config/routes/project.rb index fb80f5d485..41583c954a 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -35,7 +35,7 @@ if Irida::Pipelines.instance.available_pipelines.any? resources :automated_workflow_executions - resources :workflow_executions, only: %i[index destroy show] do + resources :workflow_executions, only: %i[index destroy show edit update] do member do put :cancel end diff --git a/test/controllers/projects/workflow_executions_controller_test.rb b/test/controllers/projects/workflow_executions_controller_test.rb index 0a3cb72468..a7d442221e 100644 --- a/test/controllers/projects/workflow_executions_controller_test.rb +++ b/test/controllers/projects/workflow_executions_controller_test.rb @@ -214,5 +214,25 @@ class WorkflowExecutionsControllerTest < ActionDispatch::IntegrationTest assert_redirected_to namespace_project_workflow_executions_path(@namespace, @project) end + + test 'analyst or higher access level can update workflow execution name post launch' do + update_params = { workflow_execution: { name: 'New Name' } } + + put namespace_project_workflow_execution_path(@namespace, @project, @workflow_execution, format: :turbo_stream), + params: update_params + + assert_response :success + end + + test 'access level less than analyst cannot update workflow execution name post launch for an automated workflow' do + sign_in users(:ryan_doe) + + update_params = { workflow_execution: { name: 'New Name' } } + + put namespace_project_workflow_execution_path(@namespace, @project, @workflow_execution, format: :turbo_stream), + params: update_params + + assert_response :unauthorized + end end end From c01aa5cd6a74c8d5d24ca3eb099a4a4cec73f2da Mon Sep 17 00:00:00 2001 From: Deep Sidhu Date: Thu, 9 Jan 2025 11:51:09 -0600 Subject: [PATCH 5/5] Fixed rubocop warnings --- .../projects/workflow_executions_controller_test.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/controllers/projects/workflow_executions_controller_test.rb b/test/controllers/projects/workflow_executions_controller_test.rb index a7d442221e..c191cd561b 100644 --- a/test/controllers/projects/workflow_executions_controller_test.rb +++ b/test/controllers/projects/workflow_executions_controller_test.rb @@ -219,7 +219,7 @@ class WorkflowExecutionsControllerTest < ActionDispatch::IntegrationTest update_params = { workflow_execution: { name: 'New Name' } } put namespace_project_workflow_execution_path(@namespace, @project, @workflow_execution, format: :turbo_stream), - params: update_params + params: update_params assert_response :success end @@ -230,7 +230,7 @@ class WorkflowExecutionsControllerTest < ActionDispatch::IntegrationTest update_params = { workflow_execution: { name: 'New Name' } } put namespace_project_workflow_execution_path(@namespace, @project, @workflow_execution, format: :turbo_stream), - params: update_params + params: update_params assert_response :unauthorized end