From b69aec2dbca6c8ca0186262973dbeef0ac2dc4f2 Mon Sep 17 00:00:00 2001 From: Challa Venkata Raghava Reddy Date: Mon, 4 Mar 2019 22:20:49 +0530 Subject: [PATCH] streams: Add first_message_id tracking first message in stream. This field is primarily intended to support avoiding displaying the "more topics" feature in new organizations and streams, where we might know that all messages in the stream are already available in the browser. Based on original work by Roman Godov, and significantly modified by tabbott. The second migration involved here could be expensive on Zulip Cloud, but is unlikely to be an issue on other servers. --- zerver/lib/actions.py | 9 +++++- zerver/lib/events.py | 13 ++++++++ zerver/lib/import_realm.py | 11 +++++++ .../0209_stream_first_message_id.py | 20 ++++++++++++ .../0210_stream_first_message_id.py | 31 +++++++++++++++++++ zerver/models.py | 8 ++++- zerver/tests/test_events.py | 2 ++ zerver/tests/test_subs.py | 8 ++--- 8 files changed, 96 insertions(+), 6 deletions(-) create mode 100644 zerver/migrations/0209_stream_first_message_id.py create mode 100644 zerver/migrations/0210_stream_first_message_id.py diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index 75b8be4e34..ea7a94404e 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -1386,6 +1386,9 @@ def do_send_messages(messages_maybe_none: Sequence[Optional[MutableMapping[str, event['stream_name'] = message['stream'].name if message['stream'].invite_only: event['invite_only'] = True + if message['stream'].first_message_id is None: + message['stream'].first_message_id = message['message'].id + message['stream'].save(update_fields=["first_message_id"]) if message['local_id'] is not None: event['local_id'] = message['local_id'] if message['sender_queue_id'] is not None: @@ -2653,6 +2656,7 @@ def notify_subscriptions_added(user_profile: UserProfile, 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), @@ -4524,6 +4528,7 @@ def get_web_public_subs(realm: Realm) -> SubHelperT: 'description': stream.description, 'rendered_description': stream.rendered_description, '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, {}), @@ -4561,7 +4566,7 @@ def gather_subscriptions_helper(user_profile: UserProfile, all_streams = get_active_streams(user_profile.realm).select_related( "realm").values("id", "name", "invite_only", "is_announcement_only", "realm_id", "email_token", "description", "rendered_description", "date_created", - "history_public_to_subscribers") + "history_public_to_subscribers", "first_message_id") stream_dicts = [stream for stream in all_streams if stream['id'] in stream_ids] stream_hash = {} @@ -4624,6 +4629,7 @@ def gather_subscriptions_helper(user_profile: UserProfile, 'email_notifications': sub["email_notifications"], '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"]), @@ -4654,6 +4660,7 @@ def gather_subscriptions_helper(user_profile: UserProfile, 'invite_only': stream['invite_only'], 'is_announcement_only': stream['is_announcement_only'], 'stream_id': stream['id'], + 'first_message_id': stream["first_message_id"], 'is_old_stream': is_old_stream(stream["date_created"]), 'stream_weekly_traffic': get_average_weekly_stream_traffic(stream["id"], stream["date_created"], diff --git a/zerver/lib/events.py b/zerver/lib/events.py index 35c4a5133e..681782d0b2 100644 --- a/zerver/lib/events.py +++ b/zerver/lib/events.py @@ -371,6 +371,19 @@ def apply_event(state: Dict[str, Any], event['flags'], ) + # Below, we handle maintaining first_message_id. + if event['message']['type'] != "stream": + return + + for sub_dict in state['subscriptions']: + if event['message']['stream_id'] == sub_dict['stream_id']: + if sub_dict['first_message_id'] is None: + sub_dict['first_message_id'] = event['message']['id'] + for stream_dict in state['streams']: + if event['message']['stream_id'] == stream_dict['stream_id']: + if stream_dict['first_message_id'] is None: + stream_dict['first_message_id'] = event['message']['id'] + elif event['type'] == "hotspots": state['hotspots'] = event['hotspots'] elif event['type'] == "custom_profile_fields": diff --git a/zerver/lib/import_realm.py b/zerver/lib/import_realm.py index a742e880eb..9a99031b01 100644 --- a/zerver/lib/import_realm.py +++ b/zerver/lib/import_realm.py @@ -970,6 +970,17 @@ def do_import_realm(import_dir: Path, subdomain: str, processes: int=1) -> Realm user_profile.save(update_fields=["pointer"]) + # Similarly, we need to recalculate the first_message_id for stream objects. + for stream in Stream.objects.filter(realm=realm): + first_message = Message.objects.filter( + recipient__type_id=stream.id, + recipient__type=2).first() + if first_message is None: + stream.first_message_id = None + else: + stream.first_message_id = first_message.id + stream.save(update_fields=["first_message_id"]) + # Do attachments AFTER message data is loaded. # TODO: de-dup how we read these json files. fn = os.path.join(import_dir, "attachment.json") diff --git a/zerver/migrations/0209_stream_first_message_id.py b/zerver/migrations/0209_stream_first_message_id.py new file mode 100644 index 0000000000..34203b253d --- /dev/null +++ b/zerver/migrations/0209_stream_first_message_id.py @@ -0,0 +1,20 @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.18 on 2019-03-03 13:47 +from __future__ import unicode_literals + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('zerver', '0208_add_realm_night_logo_fields'), + ] + + operations = [ + migrations.AddField( + model_name='stream', + name='first_message_id', + field=models.IntegerField(db_index=True, null=True), + ), + ] diff --git a/zerver/migrations/0210_stream_first_message_id.py b/zerver/migrations/0210_stream_first_message_id.py new file mode 100644 index 0000000000..eb6045af0e --- /dev/null +++ b/zerver/migrations/0210_stream_first_message_id.py @@ -0,0 +1,31 @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.18 on 2019-02-25 12:42 +from __future__ import unicode_literals + +from django.db import migrations +from django.db.backends.postgresql_psycopg2.schema import DatabaseSchemaEditor +from django.db.migrations.state import StateApps + +def backfill_first_message_id(apps: StateApps, schema_editor: DatabaseSchemaEditor) -> None: + Stream = apps.get_model('zerver', 'Stream') + Message = apps.get_model('zerver', 'Message') + for stream in Stream.objects.all(): + first_message = Message.objects.filter( + recipient__type_id=stream.id, + recipient__type=2).first() + if first_message is None: + # No need to change anything if the outcome is the default of None + continue + + stream.first_message_id = first_message.id + stream.save() + +class Migration(migrations.Migration): + dependencies = [ + ('zerver', '0209_stream_first_message_id'), + ] + + operations = [ + migrations.RunPython(backfill_first_message_id, + reverse_code=migrations.RunPython.noop), + ] diff --git a/zerver/models.py b/zerver/models.py index a54a1b230a..38bbf5547f 100644 --- a/zerver/models.py +++ b/zerver/models.py @@ -1161,6 +1161,11 @@ class Stream(models.Model): email_token = models.CharField( max_length=32, default=generate_email_token_for_stream) # type: str + # The very first message ID in the stream. Used to help clients + # determine whether they might need to display "more topics" for a + # stream based on what messages they have cached. + first_message_id = models.IntegerField(null=True, db_index=True) # type: Optional[int] + def __str__(self) -> str: return "" % (self.name,) @@ -1186,7 +1191,8 @@ class Stream(models.Model): rendered_description=self.rendered_description, invite_only=self.invite_only, is_announcement_only=self.is_announcement_only, - history_public_to_subscribers=self.history_public_to_subscribers + history_public_to_subscribers=self.history_public_to_subscribers, + first_message_id=self.first_message_id, ) post_save.connect(flush_stream, sender=Stream) diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index c8a7d0135c..7a5903604e 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -1389,6 +1389,7 @@ class EventsRegisterTest(ZulipTestCase): ('is_announcement_only', check_bool), ('name', check_string), ('stream_id', check_int), + ('first_message_id', check_none_or(check_int)), ('history_public_to_subscribers', check_bool)]))), ]))), ]) @@ -2334,6 +2335,7 @@ class EventsRegisterTest(ZulipTestCase): ('desktop_notifications', check_bool), ('push_notifications', check_bool), ('stream_id', check_int), + ('first_message_id', check_none_or(check_int)), ('history_public_to_subscribers', check_bool), ('pin_to_top', check_bool), ('stream_weekly_traffic', check_none_or(check_int)), diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index 1aa69fc8c6..e82d4f0456 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -2212,7 +2212,7 @@ class SubscriptionAPITest(ZulipTestCase): streams_to_sub, dict(principals=ujson.dumps([user1.email, user2.email])), ) - self.assert_length(queries, 43) + self.assert_length(queries, 44) self.assert_length(events, 7) for ev in [x for x in events if x['event']['type'] not in ('message', 'stream')]: @@ -2973,7 +2973,7 @@ class SubscriptionAPITest(ZulipTestCase): [new_streams[0]], dict(principals=ujson.dumps([user1.email, user2.email])), ) - self.assert_length(queries, 43) + self.assert_length(queries, 44) # Test creating private stream. with queries_captured() as queries: @@ -2983,7 +2983,7 @@ class SubscriptionAPITest(ZulipTestCase): dict(principals=ujson.dumps([user1.email, user2.email])), invite_only=True, ) - self.assert_length(queries, 38) + self.assert_length(queries, 39) # Test creating a public stream with announce when realm has a notification stream. notifications_stream = get_stream(self.streams[0], self.test_realm) @@ -2998,7 +2998,7 @@ class SubscriptionAPITest(ZulipTestCase): principals=ujson.dumps([user1.email, user2.email]) ) ) - self.assert_length(queries, 51) + self.assert_length(queries, 52) class GetBotOwnerStreamsTest(ZulipTestCase): def test_streams_api_for_bot_owners(self) -> None: