refactor: Eliminate checks in build_stream_dict_for_sub.

We eliminate some redundant checks.

We also consistently provide a `subscribers` field
in our stream data with `[]`, even if our users
can't access subscribers.  We therefore bump
the API version and tweak the docs.  (See further
down for a detailed justification of the change.)

Even though it is sometimes fine to have redundant code
that is defensive in nature, some upcoming changes are gonna
move subscriber-related logic out of build_stream_dict_for_sub
for certain codepaths as part of our effort to streamline
the payload for subscribers within page_params.

So we can't rely on the code that I removed here
inside of build_stream_dict_for_sub.

Anyway, it makes more sense to do these checks explicitly
in the validate function.

The code in build_stream_dict_for_sub was almost effectively
a noop, since the validation function was already preventing
us from getting subscriber info.  The only difference it
made was sometimes converting `[]` to `None`, and then
subsequently omitting the subscribers field.

Neither ZT nor the webapp make any distinction between
`[]` or <missing key> for the `subscribers` data in
`page_params`.

The webapp has had this code for a long time (and now
equivalent code elsewhere in this PR):

    if (!Object.prototype.hasOwnProperty.call(sub, "subscribers")) {
        sub.subscribers = new LazySet([]);
    }

The webapp calculates access based on booleans, anyway:

    sub.can_access_subscribers =
        page_params.is_admin || sub.subscribed ||
        (!page_params.is_guest && !sub.invite_only);

And ZT would choke if `subscribers` were missing, except that
it never gets to the relevant code due to other checks:

    def get_other_subscribers_in_stream(<snip>):
        assert stream_id is not None or stream_name is not None

        if stream_id:
            assert self.is_user_subscribed_to_stream(stream_id)

            return [sub
                    for sub in self.stream_dict[stream_id]['subscribers']
                    if sub != self.user_id]
        else:
            return [sub
                    for _, stream in self.stream_dict.items()
                    for sub in stream['subscribers']
                    if stream['name'] == stream_name
                    if sub != self.user_id]

You could make a semantic argument that we should prefer
<missing key> to `[]` when subscribers aren't even available, but
we have precedent from the way that `bulk_get_subscriber_user_ids`
has traditionally populated its result:

    result: Dict[int, List[int]] =
        {stream["id"]: [] for stream in stream_dicts}

If we changed `stream_dicts` to `target_stream_dicts` we
would faciliate a move toward `None`, but it would just cause
headaches for other server code as well as the frontends
(which, to reiterate, already prefer the empty array
for convenience).

templates/zerver/api/changelog.md

@@ -10,6 +10,12 @@ below features are supported.
 
 ## Changes in Zulip 4.0
 
+**Feature level 37**
+
+* Consistently provide `subscribers` in stream data when
+  clients register for subscriptions with `include_subscribers`,
+  even if the user can't access subscribers.
+
 **Feature level 36**
 
 * [`POST /users`](/api/create-user): Restricted access to organization

version.py

@@ -28,7 +28,7 @@ DESKTOP_WARNING_VERSION = "5.2.0"
 #
 # Changes should be accompanied by documentation explaining what the
 # new level means in templates/zerver/api/changelog.md.
-API_FEATURE_LEVEL = 36
+API_FEATURE_LEVEL = 37
 
 # 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

zerver/lib/actions.py

@@ -2666,7 +2666,9 @@ def validate_user_access_to_subscribers_helper(
     if stream_dict["is_web_public"]:
         return
 
-    # Guest users can access subscribed public stream's subscribers
+    # With the exception of web public streams, a guest must
+    # be subscribed to a stream (even a public one) in order
+    # to see subscribers.
     if user_profile.is_guest:
         if check_user_subscribed(user_profile):
             return
@@ -5056,15 +5058,6 @@ def build_stream_dict_for_sub(
     result["email_address"] = encode_email_address_helper(
         stream["name"], stream["email_token"], show_sender=True)
 
-    # Important: don't show the subscribers if the stream is invite only
-    # and this user isn't on it anymore (or a realm administrator).
-    if stream["invite_only"] and not (sub["active"] or user.is_realm_admin):
-        subscribers = None
-
-    # Guest users lose access to subscribers when they are unsubscribed if the stream
-    # is not web-public.
-    if not sub["active"] and user.is_guest and not stream["is_web_public"]:
-        subscribers = None
     if subscribers is not None:
         result["subscribers"] = subscribers
 

zerver/openapi/zulip.yaml

@@ -6226,6 +6226,12 @@ paths:
                                   description: |
                                     A list of user IDs of users who are subscribed
                                     to the stream. Included only if `include_subscribers` is `true`.
+
+                                    If a user is not allowed to know the subscribers for
+                                    a stream, we will send an empty array.  API authors
+                                    should use other data to determine whether users like
+                                    guest users are forbidden to know the subscribers.
+
                         description: |
                           Present if `subscription` is present in `fetch_event_types`.
 

zerver/tests/test_subs.py

@@ -4425,12 +4425,12 @@ class GetSubscribersTest(ZulipTestCase):
         sub_data = gather_subscriptions_helper(non_admin_user)
         unsubscribed_streams = sub_data[1]
         self.assertEqual(len(unsubscribed_streams), 1)
-        self.assertFalse('subscribers' in unsubscribed_streams[0])
+        self.assertEqual(unsubscribed_streams[0]['subscribers'], [])
 
         sub_data = gather_subscriptions_helper(guest_user)
         unsubscribed_streams = sub_data[1]
         self.assertEqual(len(unsubscribed_streams), 1)
-        self.assertFalse('subscribers' in unsubscribed_streams[0])
+        self.assertEqual(unsubscribed_streams[0]['subscribers'], [])
 
     def test_gather_subscriptions_mit(self) -> None:
         """