Merge branch '223151-custom-error-for-fingerprint-collision' into 'master'

Handle duplicate fingerprint error

Closes #223151

See merge request gitlab-org/gitlab!36527

app/assets/javascripts/alert_management/components/alert_details.vue

@@ -187,7 +187,7 @@ export default {
 <template>
   <div>
     <gl-alert v-if="showErrorMsg" variant="danger" @dismiss="dismissError">
-      {{ sidebarErrorMessage || $options.i18n.errorMsg }}
+      <p v-html="sidebarErrorMessage || $options.i18n.errorMsg"></p>
     </gl-alert>
     <gl-alert
       v-if="createIssueError"

app/assets/javascripts/alert_management/components/alert_management_table.vue

@@ -339,7 +339,7 @@ export default {
         data-testid="alert-error"
         @dismiss="dismissError"
       >
-        {{ errorMessage || $options.i18n.errorMsg }}
+        <p v-html="errorMessage || $options.i18n.errorMsg"></p>
       </gl-alert>
 
       <gl-tabs content-class="gl-p-0" @input="filterAlertsByStatus">

app/assets/javascripts/alert_management/components/alert_status.vue

@@ -6,6 +6,12 @@ import { trackAlertStatusUpdateOptions } from '../constants';
 import updateAlertStatus from '../graphql/mutations/update_alert_status.mutation.graphql';
 
 export default {
+  i18n: {
+    UPDATE_ALERT_STATUS_ERROR: s__(
+      'AlertManagement|There was an error while updating the status of the alert.',
+    ),
+    UPDATE_ALERT_STATUS_INSTRUCTION: s__('AlertManagement|Please try again.'),
+  },
   statuses: {
     TRIGGERED: s__('AlertManagement|Triggered'),
     ACKNOWLEDGED: s__('AlertManagement|Acknowledged'),
@@ -52,16 +58,23 @@ export default {
             projectPath: this.projectPath,
           },
         })
-        .then(() => {
+        .then(resp => {
           this.trackStatusUpdate(status);
           this.$emit('hide-dropdown');
+
+          const errors = resp.data?.updateAlertStatus?.errors || [];
+
+          if (errors[0]) {
+            this.$emit(
+              'alert-error',
+              `${this.$options.i18n.UPDATE_ALERT_STATUS_ERROR} ${errors[0]}`,
+            );
+          }
         })
         .catch(() => {
           this.$emit(
             'alert-error',
-            s__(
-              'AlertManagement|There was an error while updating the status of the alert. Please try again.',
-            ),
+            `${this.$options.i18n.UPDATE_ALERT_STATUS_ERROR} ${this.$options.i18n.UPDATE_ALERT_STATUS_INSTRUCTION}`,
           );
         })
         .finally(() => {

app/models/alert_management/alert.rb

@@ -26,6 +26,11 @@ module AlertManagement
       ignored: :ignore
     }.freeze
 
+    OPEN_STATUSES = [
+      :triggered,
+      :acknowledged
+    ].freeze
+
     DETAILS_IGNORED_PARAMS = %w(start_time).freeze
 
     belongs_to :project
@@ -119,7 +124,7 @@ module AlertManagement
     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]) }
-    scope :open, -> { with_status(:triggered, :acknowledged) }
+    scope :open, -> { with_status(OPEN_STATUSES) }
     scope :not_resolved, -> { where.not(status: STATUSES[:resolved]) }
     scope :with_prometheus_alert, -> { includes(:prometheus_alert) }
 

app/services/alert_management/alerts/update_service.rb

@@ -73,6 +73,7 @@ module AlertManagement
 
         filter_status
         filter_assignees
+        filter_duplicate
       end
 
       def handle_changes(old_assignees:)
@@ -109,9 +110,8 @@ module AlertManagement
 
       # ------ Status-related behavior -------
       def filter_status
-        return unless status = params.delete(:status)
+        return unless params[:status]
 
-        status_key = AlertManagement::Alert::STATUSES.key(status)
         status_event = AlertManagement::Alert::STATUS_EVENTS[status_key]
 
         unless status_event
@@ -122,6 +122,13 @@ 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?
@@ -134,6 +141,39 @@ module AlertManagement
       def resolve_todos
         todo_service.resolve_todos_for_target(alert, current_user)
       end
+
+      def filter_duplicate
+        # Only need to check if changing to an open status
+        return unless params[:status_event] && AlertManagement::Alert::OPEN_STATUSES.include?(status_key)
+
+        param_errors << unresolved_alert_error if duplicate_alert?
+      end
+
+      def duplicate_alert?
+        open_alerts.any? && open_alerts.exclude?(alert)
+      end
+
+      def open_alerts
+        strong_memoize(:open_alerts) do
+          AlertManagement::Alert.for_fingerprint(alert.project, alert.fingerprint).open
+        end
+      end
+
+      def unresolved_alert_error
+        _('An %{link_start}alert%{link_end} with the same fingerprint is already open. ' \
+          'To change the status of this alert, resolve the linked alert.'
+         ) % open_alert_url_params
+      end
+
+      def open_alert_url_params
+        open_alert = open_alerts.first
+        alert_path = Gitlab::Routing.url_helpers.details_project_alert_management_path(alert.project, open_alert)
+
+        {
+          link_start: '<a href="%{url}">'.html_safe % { url: alert_path },
+          link_end: '</a>'.html_safe
+        }
+      end
     end
   end
 end

changelogs/unreleased/223151-custom-error-for-fingerprint-collision.yml

+---
+title: Display informative error for status updates on duplicate alerts
+merge_request: 36527
+author:
+type: changed

locale/gitlab.pot

@@ -2060,6 +2060,9 @@ msgstr ""
 msgid "AlertManagement|Overview"
 msgstr ""
 
+msgid "AlertManagement|Please try again."
+msgstr ""
+
 msgid "AlertManagement|Reported %{when}"
 msgstr ""
 
@@ -2096,7 +2099,7 @@ msgstr ""
 msgid "AlertManagement|There was an error while updating the assignee(s) of the alert. Please try again."
 msgstr ""
 
-msgid "AlertManagement|There was an error while updating the status of the alert. Please try again."
+msgid "AlertManagement|There was an error while updating the status of the alert."
 msgstr ""
 
 msgid "AlertManagement|This assignee cannot be assigned to this alert."
@@ -2393,6 +2396,9 @@ msgstr ""
 msgid "Amount of time (in hours) that users are allowed to skip forced configuration of two-factor authentication"
 msgstr ""
 
+msgid "An %{link_start}alert%{link_end} with the same fingerprint is already open. To change the status of this alert, resolve the linked alert."
+msgstr ""
+
 msgid "An alert has been triggered in %{project_path}."
 msgstr ""
 

spec/frontend/alert_management/components/alert_management_detail_spec.js

@@ -212,6 +212,13 @@ describe('AlertDetails', () => {
         expect(wrapper.find(GlAlert).exists()).toBe(true);
       });
 
+      it('renders html-errors correctly', () => {
+        mountComponent({
+          data: { errored: true, sidebarErrorMessage: '<span data-testid="htmlError" />' },
+        });
+        expect(wrapper.find('[data-testid="htmlError"]').exists()).toBe(true);
+      });
+
       it('does not display an error when dismissed', () => {
         mountComponent({ data: { errored: true, isErrorDismissed: true } });
         expect(wrapper.find(GlAlert).exists()).toBe(false);

spec/frontend/alert_management/components/alert_management_table_spec.js

@@ -455,10 +455,33 @@ describe('AlertManagementTable', () => {
         errored: true,
       });
 
-      wrapper.vm.$nextTick(() => {
+      return wrapper.vm.$nextTick(() => {
         expect(wrapper.find('[data-testid="alert-error"]').exists()).toBe(true);
       });
     });
+
+    it('shows an error when response includes HTML errors', () => {
+      const mockUpdatedMutationErrorResult = {
+        data: {
+          updateAlertStatus: {
+            errors: ['<span data-testid="htmlError" />'],
+            alert: {
+              iid,
+              status: 'acknowledged',
+            },
+          },
+        },
+      };
+
+      jest.spyOn(wrapper.vm.$apollo, 'mutate').mockResolvedValue(mockUpdatedMutationErrorResult);
+      findFirstStatusOption().vm.$emit('click');
+      wrapper.setData({ errored: true });
+
+      return wrapper.vm.$nextTick(() => {
+        expect(wrapper.contains('[data-testid="alert-error"]')).toBe(true);
+        expect(wrapper.contains('[data-testid="htmlError"]')).toBe(true);
+      });
+    });
   });
 
   describe('Snowplow tracking', () => {
@@ -494,14 +517,14 @@ describe('AlertManagementTable', () => {
 
     it('does NOT show pagination control when list is smaller than default page size', () => {
       findStatusTabs().vm.$emit('input', 3);
-      wrapper.vm.$nextTick(() => {
+      return wrapper.vm.$nextTick(() => {
         expect(findPagination().exists()).toBe(false);
       });
     });
 
     it('shows pagination control when list is larger than default page size', () => {
       findStatusTabs().vm.$emit('input', 0);
-      wrapper.vm.$nextTick(() => {
+      return wrapper.vm.$nextTick(() => {
         expect(findPagination().exists()).toBe(true);
       });
     });

spec/graphql/mutations/alert_management/update_alert_status_spec.rb

@@ -39,7 +39,7 @@ RSpec.describe Mutations::AlertManagement::UpdateAlertStatus do
 
           allow(alert).to receive(:save).and_return(false)
           allow(alert).to receive(:errors).and_return(
-            double(full_messages: %w(foo bar))
+            double(full_messages: %w(foo bar), :[] => nil)
           )
           expect(resolve).to eq(
             alert: alert,

spec/services/alert_management/alerts/update_service_spec.rb

@@ -6,8 +6,8 @@ RSpec.describe AlertManagement::Alerts::UpdateService do
   let_it_be(:user_with_permissions) { create(:user) }
   let_it_be(:other_user_with_permissions) { create(:user) }
   let_it_be(:user_without_permissions) { create(:user) }
-  let_it_be(:alert, reload: true) { create(:alert_management_alert, :triggered) }
-  let_it_be(:project) { alert.project }
+  let_it_be(:project) { create(:project) }
+  let_it_be(:alert, reload: true) { create(:alert_management_alert, :triggered, project: project) }
 
   let(:current_user) { user_with_permissions }
   let(:params) { {} }
@@ -66,20 +66,35 @@ RSpec.describe AlertManagement::Alerts::UpdateService do
       it_behaves_like 'error response', "Title can't be blank"
     end
 
-    context 'when a model attribute is included without assignees' do
-      let(:params) { { title: 'This is an updated alert.' } }
-
+    shared_examples 'title update' do
       it_behaves_like 'does not add a todo'
       it_behaves_like 'does not add a system note'
 
       it 'updates the attribute' do
         original_title = alert.title
 
-        expect { response }.to change { alert.title }.from(original_title).to(params[:title])
+        expect { response }.to change { alert.title }.from(original_title).to(expected_title)
         expect(response).to be_success
       end
     end
 
+    context 'when a model attribute is included without assignees' do
+      let(:params) { { title: 'This is an updated alert.' } }
+      let(:expected_title) { params[:title] }
+
+      it_behaves_like 'title update'
+    end
+
+    context 'when alert is resolved and another existing open alert' do
+      let!(:alert) { create(:alert_management_alert, :resolved, project: project) }
+      let!(:existing_alert) { create(:alert_management_alert, :triggered, project: project) }
+
+      let(:params) { { title: 'This is an updated alert.' } }
+      let(:expected_title) { params[:title] }
+
+      it_behaves_like 'title update'
+    end
+
     context 'when assignees are included' do
       shared_examples 'adds a todo' do
         let(:assignee) { expected_assignees.first }
@@ -175,6 +190,39 @@ RSpec.describe AlertManagement::Alerts::UpdateService do
           expect { response }.to change { todo.reload.state }.from('pending').to('done')
         end
       end
+
+      context 'with an opening status and existing open alert' do
+        let_it_be(:alert) { create(:alert_management_alert, :resolved, :with_fingerprint, project: project) }
+        let_it_be(:existing_alert) { create(:alert_management_alert, :triggered, fingerprint: alert.fingerprint, project: project) }
+        let_it_be(:url) { Gitlab::Routing.url_helpers.details_project_alert_management_path(project, existing_alert) }
+        let_it_be(:link) { ActionController::Base.helpers.link_to(_('alert'), url) }
+
+        let(:message) do
+          "An #{link} with the same fingerprint is already open. " \
+          'To change the status of this alert, resolve the linked alert.'
+        end
+
+        it_behaves_like 'does not add a todo'
+        it_behaves_like 'does not add a system note'
+
+        it 'has an informative message' do
+          expect(response).to be_error
+          expect(response.message).to eq(message)
+        end
+      end
+
+      context 'two existing closed alerts' do
+        let_it_be(:alert) { create(:alert_management_alert, :resolved, :with_fingerprint, project: project) }
+        let_it_be(:existing_alert) { create(:alert_management_alert, :resolved, fingerprint: alert.fingerprint, project: project) }
+
+        it 'successfully changes the status' do
+          expect { response }.to change { alert.acknowledged? }.to(true)
+          expect(response).to be_success
+          expect(response.payload[:alert]).to eq(alert)
+        end
+
+        it_behaves_like 'adds a system note'
+      end
     end
   end
 end