Skip to content

Conversation

@davidenwang
Copy link
Contributor

@davidenwang davidenwang commented Jul 17, 2024

Backfills the new broken monitor setting according to the previous value we were using which was from "Nudges" (approval)

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jul 17, 2024
@github-actions
Copy link
Contributor

This PR has a migration; here is the generated SQL for src/sentry/migrations/0742_backfill_broken_monitor_notification_setting_option.py ()

--
-- Raw Python operation
--
-- THIS OPERATION CANNOT BE WRITTEN AS SQL

@davidenwang davidenwang force-pushed the davidenwang/backfill-new-setting branch from bedd012 to 0c4c7dc Compare July 17, 2024 10:50
@davidenwang davidenwang force-pushed the davidenwang/backfill-new-setting branch from 0c4c7dc to 80e475d Compare July 17, 2024 16:50
@davidenwang davidenwang marked this pull request as ready for review July 17, 2024 16:50
@davidenwang davidenwang requested a review from a team as a code owner July 17, 2024 16:50
@davidenwang davidenwang requested a review from a team July 17, 2024 16:50
@codecov
Copy link

codecov bot commented Jul 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.12%. Comparing base (fe84550) to head (80e475d).
Report is 48 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #74415   +/-   ##
=======================================
  Coverage   78.11%   78.12%           
=======================================
  Files        6676     6676           
  Lines      298721   298729    +8     
  Branches    51424    51427    +3     
=======================================
+ Hits       233338   233374   +36     
+ Misses      59095    59062   -33     
- Partials     6288     6293    +5     

see 26 files with indirect coverage changes

Comment on lines 19 to 25
NotificationSettingOption.objects.get_or_create(
user_id=setting.user_id,
scope_type=setting.scope_type,
scope_identifier=setting.scope_identifier,
type="brokenMonitors",
defaults={"value": setting.value},
)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to remove the approval type setting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nah, the approval setting still exists and will control other classes of notifications, just wanted to split out broken monitor emails into its own category with this migration

Comment on lines 32 to 29
NotificationSettingOption.objects.create(
user_id=self.user3.id,
scope_type="user",
scope_identifier=self.user.id,
type="brokenMonitors",
value="never",
)
Copy link
Member

Choose a reason for hiding this comment

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

If there is an existing row for brokenMonitors does the value need to be updated?

Copy link
Contributor Author

@davidenwang davidenwang Jul 18, 2024

Choose a reason for hiding this comment

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

its very unlikely for there to be brokenMonitors rows unless a user has manually called the API at this point, but if they did choose to do that I just opt to respect the setting they chose, rather than overriding the value from their "approval" setting

@github-actions
Copy link
Contributor

This PR has a migration; here is the generated SQL for src/sentry/migrations/0743_backfill_broken_monitor_notification_setting_option.py ()

--
-- Raw Python operation
--
-- THIS OPERATION CANNOT BE WRITTEN AS SQL

@davidenwang davidenwang force-pushed the davidenwang/backfill-new-setting branch from 0601459 to 40bffea Compare July 22, 2024 09:26
@davidenwang davidenwang enabled auto-merge (squash) July 22, 2024 09:26
@davidenwang davidenwang merged commit 14d616f into master Jul 22, 2024
@davidenwang davidenwang deleted the davidenwang/backfill-new-setting branch July 22, 2024 09:56
davidenwang pushed a commit that referenced this pull request Jul 22, 2024
After #74415 is merged, let's go
ahead and switch over to using the new notification setting.
@github-actions github-actions bot locked and limited conversation to collaborators Aug 6, 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.

4 participants