Update mutation logic

- Stop using public_send
- Use ServiceResponse
- Fix spec styles
- Update docs

app/graphql/mutations/alert_management/base.rb

@@ -20,7 +20,7 @@ module Mutations
 
       private
 
-      def find_alert(project_path:, iid:)
+      def find_object(project_path:, iid:)
         project = resolve_project(full_path: project_path)
 
         return unless project

app/graphql/mutations/alert_management/change_alert_status.rb

-# frozen_string_literal: true
-
-module Mutations
-  module AlertManagement
-    class ChangeAlertStatus < Base
-      graphql_name 'ChangeAlertStatus'
-
-      argument :status, Types::AlertManagement::StatusEnum,
-               required: true,
-               description: "The status to set the alert"
-
-      def resolve(args)
-        alert = find_alert(project_path: args[:project_path], iid: args[:iid])
-
-        if alert
-          service = ::AlertManagement::AlertService.new(alert)
-          service.set_status!(args[:status])
-        else
-          self.errors = Array('Alert could not be found')
-        end
-
-        {
-          alert: alert,
-          errors: errors
-        }
-      end
-
-      private
-
-      attr_accessor :errors
-    end
-  end
-end

app/graphql/mutations/alert_management/update_alert_status.rb

+# frozen_string_literal: true
+
+module Mutations
+  module AlertManagement
+    class UpdateAlertStatus < Base
+      graphql_name 'UpdateAlertStatus'
+
+      argument :status, Types::AlertManagement::StatusEnum,
+               required: true,
+               description: 'The status to set the alert'
+
+      authorize :read_alert_management_alerts
+
+      def resolve(args)
+        alert = authorized_find!(project_path: args[:project_path], iid: args[:iid])
+
+        if alert
+          result = update_status(alert, args[:status])
+          prepare_response(result)
+        else
+          {
+            alert: alert,
+            errors: ['Alert could not be found']
+          }
+        end
+      end
+
+      private
+
+      def update_status(alert, status)
+        service = ::AlertManagement::UpdateAlertStatusService.new(alert, status)
+        service.execute!
+      end
+
+      def prepare_response(result)
+        {
+          alert: result.payload[:alert],
+          errors: result.message.present? ? [result.message] : []
+        }
+      end
+    end
+  end
+end

app/graphql/types/mutation_type.rb

@@ -7,7 +7,7 @@ module Types
     graphql_name 'Mutation'
 
     mount_mutation Mutations::Admin::SidekiqQueues::DeleteJobs
-    mount_mutation Mutations::AlertManagement::ChangeAlertStatus
+    mount_mutation Mutations::AlertManagement::UpdateAlertStatus
     mount_mutation Mutations::AwardEmojis::Add
     mount_mutation Mutations::AwardEmojis::Remove
     mount_mutation Mutations::AwardEmojis::Toggle

app/services/alert_management/alert_service.rb

-# frozen_string_literal: true
-
-module AlertManagement
-  class AlertService
-    attr_reader :alert
-
-    def initialize(alert)
-      @alert = alert
-    end
-
-    def set_status!(status)
-      return unless AlertManagement::Alert.statuses.key?(status.to_s)
-
-      # rubocop:disable GitlabSecurity/PublicSend
-      alert.public_send("#{status}!")
-      # rubocop:enable GitlabSecurity/PublicSend
-    end
-  end
-end

app/services/alert_management/update_alert_status_service.rb

+# frozen_string_literal: true
+
+module AlertManagement
+  class UpdateAlertStatusService
+    def initialize(alert, status)
+      @alert = alert
+      @status = status
+    end
+
+    def execute!
+      return error_response('Invalid status') unless AlertManagement::Alert.statuses.key?(status.to_s)
+
+      alert.status = status
+
+      return ServiceResponse.success(payload: { alert: alert }) if alert.save
+
+      error_response
+    end
+
+    private
+
+    def error_response(message)
+      ServiceResponse.error(payload: { alert: alert }, message: message)
+    end
+
+    attr_reader :alert, :status
+  end
+end

doc/api/graphql/reference/gitlab_schema.graphql

@@ -653,51 +653,6 @@ type Branch {
   name: String!
 }
 
