From 75b61a82611267d7dbd68bad22db75504ee2441f Mon Sep 17 00:00:00 2001 From: Sahil Batra Date: Fri, 30 Jun 2023 16:44:49 +0530 Subject: [PATCH] streams: Send stream creation events when subscribing guests. We did not send the stream creation events when subscribing guests to public streams while we do send them when subscribing non-admin users to private streams. This commit adds code to send the stream creation events when subscribing guests to public streams, so the clients can know that the stream exists and fixes the bug where client tries to process a subscription add event for a stream which it does not know about. --- api_docs/changelog.md | 7 +++++++ version.py | 2 +- zerver/actions/streams.py | 19 +++++++++++++++---- zerver/openapi/zulip.yaml | 8 ++++++-- zerver/tests/test_events.py | 12 ++++++++++++ zerver/tests/test_subs.py | 30 +++++++++++++++++++++++++++++- 6 files changed, 70 insertions(+), 8 deletions(-) diff --git a/api_docs/changelog.md b/api_docs/changelog.md index 888a207416..f50a3b03e0 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -20,6 +20,13 @@ format used by the Zulip server that they are interacting with. ## Changes in Zulip 8.0 +**Feature level 192** + +* [`GET /events`](/api/get-events): Stream creation events are now + sent when guest users gain access to a public stream by being + subscribed. Guest users previously only received these events when + subscribed to private streams. + **Feature level 191** * [`GET /events`](/api/get-events), [`POST /register`](/api/register-queue), diff --git a/version.py b/version.py index 350e0d69a7..da5b54b333 100644 --- a/version.py +++ b/version.py @@ -33,7 +33,7 @@ DESKTOP_WARNING_VERSION = "5.9.3" # Changes should be accompanied by documentation explaining what the # new level means in api_docs/changelog.md, as well as "**Changes**" # entries in the endpoint's documentation in `zulip.yaml`. -API_FEATURE_LEVEL = 191 +API_FEATURE_LEVEL = 192 # Bump the minor PROVISION_VERSION to indicate that folks should provision # only when going from an old version of the code to a newer version. Bump diff --git a/zerver/actions/streams.py b/zerver/actions/streams.py index 4b326f9732..1e6e62df5c 100644 --- a/zerver/actions/streams.py +++ b/zerver/actions/streams.py @@ -345,14 +345,16 @@ def bulk_add_subs_to_db_with_logging( RealmAuditLog.objects.bulk_create(all_subscription_logs) -def send_stream_creation_events_for_private_streams( +def send_stream_creation_events_for_previously_inaccessible_streams( realm: Realm, stream_dict: Dict[int, Stream], altered_user_dict: Dict[int, Set[int]], + altered_guests: Set[int], ) -> None: for stream_id, stream_users_ids in altered_user_dict.items(): stream = stream_dict[stream_id] + notify_user_ids = [] if not stream.is_public(): # Users newly added to invite-only streams # need a `create` notification. The former, because @@ -362,9 +364,14 @@ def send_stream_creation_events_for_private_streams( # Realm admins already have all created private streams. realm_admin_ids = {user.id for user in realm.get_admin_users_and_bots()} notify_user_ids = list(stream_users_ids - realm_admin_ids) + else: + # Guese users need a `create` notification for + # public streams as well because they need the stream + # to exist before they get the "subscribe" notification. + notify_user_ids = list(stream_users_ids & altered_guests) - if notify_user_ids: - send_stream_creation_event(realm, stream, notify_user_ids) + if notify_user_ids: + send_stream_creation_event(realm, stream, notify_user_ids) def send_peer_subscriber_events( @@ -538,8 +545,11 @@ def bulk_add_subscriptions( ) altered_user_dict: Dict[int, Set[int]] = defaultdict(set) + altered_guests: Set[int] = set() for sub_info in subs_to_add + subs_to_activate: altered_user_dict[sub_info.stream.id].add(sub_info.user.id) + if sub_info.user.is_guest: + altered_guests.add(sub_info.user.id) stream_dict = {stream.id: stream for stream in streams} @@ -555,10 +565,11 @@ def bulk_add_subscriptions( # being subscribed; we can skip these notifications when this is # being called from the new user creation flow. if not from_user_creation: - send_stream_creation_events_for_private_streams( + send_stream_creation_events_for_previously_inaccessible_streams( realm=realm, stream_dict=stream_dict, altered_user_dict=altered_user_dict, + altered_guests=altered_guests, ) send_subscription_add_events( diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 233e279aaa..d710c07ae5 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -1170,11 +1170,15 @@ paths: This event is also sent when a user gains access to a stream they previously [could not access](/help/stream-permissions), such as - when a private stream is made public, or when a user is subscribed - to a private stream. + when a private stream is made public, or when a [guest user](/help/roles-and-permissions) + is subscribed to a public (or private) stream. Note that organization administrators who are not subscribed will not be able to see content on the stream; just that it exists. + + **Changes**: Prior to Zulip 8.0 (feature level 192), this event was + not sent when guest users gained access to a public stream by being + subscribed. properties: id: $ref: "#/components/schemas/EventIdSchema" diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index e357cc120d..149af269e7 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -3261,6 +3261,18 @@ class SubscribeActionTest(BaseAction): 10, ) + stream.invite_only = False + stream.save() + + # Subscribe as a guest to a public stream. + self.user_profile = self.example_user("polonius") + action = lambda: bulk_add_subscriptions( + user_profile.realm, [stream], [self.user_profile], acting_user=None + ) + events = self.verify_action(action, include_subscribers=include_subscribers, num_events=2) + check_stream_create("events[0]", events[0]) + check_subscription_add("events[1]", events[1]) + class DraftActionTest(BaseAction): def do_enable_drafts_synchronization(self, user_profile: UserProfile) -> None: diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index 183c7b657e..71eb95d55a 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -4656,6 +4656,34 @@ class SubscriptionAPITest(ZulipTestCase): ([web_public_stream], [public_stream, private_stream]), ) + # Guest can be subscribed by other users. + normal_user = self.example_user("aaron") + with self.capture_send_event_calls(expected_num_events=6) as events: + self.common_subscribe_to_streams( + self.example_user("hamlet"), + ["Denmark"], + dict(principals=orjson.dumps([guest_user.id, normal_user.id]).decode()), + ) + + # Verify that stream creation event is sent to guest user only. + stream_create_events = [ + event + for event in events + if event["event"]["type"] == "stream" and event["event"]["op"] == "create" + ] + self.assert_length(stream_create_events, 1) + self.assertEqual(stream_create_events[0]["users"], [guest_user.id]) + + # Verify that subscription add event is sent to both the users. + subscription_add_events = [ + event + for event in events + if event["event"]["type"] == "subscription" and event["event"]["op"] == "add" + ] + self.assert_length(subscription_add_events, 2) + self.assertEqual(subscription_add_events[0]["users"], [guest_user.id]) + self.assertEqual(subscription_add_events[1]["users"], [normal_user.id]) + def test_users_getting_add_peer_event(self) -> None: """ Check users getting add_peer_event is correct @@ -4781,7 +4809,7 @@ class SubscriptionAPITest(ZulipTestCase): # Verify that peer_event events are never sent in Zephyr # realm. This does generate stream creation events from - # send_stream_creation_events_for_private_streams. + # send_stream_creation_events_for_previously_inaccessible_streams. with self.capture_send_event_calls(expected_num_events=num_streams + 1) as events: with self.assert_database_query_count(num_streams + 12): self.common_subscribe_to_streams(