ref(hybrid-cloud): Restrict notification settings UX to a single organization #50279

mikejihbe · 2023-06-04T23:32:17Z

Refactor the notification settings page to restrict viewing notification settings to a single organization. This is required to simplify some user notification setting APIs and make them compatible with hybrid cloud.

Currently the user notifications APIs return "virtual" notification settings for every project. This page doesn't really use those settings, it instead renders projects from the list of projects and merges in what gets returned. I think it should work fine if we only return actual notification settings records, which will allow us to simplify the user notification settings endpoints by removing these "virtual" entries -- assuming this was the only use case.

…ion settings to a single organization. This is required to simplify some user notification setting APIs and make them compatible with hybrid cloud.

github-actions · 2023-06-04T23:32:39Z

🚨 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 #discuss-dev-infra channel.

codecov · 2023-06-05T00:05:30Z

Codecov Report

Merging #50279 (4de4c2a) into master (09ceea5) will increase coverage by 4.66%.
The diff coverage is 92.85%.

❗ Current head 4de4c2a differs from pull request most recent head d60066b. Consider uploading reports for the commit d60066b to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #50279      +/-   ##
==========================================
+ Coverage   74.71%   79.37%   +4.66%     
==========================================
  Files        4901     4923      +22     
  Lines      206414   206690     +276     
  Branches    35296    35333      +37     
==========================================
+ Hits       154212   164068    +9856     
+ Misses      46936    37599    -9337     
+ Partials     5266     5023     -243

Impacted Files	Coverage Δ
...try/api/serializers/models/notification_setting.py	`84.44% <ø> (ø)`
src/sentry/models/integrations/external_actor.py	`95.00% <ø> (ø)`
src/sentry/models/notificationsetting.py	`95.55% <ø> (ø)`
src/sentry/notifications/helpers.py	`93.41% <0.00%> (-1.91%)`	⬇️
...settings/account/accountNotificationFineTuning.tsx	`0.00% <ø> (ø)`
...ngs/account/notifications/notificationSettings.tsx	`78.26% <ø> (ø)`
...tifications/notificationSettingsByOrganization.tsx	`100.00% <ø> (ø)`
...t/notifications/notificationSettingsByProjects.tsx	`100.00% <ø> (ø)`
...count/notifications/notificationSettingsByType.tsx	`54.21% <ø> (+1.27%)`	⬆️
...app/views/settings/account/notifications/utils.tsx	`61.66% <ø> (ø)`
... and 4 more

... and 701 files with indirect coverage changes

corps

Seems good, there is an issue with the component props.

src/sentry/api/serializers/models/notification_setting.py

src/sentry/api/endpoints/project_index.py

src/sentry/api/serializers/models/notification_setting.py

markstory · 2023-07-05T18:31:17Z

src/sentry/notifications/helpers.py

-        team_providers = ExternalActor.objects.filter(
-            actor_id=recipient.actor_id, provider__in=possible_providers
-        ).values_list("provider", flat=True)
-        return [get_provider_enum(provider) for provider in team_providers]


Was the team scenario never used? Looking at the call sites it looks like they are all User only.

markstory · 2023-07-05T18:33:41Z

