Skip to content

fix(notifications): Fix parameters and add tests for participants and subscriptions #57053

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 10 commits into from
Oct 3, 2023
Merged
Show file tree
Hide file tree
Changes from 5 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
6 changes: 4 additions & 2 deletions src/sentry/api/serializers/models/group.py
Original file line number Diff line number Diff line change
Expand Up @@ -584,7 +584,9 @@ def _get_subscriptions(
project_ids=project_ids,
type=NotificationSettingEnum.WORKFLOW,
)
query_groups = {group for group in groups if (enabled_settings[group.project_id][1])}
query_groups = {
group for group in groups if (not enabled_settings[group.project_id][2])
Copy link
Contributor

Choose a reason for hiding this comment

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

@snigdhas can we use something other than a tuple to make it more obvious what the 2nd parameter is?

Copy link
Member Author

Choose a reason for hiding this comment

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

not really - the response from notification_service needs to be json serializable. We can re-map it to SubscriptionStatus like we do below, but it's not necessarily cleaner here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@snigdhas can we do a dictionary then instead of a tuple?

}
subscriptions_by_group_id: dict[int, GroupSubscription] = {
subscription.group_id: subscription
for subscription in GroupSubscription.objects.filter(
Expand All @@ -595,7 +597,7 @@ def _get_subscriptions(

results = {}
for project_id, group_set in groups_by_project.items():
(disabled_settings, has_enabled_settings) = enabled_settings[project_id]
(disabled_settings, has_enabled_settings, _) = enabled_settings[project_id]
for group in group_set:
subscription = subscriptions_by_group_id.get(group.id)
if subscription:
Expand Down
2 changes: 1 addition & 1 deletion src/sentry/api/serializers/models/project.py
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,7 @@ def measure_span(op_tag):

if use_notifications_v2:
subscriptions = notifications_service.get_subscriptions_for_projects(
user=user.id,
user_id=user.id,
project_ids=project_ids,
type=NotificationSettingTypes.ISSUE_ALERTS,
)
Expand Down
25 changes: 19 additions & 6 deletions src/sentry/models/groupsubscription.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,13 @@
from sentry.notifications.types import (
GroupSubscriptionReason,
NotificationSettingEnum,
NotificationSettingsOptionEnum,
NotificationSettingTypes,
)
from sentry.services.hybrid_cloud.actor import RpcActor
from sentry.services.hybrid_cloud.notifications import notifications_service
from sentry.services.hybrid_cloud.user import RpcUser
from sentry.types.integrations import ExternalProviders

if TYPE_CHECKING:
from sentry.models import Group, Team, User
Expand Down Expand Up @@ -187,17 +189,28 @@ def get_participants(self, group: Group) -> ParticipantMap:
)
result = ParticipantMap()
for user in all_possible_users:
subscription_option = subscriptions_by_user_id.get(user.id)
subscription_option = subscriptions_by_user_id.get(user.id, {})
if user.id not in providers_by_recipient:
continue

for provider in providers_by_recipient[user.id]:
reason = (
for provider, val in providers_by_recipient[user.id].items():
value = NotificationSettingsOptionEnum(val)
is_subcribed = (
subscription_option
and subscription_option.reason
or GroupSubscriptionReason.implicit
and subscription_option.is_active
and value != NotificationSettingsOptionEnum.NEVER
)
result.add(provider, user, reason)
is_implicit = (
not subscription_option and value == NotificationSettingsOptionEnum.ALWAYS
)
if is_subcribed or is_implicit:
reason = (
subscription_option
and subscription_option.reason
or GroupSubscriptionReason.implicit
)
provider = ExternalProviders(provider)
result.add(provider, user, reason)
return result

notification_settings = notifications_service.get_settings_for_recipient_by_parent(
Expand Down
36 changes: 36 additions & 0 deletions src/sentry/notifications/helpers.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from __future__ import annotations

import logging
from collections import defaultdict
from typing import TYPE_CHECKING, Any, Iterable, Mapping, MutableMapping, Optional

Expand Down Expand Up @@ -49,6 +50,8 @@
User,
)

logger = logging.getLogger(__name__)


def _get_notification_setting_default(
provider: ExternalProviders,
Expand Down Expand Up @@ -89,6 +92,39 @@ def get_provider_defaults() -> list[ExternalProviderEnum]:
return provider_defaults


def get_default_for_provider(
type: NotificationSettingEnum,
provider: ExternalProviderEnum,
) -> NotificationSettingsOptionEnum:
defaults = PROVIDER_DEFAULTS
if provider not in defaults:
return NotificationSettingsOptionEnum.NEVER

# Defaults are defined for the old int enum
_type = [key for key, val in NOTIFICATION_SETTING_TYPES.items() if val == type.value]
if len(_type) != 1 or _type[0] not in NOTIFICATION_SETTINGS_ALL_SOMETIMES_V2:
logger.warning(
"Could not find default for notification type",
extra={"type": type, "_type": _type, "provider": provider},
)
return NotificationSettingsOptionEnum.NEVER

try:
default_value = NOTIFICATION_SETTINGS_ALL_SOMETIMES_V2[_type[0]]
default_enum = NotificationSettingsOptionEnum(
NOTIFICATION_SETTING_OPTION_VALUES[default_value]
)
except KeyError:
# If we don't have a default value for the type, then it's never
return NotificationSettingsOptionEnum.NEVER

if type == NotificationSettingEnum.REPORTS and provider != ExternalProviderEnum.EMAIL:
# Reports are only sent to email
return NotificationSettingsOptionEnum.NEVER

return default_enum or NotificationSettingsOptionEnum.NEVER


def get_type_defaults() -> Mapping[NotificationSettingEnum, NotificationSettingsOptionEnum]:
# this tells us what the default value is for each notification type
type_defaults = {}
Expand Down
35 changes: 20 additions & 15 deletions src/sentry/notifications/notificationcontroller.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from sentry.models.notificationsettingprovider import NotificationSettingProvider
from sentry.models.team import Team
from sentry.notifications.helpers import (
get_provider_defaults,
get_default_for_provider,
get_type_defaults,
recipient_is_team,
recipient_is_user,
Expand Down Expand Up @@ -250,7 +250,6 @@ def _get_layered_setting_providers(
)
scoped_settings = [project_settings, org_settings, user_settings]

defaults = get_provider_defaults()
layered_setting_providers: MutableMapping[
Recipient,
MutableMapping[
Expand All @@ -266,13 +265,18 @@ def _get_layered_setting_providers(
for item in scope_items:
scope = (scope_type, item)
for type in NotificationSettingEnum:
if type == NotificationSettingEnum.DEFAULT:
continue
for provider in ExternalProviderEnum:
most_specific_setting = None
for setting in scoped_settings:
user_or_team_settings = []
if recipient_is_user(recipient):
user_or_team_settings = self._filter_providers(
settings=setting, user_id=recipient.id, type=type.value
settings=setting,
user_id=recipient.id,
type=type.value,
provider=provider.value,
)
elif recipient_is_team(recipient):
user_or_team_settings = self._filter_providers(
Expand All @@ -286,11 +290,7 @@ def _get_layered_setting_providers(
break

if most_specific_setting is None:
most_specific_setting = (
NotificationSettingsOptionEnum.ALWAYS
if provider in defaults
else NotificationSettingsOptionEnum.NEVER
)
most_specific_setting = get_default_for_provider(type, provider)

layered_setting_providers[recipient][scope][type].update(
{provider: most_specific_setting}
Expand Down Expand Up @@ -384,7 +384,7 @@ def get_all_enabled_settings(

return setting_option_and_providers

def get_settings_options_for_user_by_projects(
def get_settings_for_user_by_projects(
self, user: Recipient
) -> MutableMapping[
int,
Expand Down Expand Up @@ -420,16 +420,16 @@ def get_subscriptions_status_for_projects(
user: Recipient,
project_ids: Iterable[int],
type: NotificationSettingEnum | None = None,
) -> Mapping[int, Tuple[bool, bool]]:
) -> Mapping[int, Tuple[bool, bool, bool]]:
"""
Returns whether the user is subscribed for each project.
{project_id -> (is_disabled, is_active)}
{project_id -> (is_disabled, is_active, has only inactive subscriptions)}
"""
setting_type = type or self.type
if not setting_type:
raise Exception("Must specify type")

enabled_settings = self.get_settings_options_for_user_by_projects(user)
enabled_settings = self.get_settings_for_user_by_projects(user)
subscription_status_for_projects = {}
for project, type_setting in enabled_settings.items():
has_setting = False
Expand All @@ -446,10 +446,13 @@ def get_subscriptions_status_for_projects(
any(
value == NotificationSettingsOptionEnum.ALWAYS for value in setting.values()
),
all(
value == NotificationSettingsOptionEnum.NEVER for value in setting.values()
),
)

if not has_setting:
subscription_status_for_projects[project] = (False, False)
subscription_status_for_projects[project] = (True, False, True)

return subscription_status_for_projects

Expand All @@ -476,11 +479,13 @@ def get_participants(

actor = RpcActor.from_object(recipient)
for type_map in setting.values():
for provider_map in type_map.values():
for type, provider_map in type_map.items():
if type != self.type:
continue

user_to_providers[actor] = {
EXTERNAL_PROVIDERS_REVERSE[provider]: value
for provider, value in provider_map.items()
if value != NotificationSettingsOptionEnum.NEVER
}

return user_to_providers
Expand Down
9 changes: 6 additions & 3 deletions src/sentry/notifications/utils/participants.py
Original file line number Diff line number Diff line change
Expand Up @@ -218,14 +218,17 @@ def get_participants_for_release(
if should_use_notifications_v2(organization):
providers_by_recipient = notifications_service.get_participants(
recipients=actors,
project_ids=[project.id for project in projects],
organization_id=organization.id,
type=NotificationSettingTypes.DEPLOY,
type=NotificationSettingEnum.DEPLOY,
)

users_to_reasons_by_provider = ParticipantMap()
for actor in actors:
settings = providers_by_recipient[actor.id]
for provider, val in settings.items():
settings = providers_by_recipient.get(actor.id, {})
for provider_str, val_str in settings.items():
provider = ExternalProviders(provider_str)
val = NotificationSettingsOptionEnum(val_str)
reason = get_reason(actor, val, commited_user_ids)
if reason:
users_to_reasons_by_provider.add(provider, actor, reason)
Expand Down
12 changes: 8 additions & 4 deletions src/sentry/services/hybrid_cloud/notifications/impl.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
NotificationScopeType,
NotificationSettingEnum,
NotificationSettingOptionValues,
NotificationSettingsOptionEnum,
NotificationSettingTypes,
)
from sentry.services.hybrid_cloud.actor import ActorType, RpcActor
Expand Down Expand Up @@ -224,7 +223,7 @@ def get_subscriptions_for_projects(
user_id: int,
project_ids: List[int],
type: NotificationSettingEnum,
) -> Mapping[int, Tuple[bool, bool]]:
) -> Mapping[int, Tuple[bool, bool, bool]]:
user = user_service.get_user(user_id)
if not user:
return {}
Expand All @@ -245,15 +244,20 @@ def get_participants(
project_ids: Optional[List[int]],
organization_id: Optional[int],
type: NotificationSettingEnum,
) -> MutableMapping[int, MutableMapping[ExternalProviders, NotificationSettingsOptionEnum]]:
) -> MutableMapping[
int, MutableMapping[int, str]
]: # { actor_id : { provider_str: value_str } }
controller = NotificationController(
recipients=recipients,
project_ids=project_ids,
organization_id=organization_id,
type=type,
)
participants = controller.get_participants()
return {actor.id: providers for actor, providers in participants.items()}
return {
actor.id: {provider.value: value.value for provider, value in providers.items()}
for actor, providers in participants.items()
}

class _NotificationSettingsQuery(
FilterQueryDatabaseImpl[
Expand Down
5 changes: 2 additions & 3 deletions src/sentry/services/hybrid_cloud/notifications/service.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
from sentry.notifications.types import (
NotificationSettingEnum,
NotificationSettingOptionValues,
NotificationSettingsOptionEnum,
NotificationSettingTypes,
)
from sentry.services.hybrid_cloud.actor import RpcActor
Expand Down Expand Up @@ -143,7 +142,7 @@ def get_subscriptions_for_projects(
user_id: int,
project_ids: List[int],
type: NotificationSettingEnum,
) -> Mapping[int, Tuple[bool, bool]]:
) -> Mapping[int, Tuple[bool, bool, bool]]:
pass

@rpc_method
Expand All @@ -155,7 +154,7 @@ def get_participants(
project_ids: Optional[List[int]],
organization_id: Optional[int],
type: NotificationSettingEnum,
) -> MutableMapping[int, MutableMapping[ExternalProviders, NotificationSettingsOptionEnum]]:
) -> MutableMapping[int, MutableMapping[int, str]]:
pass


Expand Down
Loading