Commit 43547079 authored by Sean Arnold's avatar Sean Arnold Committed by Ash McKenzie

Present appropriate error for duplicate open alerts

If a triggered or acknowledged alert has a fingerprint which
matches a resolved alert, we do not allow the resolved alert
to be opened. This shows an appropriate error for the user
outlining remediation steps. This error is visible from both
the alert detail view and the alert list view.
parent feb51cbe
...@@ -187,7 +187,7 @@ export default { ...@@ -187,7 +187,7 @@ export default {
<template> <template>
<div> <div>
<gl-alert v-if="showErrorMsg" variant="danger" @dismiss="dismissError"> <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>
<gl-alert <gl-alert
v-if="createIssueError" v-if="createIssueError"
......
...@@ -339,7 +339,7 @@ export default { ...@@ -339,7 +339,7 @@ export default {
data-testid="alert-error" data-testid="alert-error"
@dismiss="dismissError" @dismiss="dismissError"
> >
{{ errorMessage || $options.i18n.errorMsg }} <p v-html="errorMessage || $options.i18n.errorMsg"></p>
</gl-alert> </gl-alert>
<gl-tabs content-class="gl-p-0" @input="filterAlertsByStatus"> <gl-tabs content-class="gl-p-0" @input="filterAlertsByStatus">
......
...@@ -6,6 +6,12 @@ import { trackAlertStatusUpdateOptions } from '../constants'; ...@@ -6,6 +6,12 @@ import { trackAlertStatusUpdateOptions } from '../constants';
import updateAlertStatus from '../graphql/mutations/update_alert_status.mutation.graphql'; import updateAlertStatus from '../graphql/mutations/update_alert_status.mutation.graphql';
export default { 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: { statuses: {
TRIGGERED: s__('AlertManagement|Triggered'), TRIGGERED: s__('AlertManagement|Triggered'),
ACKNOWLEDGED: s__('AlertManagement|Acknowledged'), ACKNOWLEDGED: s__('AlertManagement|Acknowledged'),
...@@ -52,16 +58,23 @@ export default { ...@@ -52,16 +58,23 @@ export default {
projectPath: this.projectPath, projectPath: this.projectPath,
}, },
}) })
.then(() => { .then(resp => {
this.trackStatusUpdate(status); this.trackStatusUpdate(status);
this.$emit('hide-dropdown'); 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(() => { .catch(() => {
this.$emit( this.$emit(
'alert-error', 'alert-error',
s__( `${this.$options.i18n.UPDATE_ALERT_STATUS_ERROR} ${this.$options.i18n.UPDATE_ALERT_STATUS_INSTRUCTION}`,
'AlertManagement|There was an error while updating the status of the alert. Please try again.',
),
); );
}) })
.finally(() => { .finally(() => {
......
...@@ -26,6 +26,11 @@ module AlertManagement ...@@ -26,6 +26,11 @@ module AlertManagement
ignored: :ignore ignored: :ignore
}.freeze }.freeze
OPEN_STATUSES = [
:triggered,
:acknowledged
].freeze
DETAILS_IGNORED_PARAMS = %w(start_time).freeze DETAILS_IGNORED_PARAMS = %w(start_time).freeze
belongs_to :project belongs_to :project
...@@ -119,7 +124,7 @@ module AlertManagement ...@@ -119,7 +124,7 @@ module AlertManagement
scope :for_fingerprint, -> (project, fingerprint) { where(project: project, fingerprint: fingerprint) } scope :for_fingerprint, -> (project, fingerprint) { where(project: project, fingerprint: fingerprint) }
scope :for_environment, -> (environment) { where(environment: environment) } scope :for_environment, -> (environment) { where(environment: environment) }
scope :search, -> (query) { fuzzy_search(query, [:title, :description, :monitoring_tool, :service]) } 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 :not_resolved, -> { where.not(status: STATUSES[:resolved]) }
scope :with_prometheus_alert, -> { includes(:prometheus_alert) } scope :with_prometheus_alert, -> { includes(:prometheus_alert) }
......
...@@ -73,6 +73,7 @@ module AlertManagement ...@@ -73,6 +73,7 @@ module AlertManagement
filter_status filter_status
filter_assignees filter_assignees
filter_duplicate
end end
def handle_changes(old_assignees:) def handle_changes(old_assignees:)
...@@ -109,9 +110,8 @@ module AlertManagement ...@@ -109,9 +110,8 @@ module AlertManagement
# ------ Status-related behavior ------- # ------ Status-related behavior -------
def filter_status 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] status_event = AlertManagement::Alert::STATUS_EVENTS[status_key]
unless status_event unless status_event
...@@ -122,6 +122,13 @@ module AlertManagement ...@@ -122,6 +122,13 @@ module AlertManagement
params[:status_event] = status_event params[:status_event] = status_event
end end
def status_key
strong_memoize(:status_key) do
status = params.delete(:status)
AlertManagement::Alert::STATUSES.key(status)
end
end
def handle_status_change def handle_status_change
add_status_change_system_note add_status_change_system_note
resolve_todos if resolved? resolve_todos if resolved?
...@@ -134,6 +141,39 @@ module AlertManagement ...@@ -134,6 +141,39 @@ module AlertManagement
def resolve_todos def resolve_todos
todo_service.resolve_todos_for_target(alert, current_user) todo_service.resolve_todos_for_target(alert, current_user)
end 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 end
end end
---
title: Display informative error for status updates on duplicate alerts
merge_request: 36527
author:
type: changed
...@@ -2066,6 +2066,9 @@ msgstr "" ...@@ -2066,6 +2066,9 @@ msgstr ""
msgid "AlertManagement|Overview" msgid "AlertManagement|Overview"
msgstr "" msgstr ""
msgid "AlertManagement|Please try again."
msgstr ""
msgid "AlertManagement|Reported %{when}" msgid "AlertManagement|Reported %{when}"
msgstr "" msgstr ""
...@@ -2102,7 +2105,7 @@ msgstr "" ...@@ -2102,7 +2105,7 @@ msgstr ""
msgid "AlertManagement|There was an error while updating the assignee(s) of the alert. Please try again." msgid "AlertManagement|There was an error while updating the assignee(s) of the alert. Please try again."
msgstr "" 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 "" msgstr ""
msgid "AlertManagement|This assignee cannot be assigned to this alert." msgid "AlertManagement|This assignee cannot be assigned to this alert."
...@@ -2399,6 +2402,9 @@ msgstr "" ...@@ -2399,6 +2402,9 @@ msgstr ""
msgid "Amount of time (in hours) that users are allowed to skip forced configuration of two-factor authentication" msgid "Amount of time (in hours) that users are allowed to skip forced configuration of two-factor authentication"
msgstr "" 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}." msgid "An alert has been triggered in %{project_path}."
msgstr "" msgstr ""
......
...@@ -212,6 +212,13 @@ describe('AlertDetails', () => { ...@@ -212,6 +212,13 @@ describe('AlertDetails', () => {
expect(wrapper.find(GlAlert).exists()).toBe(true); 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', () => { it('does not display an error when dismissed', () => {
mountComponent({ data: { errored: true, isErrorDismissed: true } }); mountComponent({ data: { errored: true, isErrorDismissed: true } });
expect(wrapper.find(GlAlert).exists()).toBe(false); expect(wrapper.find(GlAlert).exists()).toBe(false);
......
...@@ -455,10 +455,33 @@ describe('AlertManagementTable', () => { ...@@ -455,10 +455,33 @@ describe('AlertManagementTable', () => {
errored: true, errored: true,
}); });
wrapper.vm.$nextTick(() => { return wrapper.vm.$nextTick(() => {
expect(wrapper.find('[data-testid="alert-error"]').exists()).toBe(true); 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', () => { describe('Snowplow tracking', () => {
...@@ -494,14 +517,14 @@ describe('AlertManagementTable', () => { ...@@ -494,14 +517,14 @@ describe('AlertManagementTable', () => {
it('does NOT show pagination control when list is smaller than default page size', () => { it('does NOT show pagination control when list is smaller than default page size', () => {
findStatusTabs().vm.$emit('input', 3); findStatusTabs().vm.$emit('input', 3);
wrapper.vm.$nextTick(() => { return wrapper.vm.$nextTick(() => {
expect(findPagination().exists()).toBe(false); expect(findPagination().exists()).toBe(false);
}); });
}); });
it('shows pagination control when list is larger than default page size', () => { it('shows pagination control when list is larger than default page size', () => {
findStatusTabs().vm.$emit('input', 0); findStatusTabs().vm.$emit('input', 0);
wrapper.vm.$nextTick(() => { return wrapper.vm.$nextTick(() => {
expect(findPagination().exists()).toBe(true); expect(findPagination().exists()).toBe(true);
}); });
}); });
......
...@@ -39,7 +39,7 @@ RSpec.describe Mutations::AlertManagement::UpdateAlertStatus do ...@@ -39,7 +39,7 @@ RSpec.describe Mutations::AlertManagement::UpdateAlertStatus do
allow(alert).to receive(:save).and_return(false) allow(alert).to receive(:save).and_return(false)
allow(alert).to receive(:errors).and_return( allow(alert).to receive(:errors).and_return(
double(full_messages: %w(foo bar)) double(full_messages: %w(foo bar), :[] => nil)
) )
expect(resolve).to eq( expect(resolve).to eq(
alert: alert, alert: alert,
......
...@@ -6,8 +6,8 @@ RSpec.describe AlertManagement::Alerts::UpdateService do ...@@ -6,8 +6,8 @@ RSpec.describe AlertManagement::Alerts::UpdateService do
let_it_be(:user_with_permissions) { create(:user) } let_it_be(:user_with_permissions) { create(:user) }
let_it_be(:other_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(:user_without_permissions) { create(:user) }
let_it_be(:alert, reload: true) { create(:alert_management_alert, :triggered) } let_it_be(:project) { create(:project) }
let_it_be(:project) { alert.project } let_it_be(:alert, reload: true) { create(:alert_management_alert, :triggered, project: project) }
let(:current_user) { user_with_permissions } let(:current_user) { user_with_permissions }
let(:params) { {} } let(:params) { {} }
...@@ -66,20 +66,35 @@ RSpec.describe AlertManagement::Alerts::UpdateService do ...@@ -66,20 +66,35 @@ RSpec.describe AlertManagement::Alerts::UpdateService do
it_behaves_like 'error response', "Title can't be blank" it_behaves_like 'error response', "Title can't be blank"
end end
context 'when a model attribute is included without assignees' do shared_examples 'title update' do
let(:params) { { title: 'This is an updated alert.' } }
it_behaves_like 'does not add a todo' it_behaves_like 'does not add a todo'
it_behaves_like 'does not add a system note' it_behaves_like 'does not add a system note'
it 'updates the attribute' do it 'updates the attribute' do
original_title = alert.title 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 expect(response).to be_success
end end
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 context 'when assignees are included' do
shared_examples 'adds a todo' do shared_examples 'adds a todo' do
let(:assignee) { expected_assignees.first } let(:assignee) { expected_assignees.first }
...@@ -175,6 +190,39 @@ RSpec.describe AlertManagement::Alerts::UpdateService do ...@@ -175,6 +190,39 @@ RSpec.describe AlertManagement::Alerts::UpdateService do
expect { response }.to change { todo.reload.state }.from('pending').to('done') expect { response }.to change { todo.reload.state }.from('pending').to('done')
end end
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 end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment