Merge branch 'ck3g-update-alert-management-status-enum-to-use-status-names' into 'master'

Make Alert::STATUSES constant private

See merge request gitlab-org/gitlab!44589

app/finders/alert_management/alerts_finder.rb

@@ -2,8 +2,8 @@
 
 module AlertManagement
   class AlertsFinder
-    # @return [Hash<Integer,Integer>] Mapping of status id to count
-    #          ex) { 0: 6, ...etc }
+    # @return [Hash<Symbol,Integer>] Mapping of status id to count
+    #          ex) { triggered: 6, ...etc }
     def self.counts_by_status(current_user, project, params = {})
       new(current_user, project, params).execute.counts_by_status
     end
@@ -35,7 +35,7 @@ module AlertManagement
     end
 
     def by_status(collection)
-      values = AlertManagement::Alert::STATUSES.values & Array(params[:status])
+      values = AlertManagement::Alert.status_names & Array(params[:status])
 
       values.present? ? collection.for_status(values) : collection
     end

app/graphql/types/alert_management/alert_status_counts_type.rb

@@ -9,11 +9,11 @@ module Types
 
       authorize :read_alert_management_alert
 
-      ::Gitlab::AlertManagement::AlertStatusCounts::STATUSES.each_key do |status|
+      ::AlertManagement::Alert.status_names.each do |status|
         field status,
               GraphQL::INT_TYPE,
               null: true,
-              description: "Number of alerts with status #{status.upcase} for the project"
+              description: "Number of alerts with status #{status.to_s.upcase} for the project"
       end
 
       field :open,

app/graphql/types/alert_management/alert_type.rb

@@ -40,7 +40,8 @@ module Types
       field :status,
             AlertManagement::StatusEnum,
             null: true,
-            description: 'Status of the alert'
+            description: 'Status of the alert',
+            method: :status_name
 
       field :service,
             GraphQL::STRING_TYPE,

app/graphql/types/alert_management/status_enum.rb

@@ -6,8 +6,8 @@ module Types
       graphql_name 'AlertManagementStatus'
       description 'Alert status values'
 
-      ::AlertManagement::Alert::STATUSES.each do |name, value|
-        value name.upcase, value: value, description: "#{name.to_s.titleize} status"
+      ::AlertManagement::Alert.status_names.each do |status|
+        value status.to_s.upcase, value: status, description: "#{status.to_s.titleize} status"
       end
     end
   end

app/models/alert_management/alert.rb

@@ -20,6 +20,7 @@ module AlertManagement
       resolved: 2,
       ignored: 3
     }.freeze
+    private_constant :STATUSES
 
     belongs_to :project
     belongs_to :issue, optional: true
@@ -109,7 +110,7 @@ module AlertManagement
     delegate :details_url, to: :present
 
     scope :for_iid, -> (iid) { where(iid: iid) }
-    scope :for_status, -> (status) { where(status: status) }
+    scope :for_status, -> (status) { with_status(status) }
     scope :for_fingerprint, -> (project, fingerprint) { where(project: project, fingerprint: fingerprint) }
     scope :for_environment, -> (environment) { where(environment: environment) }
     scope :search, -> (query) { fuzzy_search(query, [:title, :description, :monitoring_tool, :service]) }
@@ -130,13 +131,33 @@ module AlertManagement
     # Ascending sort order sorts statuses: Ignored > Resolved > Acknowledged > Triggered
     # Descending sort order sorts statuses: Triggered > Acknowledged > Resolved > Ignored
     # https://gitlab.com/gitlab-org/gitlab/-/issues/221242#what-is-the-expected-correct-behavior
-    scope :order_status,        -> (sort_order) { order(status: sort_order == :asc ? :desc : :asc) }
+    scope :order_status, -> (sort_order) { order(status: sort_order == :asc ? :desc : :asc) }
 
-    scope :counts_by_status, -> { group(:status).count }
     scope :counts_by_project_id, -> { group(:project_id).count }
 
     alias_method :state, :status_name
 
+    def self.state_machine_statuses
+      @state_machine_statuses ||= state_machines[:status].states.to_h { |s| [s.name, s.value] }
+    end
+    private_class_method :state_machine_statuses
+
+    def self.status_value(name)
+      state_machine_statuses[name]
+    end
+
+    def self.status_name(raw_status)
+      state_machine_statuses.key(raw_status)
+    end
+
+    def self.counts_by_status
+      group(:status).count.transform_keys { |k| status_name(k) }
+    end
+
+    def self.status_names
+      @status_names ||= state_machine_statuses.keys
+    end
+
     def self.sort_by_attribute(method)
       case method.to_s
       when 'started_at_asc'     then order_start_time(:asc)