static/app/views/settings/account/accountNotificationFineTuning.tsx

    const fineTuneType = getNotificationTypeFromPathname(pathnameType);
    const endpoints = [
      ['notifications', '/users/me/notifications/'],
      ['fineTuneData', `/users/me/notifications/${fineTuneType}/`],
    ];

    if (isGroupedByProject(fineTuneType)) {
-      endpoints.push(['projects', '/projects/']);
+      endpoints.push(['projects', `/projects/?organization_id=${orgId}`]);


Suggested change

endpoints.push(['projects', `/projects/?organization_id=${orgId}`]);

endpoints.push(['projects', `/projects/?organizationId=${orgId}`]);

To go with the other casing change.

markstory · 2023-07-05T18:35:45Z

static/app/views/settings/account/notifications/notificationSettingsByProjects.tsx

+        'projects',
+        `/projects/`,
+        {
+          query: {organization_id: this.props.organizationId},


Suggested change

query: {organization_id: this.props.organizationId},

query: {organizationId: this.props.organizationId},

To go with the other change.

static/app/views/settings/account/notifications/notificationSettingsByProjects.tsx

markstory · 2023-07-05T18:40:05Z

static/app/views/settings/account/notifications/notificationSettingsByProjects.tsx

+          {canSearch &&
+            this.renderSearchInput({
+              stateKey: 'projects',
+              url: `/projects/?organization_id=${this.props.organizationId}`,


Suggested change

url: `/projects/?organization_id=${this.props.organizationId}`,

url: `/projects/?organizationId=${this.props.organizationId}`,

markstory · 2023-07-05T18:41:16Z

static/app/views/settings/account/notifications/utils.tsx

+      // issue, quotaErrors, quotaTransactions, quotaAttachments,
+      // quotaReplays, quotaWarnings, quotaSpendAllocations


Are these cases that still need to be handled or cases where the default is ''?

The latter, I'll clarify

markstory · 2023-07-05T18:41:31Z

tests/sentry/api/endpoints/test_project_index.py

+
+        self.login_as(user=user, superuser=False)
+
+        response = self.get_success_response(qs_params={"organization_id": str(org.id)})


Suggested change

response = self.get_success_response(qs_params={"organization_id": str(org.id)})

response = self.get_success_response(qs_params={"organizationId": str(org.id)})

…ttingsByProjects.tsx Co-authored-by: Mark Story <[email protected]>

vbro and others added 5 commits May 18, 2023 09:19

feat(hc): make NotificationSetting.target_id nullable

d54060e

fix typing

f02200a

set fetch_actor=False in test_user_notification_settings

b9480e6

rebase and update migration

a6f69ff

Refactor the notification settings page to restrict viewing notificat…

0a0d0f0

…ion settings to a single organization. This is required to simplify some user notification setting APIs and make them compatible with hybrid cloud.

github-actions bot added Scope: Frontend Automatically applied to PRs that change frontend components Scope: Backend Automatically applied to PRs that change backend components labels Jun 4, 2023

mikejihbe marked this pull request as ready for review June 5, 2023 02:17

mikejihbe requested a review from a team as a code owner June 5, 2023 02:17

corps approved these changes Jun 5, 2023

View reviewed changes

Fix up some FE tests

c7245ed

vercel bot deployed to Preview June 6, 2023 05:34 View deployment

mikejihbe marked this pull request as draft June 12, 2023 18:12

mikejihbe added 2 commits June 15, 2023 22:09

Merge branch 'master' into ihbe/notification-settings-ux

416ca87

Fix up more notification fine tune settings UI

02ddcaa

vercel bot deployed to Preview June 16, 2023 07:31 View deployment

mikejihbe added 2 commits June 22, 2023 13:20

Merge branch 'master' into ihbe/notification-settings-ux

5144248

Fix up functionality. Styles are still a little borked

2bf272b

vercel bot deployed to Preview June 23, 2023 21:15 View deployment

Fix some bugs

5502823

vercel bot deployed to Preview June 23, 2023 21:47 View deployment

Fix styles

dfd80a7

vercel bot deployed to Preview June 23, 2023 21:54 View deployment

corps reviewed Jun 23, 2023

View reviewed changes

src/sentry/api/serializers/models/notification_setting.py Outdated Show resolved Hide resolved

corps reviewed Jun 23, 2023

View reviewed changes

src/sentry/api/serializers/models/notification_setting.py Outdated Show resolved Hide resolved

cleanup

1f625f0

vercel bot deployed to Preview June 25, 2023 06:03 View deployment

Merge branch 'master' into ihbe/notification-settings-ux

f3c92ea

Merge branch 'master' into ihbe/notification-settings-ux

f973268

vercel bot deployed to Preview July 5, 2023 01:33 View deployment

mikejihbe marked this pull request as ready for review July 5, 2023 01:47

Fix team tests

c973c3b

vercel bot deployed to Preview July 5, 2023 05:52 View deployment

markstory reviewed Jul 5, 2023

View reviewed changes

Feedback

ad82f05

vercel bot deployed to Preview July 11, 2023 18:40 View deployment

Duplicate out new serializer backend

bca8f6c

vercel bot deployed to Preview July 11, 2023 22:55 View deployment

Merge branch 'master' into ihbe/notification-settings-ux

7098000

vercel bot had a problem deploying to Preview July 11, 2023 23:14 Failure

Tiny fixes

236e000

vercel bot had a problem deploying to Preview July 11, 2023 23:39 Failure

Fixes

3f22062

vercel bot deployed to Preview July 12, 2023 01:43 View deployment

More matcher fixes

ba88e43

vercel bot deployed to Preview July 12, 2023 03:30 View deployment

Moo

1aba6cc

vercel bot deployed to Preview July 12, 2023 07:16 View deployment

Update static/app/views/settings/account/notifications/notificationSe…

4de4c2a

…ttingsByProjects.tsx Co-authored-by: Mark Story <[email protected]>

vercel bot deployed to Preview July 13, 2023 05:22 View deployment

Merge branch 'master' into ihbe/notification-settings-ux

d60066b

vercel bot deployed to Preview July 13, 2023 21:29 View deployment

Merge branch 'master' into ihbe/notification-settings-ux

6cea66c

vercel bot deployed to Preview July 15, 2023 02:46 View deployment

corps merged commit aa870e3 into master Jul 17, 2023

corps deleted the ihbe/notification-settings-ux branch July 17, 2023 16:50

github-actions bot locked and limited conversation to collaborators Aug 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

ref(hybrid-cloud): Restrict notification settings UX to a single organization #50279

ref(hybrid-cloud): Restrict notification settings UX to a single organization #50279

Uh oh!

mikejihbe commented Jun 4, 2023

Uh oh!

github-actions bot commented Jun 4, 2023

Uh oh!

codecov bot commented Jun 5, 2023 •

edited

Loading

Uh oh!

corps left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

markstory Jul 5, 2023

Uh oh!

markstory Jul 5, 2023

Uh oh!

markstory Jul 5, 2023

Uh oh!

Uh oh!

markstory Jul 5, 2023

Uh oh!

markstory Jul 5, 2023

Uh oh!

corps Jul 11, 2023

Uh oh!

markstory Jul 5, 2023

Uh oh!

Uh oh!

	endpoints.push(['projects', `/projects/?organization_id=${orgId}`]);
	endpoints.push(['projects', `/projects/?organizationId=${orgId}`]);

	query: {organization_id: this.props.organizationId},
	query: {organizationId: this.props.organizationId},

	url: `/projects/?organization_id=${this.props.organizationId}`,
	url: `/projects/?organizationId=${this.props.organizationId}`,

		// issue, quotaErrors, quotaTransactions, quotaAttachments,
		// quotaReplays, quotaWarnings, quotaSpendAllocations


		self.login_as(user=user, superuser=False)

		response = self.get_success_response(qs_params={"organization_id": str(org.id)})

Uh oh!

ref(hybrid-cloud): Restrict notification settings UX to a single organization #50279

ref(hybrid-cloud): Restrict notification settings UX to a single organization #50279

Uh oh!

Conversation

mikejihbe commented Jun 4, 2023

Uh oh!

github-actions bot commented Jun 4, 2023

Uh oh!

codecov bot commented Jun 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

corps left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

markstory Jul 5, 2023

Choose a reason for hiding this comment

Uh oh!

markstory Jul 5, 2023

Choose a reason for hiding this comment

Uh oh!

markstory Jul 5, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

markstory Jul 5, 2023

Choose a reason for hiding this comment

Uh oh!

markstory Jul 5, 2023

Choose a reason for hiding this comment

Uh oh!

corps Jul 11, 2023

Choose a reason for hiding this comment

Uh oh!

markstory Jul 5, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Jun 5, 2023 •

edited

Loading