refactor: Pass realm to bulk_add_subscriptions.

I think it's important that the callers understand
that bulk_add_subscriptions assumes all streams
are being created within a single realm, so I make
it an explicit parameter.

This may be overkill--I would also be happy if we
just included the assertions from this commit.
This commit is contained in:
Steve Howell 2020-10-13 13:16:27 +00:00 committed by Tim Abbott
parent efc931a671
commit 9df9934ed6
12 changed files with 40 additions and 27 deletions

View File

@ -85,8 +85,9 @@ def create_integration_bot(integration: WebhookIntegration, bot_name: Optional[s
def create_integration_stream(integration: WebhookIntegration, bot: UserProfile) -> None:
assert isinstance(bot.bot_owner, UserProfile)
stream, created = create_stream_if_needed(bot.bot_owner.realm, integration.stream_name)
bulk_add_subscriptions([stream], [bot, bot.bot_owner], from_stream_creation=created)
realm = bot.bot_owner.realm
stream, created = create_stream_if_needed(realm, integration.stream_name)
bulk_add_subscriptions(realm, [stream], [bot, bot.bot_owner], from_stream_creation=created)
def get_integration(integration_name: str) -> WebhookIntegration:
integration = INTEGRATIONS[integration_name]

View File

@ -438,7 +438,7 @@ def process_new_human_user(user_profile: UserProfile,
if stream not in streams:
streams.append(stream)
bulk_add_subscriptions(streams, [user_profile], acting_user=acting_user)
bulk_add_subscriptions(realm, streams, [user_profile], acting_user=acting_user)
add_new_user_history(user_profile, streams)
@ -2820,13 +2820,23 @@ def get_last_message_id() -> int:
return last_id
SubT = Tuple[List[Tuple[UserProfile, Stream]], List[Tuple[UserProfile, Stream]]]
def bulk_add_subscriptions(streams: Iterable[Stream],
users: Iterable[UserProfile],
color_map: Mapping[str, str]={},
from_stream_creation: bool=False,
acting_user: Optional[UserProfile]=None) -> SubT:
def bulk_add_subscriptions(
realm: Realm,
streams: Iterable[Stream],
users: Iterable[UserProfile],
color_map: Mapping[str, str]={},
from_stream_creation: bool=False,
acting_user: Optional[UserProfile]=None
) -> SubT:
users = list(users)
# Sanity check out callers
for stream in streams:
assert stream.realm_id == realm.id
for user in users:
assert user.realm_id == realm.id
recipients_map: Dict[int, int] = {stream.id: stream.recipient_id for stream in streams}
recipient_ids = list(recipients_map.values())
stream_map = {recipients_map[stream.id]: stream for stream in streams}
@ -2836,8 +2846,6 @@ def bulk_add_subscriptions(streams: Iterable[Stream],
for sub in all_subs_query:
subs_by_user[sub.user_profile_id].append(sub)
realm = users[0].realm
already_subscribed: List[Tuple[UserProfile, Stream]] = []
subs_to_activate: List[Tuple[Subscription, Stream]] = []
subs_to_add: List[Tuple[Subscription, Stream]] = []

View File

@ -869,12 +869,13 @@ Output:
# Subscribe to a stream directly
def subscribe(self, user_profile: UserProfile, stream_name: str) -> Stream:
realm = user_profile.realm
try:
stream = get_stream(stream_name, user_profile.realm)
from_stream_creation = False
except Stream.DoesNotExist:
stream, from_stream_creation = create_stream_if_needed(user_profile.realm, stream_name)
bulk_add_subscriptions([stream], [user_profile], from_stream_creation=from_stream_creation)
stream, from_stream_creation = create_stream_if_needed(realm, stream_name)
bulk_add_subscriptions(realm, [stream], [user_profile], from_stream_creation=from_stream_creation)
return stream
def unsubscribe(self, user_profile: UserProfile, stream_name: str) -> None:

View File

@ -28,7 +28,7 @@ class Command(ZulipBaseCommand):
for stream_name in set(stream_names):
for user_profile in user_profiles:
stream = ensure_stream(realm, stream_name, acting_user=None)
_ignore, already_subscribed = bulk_add_subscriptions([stream], [user_profile])
_ignore, already_subscribed = bulk_add_subscriptions(realm, [stream], [user_profile])
was_there_already = user_profile.id in (tup[0].id for tup in already_subscribed)
print("{} {} to {}".format(
"Already subscribed" if was_there_already else "Subscribed",

View File

@ -72,4 +72,4 @@ class Command(ZulipBaseCommand):
do_deactivate_stream(stream_to_destroy, acting_user=None)
if len(users_to_activate) > 0:
print(f"Adding {len(users_to_activate)} subscriptions")
bulk_add_subscriptions([stream_to_keep], users_to_activate)
bulk_add_subscriptions(realm, [stream_to_keep], users_to_activate)

View File

@ -205,10 +205,11 @@ class TestRealmAuditLog(ZulipTestCase):
def test_subscriptions(self) -> None:
now = timezone_now()
user = [self.example_user('hamlet')]
stream = [self.make_stream('test_stream')]
acting_user = self.example_user('iago')
bulk_add_subscriptions(stream, user, acting_user=acting_user)
bulk_add_subscriptions(user[0].realm, stream, user, acting_user=acting_user)
subscription_creation_logs = RealmAuditLog.objects.filter(event_type=RealmAuditLog.SUBSCRIPTION_CREATED,
event_time__gte=now, acting_user=acting_user, modified_user=user[0],
modified_stream=stream[0])

View File

@ -1910,7 +1910,7 @@ class SubscribeActionTest(BaseAction):
stream.save()
user_profile = self.example_user('hamlet')
action = lambda: bulk_add_subscriptions([stream], [user_profile])
action = lambda: bulk_add_subscriptions(user_profile.realm, [stream], [user_profile])
events = self.verify_action(
action,
include_subscribers=include_subscribers,

View File

@ -2946,7 +2946,7 @@ class SubscriptionAPITest(ZulipTestCase):
realm3 = user_profile.realm
stream = get_stream('multi_user_stream', realm)
with tornado_redirected_to_list(events):
bulk_add_subscriptions([stream], [user_profile])
bulk_add_subscriptions(realm, [stream], [user_profile])
self.assert_length(events, 2)
add_event, add_peer_event = events
@ -2975,14 +2975,14 @@ class SubscriptionAPITest(ZulipTestCase):
stream = ensure_stream(realm, stream_name, invite_only=True)
existing_user_profile = self.example_user('hamlet')
bulk_add_subscriptions([stream], [existing_user_profile])
bulk_add_subscriptions(realm, [stream], [existing_user_profile])
# Now subscribe Cordelia to the stream, capturing events
user_profile = self.example_user('cordelia')
events: List[Mapping[str, Any]] = []
with tornado_redirected_to_list(events):
bulk_add_subscriptions([stream], [user_profile])
bulk_add_subscriptions(realm, [stream], [user_profile])
self.assert_length(events, 3)
create_event, add_event, add_peer_event = events
@ -3014,7 +3014,7 @@ class SubscriptionAPITest(ZulipTestCase):
new_stream = ensure_stream(realm, "private stream", invite_only=True)
events = []
with tornado_redirected_to_list(events):
bulk_add_subscriptions([new_stream], [self.example_user("iago")])
bulk_add_subscriptions(realm, [new_stream], [self.example_user("iago")])
self.assert_length(events, 3)
create_event, add_event, add_peer_event = events

View File

@ -385,7 +385,7 @@ def accounts_register(request: HttpRequest) -> HttpResponse:
acting_user=None)
if realm_creation:
bulk_add_subscriptions([realm.signup_notifications_stream], [user_profile])
bulk_add_subscriptions(realm, [realm.signup_notifications_stream], [user_profile])
send_initial_realm_messages(realm)
# Because for realm creation, registration happens on the

View File

@ -413,7 +413,8 @@ EMPTY_PRINCIPALS: Union[Sequence[str], Sequence[int]] = []
@require_non_guest_user
@has_request_variables
def add_subscriptions_backend(
request: HttpRequest, user_profile: UserProfile,
request: HttpRequest,
user_profile: UserProfile,
streams_raw: Iterable[Dict[str, str]]=REQ("subscriptions", validator=add_subscriptions_schema),
invite_only: bool=REQ(validator=check_bool, default=False),
stream_post_policy: int=REQ(validator=check_int_in(
@ -427,6 +428,7 @@ def add_subscriptions_backend(
),
authorization_errors_fatal: bool=REQ(validator=check_bool, default=True),
) -> HttpResponse:
realm = user_profile.realm
stream_dicts = []
color_map = {}
for stream_dict in streams_raw:
@ -465,7 +467,7 @@ def add_subscriptions_backend(
streams = authorized_streams + created_streams
if len(principals) > 0:
if user_profile.realm.is_zephyr_mirror_realm and not all(stream.invite_only for stream in streams):
if realm.is_zephyr_mirror_realm and not all(stream.invite_only for stream in streams):
return json_error(_("You can only invite other Zephyr mirroring users to private streams."))
if not user_profile.can_subscribe_other_users():
if user_profile.realm.invite_to_stream_policy == Realm.POLICY_ADMINS_ONLY:
@ -479,7 +481,7 @@ def add_subscriptions_backend(
else:
subscribers = {user_profile}
(subscribed, already_subscribed) = bulk_add_subscriptions(streams, subscribers,
(subscribed, already_subscribed) = bulk_add_subscriptions(realm, streams, subscribers,
acting_user=user_profile, color_map=color_map)
# We can assume unique emails here for now, but we should eventually
@ -686,7 +688,7 @@ def json_stream_exists(request: HttpRequest, user_profile: UserProfile, stream_n
# So if we're not yet subscribed and autosubscribe is enabled, we
# should join.
if sub is None and autosubscribe:
bulk_add_subscriptions([stream], [user_profile], acting_user=user_profile)
bulk_add_subscriptions(user_profile.realm, [stream], [user_profile], acting_user=user_profile)
result["subscribed"] = True
return json_success(result) # results are ignored for HEAD requests

View File

@ -53,7 +53,7 @@ From image editing program:
bot_type=UserProfile.DEFAULT_BOT, acting_user=None)
self.set_avatar(twitter_bot, 'static/images/features/twitter.png')
bulk_add_subscriptions([stream], list(UserProfile.objects.filter(realm=realm)))
bulk_add_subscriptions(realm, [stream], list(UserProfile.objects.filter(realm=realm)))
staged_messages: List[Dict[str, Any]] = [
{'sender': starr,

View File

@ -25,6 +25,6 @@ class Command(ZulipBaseCommand):
acting_user=None,
)
assert realm.signup_notifications_stream is not None
bulk_add_subscriptions([realm.signup_notifications_stream], [user])
bulk_add_subscriptions(realm, [realm.signup_notifications_stream], [user])
send_initial_realm_messages(realm)