Skip to content
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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Nov 2, 2021
Merged

feat(notifications): adds approval notifications to the UI #29651

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cacac9e
adds approval notifications to the UI
Oct 28, 2021
3433d51
cleanup
Oct 28, 2021
28adadd
remove file
Oct 29, 2021
0452928
merge from master
Oct 29, 2021
b5549f0
adds todo
Oct 29, 2021
f1f0543
Merge branch 'master' of github.com:getsentry/sentry into feat/approv…
Nov 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/sentry/models/notificationsetting.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ def provider_str(self) -> str:
(NotificationSettingTypes.DEPLOY, "deploy"),
(NotificationSettingTypes.ISSUE_ALERTS, "issue"),
(NotificationSettingTypes.WORKFLOW, "workflow"),
(NotificationSettingTypes.APPROVAL, "approval"),
Copy link
Contributor

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.

Copy link
Contributor Author

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!

Copy link
Contributor Author

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?

Copy link
Contributor

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.

),
null=False,
)
Expand Down
2 changes: 2 additions & 0 deletions src/sentry/notifications/defaults.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,14 @@
NotificationSettingTypes.DEPLOY: NotificationSettingOptionValues.COMMITTED_ONLY,
NotificationSettingTypes.ISSUE_ALERTS: NotificationSettingOptionValues.ALWAYS,
NotificationSettingTypes.WORKFLOW: NotificationSettingOptionValues.SUBSCRIBE_ONLY,
NotificationSettingTypes.APPROVAL: NotificationSettingOptionValues.ALWAYS,
}

NOTIFICATION_SETTINGS_ALL_NEVER = {
NotificationSettingTypes.DEPLOY: NotificationSettingOptionValues.NEVER,
NotificationSettingTypes.ISSUE_ALERTS: NotificationSettingOptionValues.NEVER,
NotificationSettingTypes.WORKFLOW: NotificationSettingOptionValues.NEVER,
NotificationSettingTypes.APPROVAL: NotificationSettingOptionValues.NEVER,
}

