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.

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): remove notification double write feature flag #57863

Merged
merged 4 commits into from
Oct 16, 2023
Merged
Show file tree
Hide file tree
Changes from 3 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
4 changes: 2 additions & 2 deletions fixtures/backup/model_dependencies/sorted.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@
"sentry.deletedorganization",
"sentry.email",
"sentry.organization",
"sentry.organizationmapping",
"sentry.identityprovider",
"sentry.docintegration",
"sentry.integration",
"sentry.integrationfeature",
"sentry.organizationmapping",
"sentry.user",
"sentry.project",
"sentry.projectbookmark",
Expand Down Expand Up @@ -150,7 +150,6 @@
"sentry.groupseen",
"sentry.groupshare",
"sentry.groupsnooze",
"sentry.organizationmembermapping",
"sentry.groupsubscription",
"sentry.externalactor",
"sentry.integrationexternalproject",
Expand All @@ -163,6 +162,7 @@
"sentry.latestreporeleaseenvironment",
"sentry.notificationsettingoption",
"sentry.notificationsettingprovider",
"sentry.organizationmembermapping",
"sentry.notificationsetting",
"sentry.organizationaccessrequest",
"sentry.organizationmemberteamreplica",
Expand Down
4 changes: 2 additions & 2 deletions fixtures/backup/model_dependencies/truncate.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@
"sentry_deletedorganization",
"sentry_email",
"sentry_organization",
"sentry_organizationmapping",
"sentry_identityprovider",
"sentry_docintegration",
"sentry_integration",
"sentry_integrationfeature",
"sentry_organizationmapping",
"auth_user",
"sentry_project",
"sentry_projectbookmark",
Expand Down Expand Up @@ -150,7 +150,6 @@
"sentry_groupseen",
"sentry_groupshare",
"sentry_groupsnooze",
"sentry_organizationmembermapping",
"sentry_groupsubscription",
"sentry_externalactor",
"sentry_integrationexternalproject",
Expand All @@ -163,6 +162,7 @@
"sentry_latestrelease",
"sentry_notificationsettingoption",
"sentry_notificationsettingprovider",
"sentry_organizationmembermapping",
"sentry_notificationsetting",
"sentry_organizationaccessrequest",
"sentry_organizationmember_teamsreplica",
Expand Down
26 changes: 11 additions & 15 deletions src/sentry/api/endpoints/user_notification_fine_tuning.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
from sentry.models.notificationsettingoption import NotificationSettingOption
from sentry.models.options.user_option import UserOption
from sentry.models.useremail import UserEmail
from sentry.notifications.helpers import is_double_write_enabled
from sentry.notifications.types import (
FineTuningAPIKey,
NotificationScopeEnum,
Expand Down Expand Up @@ -123,8 +122,6 @@ def _handle_put_reports(user, data):

value = set(user_option.value or [])

double_write_enabled = is_double_write_enabled(user_id=user.id)

# The set of IDs of the organizations of which the user is a member.
org_ids = {o.id for o in user_service.get_organizations(user_id=user.id, only_visible=True)}
for org_id, enabled in data.items():
Expand All @@ -146,18 +143,17 @@ def _handle_put_reports(user, data):
elif not enabled:
value.add(org_id)

if double_write_enabled:
NotificationSettingOption.objects.create_or_update(
scope_type=NotificationScopeEnum.ORGANIZATION.value,
scope_identifier=org_id,
user_id=user.id,
type=NotificationSettingEnum.REPORTS.value,
values={
"value": NotificationSettingsOptionEnum.ALWAYS.value
if enabled
else NotificationSettingsOptionEnum.NEVER.value,
},
)
NotificationSettingOption.objects.create_or_update(
scope_type=NotificationScopeEnum.ORGANIZATION.value,
scope_identifier=org_id,
user_id=user.id,
type=NotificationSettingEnum.REPORTS.value,
values={
"value": NotificationSettingsOptionEnum.ALWAYS.value
if enabled
else NotificationSettingsOptionEnum.NEVER.value,
},
)

user_option.update(value=list(value))
return Response(status=status.HTTP_204_NO_CONTENT)
Expand Down
2 changes: 0 additions & 2 deletions src/sentry/conf/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -1786,8 +1786,6 @@ def custom_parameter_sort(parameter: dict) -> tuple[str, int]:
"organizations:sourcemaps-upload-release-as-artifact-bundle": False,
# Signals that the organization supports the on demand metrics prefill.
"organizations:on-demand-metrics-prefill": False,
# Enable writing to the new notification system when updating the old system
"organizations:notifications-double-write": True,
# Enable source maps debugger
"organizations:source-maps-debugger-blue-thunder-edition": False,
# Enable the new suspect commits calculation that uses all frames in the stack trace
Expand Down
1 change: 0 additions & 1 deletion src/sentry/features/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,6 @@
default_manager.add("organizations:sourcemaps-upload-release-as-artifact-bundle", OrganizationFeature, FeatureHandlerStrategy.REMOTE)
default_manager.add("organizations:recap-server", OrganizationFeature, FeatureHandlerStrategy.INTERNAL)
default_manager.add("organizations:notification-settings-v2", OrganizationFeature, FeatureHandlerStrategy.INTERNAL)
default_manager.add("organizations:notifications-double-write", OrganizationFeature, FeatureHandlerStrategy.REMOTE)
default_manager.add("organizations:release-ui-v2", OrganizationFeature, FeatureHandlerStrategy.REMOTE)
default_manager.add("organizations:source-maps-debugger-blue-thunder-edition", OrganizationFeature, FeatureHandlerStrategy.REMOTE)
default_manager.add("organizations:suspect-commits-all-frames", OrganizationFeature, FeatureHandlerStrategy.INTERNAL)
Expand Down
32 changes: 1 addition & 31 deletions src/sentry/notifications/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,11 @@

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

from django.contrib.auth.models import AnonymousUser

from sentry import features
from sentry.models.organizationmapping import OrganizationMapping
from sentry.models.organizationmembermapping import OrganizationMemberMapping
from sentry.notifications.defaults import (
NOTIFICATION_SETTING_DEFAULTS,
NOTIFICATION_SETTINGS_ALL_SOMETIMES,
Expand Down Expand Up @@ -717,34 +715,6 @@ def get_recipient_from_team_or_user(user_id: int | None, team_id: int | None) ->
return recipient


def is_double_write_enabled(
user_id: Optional[int] = None, organization_id_for_team: Optional[int] = None
):
from sentry.services.hybrid_cloud.organization_mapping.serial import (
serialize_organization_mapping,
)

# all operations are expected to happen in the control siolo
if organization_id_for_team is not None:
org_ids = [organization_id_for_team]
elif user_id is not None:
org_ids = OrganizationMemberMapping.objects.filter(user_id=user_id).values_list(
"organization_id", flat=True
)
else:
raise ValueError("Need organization_id_for_team or user_id")
orgs = list(
map(
serialize_organization_mapping,
OrganizationMapping.objects.filter(organization_id__in=list(org_ids)),
)
)
# if no orgs, then automatically enable the feature
if not orgs:
return True
return any(features.has("organizations:notifications-double-write", org) for org in orgs)


PROVIDER_DEFAULTS: list[ExternalProviderEnum] = get_provider_defaults()
TYPE_DEFAULTS: Mapping[
NotificationSettingEnum, NotificationSettingsOptionEnum
Expand Down
38 changes: 12 additions & 26 deletions src/sentry/notifications/manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
from sentry.notifications.helpers import (
get_scope,
get_scope_type,
is_double_write_enabled,
should_use_notifications_v2,
transform_to_notification_settings_by_recipient,
validate,
Expand Down Expand Up @@ -210,11 +209,6 @@ def update_settings(
**{id_key: actor_id},
)

if not is_double_write_enabled(
user_id=user_id, organization_id_for_team=organization_id_for_team
):
return

# implement the double write now
query_args = {
"type": NOTIFICATION_SETTING_TYPES[type],
Expand Down Expand Up @@ -255,23 +249,19 @@ def remove_settings(
user_id = user.id

# get the actor type and actor id
use_double_write = is_double_write_enabled(
user_id=user_id, organization_id_for_team=organization_id_for_team
scope_type, scope_identifier = get_scope(
team=team_id, user=user_id, project=project, organization=organization
)
if use_double_write:
scope_type, scope_identifier = get_scope(
team=team_id, user=user_id, project=project, organization=organization
)
scope_type_str = NOTIFICATION_SCOPE_TYPE[scope_type]
# remove the option setting
NotificationSettingOption.objects.filter(
scope_type=scope_type_str,
scope_identifier=scope_identifier,
team_id=team_id,
user_id=user_id,
type=type,
).delete()
# the provider setting is updated elsewhere
scope_type_str = NOTIFICATION_SCOPE_TYPE[scope_type]
# remove the option setting
NotificationSettingOption.objects.filter(
scope_type=scope_type_str,
scope_identifier=scope_identifier,
team_id=team_id,
user_id=user_id,
type=type,
).delete()
# the provider setting is updated elsewhere

self.find_settings(
provider,
Expand Down Expand Up @@ -614,10 +604,6 @@ def update_settings_bulk(

user_id = user.id if user else None
team_id = team.id if team else None
if not is_double_write_enabled(
user_id=user_id, organization_id_for_team=organization_id_for_team
):
return

enabled_providers = defaultdict(set)
disabled_providers = defaultdict(set)
Expand Down
5 changes: 2 additions & 3 deletions src/sentry/services/hybrid_cloud/notifications/impl.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
from sentry.models.notificationsettingoption import NotificationSettingOption
from sentry.models.notificationsettingprovider import NotificationSettingProvider
from sentry.models.user import User
from sentry.notifications.helpers import get_scope_type, is_double_write_enabled
from sentry.notifications.helpers import get_scope_type
from sentry.notifications.notificationcontroller import NotificationController
from sentry.notifications.types import (
NotificationScopeEnum,
Expand Down Expand Up @@ -88,8 +88,7 @@ def bulk_update_settings(
skip_provider_updates=True,
)
# update the providers at the end
if is_double_write_enabled(user_id=user_id):
NotificationSetting.objects.update_provider_settings(user_id, None)
NotificationSetting.objects.update_provider_settings(user_id, None)

# TODO(snigdha): This can be removed in V2.
def get_settings_for_users(
Expand Down
33 changes: 0 additions & 33 deletions tests/sentry/api/endpoints/test_user_notification_details.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
from sentry.models.notificationsetting import NotificationSetting
from sentry.models.notificationsettingoption import NotificationSettingOption
from sentry.models.notificationsettingprovider import NotificationSettingProvider
from sentry.models.organizationmembermapping import OrganizationMemberMapping
from sentry.notifications.types import NotificationSettingOptionValues, NotificationSettingTypes
from sentry.testutils.cases import APITestCase
from sentry.testutils.helpers.features import with_feature
from sentry.testutils.silo import control_silo_test
from sentry.types.integrations import ExternalProviders

Expand Down Expand Up @@ -83,38 +81,7 @@ def test_subscribe_by_default(self):
class UserNotificationDetailsPutTest(UserNotificationDetailsTestBase):
method = "put"

@with_feature({"organizations:notifications-double-write": False})
def test_saves_and_returns_values(self):
data = {
"deployNotifications": 2,
"personalActivityNotifications": True,
"selfAssignOnResolve": True,
}
# make an org mapping for the user
OrganizationMemberMapping.objects.create(
organization_id=self.organization.id, user_id=self.user.id
)

response = self.get_success_response("me", **data)

assert response.data.get("deployNotifications") == 2
assert response.data.get("personalActivityNotifications") is True
assert response.data.get("selfAssignOnResolve") is True
assert response.data.get("subscribeByDefault") is True
assert response.data.get("workflowNotifications") == 1

value = NotificationSetting.objects.get_settings(
ExternalProviders.EMAIL,
NotificationSettingTypes.DEPLOY,
user_id=self.user.id,
)
assert value == NotificationSettingOptionValues.ALWAYS
# ensure double write does not happen
assert not NotificationSettingOption.objects.filter(user_id=self.user.id).exists()
assert not NotificationSettingProvider.objects.filter(user_id=self.user.id).exists()

@with_feature("organizations:notifications-double-write")
def test_double_write(self):
org = self.create_organization()
self.create_member(user=self.user, organization=org)
data = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
from sentry.models.useremail import UserEmail
from sentry.notifications.types import NotificationSettingOptionValues, NotificationSettingTypes
from sentry.testutils.cases import APITestCase
from sentry.testutils.helpers.features import with_feature
from sentry.testutils.silo import control_silo_test
from sentry.types.integrations import ExternalProviders

Expand Down Expand Up @@ -152,7 +151,6 @@ def test_saves_and_returns_workflow(self):
assert value1 == NotificationSettingOptionValues.DEFAULT
assert value2 == NotificationSettingOptionValues.NEVER

@with_feature("organizations:notifications-double-write")
def test_double_write(self):
data = {str(self.project.id): 1, str(self.project2.id): 2}
self.get_success_response("me", "workflow", status_code=204, **data)
Expand Down Expand Up @@ -333,7 +331,6 @@ def test_enable_weekly_reports_from_default_setting(self):
== set()
)

@with_feature("organizations:notifications-double-write")
def test_double_write_weekly_report(self):
data = {str(self.organization.id): 1, str(self.organization2.id): "1"}
self.get_success_response("me", "reports", status_code=204, **data)
Expand Down
3 changes: 0 additions & 3 deletions tests/sentry/api/endpoints/test_user_notification_settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
from sentry.models.notificationsettingprovider import NotificationSettingProvider
from sentry.notifications.types import NotificationSettingOptionValues, NotificationSettingTypes
from sentry.testutils.cases import APITestCase
from sentry.testutils.helpers.features import with_feature
from sentry.testutils.silo import control_silo_test
from sentry.types.integrations import ExternalProviders

Expand Down Expand Up @@ -125,7 +124,6 @@ def test_simple(self):
== NotificationSettingOptionValues.ALWAYS
)

@with_feature("organizations:notifications-double-write")
def test_double_write(self):
org = self.create_organization()
self.create_member(user=self.user, organization=org)
Expand Down Expand Up @@ -180,7 +178,6 @@ def test_double_write(self):
**query_args, value="never", provider="slack"
)

@with_feature("organizations:notifications-double-write")
def test_double_write_with_email_off(self):
org = self.create_organization()
self.create_member(user=self.user, organization=org)
Expand Down
1 change: 0 additions & 1 deletion tests/sentry/api/serializers/test_organization.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,6 @@ def test_simple(self):
"invite-members",
"invite-members-rate-limits",
"minute-resolution-sessions",
"notifications-double-write",
"open-membership",
"project-stats",
"relay",
Expand Down
10 changes: 4 additions & 6 deletions tests/sentry/models/test_notificationsetting.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
from sentry.silo import SiloMode
from sentry.tasks.deletion.hybrid_cloud import schedule_hybrid_cloud_foreign_key_jobs_control
from sentry.testutils.cases import TestCase
from sentry.testutils.helpers import Feature
from sentry.testutils.outbox import outbox_runner
from sentry.testutils.silo import assume_test_silo_mode, control_silo_test
from sentry.types.integrations import ExternalProviders
Expand All @@ -29,11 +28,10 @@ def assert_no_notification_settings():


def create_setting(**kwargs):
with Feature({"organizations:notifications-double-write": True}):
NotificationSetting.objects.update_settings(
value=NotificationSettingOptionValues.ALWAYS,
**_get_kwargs(kwargs),
)
NotificationSetting.objects.update_settings(
value=NotificationSettingOptionValues.ALWAYS,
**_get_kwargs(kwargs),
)


@control_silo_test(stable=True)
Expand Down
Loading
Loading