Skip to content

Conversation

@ceorourke
Copy link
Member

@ceorourke ceorourke commented Nov 27, 2024

Remove an unused feature to comment on incidents. This will be replaced with comments on metric issues.

This will be followed by PRs to remove usage of the IncidentSeen and IncidentSubscription models and then they will be removed.

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Nov 27, 2024
incident=incident,
status=result["status"],
user=serialize_generic_user(request.user),
comment=result.get("comment"),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only call to update_incident_status that passes user and comment so I checked the API access logs to confirm the only usage is by customers who are probably using it to close incidents. I also ensured that we haven't sent the incident activity emails just in case a customer was for some reason using that part of it.

@codecov
Copy link

codecov bot commented Nov 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #81386      +/-   ##
==========================================
- Coverage   80.37%   80.37%   -0.01%     
==========================================
  Files        7231     7230       -1     
  Lines      319890   319813      -77     
  Branches    20810    20810              
==========================================
- Hits       257120   257053      -67     
+ Misses      62377    62367      -10     
  Partials      393      393              

queue="incidents",
silo_mode=SiloMode.REGION,
)
def send_subscriber_notifications(activity_id: int) -> None:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirmed via sendgrid that we haven't sent one of these in the last 30 days. There are no IncidentActivity rows with a type of comment since 2021, and since user isn't passed to update_incident_status nobody was getting subscribed so we never found a subscriber to execute the code on lines 83 and 84.

@ceorourke ceorourke marked this pull request as ready for review November 27, 2024 21:30
@ceorourke ceorourke requested a review from a team as a code owner November 27, 2024 21:30
Copy link
Contributor

@saponifi3d saponifi3d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 - awesome! LGTM

Are we using the IncidentActivityType.COMMENT after these changes? if not, can we also remove that from the IncidentActivityType enum?

@ceorourke ceorourke merged commit 8096ebc into master Dec 2, 2024
50 checks passed
@ceorourke ceorourke deleted the ceorourke/rm-comment-incidentactivity branch December 2, 2024 18:32
andrewshie-sentry pushed a commit that referenced this pull request Dec 2, 2024
Remove an unused feature to comment on incidents. This will be replaced
with comments on metric issues.

This will be followed by PRs to remove usage of the `IncidentSeen` and
`IncidentSubscription` models and then they will be removed.
ceorourke added a commit that referenced this pull request Dec 3, 2024
Follow up to #81386 to remove
usages of the `IncidentSeen` and `IncidentSubscription` models before
dropping them.
@github-actions github-actions bot locked and limited conversation to collaborators Dec 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants