From 795b2ba14e094fb13540a822cefa72fe35593841 Mon Sep 17 00:00:00 2001
From: Sanchit Sharma
Date: Sun, 7 Jul 2024 19:01:08 +0530
Subject: [PATCH] do_deactivate_stream: Remove unnecessary mutations.
Streams should not be marked as private, and subscribers
of the deactivated stream should not be removed.
Update the confirmation message when archiving a stream.
---
.../confirm_deactivate_stream.hbs | 13 +-
zerver/actions/message_send.py | 12 +-
zerver/actions/streams.py | 123 +++++-------------
zerver/lib/streams.py | 8 +-
zerver/openapi/curl_param_value_generators.py | 2 +-
zerver/tests/test_events.py | 4 -
zerver/tests/test_subs.py | 105 ++++++---------
7 files changed, 102 insertions(+), 165 deletions(-)
diff --git a/web/templates/confirm_dialog/confirm_deactivate_stream.hbs b/web/templates/confirm_dialog/confirm_deactivate_stream.hbs
index c420712735..230f572a09 100644
--- a/web/templates/confirm_dialog/confirm_deactivate_stream.hbs
+++ b/web/templates/confirm_dialog/confirm_deactivate_stream.hbs
@@ -1,9 +1,20 @@
{{#tr}}
- Archiving channel will immediately unsubscribe everyone. This action cannot be undone.
+ Archiving this channel will:
{{#*inline "z-stream"}}{{{stream_name_with_privacy_symbol_html}}}{{/inline}}
{{/tr}}
+
+
+ - {{#tr}}Remove it from the left sidebar for all users.{{/tr}}
+ - {{#tr}}Prevent new messages from being sent to this channel.{{/tr}}
+ - {{#tr}}Prevent messages in this channel from being edited, deleted, or moved.{{/tr}}
+
+ {{#tr}}
+ Users can still search for messages in archived channels.
+ This action cannot be undone.
+ {{/tr}}
+
{{#if is_announcement_stream}}
{{#tr}}Archiving this channel will also disable settings that were configured to use this channel:{{/tr}}
diff --git a/zerver/actions/message_send.py b/zerver/actions/message_send.py
index f521af32aa..6753e1ab66 100644
--- a/zerver/actions/message_send.py
+++ b/zerver/actions/message_send.py
@@ -1682,6 +1682,7 @@ def check_message(
mention_backend: MentionBackend | None = None,
limit_unread_user_ids: set[int] | None = None,
disable_external_notifications: bool = False,
+ archived_channel_notice: bool = False,
) -> SendMessageRequest:
"""See
https://zulip.readthedocs.io/en/latest/subsystems/sending-messages.html
@@ -1727,7 +1728,10 @@ def check_message(
if not skip_stream_access_check:
access_stream_for_send_message(
- sender=sender, stream=stream, forwarder_user_profile=forwarder_user_profile
+ sender=sender,
+ stream=stream,
+ forwarder_user_profile=forwarder_user_profile,
+ archived_channel_notice=archived_channel_notice,
)
else:
# Defensive assertion - the only currently supported use case
@@ -1859,6 +1863,7 @@ def _internal_prep_message(
disable_external_notifications: bool = False,
forged: bool = False,
forged_timestamp: float | None = None,
+ archived_channel_notice: bool = False,
) -> SendMessageRequest | None:
"""
Create a message object and checks it, but doesn't send it or save it to the database.
@@ -1890,6 +1895,7 @@ def _internal_prep_message(
disable_external_notifications=disable_external_notifications,
forged=forged,
forged_timestamp=forged_timestamp,
+ archived_channel_notice=archived_channel_notice,
)
except JsonableError as e:
logging.exception(
@@ -1913,6 +1919,7 @@ def internal_prep_stream_message(
limit_unread_user_ids: set[int] | None = None,
forged: bool = False,
forged_timestamp: float | None = None,
+ archived_channel_notice: bool = False,
) -> SendMessageRequest | None:
"""
See _internal_prep_message for details of how this works.
@@ -1930,6 +1937,7 @@ def internal_prep_stream_message(
limit_unread_user_ids=limit_unread_user_ids,
forged=forged,
forged_timestamp=forged_timestamp,
+ archived_channel_notice=archived_channel_notice,
)
@@ -2008,6 +2016,7 @@ def internal_send_stream_message(
email_gateway: bool = False,
message_type: int = Message.MessageType.NORMAL,
limit_unread_user_ids: set[int] | None = None,
+ archived_channel_notice: bool = False,
) -> int | None:
message = internal_prep_stream_message(
sender,
@@ -2017,6 +2026,7 @@ def internal_send_stream_message(
email_gateway=email_gateway,
message_type=message_type,
limit_unread_user_ids=limit_unread_user_ids,
+ archived_channel_notice=archived_channel_notice,
)
if message is None:
diff --git a/zerver/actions/streams.py b/zerver/actions/streams.py
index 43a23a5754..181a93c795 100644
--- a/zerver/actions/streams.py
+++ b/zerver/actions/streams.py
@@ -23,7 +23,7 @@ from zerver.lib.cache import (
from zerver.lib.exceptions import JsonableError
from zerver.lib.mention import silent_mention_syntax_for_user
from zerver.lib.message import get_last_message_id
-from zerver.lib.queue import queue_event_on_commit, queue_json_publish
+from zerver.lib.queue import queue_event_on_commit
from zerver.lib.stream_color import pick_colors
from zerver.lib.stream_subscription import (
SubInfo,
@@ -71,88 +71,25 @@ from zerver.models.users import active_non_guest_user_ids, active_user_ids, get_
from zerver.tornado.django_api import send_event_on_commit
-def send_user_remove_events_on_stream_deactivation(
- stream: Stream, subscribed_users: list[UserProfile]
-) -> None:
- guest_subscribed_users = [user for user in subscribed_users if user.is_guest]
-
- if len(guest_subscribed_users) == 0: # nocoverage
- # Save a few database queries in the case that the stream
- # didn't contain any guest users.
- return
-
- other_subscriptions_of_subscribed_users = get_subscribers_of_target_user_subscriptions(
- guest_subscribed_users
- )
- users_involved_in_dms_dict = get_users_involved_in_dms_with_target_users(
- guest_subscribed_users, stream.realm
- )
-
- subscriber_ids = {user.id for user in subscribed_users}
- inaccessible_user_dict: dict[int, set[int]] = defaultdict(set)
- for guest_user in guest_subscribed_users:
- users_accessible_by_guest = (
- {guest_user.id}
- | other_subscriptions_of_subscribed_users[guest_user.id]
- | users_involved_in_dms_dict[guest_user.id]
- )
- users_inaccessible_to_guest = subscriber_ids - users_accessible_by_guest
- for user_id in users_inaccessible_to_guest:
- inaccessible_user_dict[user_id].add(guest_user.id)
-
- for user_id, notify_user_ids in inaccessible_user_dict.items():
- event_remove_user = dict(
- type="realm_user",
- op="remove",
- person=dict(user_id=user_id, full_name=str(UserProfile.INACCESSIBLE_USER_NAME)),
- )
- send_event_on_commit(stream.realm, event_remove_user, list(notify_user_ids))
-
-
@transaction.atomic(savepoint=False)
def do_deactivate_stream(stream: Stream, *, acting_user: UserProfile | None) -> None:
# If the stream is already deactivated, this is a no-op
if stream.deactivated is True:
raise JsonableError(_("Channel is already deactivated"))
- # We want to mark all messages in the to-be-deactivated stream as
- # read for all users; otherwise they will pollute queries like
- # "Get the user's first unread message". Since this can be an
- # expensive operation, we do it via the deferred_work queue
- # processor.
- deferred_work_event = {
- "type": "mark_stream_messages_as_read_for_everyone",
- "stream_recipient_id": stream.recipient_id,
- }
- transaction.on_commit(lambda: queue_json_publish("deferred_work", deferred_work_event))
-
# Get the affected user ids *before* we deactivate everybody.
affected_user_ids = can_access_stream_user_ids(stream)
- stream_subscribers = get_active_subscriptions_for_stream_id(
- stream.id, include_deactivated_users=True
- ).select_related("user_profile")
- subscribed_users = [sub.user_profile for sub in stream_subscribers]
- stream_subscribers.update(active=False)
-
- was_invite_only = stream.invite_only
was_public = stream.is_public()
was_web_public = stream.is_web_public
-
- # We do not use do_change_stream_permission because no users need to
- # be notified, and we do not want to create audit log entries for
- # changing stream privacy. And due to this we also need to duplicate
- # the code to unset is_web_public field on attachments below.
stream.deactivated = True
- stream.invite_only = True
-
- stream.save(update_fields=["deactivated", "invite_only"])
+ stream.save(update_fields=["deactivated"])
assert stream.recipient_id is not None
if was_web_public:
assert was_public
# Unset the is_web_public and is_realm_public cache on attachments,
- # since the stream is now private.
+ # since the stream is now archived.
Attachment.objects.filter(messages__recipient_id=stream.recipient_id).update(
is_web_public=None, is_realm_public=None
)
@@ -160,7 +97,7 @@ def do_deactivate_stream(stream: Stream, *, acting_user: UserProfile | None) ->
is_web_public=None, is_realm_public=None
)
elif was_public:
- # Unset the is_realm_public cache on attachments, since the stream is now private.
+ # Unset the is_realm_public cache on attachments, since the stream is now archived.
Attachment.objects.filter(messages__recipient_id=stream.recipient_id).update(
is_realm_public=None
)
@@ -178,13 +115,9 @@ 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(invite_only=was_invite_only))
event = dict(type="stream", op="delete", streams=[stream_dict])
send_event_on_commit(stream.realm, event, affected_user_ids)
- if stream.realm.can_access_all_users_group.named_user_group.name != SystemGroups.EVERYONE:
- send_user_remove_events_on_stream_deactivation(stream, subscribed_users)
-
event_time = timezone_now()
RealmAuditLog.objects.create(
realm=stream.realm,
@@ -194,6 +127,16 @@ def do_deactivate_stream(stream: Stream, *, acting_user: UserProfile | None) ->
event_time=event_time,
)
+ sender = get_system_bot(settings.NOTIFICATION_BOT, stream.realm_id)
+ with override_language(stream.realm.default_language):
+ internal_send_stream_message(
+ sender,
+ stream,
+ topic_name=str(Realm.STREAM_EVENTS_NOTIFICATION_TOPIC_NAME),
+ content=_("Channel {channel_name} has been archived.").format(channel_name=stream.name),
+ archived_channel_notice=True,
+ )
+
def deactivated_streams_by_old_name(realm: Realm, stream_name: str) -> QuerySet[Stream]:
fixed_length_prefix = ".......!DEACTIVATED:"
@@ -223,32 +166,33 @@ def deactivated_streams_by_old_name(realm: Realm, stream_name: str) -> QuerySet[
@transaction.atomic(savepoint=False)
def do_unarchive_stream(stream: Stream, new_name: str, *, acting_user: UserProfile | None) -> None:
realm = stream.realm
+ stream_subscribers = get_active_subscriptions_for_stream_id(
+ stream.id, include_deactivated_users=True
+ ).select_related("user_profile")
+
if not stream.deactivated:
raise JsonableError(_("Channel is not currently deactivated"))
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)
)
+ if stream.invite_only and not stream_subscribers:
+ raise JsonableError(_("Channel is private and have no subscribers"))
assert stream.recipient_id is not None
stream.deactivated = False
stream.name = new_name
+ if stream.invite_only and stream.is_web_public:
+ # Previously, because archiving a channel set invite_only=True
+ # without mutating is_web_public, it was possible for archived
+ # channels to have this invalid state. Fix that.
+ stream.is_web_public = False
- # We only set invite_only=True during deactivation, which can lead
- # to the invalid state of to invite-only but also web-public
- # streams. Explicitly reset the access; we do not use
- # do_change_stream_permission because no users need be notified,
- # and it cannot handle the broken state that may currently exist.
- stream.is_web_public = False
- stream.invite_only = True
- stream.history_public_to_subscribers = True
stream.save(
update_fields=[
"name",
"deactivated",
"is_web_public",
- "invite_only",
- "history_public_to_subscribers",
]
)
@@ -280,17 +224,12 @@ def do_unarchive_stream(stream: Stream, new_name: str, *, acting_user: UserProfi
recent_traffic = get_streams_traffic({stream.id}, realm)
- # All admins always get to know about private streams' existence,
- # but we only subscribe the realm owners.
- send_stream_creation_event(
- realm, stream, [user.id for user in realm.get_admin_users_and_bots()], recent_traffic
- )
- bulk_add_subscriptions(
- realm=realm,
- streams=[stream],
- users=realm.get_human_owner_users(),
- acting_user=acting_user,
- )
+ subscribed_users = {sub.user_profile for sub in stream_subscribers}
+ admin_users_and_bots = set(realm.get_admin_users_and_bots())
+
+ notify_users = admin_users_and_bots | subscribed_users
+
+ send_stream_creation_event(realm, stream, [user.id for user in notify_users], recent_traffic)
sender = get_system_bot(settings.NOTIFICATION_BOT, stream.realm_id)
with override_language(stream.realm.default_language):
diff --git a/zerver/lib/streams.py b/zerver/lib/streams.py
index 78c903fefd..9c74418354 100644
--- a/zerver/lib/streams.py
+++ b/zerver/lib/streams.py
@@ -263,7 +263,10 @@ def check_stream_access_based_on_stream_post_policy(sender: UserProfile, stream:
def access_stream_for_send_message(
- sender: UserProfile, stream: Stream, forwarder_user_profile: UserProfile | None
+ sender: UserProfile,
+ stream: Stream,
+ forwarder_user_profile: UserProfile | None,
+ archived_channel_notice: bool = False,
) -> None:
# Our caller is responsible for making sure that `stream` actually
# matches the realm of the sender.
@@ -287,6 +290,9 @@ def access_stream_for_send_message(
else:
raise JsonableError(_("User not authorized for this query"))
+ if archived_channel_notice:
+ return
+
if is_cross_realm_bot_email(sender.delivery_email):
return
diff --git a/zerver/openapi/curl_param_value_generators.py b/zerver/openapi/curl_param_value_generators.py
index 107388e9a8..0a17ed9bd5 100644
--- a/zerver/openapi/curl_param_value_generators.py
+++ b/zerver/openapi/curl_param_value_generators.py
@@ -126,7 +126,7 @@ def add_emoji_to_message() -> dict[str, object]:
# The message ID here is hardcoded based on the corresponding value
# for the example message IDs we use in zulip.yaml.
- message_id = 47
+ message_id = 48
emoji_name = "octopus"
emoji_code = "1f419"
reaction_type = "unicode_emoji"
diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py
index ca0ac83765..c58cc06903 100644
--- a/zerver/tests/test_events.py
+++ b/zerver/tests/test_events.py
@@ -3391,8 +3391,6 @@ class NormalActionsTest(BaseAction):
with self.verify_action(num_events=2, archived_channels=True) as events:
do_deactivate_stream(stream, acting_user=None)
check_stream_delete("events[0]", events[0])
- check_realm_user_remove("events[1]", events[1])
- self.assertEqual(events[1]["person"]["user_id"], hamlet.id)
# Test that if the subscribers of deactivated stream are involved in
# DMs with guest, then the guest does not get "remove" event for them.
@@ -3407,8 +3405,6 @@ class NormalActionsTest(BaseAction):
with self.verify_action(num_events=2, archived_channels=True) as events:
do_deactivate_stream(stream, acting_user=None)
check_stream_delete("events[0]", events[0])
- check_realm_user_remove("events[1]", events[1])
- self.assertEqual(events[1]["person"]["user_id"], iago.id)
def test_subscribe_other_user_never_subscribed(self) -> None:
for i, include_streams in enumerate([True, False]):
diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py
index 16ebe32c91..8321bc651d 100644
--- a/zerver/tests/test_subs.py
+++ b/zerver/tests/test_subs.py
@@ -94,7 +94,6 @@ from zerver.lib.test_helpers import (
cache_tries_captured,
get_subscription,
most_recent_message,
- most_recent_usermessage,
queries_captured,
reset_email_visibility_to_everyone_in_zulip_realm,
)
@@ -1424,7 +1423,7 @@ class StreamAdminTest(ZulipTestCase):
)
.exists()
)
- self.assertFalse(subscription_exists)
+ self.assertTrue(subscription_exists)
def test_deactivate_stream_removes_default_stream(self) -> None:
stream = self.make_stream("new_stream")
@@ -1454,33 +1453,6 @@ class StreamAdminTest(ZulipTestCase):
do_deactivate_stream(streams_to_remove[0], acting_user=None)
self.assertEqual(get_streams(default_stream_groups[0]), streams_to_keep)
- def test_deactivate_stream_marks_messages_as_read(self) -> None:
- hamlet = self.example_user("hamlet")
- cordelia = self.example_user("cordelia")
- stream = self.make_stream("new_stream")
- self.subscribe(hamlet, stream.name)
- self.subscribe(cordelia, stream.name)
- self.subscribe(hamlet, "Denmark")
- self.subscribe(cordelia, "Denmark")
-
- self.send_stream_message(hamlet, stream.name)
- new_stream_usermessage = most_recent_usermessage(cordelia)
-
- # We send a message to a different stream too, to verify that the
- # deactivation of new_stream won't corrupt read state of UserMessage elsewhere.
- self.send_stream_message(hamlet, "Denmark")
- denmark_usermessage = most_recent_usermessage(cordelia)
-
- self.assertFalse(new_stream_usermessage.flags.read)
- self.assertFalse(denmark_usermessage.flags.read)
-
- with self.captureOnCommitCallbacks(execute=True):
- do_deactivate_stream(stream, acting_user=None)
- new_stream_usermessage.refresh_from_db()
- denmark_usermessage.refresh_from_db()
- self.assertTrue(new_stream_usermessage.flags.read)
- self.assertFalse(denmark_usermessage.flags.read)
-
def test_deactivated_streams_by_old_name(self) -> None:
realm = get_realm("zulip")
stream = self.make_stream("new_stream")
@@ -1506,6 +1478,31 @@ class StreamAdminTest(ZulipTestCase):
with self.assertRaisesRegex(JsonableError, "Channel named existing already exists"):
do_unarchive_stream(stream, new_name="existing", acting_user=None)
+ def test_unarchive_stream_private_with_no_subscribers(self) -> None:
+ stream = self.make_stream("private", invite_only=True)
+ do_deactivate_stream(stream, acting_user=None)
+ with self.assertRaisesRegex(JsonableError, "Channel is private and have no subscribers"):
+ do_unarchive_stream(stream, new_name="private", acting_user=None)
+
+ def test_unarchive_stream_private_and_web_public(self) -> None:
+ hamlet = self.example_user("hamlet")
+ cordelia = self.example_user("cordelia")
+
+ stream = self.make_stream("private", invite_only=True)
+ self.subscribe(hamlet, stream.name)
+ self.subscribe(cordelia, stream.name)
+ do_deactivate_stream(stream, acting_user=None)
+ stream = Stream.objects.get(id=stream.id)
+ # Previously, archiving a channel set invite_only=True without changing is_web_public.
+ # This led to archived channels potentially being in an invalid state.
+ stream.is_web_public = True
+ stream.save(update_fields=["is_web_public"])
+ with self.capture_send_event_calls(expected_num_events=2):
+ do_unarchive_stream(stream, new_name="private", acting_user=None)
+
+ stream = Stream.objects.get(id=stream.id)
+ self.assertFalse(stream.is_web_public)
+
def test_unarchive_stream(self) -> None:
desdemona = self.example_user("desdemona")
iago = self.example_user("iago")
@@ -1513,49 +1510,30 @@ class StreamAdminTest(ZulipTestCase):
cordelia = self.example_user("cordelia")
stream = self.make_stream("new_stream", is_web_public=True)
+ was_invite_only = stream.invite_only
+ was_web_public = stream.is_web_public
+ was_history_public = stream.history_public_to_subscribers
+
self.subscribe(hamlet, stream.name)
self.subscribe(cordelia, stream.name)
do_deactivate_stream(stream, acting_user=None)
- with self.capture_send_event_calls(expected_num_events=4) as events:
+ with self.capture_send_event_calls(expected_num_events=2) as events:
do_unarchive_stream(stream, new_name="new_stream", acting_user=None)
- # Tell all admins and owners that the stream exists
+ # Tell all subscribers and admins and owners that the stream exists
self.assertEqual(events[0]["event"]["op"], "create")
self.assertEqual(events[0]["event"]["streams"][0]["name"], "new_stream")
self.assertEqual(events[0]["event"]["streams"][0]["stream_id"], stream.id)
- self.assertEqual(set(events[0]["users"]), {iago.id, desdemona.id})
-
- # Tell the owners that they're subscribed to it
- self.assertEqual(events[1]["event"]["op"], "add")
- self.assertEqual(events[1]["event"]["subscriptions"][0]["name"], "new_stream")
- self.assertEqual(events[1]["event"]["subscriptions"][0]["stream_id"], stream.id)
- self.assertEqual(events[1]["users"], [desdemona.id])
-
- # iago (as an admin) gets to know that desdemona (the owner) is now subscribed.
- self.assertEqual(
- events[2],
- {
- "event": {
- "op": "peer_add",
- "stream_ids": [stream.id],
- "type": "subscription",
- "user_ids": [desdemona.id],
- },
- "users": [iago.id],
- },
- )
-
- # Send a message there logging the reactivation
- self.assertEqual(events[3]["event"]["type"], "message")
+ self.assertEqual(set(events[0]["users"]), {hamlet.id, cordelia.id, iago.id, desdemona.id})
stream = Stream.objects.get(id=stream.id)
self.assertFalse(stream.deactivated)
- self.assertTrue(stream.invite_only)
- self.assertFalse(stream.is_web_public)
- self.assertTrue(stream.history_public_to_subscribers)
+ self.assertEqual(stream.invite_only, was_invite_only)
+ self.assertEqual(stream.is_web_public, was_web_public)
+ self.assertEqual(stream.history_public_to_subscribers, was_history_public)
self.assertEqual(
- [desdemona.id],
+ [hamlet.id, cordelia.id],
[
sub.user_profile_id
for sub in get_active_subscriptions_for_stream_id(
@@ -2473,7 +2451,7 @@ class StreamAdminTest(ZulipTestCase):
realm = stream.realm
stream_id = stream.id
- with self.capture_send_event_calls(expected_num_events=1) as events:
+ with self.capture_send_event_calls(expected_num_events=2) as events:
result = self.client_delete("/json/streams/" + str(stream_id))
self.assert_json_success(result)
@@ -2493,15 +2471,12 @@ class StreamAdminTest(ZulipTestCase):
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.
+ # An archived stream is deactivated, but subscribers and
+ # permissions settings are not immediately changed.
deactivated_stream_name = active_name
deactivated_stream = get_stream(deactivated_stream_name, realm)
self.assertTrue(deactivated_stream.deactivated)
- 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, [])
# It doesn't show up in the list of public streams anymore.
result = self.client_get("/json/streams", {"include_subscribed": "false"})