-"""
-Autogenerated input type of ChangeAlertStatus
-"""
-input ChangeAlertStatusInput {
-  """
-  A unique identifier for the client performing the mutation.
-  """
-  clientMutationId: String
-
-  """
-  The iid of the alert to mutate
-  """
-  iid: String!
-
-  """
-  The project the alert to mutate is in
-  """
-  projectPath: ID!
-
-  """
-  The status to set the alert
-  """
-  status: AlertManagementStatus!
-}
-
-"""
-Autogenerated return type of ChangeAlertStatus
-"""
-type ChangeAlertStatusPayload {
-  """
-  The alert after mutation
-  """
-  alert: AlertManagementAlert
-
-  """
-  A unique identifier for the client performing the mutation.
-  """
-  clientMutationId: String
-
-  """
-  Reasons why the mutation failed.
-  """
-  errors: [String!]!
-}
-
 type Commit {
   """
   Author of the commit
@@ -6087,7 +6042,6 @@ type Mutation {
   addProjectToSecurityDashboard(input: AddProjectToSecurityDashboardInput!): AddProjectToSecurityDashboardPayload
   adminSidekiqQueuesDeleteJobs(input: AdminSidekiqQueuesDeleteJobsInput!): AdminSidekiqQueuesDeleteJobsPayload
   boardListUpdateLimitMetrics(input: BoardListUpdateLimitMetricsInput!): BoardListUpdateLimitMetricsPayload
-  changeAlertStatus(input: ChangeAlertStatusInput!): ChangeAlertStatusPayload
   createBranch(input: CreateBranchInput!): CreateBranchPayload
   createDiffNote(input: CreateDiffNoteInput!): CreateDiffNotePayload
   createEpic(input: CreateEpicInput!): CreateEpicPayload
@@ -6120,6 +6074,7 @@ type Mutation {
   todoRestoreMany(input: TodoRestoreManyInput!): TodoRestoreManyPayload
   todosMarkAllDone(input: TodosMarkAllDoneInput!): TodosMarkAllDonePayload
   toggleAwardEmoji(input: ToggleAwardEmojiInput!): ToggleAwardEmojiPayload
+  updateAlertStatus(input: UpdateAlertStatusInput!): UpdateAlertStatusPayload
   updateEpic(input: UpdateEpicInput!): UpdateEpicPayload
 
   """
@@ -9775,6 +9730,51 @@ enum TypeEnum {
   project
 }
 
+"""
+Autogenerated input type of UpdateAlertStatus
+"""
+input UpdateAlertStatusInput {
+  """
+  A unique identifier for the client performing the mutation.
+  """
+  clientMutationId: String
+
+  """
+  The iid of the alert to mutate
+  """
+  iid: String!
+
+  """
+  The project the alert to mutate is in
+  """
+  projectPath: ID!
+
+  """
+  The status to set the alert
+  """
+  status: AlertManagementStatus!
+}
+
+"""
+Autogenerated return type of UpdateAlertStatus
+"""
+type UpdateAlertStatusPayload {
+  """
+  The alert after mutation
+  """
+  alert: AlertManagementAlert
+
+  """
+  A unique identifier for the client performing the mutation.
+  """
+  clientMutationId: String
+
+  """
+  Reasons why the mutation failed.
+  """
+  errors: [String!]!
+}
+
 input UpdateDiffImagePositionInput {
   """
   Total height of the image

doc/api/graphql/reference/index.md

@@ -140,16 +140,6 @@ Autogenerated return type of BoardListUpdateLimitMetrics
 | `commit` | Commit | Commit for the branch |
 | `name` | String! | Name of the branch |
 
-## ChangeAlertStatusPayload
-
-Autogenerated return type of ChangeAlertStatus
-
-| Name  | Type  | Description |
-| ---   |  ---- | ----------  |
-| `alert` | AlertManagementAlert | The alert after mutation |
-| `clientMutationId` | String | A unique identifier for the client performing the mutation. |
-| `errors` | String! => Array | Reasons why the mutation failed. |
-
 ## Commit
 
 | Name  | Type  | Description |
@@ -1525,6 +1515,16 @@ Represents a directory
 | `type` | EntryType! | Type of tree entry |
 | `webUrl` | String | Web URL for the tree entry (directory) |
 
+## UpdateAlertStatusPayload
+
+Autogenerated return type of UpdateAlertStatus
+
+| Name  | Type  | Description |
+| ---   |  ---- | ----------  |
+| `alert` | AlertManagementAlert | The alert after mutation |
+| `clientMutationId` | String | A unique identifier for the client performing the mutation. |
+| `errors` | String! => Array | Reasons why the mutation failed. |
+
 ## UpdateEpicPayload
 
 Autogenerated return type of UpdateEpic

spec/graphql/mutations/alert_management/change_alert_status_spec.rb → spec/graphql/mutations/alert_management/update_alert_status_spec.rb

@@ -2,9 +2,9 @@
 
 require 'spec_helper'
 
-describe Mutations::AlertManagement::ChangeAlertStatus do
+describe Mutations::AlertManagement::UpdateAlertStatus do
   let_it_be(:current_user) { create(:user) }
-  let_it_be(:alert) { create(:alert_management_alert) }
+  let_it_be(:alert) { create(:alert_management_alert, status: 'triggered') }
   let(:project) { alert.project }
   let(:new_status) { 'acknowledged' }
   let(:args) { { status: new_status, project_path: project.full_path, iid: alert.iid } }
@@ -17,19 +17,13 @@ describe Mutations::AlertManagement::ChangeAlertStatus do
         project.add_developer(current_user)
       end
 
-      it 'does not change the status' do
+      it 'changes the status' do
         expect { resolve }.to change { alert.reload.status }.from(alert.status).to(new_status)
       end
     end
 
-    context 'user has no access' do
-      it 'does not change the status' do
-        expect { resolve }.not_to change { alert.reload.status }
-      end
-
-      it 'sets the error' do
-        expect(resolve.dig(:errors)).not_to be_empty
-      end
+    it 'raises an error if the resource is not accessible to the user' do
+      expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)
     end
   end
 

spec/services/alert_management/alert_service_spec.rb → spec/services/alert_management/update_alert_status_service_spec.rb

@@ -2,26 +2,27 @@
 
 require 'spec_helper'
 
-describe AlertManagement::AlertService do
-  let_it_be(:alert) { create(:alert_management_alert) }
-  let(:instance) { described_class.new(alert) }
+describe AlertManagement::UpdateAlertStatusService do
+  let_it_be(:alert) { create(:alert_management_alert, status: 'triggered') }
 
-  describe '#set_status!' do
-    subject(:set_status) { instance.set_status!(new_status) }
+  describe '#execute!' do
+    subject(:execute) { described_class.new(alert, new_status).execute! }
 
     let(:new_status) { 'acknowledged' }
 
     it 'updates the status' do
-      expect { set_status }.to change { alert.status }.to(new_status)
+      expect { execute }.to change { alert.status }.to(new_status)
     end
 
     context 'with unknown status' do
       let(:new_status) { 'random_status' }
 
-      it { is_expected.to be_nil }
+      it 'returns an error' do
+        expect(execute.status).to eq(:error)
+      end
 
       it 'does not update the status' do
-        expect { set_status }.not_to change { alert.status }
+        expect { execute }.not_to change { alert.status }
       end
     end
   end