From c6fc25e5df5304dd283ea3e2e42937f9ba9d5790 Mon Sep 17 00:00:00 2001 From: sanchi-t Date: Sun, 29 Sep 2024 20:47:01 +0530 Subject: [PATCH] do_deactivate_stream: Do not rename streams during archiving. Functions related to archived streams are also updated. --- zerver/actions/streams.py | 22 +++++----------------- zerver/tests/test_realm.py | 3 +-- zerver/tests/test_subs.py | 30 ++++++++++-------------------- 3 files changed, 16 insertions(+), 39 deletions(-) diff --git a/zerver/actions/streams.py b/zerver/actions/streams.py index 2a666c9283..28abb82712 100644 --- a/zerver/actions/streams.py +++ b/zerver/actions/streams.py @@ -1,4 +1,3 @@ -import hashlib from collections import defaultdict from collections.abc import Collection, Iterable, Mapping from typing import Any, TypeAlias @@ -146,20 +145,8 @@ def do_deactivate_stream(stream: Stream, *, acting_user: UserProfile | None) -> # the code to unset is_web_public field on attachments below. stream.deactivated = True stream.invite_only = True - # Preserve as much as possible the original stream name while giving it a - # special prefix that both indicates that the stream is deactivated and - # frees up the original name for reuse. - old_name = stream.name - # Prepend a substring of the hashed stream ID to the new stream name - streamID = str(stream.id) - stream_id_hash_object = hashlib.sha512(streamID.encode()) - hashed_stream_id = stream_id_hash_object.hexdigest()[0:7] - - new_name = (hashed_stream_id + "!DEACTIVATED:" + old_name)[: Stream.MAX_NAME_LENGTH] - - stream.name = new_name[: Stream.MAX_NAME_LENGTH] - stream.save(update_fields=["name", "deactivated", "invite_only"]) + stream.save(update_fields=["deactivated", "invite_only"]) assert stream.recipient_id is not None if was_web_public: @@ -191,7 +178,7 @@ def do_deactivate_stream(stream: Stream, *, acting_user: UserProfile | None) -> do_remove_streams_from_default_stream_group(stream.realm, group, [stream]) stream_dict = stream_to_dict(stream) - stream_dict.update(dict(name=old_name, invite_only=was_invite_only)) + stream_dict.update(dict(invite_only=was_invite_only)) event = dict(type="stream", op="delete", streams=[stream_dict]) send_event_on_commit(stream.realm, event, affected_user_ids) @@ -222,7 +209,8 @@ def deactivated_streams_by_old_name(realm: Realm, stream_name: str) -> QuerySet[ # characters, followed by `!DEACTIVATED:`, followed by at # most MAX_NAME_LENGTH-(length of the prefix) of the name # they provided: - Q(name__regex=rf"^{fixed_length_prefix}{truncated_name}") + Q(name=stream_name) + | Q(name__regex=rf"^{fixed_length_prefix}{truncated_name}") # Finally, we go looking for the pre-1b6f68bb59dc version, # which is any number of `!` followed by `DEACTIVATED:` # and a prefix of the old stream name @@ -237,7 +225,7 @@ def do_unarchive_stream(stream: Stream, new_name: str, *, acting_user: UserProfi realm = stream.realm if not stream.deactivated: raise JsonableError(_("Channel is not currently deactivated")) - if Stream.objects.filter(realm=realm, name=new_name).exists(): + if stream.name != new_name and Stream.objects.filter(realm=realm, name=new_name).exists(): raise JsonableError( _("Channel named {channel_name} already exists").format(channel_name=new_name) ) diff --git a/zerver/tests/test_realm.py b/zerver/tests/test_realm.py index ae53ca74c6..6c09c8504f 100644 --- a/zerver/tests/test_realm.py +++ b/zerver/tests/test_realm.py @@ -656,8 +656,7 @@ class RealmTest(ZulipTestCase): stats = merge_streams(realm, denmark, atlantis) self.assertEqual(stats, (1, 1, 1)) - with self.assertRaises(Stream.DoesNotExist): - get_stream("Atlantis", realm) + self.assertEqual(get_stream("Atlantis", realm).deactivated, True) stats = merge_streams(realm, denmark, new_stream_announcements_stream) self.assertEqual(stats, (2, new_stream_announcements_stream_messages_count, 10)) diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index 2d27da6f49..f3a6fd3a7a 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -1487,12 +1487,6 @@ class StreamAdminTest(ZulipTestCase): do_deactivate_stream(stream, acting_user=None) self.assertEqual(set(deactivated_streams_by_old_name(realm, "new_stream")), {stream}) - second_stream = self.make_stream("new_stream") - do_deactivate_stream(second_stream, acting_user=None) - self.assertEqual( - set(deactivated_streams_by_old_name(realm, "new_stream")), {stream, second_stream} - ) - self.make_stream("!DEACTIVATED:old_style") # This is left active old_style = self.make_stream("old_style") do_deactivate_stream(old_style, acting_user=None) @@ -2479,11 +2473,6 @@ class StreamAdminTest(ZulipTestCase): realm = stream.realm stream_id = stream.id - # Simulate that a stream by the same name has already been - # deactivated, just to exercise our renaming logic: - # Since we do not know the id of these simulated stream we prepend the name with a random hashed_stream_id - ensure_stream(realm, "DB32B77!DEACTIVATED:" + active_name, acting_user=None) - with self.capture_send_event_calls(expected_num_events=1) as events: result = self.client_delete("/json/streams/" + str(stream_id)) self.assert_json_success(result) @@ -2498,16 +2487,18 @@ class StreamAdminTest(ZulipTestCase): self.assertEqual(event["op"], "delete") self.assertEqual(event["streams"][0]["stream_id"], stream.id) - with self.assertRaises(Stream.DoesNotExist): - Stream.objects.get(realm=get_realm("zulip"), name=active_name) - - # A deleted stream's name is changed, is deactivated, is invite-only, - # and has no subscribers. hashed_stream_id = hashlib.sha512(str(stream_id).encode()).hexdigest()[0:7] - deactivated_stream_name = hashed_stream_id + "!DEACTIVATED:" + active_name + old_deactivated_stream_name = hashed_stream_id + "!DEACTIVATED:" + active_name + + with self.assertRaises(Stream.DoesNotExist): + Stream.objects.get(realm=get_realm("zulip"), name=old_deactivated_stream_name) + + # An archived stream is deactivated, is invite-only, + # and has no subscribers. + deactivated_stream_name = active_name deactivated_stream = get_stream(deactivated_stream_name, realm) self.assertTrue(deactivated_stream.deactivated) - self.assertTrue(deactivated_stream.invite_only) + self.assertEqual(deactivated_stream.invite_only, True) self.assertEqual(deactivated_stream.name, deactivated_stream_name) subscribers = self.users_subscribed_to_stream(deactivated_stream_name, realm) self.assertEqual(subscribers, []) @@ -2515,10 +2506,9 @@ class StreamAdminTest(ZulipTestCase): # It doesn't show up in the list of public streams anymore. result = self.client_get("/json/streams", {"include_subscribed": "false"}) public_streams = [s["name"] for s in self.assert_json_success(result)["streams"]] - self.assertNotIn(active_name, public_streams) self.assertNotIn(deactivated_stream_name, public_streams) - # Even if you could guess the new name, you can't subscribe to it. + # You can't subscribe to archived stream. result = self.common_subscribe_to_streams( self.example_user("hamlet"), [deactivated_stream_name], allow_fail=True )