From 9df9934ed62ffcecec0e6ba66701090416587ab8 Mon Sep 17 00:00:00 2001 From: Steve Howell Date: Tue, 13 Oct 2020 13:16:27 +0000 Subject: [PATCH] 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. --- tools/generate-integration-docs-screenshot | 5 ++-- zerver/lib/actions.py | 24 ++++++++++++------- zerver/lib/test_classes.py | 5 ++-- .../commands/add_users_to_streams.py | 2 +- zerver/management/commands/merge_streams.py | 2 +- zerver/tests/test_audit_log.py | 3 ++- zerver/tests/test_events.py | 2 +- zerver/tests/test_subs.py | 8 +++---- zerver/views/registration.py | 2 +- zerver/views/streams.py | 10 ++++---- .../commands/add_mock_conversation.py | 2 +- zilencer/management/commands/add_new_realm.py | 2 +- 12 files changed, 40 insertions(+), 27 deletions(-) diff --git a/tools/generate-integration-docs-screenshot b/tools/generate-integration-docs-screenshot index ef1808d464..9af997b38f 100755 --- a/tools/generate-integration-docs-screenshot +++ b/tools/generate-integration-docs-screenshot @@ -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] diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index 2fdcd26167..6bc8f65bcc 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -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]] = [] diff --git a/zerver/lib/test_classes.py b/zerver/lib/test_classes.py index dd1f54827e..2eb00e9311 100644 --- a/zerver/lib/test_classes.py +++ b/zerver/lib/test_classes.py @@ -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: diff --git a/zerver/management/commands/add_users_to_streams.py b/zerver/management/commands/add_users_to_streams.py index 0058bc3249..b7b9d9895c 100644 --- a/zerver/management/commands/add_users_to_streams.py +++ b/zerver/management/commands/add_users_to_streams.py @@ -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", diff --git a/zerver/management/commands/merge_streams.py b/zerver/management/commands/merge_streams.py index 1f104fbeba..9eb69b3075 100644 --- a/zerver/management/commands/merge_streams.py +++ b/zerver/management/commands/merge_streams.py @@ -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) diff --git a/zerver/tests/test_audit_log.py b/zerver/tests/test_audit_log.py index fd9ddd5b33..b1cdc4aea8 100644 --- a/zerver/tests/test_audit_log.py +++ b/zerver/tests/test_audit_log.py @@ -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]) diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index c84e6a6951..06ebc8bf54 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -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, diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index 0718b2671f..4e9d5c5d34 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -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 diff --git a/zerver/views/registration.py b/zerver/views/registration.py index fec9e10784..35e16d0643 100644 --- a/zerver/views/registration.py +++ b/zerver/views/registration.py @@ -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 diff --git a/zerver/views/streams.py b/zerver/views/streams.py index d39ff07f5c..285c1f37fa 100644 --- a/zerver/views/streams.py +++ b/zerver/views/streams.py @@ -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 diff --git a/zilencer/management/commands/add_mock_conversation.py b/zilencer/management/commands/add_mock_conversation.py index 1be57f9882..a86313f720 100644 --- a/zilencer/management/commands/add_mock_conversation.py +++ b/zilencer/management/commands/add_mock_conversation.py @@ -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, diff --git a/zilencer/management/commands/add_new_realm.py b/zilencer/management/commands/add_new_realm.py index 6f20a44513..8a21252eac 100644 --- a/zilencer/management/commands/add_new_realm.py +++ b/zilencer/management/commands/add_new_realm.py @@ -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)