subs: Allow filtering by is_user_active in get_active_subscriptions.

get_active_subscriptions_for_stream_id should allow specifying whether
subscriptions of deactivated users should be included in the result.
Active subs of deactivated users are  a subtlety that's easy to miss
when writing relevant code, so we make include_deactivated_users a
mandatory kwarg - this will force callers to definitely give thought to
whether such subs should be included or not.

This commit is just a refactoring, we keep original behavior everywhere
- there are places where subs of deactivates users should probably be
excluded but aren't - we don't fix that here, it'll be addressed in
follow-up commits.

zerver/lib/actions.py

@@ -311,14 +311,16 @@ def can_access_stream_user_ids(stream: Stream) -> Set[int]:
 
 def private_stream_user_ids(stream_id: int) -> Set[int]:
     # TODO: Find similar queries elsewhere and de-duplicate this code.
-    subscriptions = get_active_subscriptions_for_stream_id(stream_id)
+    subscriptions = get_active_subscriptions_for_stream_id(
+        stream_id, include_deactivated_users=True
+    )
     return {sub["user_profile_id"] for sub in subscriptions.values("user_profile_id")}
 
 
 def public_stream_user_ids(stream: Stream) -> Set[int]:
-    guest_subscriptions = get_active_subscriptions_for_stream_id(stream.id).filter(
+    guest_subscriptions = get_active_subscriptions_for_stream_id(
-        user_profile__role=UserProfile.ROLE_GUEST
+        stream.id, include_deactivated_users=True
-    )
+    ).filter(user_profile__role=UserProfile.ROLE_GUEST)
     guest_subscriptions = {
         sub["user_profile_id"] for sub in guest_subscriptions.values("user_profile_id")
     }
