From e556481ba093a6afd2f9887cf32f44f1b070a1e3 Mon Sep 17 00:00:00 2001 From: Tim Abbott Date: Mon, 27 Sep 2021 16:10:40 -0700 Subject: [PATCH] streams: Remove duplicates of get_web_public_streams_queryset. This is a somewhat subtle function, that deserves a few comments explaining subtle details of its logic, and there's no good reason to have multiple copies of that logic that are slightly inconsistent. Because the main changes here are just checking for invariant failures, the behavioral change here should be limited to ensuring deactivated streams are not considered available even if they were tagged as web public streams before deactivation. --- zerver/lib/actions.py | 13 ++++++++++--- zerver/lib/streams.py | 13 +++++++++---- zerver/models.py | 4 +++- 3 files changed, 22 insertions(+), 8 deletions(-) diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index 1436f558aa..d7eb7387d6 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -150,6 +150,7 @@ from zerver.lib.streams import ( check_stream_name, create_stream_if_needed, get_default_value_for_history_public_to_subscribers, + get_web_public_streams_queryset, render_stream_description, send_stream_creation_event, subscribed_to_stream, @@ -6596,7 +6597,7 @@ def get_web_public_subs(realm: Realm) -> SubscriptionInfo: return color subscribed = [] - for stream in Stream.objects.filter(realm=realm, is_web_public=True, deactivated=False): + for stream in get_web_public_streams_queryset(realm): stream_dict = stream.to_dict() # Add versions of the Subscription fields based on a simulated @@ -7483,7 +7484,7 @@ def get_occupied_streams(realm: Realm) -> QuerySet: def get_web_public_streams(realm: Realm) -> List[Dict[str, Any]]: # nocoverage - query = Stream.objects.filter(realm=realm, deactivated=False, is_web_public=True) + query = get_web_public_streams_queryset(realm) streams = Stream.get_client_data(query) return streams @@ -7529,7 +7530,13 @@ def do_get_streams( invite_only_check = Q(invite_only=False) add_filter_option(invite_only_check) if include_web_public: - web_public_check = Q(is_web_public=True) + # This should match get_web_public_streams_queryset + web_public_check = Q( + is_web_public=True, + invite_only=False, + history_public_to_subscribers=True, + deactivated=False, + ) add_filter_option(web_public_check) if include_owner_subscribed and user_profile.is_bot: bot_owner = user_profile.bot_owner diff --git a/zerver/lib/streams.py b/zerver/lib/streams.py index f7b74de026..fd1265eec8 100644 --- a/zerver/lib/streams.py +++ b/zerver/lib/streams.py @@ -399,15 +399,20 @@ def get_public_streams_queryset(realm: Realm) -> "QuerySet[Stream]": def get_web_public_streams_queryset(realm: Realm) -> "QuerySet[Stream]": - # In theory, is_web_public=True implies invite_only=False and - # history_public_to_subscribers=True, but it's safer to include - # this in the query. + # This should match the include_web_public code path in do_get_streams. return Stream.objects.filter( realm=realm, + is_web_public=True, + # In theory, nothing conflicts with allowing web-public access + # to deactivated streams. However, we should offer a way to + # review archived streams and adjust their settings before + # allowing that configuration to exist. deactivated=False, + # In theory, is_web_public=True implies invite_only=False and + # history_public_to_subscribers=True, but it's safer to include + # these in the query. invite_only=False, history_public_to_subscribers=True, - is_web_public=True, ) diff --git a/zerver/models.py b/zerver/models.py index 260e781b82..cce4c6f343 100644 --- a/zerver/models.py +++ b/zerver/models.py @@ -874,7 +874,9 @@ class Realm(models.Model): if not self.web_public_streams_enabled(): return False - return Stream.objects.filter(realm=self, is_web_public=True).exists() + from zerver.lib.streams import get_web_public_streams_queryset + + return get_web_public_streams_queryset(self).exists() post_save.connect(flush_realm, sender=Realm)