ref(notifications): modify legacy option code #60437
Conversation
github-actions
bot
added
the
Scope: Backend
| from sentry.types.integrations import ExternalProviders | ||
| from sentry.notifications.types import UserOptionsSettingsKey | ||
|
|
||
| USER_OPTION_SETTINGS = { |
There was a problem hiding this comment.
Moved this from legacy_mappings since it was the only thing left and this isn't used anywhere else
…tsentry/sentry into ref/remove-legacy-mapping-option-code
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #60437 +/- ##
==========================================
+ Coverage 79.81% 80.83% +1.01%
==========================================
Files 5180 5179 -1
Lines 227644 227588 -56
Branches 38305 38296 -9
==========================================
+ Hits 181702 183962 +2260
+ Misses 40211 37996 -2215
+ Partials 5731 5630 -101
|
github-actions
bot
added
the
Scope: Frontend
|
🚨 Warning: This pull request contains Frontend and Backend changes! It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently. Have questions? Please ask in the |
| @@ -62,7 +62,7 @@ const patterns: RegExp[] = [ | |||
| new RegExp('^api/0/users/[^/]+/ips/$'), | |||
| new RegExp('^api/0/users/[^/]+/notification-settings/$'), | |||
| new RegExp('^api/0/users/[^/]+/notifications/$'), | |||
| new RegExp('^api/0/users/[^/]+/notifications/[^/]+/$'), | |||
There was a problem hiding this comment.
Need this change for tests to pass so I think I have to have this change
|
PR reverted: bc271a9 |
This reverts commit 8299cd3. Co-authored-by: scefali <8533851+scefali@users.noreply.github.com>
Before we did the changes for notification v2, we had a bunch of complicated mapping logic to map old options to
NotificationSettingrows. It was very complicated and hard to understand. Now that we have migrated weekly reports to the new notification system and removed some complexity in the UI, we can remove a bunch of code. None of these endpoints are documented or used by APIs (purely used by the UI).For
UserNotificationDetailsEndpoint, now it only handles the legacy options ofpersonalActivityNotificationsandselfAssignOnResolvewhich are not true notification settings. In the future, we probably should handle them differently but for now, I'm making the endpoint a lot more simple by ripping out the code that tries to setNotificationSettingrows.I also renamed
UserNotificationFineTuningEndpointtoUserNotificationEmailEndpointand ripped out all the non-email functionality. No more dependency onNotificationSettingat all.Note, for some reason a bunch of tests broke that relied on a mock
sentry.notifications.notify.notify. I think it had do with this change which I had to make because of some circular import problem that got exposed. Turns out we didn't need the mock at all so I just ripped it out.