NOTIFICATION_SETTING_DEFAULTS = {
Expand Down
4 changes: 2 additions & 2 deletions src/sentry/notifications/helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,13 +241,13 @@ def validate(type: NotificationSettingTypes, value: NotificationSettingOptionVal

def get_scope_type(type: NotificationSettingTypes) -> NotificationScopeType:
"""In which scope (proj or org) can a user set more specific settings?"""
if type in [NotificationSettingTypes.DEPLOY]:
if type in [NotificationSettingTypes.DEPLOY, NotificationSettingTypes.APPROVAL]:
return NotificationScopeType.ORGANIZATION

if type in [NotificationSettingTypes.WORKFLOW, NotificationSettingTypes.ISSUE_ALERTS]:
return NotificationScopeType.PROJECT

raise Exception(f"type {type}, must be alerts, deploy, or workflow")
raise Exception(f"type {type}, must be alerts, deploy, workflow, or approval")


def get_scope(
Expand Down
10 changes: 10 additions & 0 deletions src/sentry/notifications/types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,16 @@ class NotificationSettingTypes(Enum):
# Notifications for changes in assignment, resolution, comments, etc.
WORKFLOW = 30

# Notifications that require approval like a request to invite a member
APPROVAL = 40


NOTIFICATION_SETTING_TYPES = {
NotificationSettingTypes.DEFAULT: "default",
NotificationSettingTypes.DEPLOY: "deploy",
NotificationSettingTypes.ISSUE_ALERTS: "alerts",
NotificationSettingTypes.WORKFLOW: "workflow",
NotificationSettingTypes.APPROVAL: "approval",
}


Expand Down Expand Up @@ -104,6 +108,7 @@ class FineTuningAPIKey(Enum):
EMAIL = "email"
REPORTS = "reports"
WORKFLOW = "workflow"
APPROVAL = "approval"


class UserOptionsSettingsKey(Enum):
Expand All @@ -112,6 +117,7 @@ class UserOptionsSettingsKey(Enum):
SELF_ASSIGN = "selfAssignOnResolve"
SUBSCRIBE_BY_DEFAULT = "subscribeByDefault"
WORKFLOW = "workflowNotifications"
APPROVAL = "approvalNotifications"


VALID_VALUES_FOR_KEY = {
Expand All @@ -124,6 +130,10 @@ class UserOptionsSettingsKey(Enum):
NotificationSettingOptionValues.ALWAYS,
NotificationSettingOptionValues.NEVER,
},
NotificationSettingTypes.APPROVAL: {
NotificationSettingOptionValues.ALWAYS,
NotificationSettingOptionValues.NEVER,
},
NotificationSettingTypes.WORKFLOW: {
NotificationSettingOptionValues.ALWAYS,
NotificationSettingOptionValues.SUBSCRIBE_ONLY,
Expand Down
2 changes: 2 additions & 0 deletions static/app/data/forms/accountNotificationSettings.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import {t} from 'app/locale';
import {Field, JsonFormObject} from 'app/views/settings/components/forms/type';

// TODO: cleanup unused fields and exports

// Export route to make these forms searchable by label/help
export const route = '/settings/account/notifications/';

Expand Down
2 changes: 1 addition & 1 deletion static/app/views/settings/account/accountNotificationFineTuning.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ class AccountNotificationFineTuning extends AsyncView<Props, State> {
const {params} = this.props;
const {fineTuneType} = params;

if (['alerts', 'deploy', 'workflow'].includes(fineTuneType)) {
if (['alerts', 'deploy', 'workflow', 'approval'].includes(fineTuneType)) {
return <NotificationSettingsByType notificationType={fineTuneType} />;
}

Expand Down
1 change: 1 addition & 0 deletions static/app/views/settings/account/notifications/constants.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ export const NOTIFICATION_SETTINGS_TYPES = [
'alerts',
'workflow',
'deploy',
'approval',
'reports',
'email',
];
Expand Down
9 changes: 8 additions & 1 deletion static/app/views/settings/account/notifications/fields.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ export type FineTuneField = {
defaultFieldName?: string;
};

// TODO: clean up unused fields
export const ACCOUNT_NOTIFICATION_FIELDS: Record<string, FineTuneField> = {
alerts: {
title: 'Project Alerts',
Expand Down Expand Up @@ -69,7 +70,13 @@ export const ACCOUNT_NOTIFICATION_FIELDS: Record<string, FineTuneField> = {
],
defaultFieldName: 'weeklyReports',
},

approval: {
title: t('Approvals'),
description: t('Notifications from teammates that require review or approval.'),
type: 'select',
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

// No choices here because it's going to have dynamic content
// Component will create choices,
},
email: {
title: t('Email Routing'),
description: t(
Expand Down
10 changes: 10 additions & 0 deletions static/app/views/settings/account/notifications/fields2.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,16 @@ export const NOTIFICATION_SETTING_FIELDS: Record<string, NotificationSettingFiel
['email+slack', t('Send to Email and Slack')],
],
},
approval: {
name: 'approval',
type: 'select',
label: t('Approvals'),
choices: [
['always', t('On')],
['never', t('Off')],
],
help: t('Notifications from teammates that require review or approval.'),
},
reports: {
name: 'weekly reports',
type: 'blank',
Expand Down
22 changes: 18 additions & 4 deletions static/app/views/settings/account/notifications/notificationSettings.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import AsyncComponent from 'app/components/asyncComponent';
import Link from 'app/components/links/link';
import {IconMail} from 'app/icons';
import {t} from 'app/locale';
import {Organization} from 'app/types';
import withOrganizations from 'app/utils/withOrganizations';
import {
CONFIRMATION_MESSAGE,
NOTIFICATION_SETTINGS_TYPES,
Expand All @@ -26,7 +28,9 @@ import {FieldObject} from 'app/views/settings/components/forms/type';
import SettingsPageHeader from 'app/views/settings/components/settingsPageHeader';
import TextBlock from 'app/views/settings/components/text/textBlock';

type Props = AsyncComponent['props'];
type Props = AsyncComponent['props'] & {
organizations: Organization[];
};

type State = {
notificationSettings: NotificationSettingsObject;
Expand Down Expand Up @@ -79,11 +83,21 @@ class NotificationSettings extends AsyncComponent<Props, State> {
return updatedNotificationSettings;
};

get notificationSettingsType() {
const hasApprovalFeatureFlag =
this.props.organizations.filter(org => org.features?.includes('slack-requests'))
.length > 0;
// filter out approvals if the feature flag isn't set
return NOTIFICATION_SETTINGS_TYPES.filter(
type => type !== 'approval' || hasApprovalFeatureFlag
);
}

getInitialData(): {[key: string]: string} {
const {notificationSettings} = this.state;

return Object.fromEntries(
NOTIFICATION_SETTINGS_TYPES.map(notificationType => [
this.notificationSettingsType.map(notificationType => [
notificationType,
decideDefault(notificationType, notificationSettings),
])
Expand All @@ -94,7 +108,7 @@ class NotificationSettings extends AsyncComponent<Props, State> {
const {notificationSettings} = this.state;

const fields: FieldObject[] = [];
for (const notificationType of NOTIFICATION_SETTINGS_TYPES) {
for (const notificationType of this.notificationSettingsType) {
const field = Object.assign({}, NOTIFICATION_SETTING_FIELDS[notificationType], {
getData: data => this.getStateToPutForDefault(data, notificationType),
help: (
Expand Down Expand Up @@ -162,4 +176,4 @@ class NotificationSettings extends AsyncComponent<Props, State> {
}
}

export default NotificationSettings;
export default withOrganizations(NotificationSettings);
6 changes: 5 additions & 1 deletion static/app/views/settings/account/notifications/notificationSettingsByType.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,11 +167,15 @@ class NotificationSettingsByType extends AsyncComponent<Props, State> {
const {notificationType} = this.props;
const {notificationSettings} = this.state;

const help = isGroupedByProject(notificationType)
? t('This is the default for all projects.')
: t('This is the default for all organizations.');

const defaultField = Object.assign(
{},
NOTIFICATION_SETTING_FIELDS[notificationType],
{
help: t('This is the default for all projects.'),
help,
getData: data => this.getStateToPutForDefault(data),
}
);
Expand Down