Commit e88b5f8f authored by Vitali Tatarintev's avatar Vitali Tatarintev

Get rid of Alert::STATUSES constant

Encapsulate Alert's status implementation details.
Provide methods to operate with the Alert's status by its name
instead of raw values.
parent e96a8b53
......@@ -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
......
......@@ -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,
......
......@@ -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,
......
......@@ -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
......
......@@ -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]) }
......@@ -132,11 +133,31 @@ module AlertManagement
# 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 }
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)
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......@@ -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
......
......@@ -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)
......
......@@ -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 }
......
......@@ -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)
......
......@@ -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,
......
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