Skip to content

feat(notifications-v2): Read from notification settings for weekly reports #56621

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

Merged
merged 2 commits into from
Sep 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
24 changes: 24 additions & 0 deletions src/sentry/notifications/notificationcontroller.py
Original file line number Diff line number Diff line change
Expand Up @@ -553,3 +553,27 @@ def get_notification_provider_value_for_recipient_and_type(
return value

return NotificationSettingsOptionEnum.NEVER

def get_users_for_weekly_reports(self) -> list[int]:
Copy link
Contributor

Choose a reason for hiding this comment

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

@snigdhas Seems like we can use a combination of get_notification_value_for_recipient_and_type and get_notification_provider_value_for_recipient_and_type to make this logic super simple, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could, but we'd end up calling self._get_layered_setting_options N times for N users, and each call would do iterate over the entire dict. We could store the layered settings in memory instead of computing it but I'm not sure that's necessary

if not self.organization_id:
raise Exception("Must specify organization_id")

if self.type != NotificationSettingEnum.REPORTS:
raise Exception(f"Type mismatch: the controller was initialized with type: {self.type}")

enabled_settings = self.get_all_enabled_settings(type=NotificationSettingEnum.REPORTS.value)
users = []
for recipient, setting in enabled_settings.items():
if not recipient_is_user(recipient):
continue

for type_map in setting.values():
provider_map = type_map[NotificationSettingEnum.REPORTS]
if (
ExternalProviderEnum.EMAIL in provider_map
and provider_map[ExternalProviderEnum.EMAIL]
== NotificationSettingsOptionEnum.ALWAYS
):
users.append(recipient.id)

return users
37 changes: 35 additions & 2 deletions src/sentry/tasks/weekly_reports.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@
OrganizationMember,
OrganizationStatus,
)
from sentry.notifications.helpers import should_use_notifications_v2
from sentry.notifications.notificationcontroller import NotificationController
from sentry.notifications.types import NotificationSettingEnum
from sentry.services.hybrid_cloud.user.model import RpcUser
from sentry.services.hybrid_cloud.user_option import user_option_service
from sentry.silo import SiloMode
from sentry.snuba.dataset import Dataset
Expand Down Expand Up @@ -225,10 +229,16 @@ def prepare_organization_report(
)
return

use_notifications_v2 = should_use_notifications_v2(ctx.organization)

# Finally, deliver the reports
with sentry_sdk.start_span(op="weekly_reports.deliver_reports"):
deliver_reports(
ctx, dry_run=dry_run, target_user=target_user, email_override=email_override
ctx,
dry_run=dry_run,
target_user=target_user,
email_override=email_override,
use_notifications_v2=use_notifications_v2,
)


Expand Down Expand Up @@ -637,10 +647,33 @@ def fetch_key_performance_issue_groups(ctx):
# For all users in the organization, we generate the template context for the user, and send the email.


def deliver_reports(ctx, dry_run=False, target_user=None, email_override=None):
def deliver_reports(
ctx, dry_run=False, target_user=None, email_override=None, use_notifications_v2=False
):
# Specify a sentry user to send this email.
if email_override:
send_email(ctx, target_user, dry_run=dry_run, email_override=email_override)
elif use_notifications_v2:
user_list = list(
OrganizationMember.objects.filter(
user_is_active=True,
organization_id=ctx.organization.id,
)
.filter(flags=F("flags").bitand(~OrganizationMember.flags["member-limit:restricted"]))
.values_list("user_id", flat=True)
)

users = [RpcUser(id=user_id) for user_id in user_list]
controller = NotificationController(
recipients=users,
organization_id=ctx.organization.id,
type=NotificationSettingEnum.REPORTS,
)

user_ids = controller.get_users_for_weekly_reports()
for user_id in user_ids:
send_email(ctx, user_id, dry_run=dry_run)

else:
# We save the subscription status of the user in a field in UserOptions.
user_list = list(
Expand Down
23 changes: 23 additions & 0 deletions tests/sentry/notifications/test_notificationcontroller.py
Original file line number Diff line number Diff line change
Expand Up @@ -752,3 +752,26 @@ def test_get_notification_provider_value_for_recipient_and_type_with_layering(se
)
== NotificationSettingsOptionEnum.NEVER
)

def test_get_users_for_weekly_reports(self):
controller = NotificationController(
recipients=[self.user],
organization_id=self.organization.id,
type=NotificationSettingEnum.REPORTS,
)
assert controller.get_users_for_weekly_reports() == [self.user.id]

add_notification_setting_option(
scope_type=NotificationScopeEnum.USER,
scope_identifier=self.user.id,
type=NotificationSettingEnum.REPORTS,
value=NotificationSettingsOptionEnum.NEVER,
user_id=self.user.id,
)

controller = NotificationController(
recipients=[self.user],
organization_id=self.organization.id,
type=NotificationSettingEnum.REPORTS,
)
assert controller.get_users_for_weekly_reports() == []