app/services/alert_management/alerts/update_service.rb

@@ -13,6 +13,7 @@ module AlertManagement
         @current_user = current_user
         @params = params
         @param_errors = []
+        @status = params.delete(:status)
       end
 
       def execute
@@ -35,7 +36,7 @@ module AlertManagement
 
       private
 
-      attr_reader :alert, :current_user, :params, :param_errors
+      attr_reader :alert, :current_user, :params, :param_errors, :status
       delegate :resolved?, to: :alert
 
       def allowed?
@@ -68,8 +69,12 @@ module AlertManagement
         param_errors << message
       end
 
+      def param_errors?
+        params.empty? && status.blank?
+      end
+
       def filter_params
-        param_errors << _('Please provide attributes to update') if params.empty?
+        param_errors << _('Please provide attributes to update') if param_errors?
 
         filter_status
         filter_assignees
@@ -110,9 +115,9 @@ module AlertManagement
 
       # ------ Status-related behavior -------
       def filter_status
-        return unless params[:status]
+        return unless status
 
-        status_event = alert.status_event_for(status_key)
+        status_event = alert.status_event_for(status)
 
         unless status_event
           param_errors << _('Invalid status')
@@ -122,13 +127,6 @@ module AlertManagement
         params[:status_event] = status_event
       end
 
-      def status_key
-        strong_memoize(:status_key) do
-          status = params.delete(:status)
-          AlertManagement::Alert::STATUSES.key(status)
-        end
-      end
-
       def handle_status_change
         add_status_change_system_note
         resolve_todos if resolved?
@@ -144,7 +142,7 @@ module AlertManagement
 
       def filter_duplicate
         # Only need to check if changing to an open status
-        return unless params[:status_event] && AlertManagement::Alert.open_status?(status_key)
+        return unless params[:status_event] && AlertManagement::Alert.open_status?(status)
 
         param_errors << unresolved_alert_error if duplicate_alert?
       end

lib/gitlab/alert_management/alert_status_counts.rb

@@ -6,8 +6,6 @@ module Gitlab
     class AlertStatusCounts
       include Gitlab::Utils::StrongMemoize
 
-      STATUSES = ::AlertManagement::Alert::STATUSES
-
       attr_reader :project
 
       def self.declarative_policy_class
@@ -21,7 +19,7 @@ module Gitlab
       end
 
       # Define method for each status
-      STATUSES.each_key do |status|
+      ::AlertManagement::Alert.status_names.each do |status|
         define_method(status) { counts[status] }
       end
 
@@ -44,9 +42,7 @@ module Gitlab
       end
 
       def counts_by_status
-        ::AlertManagement::AlertsFinder
-          .counts_by_status(current_user, project, params)
-          .transform_keys { |status_id| STATUSES.key(status_id) }
+        ::AlertManagement::AlertsFinder.counts_by_status(current_user, project, params)
       end
     end
   end

spec/factories/alert_management/alerts.rb

@@ -56,22 +56,22 @@ FactoryBot.define do
     end
 
     trait :triggered do
-      status { AlertManagement::Alert::STATUSES[:triggered] }
+      status { AlertManagement::Alert.status_value(:triggered) }
       without_ended_at
     end
 
     trait :acknowledged do
-      status { AlertManagement::Alert::STATUSES[:acknowledged] }
+      status { AlertManagement::Alert.status_value(:acknowledged) }
       without_ended_at
     end
 
     trait :resolved do
-      status { AlertManagement::Alert::STATUSES[:resolved] }
+      status { AlertManagement::Alert.status_value(:resolved) }
       with_ended_at
     end
 
     trait :ignored do
-      status { AlertManagement::Alert::STATUSES[:ignored] }
+      status { AlertManagement::Alert.status_value(:ignored) }
       without_ended_at
     end
 

spec/finders/alert_management/alerts_finder_spec.rb

@@ -39,19 +39,19 @@ RSpec.describe AlertManagement::AlertsFinder, '#execute' do
       end
 
       context 'status given' do
-        let(:params) { { status: AlertManagement::Alert::STATUSES[:resolved] } }
+        let(:params) { { status: :resolved } }
 
         it { is_expected.to match_array(resolved_alert) }
 
         context 'with an array of statuses' do
           let(:triggered_alert) { create(:alert_management_alert) }
