Merge branch '221242-change-alert-severity-and-status-sort-order' into 'master'

Resolve "Fix severity and status sort order on alert list"

Closes #221242

See merge request gitlab-org/gitlab!35774

app/graphql/types/alert_management/alert_sort_enum.rb

@@ -16,10 +16,10 @@ module Types
       value 'UPDATED_TIME_DESC', 'Created time by descending order', value: :updated_at_desc
       value 'EVENT_COUNT_ASC', 'Events count by ascending order', value: :event_count_asc
       value 'EVENT_COUNT_DESC', 'Events count by descending order', value: :event_count_desc
-      value 'SEVERITY_ASC', 'Severity by ascending order', value: :severity_asc
-      value 'SEVERITY_DESC', 'Severity by descending order', value: :severity_desc
-      value 'STATUS_ASC', 'Status by ascending order', value: :status_asc
-      value 'STATUS_DESC', 'Status by descending order', value: :status_desc
+      value 'SEVERITY_ASC', 'Severity from less critical to more critical', value: :severity_asc
+      value 'SEVERITY_DESC', 'Severity from more critical to less critical', value: :severity_desc
+      value 'STATUS_ASC', 'Status by order: Ignored > Resolved > Acknowledged > Triggered', value: :status_asc
+      value 'STATUS_DESC', 'Status by order: Triggered > Acknowledged > Resolved > Ignored', value: :status_desc
     end
   end
 end

app/models/alert_management/alert.rb

@@ -121,8 +121,16 @@ module AlertManagement
     scope :order_start_time,    -> (sort_order) { order(started_at: sort_order) }
     scope :order_end_time,      -> (sort_order) { order(ended_at: sort_order) }
     scope :order_event_count,   -> (sort_order) { order(events: sort_order) }
-    scope :order_severity,      -> (sort_order) { order(severity: sort_order) }
-    scope :order_status,        -> (sort_order) { order(status: sort_order) }
+
+    # Ascending sort order sorts severity from less critical to more critical.
+    # Descending sort order sorts severity from more critical to less critical.
+    # https://gitlab.com/gitlab-org/gitlab/-/issues/221242#what-is-the-expected-correct-behavior
+    scope :order_severity,      -> (sort_order) { order(severity: sort_order == :asc ? :desc : :asc) }
+
+    # 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 :counts_by_status, -> { group(:status).count }
     scope :counts_by_project_id, -> { group(:project_id).count }

changelogs/unreleased/221242-change-alert-severity-and-status-sort-order.yml

+---
+title: Change the sort order for alert severity and status.
+merge_request: 35774
+author:
+type: fixed

doc/api/graphql/reference/gitlab_schema.graphql

