streams: Handle guest user ids for stream settings changes' events.

zerver/lib/actions.py

@@ -73,7 +73,7 @@ from zerver.models import Realm, RealmEmoji, Stream, UserProfile, UserActivity,
     get_user_profile_by_id, PreregistrationUser, get_display_recipient, \
     get_realm, bulk_get_recipients, get_stream_recipient, get_stream_recipients, \
     email_allowed_for_realm, email_to_username, display_recipient_cache_key, \
-    get_user, get_stream_cache_key, \
+    get_user, get_stream_cache_key, active_non_guest_user_ids, \
     UserActivityInterval, active_user_ids, get_active_streams, \
     realm_filters_for_realm, RealmFilter, stream_name_in_use, \
     get_old_unclaimed_attachments, is_cross_realm_bot_email, \
@@ -179,7 +179,8 @@ def can_access_stream_user_ids(stream: Stream) -> Set[int]:
     # a stream, such as its name/description.
     if stream.is_public():
         # For a public stream, this is everyone in the realm
-        return set(active_user_ids(stream.realm_id))
+        # except unsubscribed guest users
+        return public_stream_user_ids(stream)
     else:
         # for a private stream, it's subscribers plus realm admins.
         return private_stream_user_ids(stream.id) | {user.id for user in stream.realm.get_admin_users()}
@@ -189,6 +190,12 @@ def private_stream_user_ids(stream_id: int) -> Set[int]:
     subscriptions = get_active_subscriptions_for_stream_id(stream_id)
     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(user_profile__is_guest=True)
