-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(notifications): adds approval notifications to the UI #29651
Conversation
size-limit report
|
approval: { | ||
title: t('Approvals'), | ||
description: t('Notifications from teammates that require review or approval.'), | ||
type: 'select', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't this just be on
or off
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NisanthanNanthakumar Correct, it would be on or off. The issue is that we only use the title
and description
from this (
sentry/static/app/views/settings/account/notifications/notificationSettingsByType.tsx
Line 229 in 3433d51
const {title, description} = ACCOUNT_NOTIFICATION_FIELDS[notificationType]; |
It looks like we used to read all fields a change was made to basically bypass logic depending in the field:
sentry/static/app/views/settings/account/accountNotificationFineTuning.tsx
Lines 171 to 178 in 3433d51
if (['alerts', 'deploy', 'workflow', 'approval'].includes(fineTuneType)) { | |
return <NotificationSettingsByType notificationType={fineTuneType} />; | |
} | |
const {notifications, projects, fineTuneData, projectsPageLinks} = this.state; | |
const isProject = isGroupedByProject(fineTuneType); | |
const field = ACCOUNT_NOTIFICATION_FIELDS[fineTuneType]; |
Looks like a lot of the unused fields weren't cleaned up. We are looking at the options for some of these fields but not all of them. IMO this needs to be fixed because it was very confusing when the fields are treated differently from this file but you won't know until you look at other parts of the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gonna put in another TODO for cleanup
@@ -72,6 +72,7 @@ def provider_str(self) -> str: | |||
(NotificationSettingTypes.DEPLOY, "deploy"), | |||
(NotificationSettingTypes.ISSUE_ALERTS, "issue"), | |||
(NotificationSettingTypes.WORKFLOW, "workflow"), | |||
(NotificationSettingTypes.APPROVAL, "approval"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will actually require a migration because it changes a DB constraint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mgaeta good point...let me generate that, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mgaeta I am seeing:
sentry django makemigrations sentry -n approval_notification_type
....
No changes detected in app 'sentry'
Maybe we don't need migrations for this kind of change anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope so that would save a lot of time.
…al-notifications-ui
This PR adds the UI for approval notifications which is hidden behind the feature flag
organization:slack-requsts
. These approval notifications require someone with the right permissions to approve or reject requests. These notifications are at an organization level as well.