-          let(:params) { { status: [AlertManagement::Alert::STATUSES[:resolved]] } }
+          let(:params) { { status: [:resolved] } }
 
           it { is_expected.to match_array(resolved_alert) }
         end
 
         context 'with no alerts of status' do
-          let(:params) { { status: AlertManagement::Alert::STATUSES[:acknowledged] } }
+          let(:params) { { status: :acknowledged } }
 
           it { is_expected.to be_empty }
         end
@@ -169,12 +169,6 @@ RSpec.describe AlertManagement::AlertsFinder, '#execute' do
         end
 
         context 'when sorting by status' do
-          let(:statuses) { AlertManagement::Alert::STATUSES }
-          let(:triggered) { statuses[:triggered] }
-          let(:acknowledged) { statuses[:acknowledged] }
-          let(:resolved) { statuses[:resolved] }
-          let(:ignored) { statuses[:ignored] }
-
           let_it_be(:alert_triggered) { create(:alert_management_alert, project: project) }
           let_it_be(:alert_acknowledged) { create(:alert_management_alert, :acknowledged, project: project) }
           let_it_be(:alert_resolved) { create(:alert_management_alert, :resolved, project: project) }
@@ -184,7 +178,7 @@ RSpec.describe AlertManagement::AlertsFinder, '#execute' do
             let(:params) { { sort: 'status_asc' } }
 
             it 'sorts by status: Ignored > Resolved > Acknowledged > Triggered' do
-              expect(execute.map(&:status).uniq).to eq([ignored, resolved, acknowledged, triggered])
+              expect(execute.map(&:status_name).uniq).to eq([:ignored, :resolved, :acknowledged, :triggered])
             end
           end
 
@@ -192,7 +186,7 @@ RSpec.describe AlertManagement::AlertsFinder, '#execute' do
             let(:params) { { sort: 'status_desc' } }
 
             it 'sorts by status: Triggered > Acknowledged > Resolved > Ignored' do
-              expect(execute.map(&:status).uniq).to eq([triggered, acknowledged, resolved, ignored])
+              expect(execute.map(&:status_name).uniq).to eq([:triggered, :acknowledged, :resolved, :ignored])
             end
           end
         end
@@ -261,12 +255,12 @@ RSpec.describe AlertManagement::AlertsFinder, '#execute' do
       project.add_developer(current_user)
     end
 
-    it { is_expected.to match({ 2 => 1, 3 => 1 }) } # one resolved and one ignored
+    it { is_expected.to match(resolved: 1, ignored: 1) }
 
     context 'when filtering params are included' do
-      let(:params) { { status: AlertManagement::Alert::STATUSES[:resolved] } }
+      let(:params) { { status: :resolved } }
 
-      it { is_expected.to match({ 2 => 1 }) } # one resolved
+      it { is_expected.to match(resolved: 1) }
     end
   end
 end

spec/graphql/types/alert_management/status_enum_spec.rb

@@ -9,10 +9,10 @@ RSpec.describe GitlabSchema.types['AlertManagementStatus'] do
     using RSpec::Parameterized::TableSyntax
 
     where(:status_name, :status_value) do
-      'TRIGGERED'    | 0
-      'ACKNOWLEDGED' | 1
-      'RESOLVED'     | 2
-      'IGNORED'      | 3
+      'TRIGGERED'    | :triggered
+      'ACKNOWLEDGED' | :acknowledged
+      'RESOLVED'     | :resolved
+      'IGNORED'      | :ignored
     end
 
     with_them do

spec/lib/gitlab/alert_management/alert_status_counts_spec.rb

@@ -18,7 +18,7 @@ RSpec.describe Gitlab::AlertManagement::AlertStatusCounts do
         expect(counts.open).to eq(0)
         expect(counts.all).to eq(0)
 
-        AlertManagement::Alert::STATUSES.each_key do |status|
+        ::AlertManagement::Alert.status_names.each do |status|
           expect(counts.send(status)).to eq(0)
         end
       end
@@ -39,7 +39,7 @@ RSpec.describe Gitlab::AlertManagement::AlertStatusCounts do
       end
 
       context 'when filtering params are included' do
-        let(:params) { { status: AlertManagement::Alert::STATUSES[:resolved] } }
+        let(:params) { { status: :resolved } }
 
         it 'returns the correct counts for each status' do
           expect(counts.open).to eq(0)

spec/models/alert_management/alert_spec.rb

@@ -189,14 +189,14 @@ RSpec.describe AlertManagement::Alert do
     end
 
     describe '.for_status' do
