stream: Deduplicate lists of stream/subscriptions fields.

While the result of this change doesn't completely do what we need, it
does remove a huge amount of duplicated lists of fields.  With a bit
more similar work, we should be able to eliminate a broad category of
potential bugs involving Stream and Subscription objects being
represented inconsistently in the API.

Work towards #13787.

zerver/lib/actions.py

@@ -2772,33 +2772,26 @@ def notify_subscriptions_added(user_profile: UserProfile,
                    'names': [stream.name for sub, stream in sub_pairs],
                    'realm': user_profile.realm.string_id})
 
+    sub_dicts = []
+    for (subscription, stream) in sub_pairs:
+        sub_dict = stream.to_dict()
+        for field_name in Subscription.API_FIELDS:
+            if field_name == "active":
+                # Skip the "active" field, it's implied by context
+                continue
+            sub_dict[field_name] = getattr(subscription, field_name)
+
+        sub_dict['in_home_view'] = not subscription.is_muted
+        sub_dict['email_address'] = encode_email_address(stream, show_sender=True)
+        sub_dict['is_old_stream'] = is_old_stream(stream.date_created)
+        sub_dict['stream_weekly_traffic'] = get_average_weekly_stream_traffic(
+            stream.id, stream.date_created, recent_traffic)
+        sub_dict['subscribers'] = stream_user_ids(stream)
+        sub_dicts.append(sub_dict)
+
     # Send a notification to the user who subscribed.