+    guest_subscriptions = {sub['user_profile_id'] for sub in guest_subscriptions.values('user_profile_id')}
+    return set(active_non_guest_user_ids(stream.realm_id)) | guest_subscriptions
+
 def bot_owner_user_ids(user_profile: UserProfile) -> Set[int]:
     is_private_bot = (
         user_profile.default_sending_stream and
@@ -1644,7 +1651,7 @@ def create_stream_if_needed(realm: Realm,
     if created:
         Recipient.objects.create(type_id=stream.id, type=Recipient.STREAM)
         if stream.is_public():
-            send_stream_creation_event(stream, active_user_ids(stream.realm_id))
+            send_stream_creation_event(stream, active_non_guest_user_ids(stream.realm_id))
         else:
             realm_admin_ids = [user.id for user in stream.realm.get_admin_users()]
             send_stream_creation_event(stream, realm_admin_ids)
@@ -2403,7 +2410,7 @@ def get_peer_user_ids_for_stream_change(stream: Stream,
         # We now do "peer_add" or "peer_remove" events even for streams
         # users were never subscribed to, in order for the neversubscribed
         # structure to stay up-to-date.
-        return set(active_user_ids(stream.realm_id)) - set(altered_user_ids)
+        return set(active_non_guest_user_ids(stream.realm_id)) - set(altered_user_ids)
 
 def get_user_ids_for_streams(streams: Iterable[Stream]) -> Dict[int, List[int]]:
     stream_ids = [stream.id for stream in streams]

zerver/lib/cache.py

@@ -331,6 +331,9 @@ def realm_user_dicts_cache_key(realm_id: int) -> str:
 def active_user_ids_cache_key(realm_id: int) -> str:
     return "active_user_ids:%s" % (realm_id,)
 
+def active_non_guest_user_ids_cache_key(realm_id: int) -> str:
+    return "active_non_guest_user_ids:%s" % (realm_id,)
+
 bot_dict_fields = ['id', 'full_name', 'short_name', 'bot_type', 'email',
                    'is_active', 'default_sending_stream__name',
                    'realm_id',
@@ -388,6 +391,10 @@ def flush_user_profile(sender: Any, **kwargs: Any) -> None:
 
     if changed(['is_active']):
         cache_delete(active_user_ids_cache_key(user_profile.realm_id))
+        cache_delete(active_non_guest_user_ids_cache_key(user_profile.realm_id))
+
+    if changed(['is_guest']):
+        cache_delete(active_non_guest_user_ids_cache_key(user_profile.realm_id))
 
     if changed(['email', 'full_name', 'short_name', 'id', 'is_mirror_dummy']):
         delete_display_recipient_cache(user_profile)
@@ -420,6 +427,7 @@ def flush_realm(sender: Any, **kwargs: Any) -> None:
         cache_delete(active_user_ids_cache_key(realm.id))
         cache_delete(bot_dicts_in_realm_cache_key(realm))
         cache_delete(realm_alert_words_cache_key(realm))
+        cache_delete(active_non_guest_user_ids_cache_key(realm.id))
 
 def realm_alert_words_cache_key(realm: 'Realm') -> str:
     return "realm_alert_words:%s" % (realm.string_id,)

zerver/models.py

@@ -16,7 +16,7 @@ from django.core.validators import URLValidator, MinLengthValidator, \
     RegexValidator
 from django.dispatch import receiver
 from zerver.lib.cache import cache_with_key, flush_user_profile, flush_realm, \
-    user_profile_by_api_key_cache_key, \
+    user_profile_by_api_key_cache_key, active_non_guest_user_ids_cache_key, \
     user_profile_by_id_cache_key, user_profile_by_email_cache_key, \
     user_profile_cache_key, generic_bulk_cached_fetch, cache_set, flush_stream, \
     display_recipient_cache_key, cache_delete, active_user_ids_cache_key, \
@@ -1561,6 +1561,15 @@ def active_user_ids(realm_id: int) -> List[int]:
     ).values_list('id', flat=True)
     return list(query)
 
+@cache_with_key(active_non_guest_user_ids_cache_key, timeout=3600*24*7)
+def active_non_guest_user_ids(realm_id: int) -> List[int]:
+    query = UserProfile.objects.filter(
+        realm_id=realm_id,
+        is_active=True,
+        is_guest=False,
+    ).values_list('id', flat=True)
+    return list(query)
+
 def get_membership_realms(email: str) -> List[Realm]:
     if settings.PRODUCTION:  # nocoverage
         return []

zerver/tests/test_subs.py

@@ -42,8 +42,8 @@ from zerver.lib.test_runner import (
 
 from zerver.models import (
     get_display_recipient, Message, Realm, Recipient, Stream, Subscription,
-    DefaultStream, UserProfile, get_user_profile_by_id, active_user_ids,
+    DefaultStream, UserProfile, get_user_profile_by_id, active_non_guest_user_ids,
-    get_default_stream_groups, flush_per_request_caches, DefaultStreamGroup
+    get_default_stream_groups, flush_per_request_caches, DefaultStreamGroup,
 )
 
 from zerver.lib.actions import (
@@ -97,7 +97,7 @@ class TestCreateStreams(ZulipTestCase):
         self.assertEqual(events[0]['event']['type'], 'stream')
         self.assertEqual(events[0]['event']['op'], 'create')
         # Send public stream creation event to all active users.
-        self.assertEqual(events[0]['users'], active_user_ids(realm.id))
+        self.assertEqual(events[0]['users'], active_non_guest_user_ids(realm.id))
         self.assertEqual(events[0]['event']['streams'][0]['name'], "Public stream")
 
         events = []
@@ -488,11 +488,13 @@ class StreamAdminTest(ZulipTestCase):
         stream_name2_exists = get_stream('stream_name2', realm)
         self.assertTrue(stream_name2_exists)
 
-        self.assertEqual(notified_user_ids, set(active_user_ids(realm.id)))
+        self.assertEqual(notified_user_ids, set(active_non_guest_user_ids(realm.id)))
         self.assertIn(user_profile.id,
                       notified_user_ids)
         self.assertIn(self.example_user('prospero').id,
                       notified_user_ids)
+        self.assertNotIn(self.example_user('polonius').id,
+                         notified_user_ids)
 
         # Test case to handle unicode stream name change
         # *NOTE: Here Encoding is needed when Unicode string is passed as an argument*
@@ -616,11 +618,13 @@ class StreamAdminTest(ZulipTestCase):
         notified_user_ids = set(events[0]['users'])
 
         stream = get_stream('stream_name1', realm)
-        self.assertEqual(notified_user_ids, set(active_user_ids(realm.id)))
+        self.assertEqual(notified_user_ids, set(active_non_guest_user_ids(realm.id)))
         self.assertIn(user_profile.id,
                       notified_user_ids)
         self.assertIn(self.example_user('prospero').id,
                       notified_user_ids)
+        self.assertNotIn(self.example_user('polonius').id,
+                         notified_user_ids)
 
         self.assertEqual('Test description', stream.description)
 
@@ -2014,7 +2018,8 @@ class SubscriptionAPITest(ZulipTestCase):
             set([user1.id, user2.id, self.test_user.id])
         )
 
-        self.assertEqual(len(add_peer_event['users']), 19)
+        self.assertNotIn(self.example_user('polonius').id, add_peer_event['users'])
+        self.assertEqual(len(add_peer_event['users']), 18)
         self.assertEqual(add_peer_event['event']['type'], 'subscription')
         self.assertEqual(add_peer_event['event']['op'], 'peer_add')
         self.assertEqual(add_peer_event['event']['user_id'], self.user_profile.id)
@@ -2045,7 +2050,8 @@ class SubscriptionAPITest(ZulipTestCase):
 
         # We don't send a peer_add event to othello
         self.assertNotIn(user_profile.id, add_peer_event['users'])
-        self.assertEqual(len(add_peer_event['users']), 19)
+        self.assertNotIn(self.example_user('polonius').id, add_peer_event['users'])
+        self.assertEqual(len(add_peer_event['users']), 18)
         self.assertEqual(add_peer_event['event']['type'], 'subscription')
         self.assertEqual(add_peer_event['event']['op'], 'peer_add')
         self.assertEqual(add_peer_event['event']['user_id'], user_profile.id)