From 00474608c5b70f852e63dc8c38ad6db5e861fb28 Mon Sep 17 00:00:00 2001 From: Prakhar Pratyush Date: Wed, 17 Apr 2024 17:23:32 +0530 Subject: [PATCH] zulip_update: Send group DM for realm imported from other product. When the export is NOT generated by another zulip server, while importing: * Set the 'zulip_update_announcements_level' to the latest level as we don't want to send all the older update messages to them. * Send a group DM to admins, suggesting them to configure the stream in order to avoid missing future update messages. Fixes #29041. --- zerver/lib/import_realm.py | 10 ++ zerver/lib/zulip_update_announcements.py | 46 +++++--- zerver/tests/test_import_export.py | 52 ++++++--- zerver/tests/test_mattermost_importer.py | 4 +- zerver/tests/test_rocketchat_importer.py | 6 +- zerver/tests/test_slack_importer.py | 3 +- .../tests/test_zulip_update_announcements.py | 109 ++++++++++++++++++ 7 files changed, 189 insertions(+), 41 deletions(-) diff --git a/zerver/lib/import_realm.py b/zerver/lib/import_realm.py index 58b99e253f..0ea7e8165a 100644 --- a/zerver/lib/import_realm.py +++ b/zerver/lib/import_realm.py @@ -39,6 +39,7 @@ from zerver.lib.user_counts import realm_user_count_by_role from zerver.lib.user_groups import create_system_user_groups_for_realm from zerver.lib.user_message import UserMessageLite, bulk_insert_ums from zerver.lib.utils import generate_api_key, process_list_in_batches +from zerver.lib.zulip_update_announcements import send_zulip_update_announcements from zerver.models import ( AlertWord, Attachment, @@ -1508,6 +1509,15 @@ def do_import_realm(import_dir: Path, subdomain: str, processes: int = 1) -> Rea # Realm object is reactivated. maybe_enqueue_audit_log_upload(realm) + # If the export was NOT generated by another zulip server, the + # 'zulip_update_announcements_level' is set to None by default. + # Set it to the latest level to avoid receiving older update messages. + is_realm_imported_from_other_zulip_server = RealmAuditLog.objects.filter( + realm=realm, event_type=RealmAuditLog.REALM_EXPORTED, acting_user=None + ).exists() + if not is_realm_imported_from_other_zulip_server: + send_zulip_update_announcements(skip_delay=False, realm_imported_from_other_product=realm) + return realm diff --git a/zerver/lib/zulip_update_announcements.py b/zerver/lib/zulip_update_announcements.py index 8867f536f8..b1a9aa4415 100644 --- a/zerver/lib/zulip_update_announcements.py +++ b/zerver/lib/zulip_update_announcements.py @@ -145,8 +145,11 @@ def is_group_direct_message_sent_to_admins_within_days(realm: Realm, days: int) realm=realm, event_type=RealmAuditLog.REALM_PROPERTY_CHANGED, extra_data__contains={ + # Note: We're looking for the transition away from None, + # which usually will be to level 0, but can be to a higher + # initial level if the organization was imported from + # another chat tool. RealmAuditLog.OLD_VALUE: None, - RealmAuditLog.NEW_VALUE: 0, "property": "zulip_update_announcements_level", }, ).first() @@ -204,12 +207,19 @@ def send_messages_and_update_level( realm.save(update_fields=["zulip_update_announcements_level"]) -def send_zulip_update_announcements(skip_delay: bool) -> None: +def send_zulip_update_announcements( + skip_delay: bool, realm_imported_from_other_product: Optional[Realm] = None +) -> None: latest_zulip_update_announcements_level = get_latest_zulip_update_announcements_level() - realms = get_realms_behind_zulip_update_announcements_level( - level=latest_zulip_update_announcements_level - ) + if realm_imported_from_other_product: + realms = [realm_imported_from_other_product] + else: + realms = list( + get_realms_behind_zulip_update_announcements_level( + level=latest_zulip_update_announcements_level + ) + ) for realm in realms: # Refresh the realm from the database and check its @@ -228,16 +238,17 @@ def send_zulip_update_announcements(skip_delay: bool) -> None: new_zulip_update_announcements_level = None if realm_zulip_update_announcements_level is None: - # realm predates the zulip update announcements feature. + # This realm predates the zulip update announcements feature, or + # was imported from another product (Slack, Mattermost, etc.). # Group DM the administrators to set or verify the stream for # zulip update announcements. group_direct_message = internal_prep_group_direct_message_for_old_realm(realm, sender) messages = [group_direct_message] - new_zulip_update_announcements_level = 0 - elif ( - realm_zulip_update_announcements_level == 0 - and realm.zulip_update_announcements_stream is None - ): + if realm_imported_from_other_product: + new_zulip_update_announcements_level = latest_zulip_update_announcements_level + else: + new_zulip_update_announcements_level = 0 + elif realm.zulip_update_announcements_stream is None: # We wait for a week after sending group DMs to let admins configure # stream for zulip update announcements. After that, they miss updates # until they don't configure. @@ -253,13 +264,12 @@ def send_zulip_update_announcements(skip_delay: bool) -> None: ): continue - if realm.zulip_update_announcements_stream is not None: - messages = internal_prep_zulip_update_announcements_stream_messages( - current_level=realm_zulip_update_announcements_level, - latest_level=latest_zulip_update_announcements_level, - sender=sender, - realm=realm, - ) + messages = internal_prep_zulip_update_announcements_stream_messages( + current_level=realm_zulip_update_announcements_level, + latest_level=latest_zulip_update_announcements_level, + sender=sender, + realm=realm, + ) new_zulip_update_announcements_level = latest_zulip_update_announcements_level diff --git a/zerver/tests/test_import_export.py b/zerver/tests/test_import_export.py index 5120495ed1..3a5dcaaf8f 100644 --- a/zerver/tests/test_import_export.py +++ b/zerver/tests/test_import_export.py @@ -346,12 +346,24 @@ class RealmImportExportTest(ExportFile): consent_message_id=consent_message_id, ) + def export_realm_and_create_auditlog( + self, + original_realm: Realm, + exportable_user_ids: Optional[Set[int]] = None, + consent_message_id: Optional[int] = None, + public_only: bool = False, + ) -> None: + RealmAuditLog.objects.create( + realm=original_realm, event_type=RealmAuditLog.REALM_EXPORTED, event_time=timezone_now() + ) + self.export_realm(original_realm, exportable_user_ids, consent_message_id, public_only) + def test_export_files_from_local(self) -> None: user = self.example_user("hamlet") realm = user.realm self.upload_files_for_user(user) self.upload_files_for_realm(user) - self.export_realm(realm) + self.export_realm_and_create_auditlog(realm) self.verify_attachment_json(user) self.verify_uploads(user, is_s3=False) @@ -382,7 +394,7 @@ class RealmImportExportTest(ExportFile): is_message_realm_public=True, ) - self.export_realm(realm, public_only=True) + self.export_realm_and_create_auditlog(realm, public_only=True) # The attachment row shouldn't have been exported: self.assertEqual(read_json("attachment.json")["zerver_attachment"], []) @@ -401,7 +413,7 @@ class RealmImportExportTest(ExportFile): self.upload_files_for_user(user) self.upload_files_for_realm(user) - self.export_realm(realm) + self.export_realm_and_create_auditlog(realm) self.verify_attachment_json(user) self.verify_uploads(user, is_s3=True) @@ -423,7 +435,7 @@ class RealmImportExportTest(ExportFile): realm_user_default.default_language = "de" realm_user_default.save() - self.export_realm(realm) + self.export_realm_and_create_auditlog(realm) data = read_json("realm.json") self.assert_length(data["zerver_userprofile_crossrealm"], 3) @@ -500,7 +512,7 @@ class RealmImportExportTest(ExportFile): self.example_user("iago"), self.example_user("hamlet") ) - self.export_realm(realm, exportable_user_ids=user_ids) + self.export_realm_and_create_auditlog(realm, exportable_user_ids=user_ids) data = read_json("realm.json") @@ -612,7 +624,7 @@ class RealmImportExportTest(ExportFile): ) assert message is not None - self.export_realm(realm, consent_message_id=message.id) + self.export_realm_and_create_auditlog(realm, consent_message_id=message.id) data = read_json("realm.json") @@ -895,6 +907,10 @@ class RealmImportExportTest(ExportFile): new_realm_emoji.author = None new_realm_emoji.save() + RealmAuditLog.objects.create( + realm=original_realm, event_type=RealmAuditLog.REALM_EXPORTED, event_time=timezone_now() + ) + getters = self.get_realm_getters() snapshots: Dict[str, object] = {} @@ -1361,7 +1377,7 @@ class RealmImportExportTest(ExportFile): def test_import_realm_with_invalid_email_addresses_fails_validation(self) -> None: realm = get_realm("zulip") - self.export_realm(realm) + self.export_realm_and_create_auditlog(realm) data = read_json("realm.json") data["zerver_userprofile"][0]["delivery_email"] = "invalid_email_address" @@ -1378,7 +1394,7 @@ class RealmImportExportTest(ExportFile): # Such data should never reasonably get generated, but we should still # be defensive against it (since it can still happen due to bugs or manual edition # of export files in an attempt to get us to import malformed data). - self.export_realm(realm) + self.export_realm_and_create_auditlog(realm) data = read_json("realm.json") data["zerver_userprofile"][0]["email"] = "invalid_email_address" @@ -1394,7 +1410,7 @@ class RealmImportExportTest(ExportFile): original_realm = Realm.objects.get(string_id="zulip") RealmUserDefault.objects.get(realm=original_realm).delete() - self.export_realm(original_realm) + self.export_realm_and_create_auditlog(original_realm) with self.settings(BILLING_ENABLED=False), self.assertLogs(level="INFO"): do_import_realm(get_output_dir(), "test-zulip") @@ -1414,7 +1430,7 @@ class RealmImportExportTest(ExportFile): def test_import_realm_notify_bouncer(self) -> None: original_realm = Realm.objects.get(string_id="zulip") - self.export_realm(original_realm) + self.export_realm_and_create_auditlog(original_realm) with self.settings(BILLING_ENABLED=False), self.assertLogs(level="INFO"), patch( "zerver.lib.remote_server.send_to_push_bouncer" @@ -1451,7 +1467,7 @@ class RealmImportExportTest(ExportFile): self.upload_files_for_user(user) self.upload_files_for_realm(user) - self.export_realm(realm) + self.export_realm_and_create_auditlog(realm) with self.settings(BILLING_ENABLED=False), self.assertLogs(level="INFO"): do_import_realm(get_output_dir(), "test-zulip") @@ -1516,7 +1532,7 @@ class RealmImportExportTest(ExportFile): self.upload_files_for_realm(user) self.upload_files_for_user(user) - self.export_realm(realm) + self.export_realm_and_create_auditlog(realm) with self.settings(BILLING_ENABLED=False), self.assertLogs(level="INFO"): do_import_realm(get_output_dir(), "test-zulip") @@ -1610,7 +1626,7 @@ class RealmImportExportTest(ExportFile): realm, authentication_methods_dict, acting_user=None ) - self.export_realm(realm) + self.export_realm_and_create_auditlog(realm) with self.settings(BILLING_ENABLED=False), self.assertLogs(level="INFO"): do_import_realm(get_output_dir(), "test-zulip") @@ -1621,7 +1637,7 @@ class RealmImportExportTest(ExportFile): imported_realm.authentication_methods_dict(), ) - self.export_realm(realm) + self.export_realm_and_create_auditlog(realm) with self.settings(BILLING_ENABLED=True), self.assertLogs(level="WARN") as mock_warn: do_import_realm(get_output_dir(), "test-zulip2") @@ -1646,7 +1662,7 @@ class RealmImportExportTest(ExportFile): do_change_realm_plan_type(realm, Realm.PLAN_TYPE_LIMITED, acting_user=None) self.upload_files_for_user(user) - self.export_realm(realm) + self.export_realm_and_create_auditlog(realm) with self.settings(BILLING_ENABLED=True), self.assertLogs(level="INFO"): imported_realm = do_import_realm(get_output_dir(), "test-zulip-1") @@ -1663,7 +1679,7 @@ class RealmImportExportTest(ExportFile): # Importing the same export data twice would cause conflict on unique fields, # so instead re-export the original realm via self.export_realm, which handles # this issue. - self.export_realm(realm) + self.export_realm_and_create_auditlog(realm) with self.settings(BILLING_ENABLED=False), self.assertLogs(level="INFO"): imported_realm = do_import_realm(get_output_dir(), "test-zulip-2") @@ -1679,13 +1695,15 @@ class RealmImportExportTest(ExportFile): def test_system_usergroup_audit_logs(self) -> None: realm = get_realm("zulip") - self.export_realm(realm) + self.export_realm_and_create_auditlog(realm) # Simulate an external export where user groups are missing. data = read_json("realm.json") data.pop("zerver_usergroup") data.pop("zerver_namedusergroup") data.pop("zerver_realmauditlog") + data["zerver_realm"][0]["zulip_update_announcements_level"] = None + data["zerver_realm"][0]["zulip_update_announcements_stream"] = None # User groups data is missing. So, all the realm group based settings # should be None. diff --git a/zerver/tests/test_mattermost_importer.py b/zerver/tests/test_mattermost_importer.py index 9d12103d37..651ee4eaa8 100644 --- a/zerver/tests/test_mattermost_importer.py +++ b/zerver/tests/test_mattermost_importer.py @@ -849,7 +849,7 @@ class MatterMostImporter(ZulipTestCase): messages = Message.objects.filter(realm=realm) for message in messages: self.assertIsNotNone(message.rendered_content) - self.assert_length(messages, 11) + self.assert_length(messages, 12) stream_messages = messages.filter(recipient__type=Recipient.STREAM).order_by("date_sent") stream_recipients = stream_messages.values_list("recipient", flat=True) @@ -880,7 +880,7 @@ class MatterMostImporter(ZulipTestCase): "date_sent" ) personal_recipients = personal_messages.values_list("recipient", flat=True) - self.assert_length(personal_messages, 4) + self.assert_length(personal_messages, 5) self.assert_length(set(personal_recipients), 3) self.assertEqual(personal_messages[0].sender.email, "ron@zulip.com") self.assertRegex(personal_messages[0].content, "hey harry\n\n\\[harry-ron.jpg\\]\\(.*\\)") diff --git a/zerver/tests/test_rocketchat_importer.py b/zerver/tests/test_rocketchat_importer.py index f1ad315404..40e09526ab 100644 --- a/zerver/tests/test_rocketchat_importer.py +++ b/zerver/tests/test_rocketchat_importer.py @@ -1003,7 +1003,7 @@ class RocketChatImporter(ZulipTestCase): self.assertIsNotNone(message.rendered_content) # After removing user_joined, added_user, discussion_created, etc. # messages. (Total messages were 66.) - self.assert_length(messages, 43) + self.assert_length(messages, 44) stream_messages = messages.filter(recipient__type=Recipient.STREAM).order_by("date_sent") stream_recipients = stream_messages.values_list("recipient", flat=True) @@ -1025,8 +1025,8 @@ class RocketChatImporter(ZulipTestCase): "date_sent" ) huddle_recipients = huddle_messages.values_list("recipient", flat=True) - self.assert_length(huddle_messages, 4) - self.assert_length(set(huddle_recipients), 1) + self.assert_length(huddle_messages, 5) + self.assert_length(set(huddle_recipients), 2) self.assertEqual(huddle_messages[0].sender.email, "hermionegranger@email.com") self.assertEqual(huddle_messages[0].content, "Hey people!") diff --git a/zerver/tests/test_slack_importer.py b/zerver/tests/test_slack_importer.py index b94a37dad2..4511feccc5 100644 --- a/zerver/tests/test_slack_importer.py +++ b/zerver/tests/test_slack_importer.py @@ -1354,6 +1354,7 @@ class SlackImporter(ZulipTestCase): { RealmAuditLog.SUBSCRIPTION_CREATED, RealmAuditLog.REALM_PLAN_TYPE_CHANGED, + RealmAuditLog.REALM_PROPERTY_CHANGED, RealmAuditLog.REALM_CREATED, RealmAuditLog.REALM_IMPORTED, RealmAuditLog.USER_GROUP_CREATED, @@ -1363,7 +1364,7 @@ class SlackImporter(ZulipTestCase): }, ) - self.assertEqual(Message.objects.filter(realm=realm).count(), 82) + self.assertEqual(Message.objects.filter(realm=realm).count(), 83) # All auth backends are enabled initially. self.assertTrue(all(realm.authentication_methods_dict().values())) diff --git a/zerver/tests/test_zulip_update_announcements.py b/zerver/tests/test_zulip_update_announcements.py index 429797da01..dbbae4bc6b 100644 --- a/zerver/tests/test_zulip_update_announcements.py +++ b/zerver/tests/test_zulip_update_announcements.py @@ -1,11 +1,15 @@ +import os from datetime import timedelta from unittest import mock +from unittest.mock import call, patch import time_machine from django.conf import settings from django.utils.timezone import now as timezone_now from typing_extensions import override +from zerver.data_import.mattermost import do_convert_data +from zerver.lib.import_realm import do_import_realm from zerver.lib.message import remove_single_newlines from zerver.lib.test_classes import ZulipTestCase from zerver.lib.zulip_update_announcements import ( @@ -306,3 +310,108 @@ class ZulipUpdateAnnouncementsTest(ZulipTestCase): input_text = "- This is a bullet.\n- This is another bullet.\n\n1. This is a list\n1. This is more list." expected_output = "- This is a bullet.\n- This is another bullet.\n\n1. This is a list\n1. This is more list." self.assertEqual(remove_single_newlines(input_text), expected_output) + + def test_zulip_updates_for_realm_imported_from_other_product(self) -> None: + with mock.patch( + "zerver.lib.zulip_update_announcements.zulip_update_announcements", + self.zulip_update_announcements, + ): + mattermost_data_dir = self.fixture_file_name("", "mattermost_fixtures") + output_dir = self.make_import_output_dir("mattermost") + + with patch("builtins.print") as mock_print, self.assertLogs(level="WARNING"): + do_convert_data( + mattermost_data_dir=mattermost_data_dir, + output_dir=output_dir, + masking_content=True, + ) + self.assertEqual( + mock_print.mock_calls, + [ + call("Generating data for", "gryffindor"), + call("Generating data for", "slytherin"), + ], + ) + + gryffindor_output_dir = os.path.join(output_dir, "gryffindor") + + with self.assertLogs(level="INFO"): + do_import_realm( + import_dir=gryffindor_output_dir, + subdomain="gryffindor", + ) + + imported_realm = get_realm("gryffindor") + notification_bot = get_system_bot(settings.NOTIFICATION_BOT, imported_realm.id) + + # Verify for realm imported from other product: + # * zulip_update_announcements_level = latest level + # * zulip_update_announcements_stream = None + # * group DM sent to admins suggesting to set the stream. + self.assertEqual(imported_realm.zulip_update_announcements_level, 2) + self.assertIsNone(imported_realm.zulip_update_announcements_stream) + group_direct_message = Message.objects.filter( + realm=imported_realm, sender=notification_bot + ).first() + assert group_direct_message is not None + self.assertIn( + "These notifications are currently turned off in your organization. " + "If you configure a stream within one week, your organization will not miss any update messages.", + group_direct_message.content, + ) + + # Two new updates added. + new_updates = [ + ZulipUpdateAnnouncement( + level=3, + message="Announcement message 3.", + ), + ZulipUpdateAnnouncement( + level=4, + message="Announcement message 4.", + ), + ] + self.zulip_update_announcements.extend(new_updates) + + # Wait for one week before starting to skip sending updates. + now = timezone_now() + with time_machine.travel(now + timedelta(days=6), tick=False): + send_zulip_update_announcements(skip_delay=False) + imported_realm.refresh_from_db() + self.assertEqual(imported_realm.zulip_update_announcements_level, 2) + + # No stream configured. Skip updates. + with time_machine.travel(now + timedelta(days=8), tick=False): + send_zulip_update_announcements(skip_delay=False) + imported_realm.refresh_from_db() + self.assertEqual(imported_realm.zulip_update_announcements_level, 4) + zulip_updates_message_query = Message.objects.filter( + realm=imported_realm, + sender=notification_bot, + recipient__type=Recipient.STREAM, + ) + self.assertFalse(zulip_updates_message_query.exists()) + + new_updates = [ + ZulipUpdateAnnouncement( + level=5, + message="Announcement message 5.", + ), + ZulipUpdateAnnouncement( + level=6, + message="Announcement message 6.", + ), + ] + self.zulip_update_announcements.extend(new_updates) + + # Stream configured, send update messages. + imported_realm.zulip_update_announcements_stream = get_stream( + "Gryffindor common room", imported_realm + ) + imported_realm.save() + + with time_machine.travel(now + timedelta(days=10), tick=False): + send_zulip_update_announcements(skip_delay=False) + imported_realm.refresh_from_db() + self.assertEqual(imported_realm.zulip_update_announcements_level, 6) + self.assertTrue(zulip_updates_message_query.exists())