-    payload = [dict(name=stream.name,
-                    stream_id=stream.id,
-                    in_home_view=not subscription.is_muted,
-                    is_muted=subscription.is_muted,
-                    invite_only=stream.invite_only,
-                    is_web_public=stream.is_web_public,
-                    is_announcement_only=stream.is_announcement_only,
-                    color=subscription.color,
-                    email_address=encode_email_address(stream, show_sender=True),
-                    desktop_notifications=subscription.desktop_notifications,
-                    audible_notifications=subscription.audible_notifications,
-                    push_notifications=subscription.push_notifications,
-                    email_notifications=subscription.email_notifications,
-                    wildcard_mentions_notify=subscription.wildcard_mentions_notify,
-                    description=stream.description,
-                    rendered_description=stream.rendered_description,
-                    pin_to_top=subscription.pin_to_top,
-                    is_old_stream=is_old_stream(stream.date_created),
-                    first_message_id=stream.first_message_id,
-                    stream_weekly_traffic=get_average_weekly_stream_traffic(
-                        stream.id, stream.date_created, recent_traffic),
-                    subscribers=stream_user_ids(stream),
-                    history_public_to_subscribers=stream.history_public_to_subscribers)
-               for (subscription, stream) in sub_pairs]
     event = dict(type="subscription", op="add",
-                 subscriptions=payload)
+                 subscriptions=sub_dicts)
     send_event(user_profile.realm, event, [user_profile.id])
 
 def get_peer_user_ids_for_stream_change(stream: Stream,
@@ -4738,10 +4731,7 @@ def get_web_public_subs(realm: Realm) -> SubHelperT:
 def gather_subscriptions_helper(user_profile: UserProfile,
                                 include_subscribers: bool=True) -> SubHelperT:
     sub_dicts = get_stream_subscriptions_for_user(user_profile).values(
-        "recipient_id", "is_muted", "color", "desktop_notifications",
+        *Subscription.API_FIELDS, "recipient_id").order_by("recipient_id")
-        "audible_notifications", "push_notifications", "email_notifications",
-        "wildcard_mentions_notify", "active", "pin_to_top"
-    ).order_by("recipient_id")
 
     sub_dicts = list(sub_dicts)
     sub_recipient_ids = [
@@ -4759,10 +4749,16 @@ def gather_subscriptions_helper(user_profile: UserProfile,
     recent_traffic = get_streams_traffic(stream_ids=stream_ids)
 
     all_streams = get_active_streams(user_profile.realm).select_related(
-        "realm").values("id", "name", "invite_only", "is_announcement_only", "realm_id",
+        "realm").values(
-                        "email_token", "description", "rendered_description", "date_created",
+            *Stream.API_FIELDS,
-                        "history_public_to_subscribers", "first_message_id", "is_web_public",
+            # date_created is used as an input for the is_old_stream computed field.
-                        "recipient_id")
+            "date_created",
+            # The realm_id and recipient_id are generally not needed in the API.
+            "realm_id",
+            "recipient_id",
+            # email_token isn't public to some users with access to
+            # the stream, so doesn't belong in API_FIELDS.
+            "email_token")
 
     stream_dicts = [stream for stream in all_streams if stream['id'] in stream_ids]
     stream_hash = {}
@@ -4803,46 +4799,46 @@ def gather_subscriptions_helper(user_profile: UserProfile,
             # This stream has been deactivated, don't include it.
             continue
 
-        subscribers = subscriber_map[stream["id"]]  # type: Optional[List[int]]
+        # We first construct a dictionary based on the standard Stream
+        # and Subscription models' API_FIELDS.
+        stream_dict = {}
+        for field_name in Stream.API_FIELDS:
+            if field_name == "id":
+                stream_dict['stream_id'] = stream["id"]
+                continue
+            stream_dict[field_name] = stream[field_name]
 
+        # Copy Subscription.API_FIELDS except for "active", which is
+        # used to determine where to the put the field.
+        for field_name in Subscription.API_FIELDS:
+            stream_dict[field_name] = sub[field_name]
+
+        # Backwards-compatibility for clients that haven't been
+        # updated for the in_home_view => is_muted API migration.
+        stream_dict['in_home_view'] = not stream_dict['is_muted']
+
+        # Add a few computed fields not directly from the data models.
+        stream_dict['is_old_stream'] = is_old_stream(stream["date_created"])
+        stream_dict['stream_weekly_traffic'] = get_average_weekly_stream_traffic(
+            stream["id"], stream["date_created"], recent_traffic)
+        stream_dict['email_address'] = encode_email_address_helper(
+            stream["name"], stream["email_token"], show_sender=True)
+
+        # Construct and add subscribers data
+        subscribers = subscriber_map[stream["id"]]  # type: Optional[List[int]]
         # 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_profile.is_realm_admin):
             subscribers = None
-
         # Guest users lose access to subscribers when they are unsubscribed.
         if not sub["active"] and user_profile.is_guest:
             subscribers = None
-
-        email_address = encode_email_address_helper(stream["name"], stream["email_token"],
-                                                    show_sender=True)
-        stream_dict = {'name': stream["name"],
-                       'in_home_view': not sub["is_muted"],
-                       'is_muted': sub["is_muted"],
-                       'invite_only': stream["invite_only"],
-                       'is_web_public': stream["is_web_public"],
-                       'is_announcement_only': stream["is_announcement_only"],
-                       'color': sub["color"],
-                       'desktop_notifications': sub["desktop_notifications"],
-                       'audible_notifications': sub["audible_notifications"],
-                       'push_notifications': sub["push_notifications"],
-                       'email_notifications': sub["email_notifications"],
-                       'wildcard_mentions_notify': sub["wildcard_mentions_notify"],
-                       'pin_to_top': sub["pin_to_top"],
-                       'stream_id': stream["id"],
-                       'first_message_id': stream["first_message_id"],
-                       'description': stream["description"],
-                       'rendered_description': stream["rendered_description"],
-                       'is_old_stream': is_old_stream(stream["date_created"]),
-                       'stream_weekly_traffic': get_average_weekly_stream_traffic(stream["id"],
-                                                                                  stream["date_created"],
-                                                                                  recent_traffic),
-                       'email_address': email_address,
-                       'history_public_to_subscribers': stream['history_public_to_subscribers']}
-
         if subscribers is not None:
             stream_dict['subscribers'] = subscribers
-        if sub["active"]:
+
+        # is_active is represented in this structure by which list we include it in.
+        is_active = stream_dict.pop("active")
+        if is_active:
             subscribed.append(stream_dict)
         else:
             unsubscribed.append(stream_dict)
@@ -4858,19 +4854,17 @@ def gather_subscriptions_helper(user_profile: UserProfile,
     for stream in never_subscribed_streams:
         is_public = (not stream['invite_only'])
         if is_public or user_profile.is_realm_admin:
-            stream_dict = {'name': stream['name'],
+            stream_dict = {}
-                           'invite_only': stream['invite_only'],
+            for field_name in Stream.API_FIELDS:
-                           'is_web_public': stream['is_web_public'],
+                if field_name == "id":
-                           'is_announcement_only': stream['is_announcement_only'],
+                    stream_dict['stream_id'] = stream["id"]
-                           'stream_id': stream['id'],
+                    continue
-                           'first_message_id': stream["first_message_id"],
+                stream_dict[field_name] = stream[field_name]
-                           'is_old_stream': is_old_stream(stream["date_created"]),
+
-                           'stream_weekly_traffic': get_average_weekly_stream_traffic(stream["id"],
+            stream_dict['is_old_stream'] = is_old_stream(stream["date_created"])
-                                                                                      stream["date_created"],
+            stream_dict['stream_weekly_traffic'] = get_average_weekly_stream_traffic(
-                                                                                      recent_traffic),
+                stream["id"], stream["date_created"], recent_traffic)
-                           'description': stream['description'],
+
-                           'rendered_description': stream["rendered_description"],
-                           'history_public_to_subscribers': stream['history_public_to_subscribers']}
             if is_public or user_profile.is_realm_admin:
                 subscribers = subscriber_map[stream["id"]]
                 if subscribers is not None:

zerver/models.py

@@ -1416,19 +1416,38 @@ class Stream(models.Model):
     class Meta:
         unique_together = ("name", "realm")
 
+    # Stream fields included whenever a Stream object is provided to
+    # Zulip clients via the API.  A few details worth noting:
+    # * "id" is represented as "stream_id" in most API interfaces.
+    # * "email_token" is not realm-public and thus is not included here.
+    # * is_in_zephyr_realm is a backend-only optimization.
+    # * "deactivated" streams are filtered from the API entirely.
+    # * "realm" and "recipient" and not exposed to clients via the API.
+    # * "date_created" should probably be added here, as it's useful information
+    #   to subscribers and is needed to compute is_old_stream.
+    # * message_retention_days should be added here once the feature is
+    #   complete.
+    API_FIELDS = [
+        "name",
+        "id",
+        "description",
+        "rendered_description",
+        "invite_only",
+        "is_web_public",
+        "is_announcement_only",
+        "history_public_to_subscribers",
+        "first_message_id",
+    ]
+
     # This is stream information that is sent to clients
     def to_dict(self) -> Dict[str, Any]:
-        return dict(
+        result = {}
-            name=self.name,
+        for field_name in self.API_FIELDS:
-            stream_id=self.id,
+            if field_name == "id":
-            description=self.description,
+                result['stream_id'] = self.id
-            rendered_description=self.rendered_description,
+                continue
-            invite_only=self.invite_only,
+            result[field_name] = getattr(self, field_name)
-            is_web_public=self.is_web_public,
+        return result
-            is_announcement_only=self.is_announcement_only,
-            history_public_to_subscribers=self.history_public_to_subscribers,
-            first_message_id=self.first_message_id,
-        )
 
 post_save.connect(flush_stream, sender=Stream)
 post_delete.connect(flush_stream, sender=Stream)
@@ -2081,6 +2100,32 @@ class Subscription(models.Model):
     def __str__(self) -> str:
         return "<Subscription: %s -> %s>" % (self.user_profile, self.recipient)
 
+    # Subscription fields included whenever a Subscription object is provided to
+    # Zulip clients via the API.  A few details worth noting:
+    # * These fields will generally be merged with Stream.API_FIELDS
+    #   data about the stream.
+    # * "user_profile" is usually implied as full API access to Subscription
+    #   is primarily done for the current user; API access to other users'
+    #   subscriptions is generally limited to boolean yes/no.
+    # * "id" and "recipient_id" are not included as they are not used
+    #   in the Zulip API; it's an internal implementation detail.
+    #   Subscription objects are always looked up in the API via
+    #   (user_profile, stream) pairs.
+    # * "active" is often excluded in API use cases where it is implied.
+    # * "is_muted" often needs to be copied to not "in_home_view" for
+    #   backwards-compatibility.
+    API_FIELDS = [
+        "active",
+        "color",
+        "is_muted",
+        "pin_to_top",
+        "audible_notifications",
+        "desktop_notifications",
+        "email_notifications",
+        "push_notifications",
+        "wildcard_mentions_notify",
+    ]
+
 @cache_with_key(user_profile_by_id_cache_key, timeout=3600*24*7)
 def get_user_profile_by_id(uid: int) -> UserProfile:
     return UserProfile.objects.select_related().get(id=uid)