diff --git a/api_docs/changelog.md b/api_docs/changelog.md index a06e024662..e992c873a0 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -20,6 +20,17 @@ format used by the Zulip server that they are interacting with. ## Changes in Zulip 8.0 +**Feature level 220** + +* [`GET /events`](/api/get-events): Stream creation events for web-public + streams are now sent to all guest users in the organization as well. + +* [`GET /events`](/api/get-events): The `subscription` events for `op: + "peer_add"` and `op: "peer_remove"` are now sent to subscribed guest + users for public streams and to all the guest users for web-public + streams; previously, they incorrectly only received these for + private streams. + **Feature level 219** * [`PATCH /realm/user_settings_defaults`](/api/update-realm-user-settings-defaults) diff --git a/version.py b/version.py index 463b66b7b1..cf4ddf8ab8 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 = 219 +API_FEATURE_LEVEL = 220 # 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/create_user.py b/zerver/actions/create_user.py index 8b780950e0..3a09aa6a1b 100644 --- a/zerver/actions/create_user.py +++ b/zerver/actions/create_user.py @@ -674,5 +674,5 @@ def do_reactivate_user(user_profile: UserProfile, *, acting_user: Optional[UserP realm=user_profile.realm, altered_user_dict=altered_user_dict, stream_dict=stream_dict, - private_peer_dict=subscriber_peer_info.private_peer_dict, + subscriber_peer_info=subscriber_peer_info, ) diff --git a/zerver/actions/streams.py b/zerver/actions/streams.py index d3d280ca5c..e64d71ee2c 100644 --- a/zerver/actions/streams.py +++ b/zerver/actions/streams.py @@ -31,7 +31,7 @@ from zerver.lib.queue import queue_event_on_commit, queue_json_publish from zerver.lib.stream_color import pick_colors from zerver.lib.stream_subscription import ( SubInfo, - bulk_get_private_peers, + SubscriberPeerInfo, bulk_get_subscriber_peer_info, get_active_subscriptions_for_stream_id, get_bulk_stream_subscriber_info, @@ -62,6 +62,7 @@ from zerver.models import ( UserGroup, UserProfile, active_non_guest_user_ids, + active_user_ids, get_system_bot, ) from zerver.tornado.django_api import send_event, send_event_on_commit @@ -466,7 +467,7 @@ def send_stream_creation_events_for_previously_inaccessible_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: + elif not stream.is_web_public: # Guese users need a `create` notification for # public streams as well because they need the stream # to exist before they get the "subscribe" notification. @@ -481,7 +482,7 @@ def send_peer_subscriber_events( realm: Realm, stream_dict: Dict[int, Stream], altered_user_dict: Dict[int, Set[int]], - private_peer_dict: Dict[int, Set[int]], + subscriber_peer_info: SubscriberPeerInfo, ) -> None: # Send peer_add/peer_remove events to other users who are tracking the # subscribers lists of streams in their browser; everyone for @@ -492,6 +493,7 @@ def send_peer_subscriber_events( private_stream_ids = [ stream_id for stream_id in altered_user_dict if stream_dict[stream_id].invite_only ] + private_peer_dict = subscriber_peer_info.private_peer_dict for stream_id in private_stream_ids: altered_user_ids = altered_user_dict[stream_id] @@ -512,14 +514,27 @@ def send_peer_subscriber_events( if not stream_dict[stream_id].invite_only and not stream_dict[stream_id].is_in_zephyr_realm ] + web_public_stream_ids = [ + stream_id for stream_id in public_stream_ids if stream_dict[stream_id].is_web_public + ] + + subscriber_dict = subscriber_peer_info.subscribed_ids + if public_stream_ids: user_streams: Dict[int, Set[int]] = defaultdict(set) - public_peer_ids = set(active_non_guest_user_ids(realm.id)) + non_guest_user_ids = set(active_non_guest_user_ids(realm.id)) + + if web_public_stream_ids: + web_public_peer_ids = set(active_user_ids(realm.id)) for stream_id in public_stream_ids: altered_user_ids = altered_user_dict[stream_id] - peer_user_ids = public_peer_ids - altered_user_ids + + if stream_id in web_public_stream_ids: + peer_user_ids = web_public_peer_ids - altered_user_ids + else: + peer_user_ids = (non_guest_user_ids | subscriber_dict[stream_id]) - altered_user_ids if peer_user_ids and altered_user_ids: if len(altered_user_ids) == 1: @@ -545,7 +560,11 @@ def send_peer_subscriber_events( send_event_on_commit(realm, event, peer_user_ids) for user_id, stream_ids in user_streams.items(): - peer_user_ids = public_peer_ids - {user_id} + if stream_id in web_public_stream_ids: + peer_user_ids = web_public_peer_ids - altered_user_ids + else: + peer_user_ids = (non_guest_user_ids | subscriber_dict[stream_id]) - altered_user_ids + event = dict( type="subscription", op=op, @@ -689,7 +708,7 @@ def bulk_add_subscriptions( realm=realm, altered_user_dict=altered_user_dict, stream_dict=stream_dict, - private_peer_dict=subscriber_peer_info.private_peer_dict, + subscriber_peer_info=subscriber_peer_info, ) return ( @@ -703,11 +722,9 @@ def send_peer_remove_events( streams: List[Stream], altered_user_dict: Dict[int, Set[int]], ) -> None: - private_streams = [stream for stream in streams if stream.invite_only] - - private_peer_dict = bulk_get_private_peers( + subscriber_peer_info = bulk_get_subscriber_peer_info( realm=realm, - private_streams=private_streams, + streams=streams, ) stream_dict = {stream.id: stream for stream in streams} @@ -716,7 +733,7 @@ def send_peer_remove_events( realm=realm, stream_dict=stream_dict, altered_user_dict=altered_user_dict, - private_peer_dict=private_peer_dict, + subscriber_peer_info=subscriber_peer_info, ) diff --git a/zerver/lib/stream_subscription.py b/zerver/lib/stream_subscription.py index 5a64eaf695..1abba00036 100644 --- a/zerver/lib/stream_subscription.py +++ b/zerver/lib/stream_subscription.py @@ -198,8 +198,7 @@ def bulk_get_subscriber_peer_info( realm_admin_ids = {user.id for user in realm.get_admin_users_and_bots()} for stream_id in private_stream_ids: - # This is the same business rule as we use in - # bulk_get_private_peers. Realm admins can see all private stream + # Realm admins can see all private stream # subscribers. subscribed_user_ids = stream_user_ids.get(stream_id, set()) subscribed_ids[stream_id] = subscribed_user_ids @@ -215,34 +214,6 @@ def bulk_get_subscriber_peer_info( ) -def bulk_get_private_peers( - realm: Realm, - private_streams: List[Stream], -) -> Dict[int, Set[int]]: - if not private_streams: - return {} - - for stream in private_streams: - # Our caller should only pass us private streams. - assert stream.invite_only - - peer_ids: Dict[int, Set[int]] = {} - - realm_admin_ids = {user.id for user in realm.get_admin_users_and_bots()} - - stream_ids = {stream.id for stream in private_streams} - stream_user_ids = get_user_ids_for_streams(stream_ids) - - for stream in private_streams: - # This is the same business rule as we use in - # bulk_get_subscriber_peer_info. Realm admins can see all private - # stream subscribers. - subscribed_user_ids = stream_user_ids.get(stream.id, set()) - peer_ids[stream.id] = subscribed_user_ids | realm_admin_ids - - return peer_ids - - def handle_stream_notifications_compatibility( user_profile: Optional[UserProfile], stream_dict: Dict[str, Any], diff --git a/zerver/lib/streams.py b/zerver/lib/streams.py index 136bcee3a8..05787cc3fc 100644 --- a/zerver/lib/streams.py +++ b/zerver/lib/streams.py @@ -31,6 +31,7 @@ from zerver.models import ( UserGroup, UserProfile, active_non_guest_user_ids, + active_user_ids, bulk_get_streams, get_realm_stream, get_stream, @@ -179,7 +180,11 @@ def create_stream_if_needed( ) if created: if stream.is_public(): - send_stream_creation_event(realm, stream, active_non_guest_user_ids(stream.realm_id)) + if stream.is_web_public: + notify_user_ids = active_user_ids(stream.realm_id) + else: + notify_user_ids = active_non_guest_user_ids(stream.realm_id) + send_stream_creation_event(realm, stream, notify_user_ids) else: realm_admin_ids = [user.id for user in stream.realm.get_admin_users_and_bots()] send_stream_creation_event(realm, stream, realm_admin_ids) diff --git a/zerver/lib/test_classes.py b/zerver/lib/test_classes.py index c66191c4bf..a7ad620872 100644 --- a/zerver/lib/test_classes.py +++ b/zerver/lib/test_classes.py @@ -1389,14 +1389,18 @@ Output: # Subscribe to a stream directly def subscribe( - self, user_profile: UserProfile, stream_name: str, invite_only: bool = False + self, + user_profile: UserProfile, + stream_name: str, + invite_only: bool = False, + is_web_public: bool = False, ) -> Stream: realm = user_profile.realm try: stream = get_stream(stream_name, user_profile.realm) except Stream.DoesNotExist: stream, from_stream_creation = create_stream_if_needed( - realm, stream_name, invite_only=invite_only + realm, stream_name, invite_only=invite_only, is_web_public=is_web_public ) bulk_add_subscriptions(realm, [stream], [user_profile], acting_user=None) return stream diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index bba6d0d717..c9b54c671a 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -782,8 +782,12 @@ paths: to see a given stream's subscriber list, they will receive this event for the existing subscriptions to the stream. - **Changes**: Prior to Zulip 8.0 (feature level 205), this event was not - sent when a user gained access to a stream due to their [role + **Changes**: Prior to Zulip 8.0 (feature level 220), this event was + incorrectly not sent to guest users when subscribers to web-public + streams and subscribed public streams changed. + + Prior to Zulip 8.0 (feature level 205), this event was not sent when + a user gained access to a stream due to their [role changing](/help/roles-and-permissions). Prior to Zulip 6.0 (feature level 134), this event was not sent when a @@ -841,10 +845,13 @@ paths: from streams. Sent to all users if the stream is public or to only the existing subscribers if the stream is private. - **Changes**: In Zulip 4.0 (feature level 35), the singular - `user_id` and `stream_id` integers included in this event - were replaced with plural `user_ids` and `stream_ids` integer - arrays. + **Changes**: Prior to Zulip 8.0 (feature level 220), this event was + incorrectly not sent to guest users when subscribers to web-public + streams and subscribed public streams changed. + + In Zulip 4.0 (feature level 35), the singular `user_id` and + `stream_id` integers included in this event were replaced + with plural `user_ids` and `stream_ids` integer arrays. In Zulip 3.0 (feature level 19), the `stream_id` field was added to identify the stream the user unsubscribed from, @@ -1197,9 +1204,11 @@ paths: 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 205), this event was - not sent when a user gained access to a stream due to their role - changing. + **Changes**: Prior to Zulip 8.0 (feature level 220), this event was + incorrectly not sent to guest users a web-public stream was created. + + Prior to Zulip 8.0 (feature level 205), this event was not sent + when a user gained access to a stream due to their role changing. Prior to Zulip 8.0 (feature level 192), this event was not sent when guest users gained access to a public stream by being diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index 5c02c810bf..389d7c584f 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -3557,6 +3557,14 @@ class UserDisplayActionTest(BaseAction): action = lambda: self.subscribe(self.example_user("hamlet"), "Test stream 2") events = self.verify_action(action, num_events=0, state_change_expected=False) + # Check that guest user receives stream creation event for web-public stream. + action = lambda: self.subscribe( + self.example_user("hamlet"), "Web public test stream", is_web_public=True + ) + events = self.verify_action(action, num_events=2, state_change_expected=True) + check_stream_create("events[0]", events[0]) + check_subscription_peer_add("events[1]", events[1]) + self.user_profile = self.example_user("hamlet") action = lambda: self.subscribe( self.example_user("hamlet"), "Private test stream", invite_only=True @@ -3779,8 +3787,32 @@ class SubscribeActionTest(BaseAction): stream.invite_only = False stream.save() - # Subscribe as a guest to a public stream. + # Test events for guest user. self.user_profile = self.example_user("polonius") + + # Guest user does not receive peer_add/peer_remove events for unsubscribed + # public streams. + action = lambda: bulk_add_subscriptions( + user_profile.realm, [stream], [self.example_user("othello")], acting_user=None + ) + events = self.verify_action( + action, + include_subscribers=include_subscribers, + num_events=0, + state_change_expected=False, + ) + + action = lambda: bulk_remove_subscriptions( + user_profile.realm, [self.example_user("othello")], [stream], acting_user=None + ) + events = self.verify_action( + action, + include_subscribers=include_subscribers, + num_events=0, + state_change_expected=False, + ) + + # Subscribe as a guest to a public stream. action = lambda: bulk_add_subscriptions( user_profile.realm, [stream], [self.user_profile], acting_user=None ) @@ -3788,6 +3820,55 @@ class SubscribeActionTest(BaseAction): check_stream_create("events[0]", events[0]) check_subscription_add("events[1]", events[1]) + action = lambda: bulk_add_subscriptions( + user_profile.realm, [stream], [self.example_user("othello")], acting_user=None + ) + events = self.verify_action( + action, + include_subscribers=include_subscribers, + state_change_expected=include_subscribers, + ) + check_subscription_peer_add("events[0]", events[0]) + + action = lambda: bulk_remove_subscriptions( + user_profile.realm, [self.example_user("othello")], [stream], acting_user=None + ) + events = self.verify_action( + action, + include_subscribers=include_subscribers, + state_change_expected=include_subscribers, + ) + check_subscription_peer_remove("events[0]", events[0]) + + stream = self.make_stream("web-public-stream", self.user_profile.realm, is_web_public=True) + # Guest user receives peer_add/peer_remove events for unsubscribed + # web-public streams. + action = lambda: bulk_add_subscriptions( + user_profile.realm, [stream], [self.example_user("othello")], acting_user=None + ) + events = self.verify_action( + action, + include_subscribers=include_subscribers, + state_change_expected=include_subscribers, + ) + + action = lambda: bulk_remove_subscriptions( + user_profile.realm, [self.example_user("othello")], [stream], acting_user=None + ) + events = self.verify_action( + action, + include_subscribers=include_subscribers, + state_change_expected=include_subscribers, + ) + + # Subscribe as a guest to web-public stream. Guest does not receive stream creation + # event for web-public stream. + 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=1) + check_subscription_add("events[0]", events[0]) + class DraftActionTest(BaseAction): def do_enable_drafts_synchronization(self, user_profile: UserProfile) -> None: diff --git a/zerver/tests/test_invite.py b/zerver/tests/test_invite.py index af90df6cdf..d758d40fe2 100644 --- a/zerver/tests/test_invite.py +++ b/zerver/tests/test_invite.py @@ -142,7 +142,7 @@ class StreamSetupTest(ZulipTestCase): new_user = self.create_simple_new_user(realm, new_user_email) - with self.assert_database_query_count(12): + with self.assert_database_query_count(13): set_up_streams_for_new_human_user( user_profile=new_user, prereg_user=prereg_user, diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index 9857129b32..cfeeedafd7 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -2598,7 +2598,7 @@ class StreamAdminTest(ZulipTestCase): those you aren't on. """ result = self.attempt_unsubscribe_of_principal( - query_count=16, + query_count=17, target_users=[self.example_user("cordelia")], is_realm_admin=True, is_subbed=True, @@ -2625,7 +2625,7 @@ class StreamAdminTest(ZulipTestCase): for name in ["cordelia", "prospero", "iago", "hamlet", "outgoing_webhook_bot"] ] result = self.attempt_unsubscribe_of_principal( - query_count=27, + query_count=28, cache_count=8, target_users=target_users, is_realm_admin=True, @@ -2686,7 +2686,7 @@ class StreamAdminTest(ZulipTestCase): def test_admin_remove_others_from_stream_legacy_emails(self) -> None: result = self.attempt_unsubscribe_of_principal( - query_count=16, + query_count=17, target_users=[self.example_user("cordelia")], is_realm_admin=True, is_subbed=True, @@ -2700,7 +2700,7 @@ class StreamAdminTest(ZulipTestCase): def test_admin_remove_multiple_users_from_stream_legacy_emails(self) -> None: result = self.attempt_unsubscribe_of_principal( - query_count=19, + query_count=20, target_users=[self.example_user("cordelia"), self.example_user("prospero")], is_realm_admin=True, is_subbed=True, @@ -2747,7 +2747,7 @@ class StreamAdminTest(ZulipTestCase): webhook_bot = self.example_user("webhook_bot") do_change_bot_owner(webhook_bot, bot_owner=user_profile, acting_user=user_profile) result = self.attempt_unsubscribe_of_principal( - query_count=13, + query_count=14, target_users=[webhook_bot], is_realm_admin=False, is_subbed=True,