From 323be5715104b1c984c1e2f6b42c7f74d571da24 Mon Sep 17 00:00:00 2001 From: Mateusz Mandera Date: Thu, 6 Jun 2019 15:10:39 +0200 Subject: [PATCH] retention: If stream has no retention policy set, use realm policy. We add the following behavior: If stream has message_retention_days set to -1, archiving for it is disabled. If stream has message_retention_days set to null, use the realm's policy. If the realm has no policy, we don't archive for this stream. --- zerver/lib/retention.py | 13 +++++++++++-- zerver/tests/test_retention.py | 35 +++++++++++++++++++++++++++++----- 2 files changed, 41 insertions(+), 7 deletions(-) diff --git a/zerver/lib/retention.py b/zerver/lib/retention.py index d54a385aaa..2747e68b00 100644 --- a/zerver/lib/retention.py +++ b/zerver/lib/retention.py @@ -3,6 +3,7 @@ from datetime import timedelta from django.conf import settings from django.db import connection, transaction +from django.db.models import Q from django.utils.timezone import now as timezone_now from zerver.models import (Message, UserMessage, ArchivedMessage, ArchivedUserMessage, Realm, Attachment, ArchivedAttachment, Reaction, ArchivedReaction, @@ -189,10 +190,18 @@ def archive_personal_and_huddle_messages() -> None: delete_messages(msg_ids) def archive_stream_messages() -> None: - streams = Stream.objects.filter(message_retention_days__isnull=False) + # We don't archive, if the stream has message_retention_days set to -1, + # or if neither the stream nor the realm have a retention policy. + streams = Stream.objects.exclude(message_retention_days=-1).filter( + Q(message_retention_days__isnull=False) | Q(realm__message_retention_days__isnull=False) + ) retention_policy_dict = {} # type: Dict[int, int] for stream in streams: - retention_policy_dict[stream.id] = stream.message_retention_days + # if stream.message_retention_days is null, use the realm's policy + if stream.message_retention_days: + retention_policy_dict[stream.id] = stream.message_retention_days + else: + retention_policy_dict[stream.id] = stream.realm.message_retention_days recipients = get_stream_recipients([stream.id for stream in streams]) for recipient in recipients: diff --git a/zerver/tests/test_retention.py b/zerver/tests/test_retention.py index f465e887c3..e13d1549fa 100644 --- a/zerver/tests/test_retention.py +++ b/zerver/tests/test_retention.py @@ -10,7 +10,7 @@ from zerver.lib.upload import create_attachment from zerver.models import (Message, Realm, UserProfile, Stream, ArchivedUserMessage, SubMessage, ArchivedMessage, Attachment, ArchivedAttachment, UserMessage, Reaction, ArchivedReaction, ArchivedSubMessage, - get_realm, get_user_profile_by_email, get_system_bot) + get_realm, get_user_profile_by_email, get_stream, get_system_bot) from zerver.lib.retention import ( archive_messages, move_messages_to_archive @@ -41,10 +41,9 @@ class RetentionTestingBase(ZulipTestCase): realm.message_retention_days = retention_period realm.save() - for stream in Stream.objects.filter(realm=realm): - if not retention_period or not stream.message_retention_days: - stream.message_retention_days = retention_period - stream.save() + def _set_stream_message_retention_value(self, stream: Stream, retention_period: Optional[int]) -> None: + stream.message_retention_days = retention_period + stream.save() def _change_messages_pub_date(self, msgs_ids: List[int], pub_date: datetime) -> None: Message.objects.filter(id__in=msgs_ids).update(pub_date=pub_date) @@ -186,6 +185,32 @@ class TestArchivingGeneral(RetentionTestingBase): self._set_realm_message_retention_value(self.zulip_realm, ZULIP_REALM_DAYS) + def test_different_stream_realm_policies(self) -> None: + verona = get_stream("Verona", self.zulip_realm) + hamlet = self.example_email("hamlet") + + msg_id = self.send_stream_message(hamlet, "Verona", "test") + usermsg_ids = self._get_usermessage_ids([msg_id]) + self._change_messages_pub_date([msg_id], timezone_now() - timedelta(days=2)) + + # Don't archive if stream's retention policy set to -1: + self._set_realm_message_retention_value(self.zulip_realm, 1) + self._set_stream_message_retention_value(verona, -1) + archive_messages() + self._verify_archive_data([], []) + + # Don't archive if stream and realm have no retention policy: + self._set_realm_message_retention_value(self.zulip_realm, None) + self._set_stream_message_retention_value(verona, None) + archive_messages() + self._verify_archive_data([], []) + + # Archive if stream has a retention policy set: + self._set_realm_message_retention_value(self.zulip_realm, None) + self._set_stream_message_retention_value(verona, 1) + archive_messages() + self._verify_archive_data([msg_id], usermsg_ids) + def test_cross_realm_messages_not_archived(self) -> None: """Check that cross-realm messages don't get archived or deleted.""" msg_id = self._send_cross_realm_message()