Follow-up from "Allow issue updates to update incident escalation status"
The following discussions from !76819 (merged) should be addressed:
-
@splattael started a discussion:Question (non-blocking)
I've seen we are fully qualifying policy names elsewhere, for example,
:read_incident_management_escalation_policy
.Is
escalation_status
generic enough to leave out the prefix?🤔 Having said that it's fine for now I think.
-
@splattael started a discussion: Suggestion (non-blocking) Thoughts on inheriting from
BaseProjectService
and follow https://docs.gitlab.com/ee/development/reusing_abstractions.html#service-classes?🤔 Services should consider inheriting from:
class PrepareUpdateService < BaseProjectService ... def initialize(issuable:, **args) super(**args) @issuable = issuable
It seems we have more service class which does not inherit from a base service class:
$ rg "class" {,ee/}app/services/incident_management/ | grep -v " < " ee/app/services/incident_management/oncall_schedules/base_service.rb: class BaseService app/services/incident_management/issuable_escalation_statuses/prepare_update_service.rb: class PrepareUpdateService app/services/incident_management/pager_duty/process_webhook_service.rb: class ProcessWebhookService ee/app/services/incident_management/escalation_rules/destroy_service.rb: class DestroyService ee/app/services/incident_management/oncall_rotations/base_service.rb: class BaseService ee/app/services/incident_management/oncall_rotations/remove_participants_service.rb: class RemoveParticipantsService ee/app/services/incident_management/oncall_shifts/read_service.rb: class ReadService ee/app/services/incident_management/escalation_policies/base_service.rb: class BaseService
Perhaps we can revisit them and update if inheriting is useful in a follow-up. WDYT?
🤔 -
@splattael started a discussion: Suggestion (non-blocking) Thoughts on moving the feature flag check into
Gitlab::IncidentManagement
? -
@splattael started a discussion: Observation (non-blocking) We tend to avoid using exceptions as control flow. (See https://wiki.c2.com/?DontUseExceptionsForFlowControl)
Thoughts on returning
invalid_param_error
instead here?Marking as non-blocking though...
-
@splattael started a discussion: Suggestion (non-blocking)
When hovering of the yellow circle above I see
New code quality degradations on this line
Minor - Method
filter_policy
has a Cognitive Complexity of 10 (exceeds 5 allowed). Consider refactoring.Any ideas how to reduce cognitive load here?
🤔
-
IncidentManagement::IssuableEscalationStatuses::CreatService
inherits fromBaseService
but does not callsuper
.