@@ -395,12 +395,12 @@ enum AlertManagementAlertSort {
   EVENT_COUNT_DESC
 
   """
-  Severity by ascending order
+  Severity from less critical to more critical
   """
   SEVERITY_ASC
 
   """
-  Severity by descending order
+  Severity from more critical to less critical
   """
   SEVERITY_DESC
 
@@ -415,12 +415,12 @@ enum AlertManagementAlertSort {
   STARTED_AT_DESC
 
   """
-  Status by ascending order
+  Status by order: Ignored > Resolved > Acknowledged > Triggered
   """
   STATUS_ASC
 
   """
-  Status by descending order
+  Status by order: Triggered > Acknowledged > Resolved > Ignored
   """
   STATUS_DESC
 

doc/api/graphql/reference/gitlab_schema.json

@@ -1103,25 +1103,25 @@
             },
             {
               "name": "SEVERITY_ASC",
-              "description": "Severity by ascending order",
+              "description": "Severity from less critical to more critical",
               "isDeprecated": false,
               "deprecationReason": null
             },
             {
               "name": "SEVERITY_DESC",
-              "description": "Severity by descending order",
+              "description": "Severity from more critical to less critical",
               "isDeprecated": false,
               "deprecationReason": null
             },
             {
               "name": "STATUS_ASC",
-              "description": "Status by ascending order",
+              "description": "Status by order: Ignored > Resolved > Acknowledged > Triggered",
               "isDeprecated": false,
               "deprecationReason": null
             },
             {
               "name": "STATUS_DESC",
-              "description": "Status by descending order",
+              "description": "Status by order: Triggered > Acknowledged > Resolved > Ignored",
               "isDeprecated": false,
               "deprecationReason": null
             }

spec/factories/alert_management/alerts.rb

@@ -75,10 +75,30 @@ FactoryBot.define do
       without_ended_at
     end
 
-    trait :low_severity do
+    trait :critical do
+      severity { 'critical' }
+    end
+
+    trait :high do
+      severity { 'high' }
+    end
+
+    trait :medium do
+      severity { 'medium' }
+    end
+
+    trait :low do
       severity { 'low' }
     end
 
+    trait :info do
+      severity { 'info' }
+    end
+
+    trait :unknown do
+      severity { 'unknown' }
+    end
+
     trait :prometheus do
       monitoring_tool { Gitlab::AlertManagement::AlertParams::MONITORING_TOOLS[:prometheus] }
     end
@@ -91,7 +111,7 @@ FactoryBot.define do
       with_monitoring_tool
       with_host
       with_description
-      low_severity
+      low
     end
   end
 end

spec/finders/alert_management/alerts_finder_spec.rb

@@ -11,7 +11,7 @@ RSpec.describe AlertManagement::AlertsFinder, '#execute' do
   let(:params) { {} }
 
   describe '#execute' do
-    subject { described_class.new(current_user, project, params).execute }
+    subject(:execute) { described_class.new(current_user, project, params).execute }
 
     context 'user is not a developer or above' do
       it { is_expected.to be_empty }
@@ -144,81 +144,55 @@ RSpec.describe AlertManagement::AlertsFinder, '#execute' do
         end
 
         context 'when sorting by severity' do
-          let_it_be(:alert_critical) { create(:alert_management_alert, project: project, severity: :critical) }
-          let_it_be(:alert_high) { create(:alert_management_alert, project: project, severity: :high) }
-          let_it_be(:alert_medium) { create(:alert_management_alert, project: project, severity: :medium) }
-          let_it_be(:alert_low) { create(:alert_management_alert, project: project, severity: :low) }
-          let_it_be(:alert_info) { create(:alert_management_alert, project: project, severity: :info) }
-          let_it_be(:alert_unknown) { create(:alert_management_alert, project: project, severity: :unknown) }
-
-          context 'sorts alerts ascending' do
+          let_it_be(:alert_critical) { create(:alert_management_alert, :critical, project: project) }
+          let_it_be(:alert_high) { create(:alert_management_alert, :high, project: project) }
+          let_it_be(:alert_medium) { create(:alert_management_alert, :medium, project: project) }
+          let_it_be(:alert_low) { create(:alert_management_alert, :low, project: project) }
+          let_it_be(:alert_info) { create(:alert_management_alert, :info, project: project) }
+          let_it_be(:alert_unknown) { create(:alert_management_alert, :unknown, project: project) }
+
+          context 'with ascending sort order' do
             let(:params) { { sort: 'severity_asc' } }
 
-            it do
-              is_expected.to eq [
-                alert_2,
-                alert_critical,
-                alert_1,
-                alert_high,
-                alert_medium,
-                alert_low,
-                alert_info,
-                alert_unknown
-              ]
+            it 'sorts alerts by severity from less critical to more critical' do
+              expect(execute.pluck(:severity).uniq).to eq(%w(unknown info low medium high critical))
             end
           end
 
-          context 'sorts alerts descending' do
+          context 'with descending sort order' do
             let(:params) { { sort: 'severity_desc' } }
 
-            it do
-              is_expected.to eq [
-                alert_unknown,
-                alert_info,
-                alert_low,
-                alert_medium,
-                alert_1,
-                alert_high,
-                alert_critical,
-                alert_2
-              ]
+            it 'sorts alerts by severity from more critical to less critical' do
+              expect(execute.pluck(:severity).uniq).to eq(%w(critical high medium low info unknown))
             end
           end
         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) }
           let_it_be(:alert_ignored) { create(:alert_management_alert, :ignored, project: project) }
 
-          context 'sorts alerts ascending' do
+          context 'with ascending sort order' do
             let(:params) { { sort: 'status_asc' } }
 
-            it do
-              is_expected.to eq [
-                alert_triggered,
-                alert_acknowledged,
-                alert_1,
-                alert_resolved,
-                alert_2,
-                alert_ignored
-              ]
+            it 'sorts by status: Ignored > Resolved > Acknowledged > Triggered' do
+              expect(execute.map(&:status).uniq).to eq([ignored, resolved, acknowledged, triggered])
             end
           end
 
-          context 'sorts alerts descending' do
+          context 'with descending sort order' do
             let(:params) { { sort: 'status_desc' } }
 
-            it do
-              is_expected.to eq [
-                alert_2,
-                alert_ignored,
-                alert_1,
-                alert_resolved,
-                alert_acknowledged,
-                alert_triggered
-              ]
+            it 'sorts by status: Triggered > Acknowledged > Resolved > Ignored' do
+              expect(execute.map(&:status).uniq).to eq([triggered, acknowledged, resolved, ignored])
             end
           end
         end

spec/requests/api/graphql/project/alert_management/alerts_spec.rb

@@ -110,14 +110,14 @@ RSpec.describe 'getting Alert Management Alerts' do
         it_behaves_like 'a working graphql query'
 
         it 'sorts in the correct order' do
-          expect(iids).to eq [resolved_alert.iid.to_s, triggered_alert.iid.to_s]
+          expect(iids).to eq [triggered_alert.iid.to_s, resolved_alert.iid.to_s]
         end
 
         context 'ascending order' do
           let(:params) { 'sort: SEVERITY_ASC' }
 
           it 'sorts in the correct order' do
-            expect(iids).to eq [triggered_alert.iid.to_s, resolved_alert.iid.to_s]
+            expect(iids).to eq [resolved_alert.iid.to_s, triggered_alert.iid.to_s]
           end
         end
       end