@@ -1241,7 +1243,9 @@ def do_deactivate_stream(
     # Get the affected user ids *before* we deactivate everybody.
     affected_user_ids = can_access_stream_user_ids(stream)
 
-    get_active_subscriptions_for_stream_id(stream.id).update(active=False)
+    get_active_subscriptions_for_stream_id(stream.id, include_deactivated_users=True).update(
+        active=False
+    )
 
     was_invite_only = stream.invite_only
     stream.deactivated = True
@@ -3231,13 +3235,7 @@ def get_subscribers_query(stream: Stream, requesting_user: Optional[UserProfile]
     """
     validate_user_access_to_subscribers(requesting_user, stream)
 
-    # Note that non-active users may still have "active" subscriptions, because we
+    return get_active_subscriptions_for_stream_id(stream.id, include_deactivated_users=False)
-    # want to be able to easily reactivate them with their old subscriptions.  This
-    # is why the query here has to look at the UserProfile.is_active flag.
-    subscriptions = get_active_subscriptions_for_stream_id(stream.id).filter(
-        user_profile__is_active=True,
-    )
-    return subscriptions
 
 
 def get_subscriber_emails(
@@ -5546,11 +5544,13 @@ def do_update_message(
         # though the messages were deleted, and we should send a
         # delete_message event to them instead.
 
-        subscribers = get_active_subscriptions_for_stream_id(stream_id).select_related(
+        subscribers = get_active_subscriptions_for_stream_id(
-            "user_profile"
+            stream_id, include_deactivated_users=True
-        )
+        ).select_related("user_profile")
         subs_to_new_stream = list(
-            get_active_subscriptions_for_stream_id(new_stream.id).select_related("user_profile")
+            get_active_subscriptions_for_stream_id(
+                new_stream.id, include_deactivated_users=True
+            ).select_related("user_profile")
         )
 
         old_stream_sub_ids = [user.user_profile_id for user in subscribers]
@@ -5685,7 +5685,9 @@ def do_update_message(
     users_to_be_notified = list(map(user_info, ums))
     if stream_being_edited is not None:
         if stream_being_edited.is_history_public_to_subscribers:
-            subscribers = get_active_subscriptions_for_stream_id(stream_id)
+            subscribers = get_active_subscriptions_for_stream_id(
+                stream_id, include_deactivated_users=True
+            )
             # We exclude long-term idle users, since they by
             # definition have no active clients.
             subscribers = subscribers.exclude(user_profile__long_term_idle=True)
@@ -5787,7 +5789,9 @@ def do_delete_messages(realm: Realm, messages: Iterable[Message]) -> None:
         stream_id = sample_message.recipient.type_id
         event["stream_id"] = stream_id
         event["topic"] = sample_message.topic_name()
-        subscribers = get_active_subscriptions_for_stream_id(stream_id)
+        subscribers = get_active_subscriptions_for_stream_id(
+            stream_id, include_deactivated_users=True
+        )
         # We exclude long-term idle users, since they by definition have no active clients.
         subscribers = subscribers.exclude(user_profile__long_term_idle=True)
         users_to_notify = list(subscribers.values_list("user_profile_id", flat=True))

zerver/lib/stream_subscription.py

@@ -22,13 +22,22 @@ class SubscriberPeerInfo:
     private_peer_dict: Dict[int, Set[int]]
 
 
-def get_active_subscriptions_for_stream_id(stream_id: int) -> QuerySet:
+def get_active_subscriptions_for_stream_id(
+    stream_id: int, *, include_deactivated_users: bool
+) -> QuerySet:
     # TODO: Change return type to QuerySet[Subscription]
-    return Subscription.objects.filter(
+    query = Subscription.objects.filter(
         recipient__type=Recipient.STREAM,
         recipient__type_id=stream_id,
         active=True,
     )
+    if not include_deactivated_users:
+        # Note that non-active users may still have "active" subscriptions, because we
+        # want to be able to easily reactivate them with their old subscriptions.  This
+        # is why the query here has to look at the is_user_active flag.
+        query = query.filter(is_user_active=True)
+
+    return query
 
 
 def get_active_subscriptions_for_stream_ids(stream_ids: Set[int]) -> QuerySet:
@@ -100,13 +109,9 @@ def get_bulk_stream_subscriber_info(
 
 
 def num_subscribers_for_stream_id(stream_id: int) -> int:
-    return (
+    return get_active_subscriptions_for_stream_id(
-        get_active_subscriptions_for_stream_id(stream_id)
+        stream_id, include_deactivated_users=False
-        .filter(
+    ).count()
-            user_profile__is_active=True,
-        )
-        .count()
-    )
 
 
 def get_user_ids_for_streams(stream_ids: Set[int]) -> Dict[int, Set[int]]:
@@ -265,16 +270,14 @@ def subscriber_ids_with_stream_history_access(stream: Stream) -> Set[int]:
     if not stream.is_history_public_to_subscribers():
         return set()
 
-    subscriptions = get_active_subscriptions_for_stream_id(stream.id)
+    subscriptions = get_active_subscriptions_for_stream_id(
+        stream.id, include_deactivated_users=False
+    )
     if stream.is_web_public:
+        return set(subscriptions.values_list("user_profile__id", flat=True))
+
     return set(
-            subscriptions.filter(user_profile__is_active=True).values_list(
+        subscriptions.exclude(user_profile__role=UserProfile.ROLE_GUEST).values_list(
             "user_profile__id", flat=True
         )
     )
-
-    return set(
-        subscriptions.filter(user_profile__is_active=True)
-        .exclude(user_profile__role=UserProfile.ROLE_GUEST)
-        .values_list("user_profile__id", flat=True)
-    )

zerver/lib/stream_topic.py

@@ -28,4 +28,6 @@ class StreamTopicTarget:
         return {row["user_profile_id"] for row in query}
 
     def get_active_subscriptions(self) -> QuerySet:
-        return get_active_subscriptions_for_stream_id(self.stream_id)
+        return get_active_subscriptions_for_stream_id(
+            self.stream_id, include_deactivated_users=True
+        )

zerver/tests/test_subs.py

@@ -569,7 +569,7 @@ class StreamAdminTest(ZulipTestCase):
         result = self.client_delete(f"/json/streams/{stream.id}")
         self.assert_json_success(result)
         subscription_exists = (
-            get_active_subscriptions_for_stream_id(stream.id)
+            get_active_subscriptions_for_stream_id(stream.id, include_deactivated_users=True)
             .filter(
                 user_profile=user_profile,
             )
@@ -593,7 +593,7 @@ class StreamAdminTest(ZulipTestCase):
         result = self.client_delete(f"/json/streams/{stream.id}")
         self.assert_json_success(result)
         subscription_exists = (
-            get_active_subscriptions_for_stream_id(stream.id)
+            get_active_subscriptions_for_stream_id(stream.id, include_deactivated_users=True)
             .filter(
                 user_profile=user_profile,
             )