-      let(:status) { AlertManagement::Alert::STATUSES[:resolved] }
+      let(:status) { :resolved }
 
       subject { AlertManagement::Alert.for_status(status) }
 
       it { is_expected.to match_array(resolved_alert) }
 
       context 'with multiple statuses' do
-        let(:status) { AlertManagement::Alert::STATUSES.values_at(:resolved, :ignored) }
+        let(:status) { [:resolved, :ignored] }
 
         it { is_expected.to match_array([resolved_alert, ignored_alert]) }
       end
@@ -241,19 +241,6 @@ RSpec.describe AlertManagement::Alert do
       it { is_expected.to eq([triggered_critical_alert, triggered_high_alert]) }
     end
 
-    describe '.counts_by_status' do
-      subject { described_class.counts_by_status }
-
-      it do
-        is_expected.to eq(
-          triggered_alert.status => 1,
-          acknowledged_alert.status => 1,
-          resolved_alert.status => 1,
-          ignored_alert.status => 1
-        )
-      end
-    end
-
     describe '.counts_by_project_id' do
       subject { described_class.counts_by_project_id }
 
@@ -278,6 +265,55 @@ RSpec.describe AlertManagement::Alert do
     end
   end
 
+  describe '.status_value' do
+    using RSpec::Parameterized::TableSyntax
+
+    where(:status, :status_value) do
+      :triggered    | 0
+      :acknowledged | 1
+      :resolved     | 2
+      :ignored      | 3
+      :unknown      | nil
+    end
+
+    with_them do
+      it 'returns status value by its name' do
+        expect(described_class.status_value(status)).to eq(status_value)
+      end
+    end
+  end
+
+  describe '.status_name' do
+    using RSpec::Parameterized::TableSyntax
+
+    where(:raw_status, :status) do
+      0  | :triggered
+      1  | :acknowledged
+      2  | :resolved
+      3  | :ignored
+      -1 | nil
+    end
+
+    with_them do
+      it 'returns status name by its values' do
+        expect(described_class.status_name(raw_status)).to eq(status)
+      end
+    end
+  end
+
+  describe '.counts_by_status' do
+    subject { described_class.counts_by_status }
+
+    it do
+      is_expected.to eq(
+        triggered: 1,
+        acknowledged: 1,
+        resolved: 1,
+        ignored: 1
+      )
+    end
+  end
+
   describe '.last_prometheus_alert_by_project_id' do
     subject { described_class.last_prometheus_alert_by_project_id }
 

spec/services/alert_management/alerts/update_service_spec.rb

@@ -160,7 +160,7 @@ RSpec.describe AlertManagement::Alerts::UpdateService do
 
     context 'when a status is included' do
       let(:params) { { status: new_status } }
-      let(:new_status) { AlertManagement::Alert::STATUSES[:acknowledged] }
+      let(:new_status) { :acknowledged }
 
       it 'successfully changes the status' do
         expect { response }.to change { alert.acknowledged? }.to(true)
@@ -171,13 +171,13 @@ RSpec.describe AlertManagement::Alerts::UpdateService do
       it_behaves_like 'adds a system note'
 
       context 'with unknown status' do
-        let(:new_status) { -1 }
+        let(:new_status) { :unknown_status }
 
         it_behaves_like 'error response', 'Invalid status'
       end
 
       context 'with resolving status' do
-        let(:new_status) { AlertManagement::Alert::STATUSES[:resolved] }
+        let(:new_status) { :resolved }
 
         it 'changes the status' do
           expect { response }.to change { alert.resolved? }.to(true)

spec/services/projects/alerting/notify_service_spec.rb

@@ -62,7 +62,7 @@ RSpec.describe Projects::Alerting::NotifyService do
                 title: payload_raw.fetch(:title),
                 started_at: Time.zone.parse(payload_raw.fetch(:start_time)),
                 severity: payload_raw.fetch(:severity),
-                status: AlertManagement::Alert::STATUSES[:triggered],
+                status: AlertManagement::Alert.status_value(:triggered),
                 events: 1,
                 hosts: payload_raw.fetch(:hosts),
                 payload: payload_raw.with_indifferent_access,
@@ -180,7 +180,7 @@ RSpec.describe Projects::Alerting::NotifyService do
                 title: payload_raw.fetch(:title),
                 started_at: Time.zone.parse(payload_raw.fetch(:start_time)),
                 severity: 'critical',
-                status: AlertManagement::Alert::STATUSES[:triggered],
+                status: AlertManagement::Alert.status_value(:triggered),
                 events: 1,
                 hosts: [],
                 payload: payload_raw.with_indifferent_access,