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.
This commit is contained in:
Sanchit Sharma 2024-07-07 19:01:08 +05:30 committed by Tim Abbott
parent f04fb937a3
commit 795b2ba14e
7 changed files with 102 additions and 165 deletions

View File

@ -1,9 +1,20 @@
<p>
{{#tr}}
Archiving channel <z-stream></z-stream> will immediately unsubscribe everyone. This action cannot be undone.
Archiving this channel <z-stream></z-stream> will:
{{#*inline "z-stream"}}<strong>{{{stream_name_with_privacy_symbol_html}}}</strong>{{/inline}}
{{/tr}}
</p>
<p>
<ul>
<li>{{#tr}}Remove it from the left sidebar for all users.{{/tr}}</li>
<li>{{#tr}}Prevent new messages from being sent to this channel.{{/tr}}</li>
<li>{{#tr}}Prevent messages in this channel from being edited, deleted, or moved.{{/tr}}</li>
</ul>
{{#tr}}
Users can still search for messages in archived channels.<br/>
This action cannot be undone.
{{/tr}}
</p>
{{#if is_announcement_stream}}
<p class="notification_stream_archive_warning">{{#tr}}Archiving this channel will also disable settings that were configured to use this channel:{{/tr}}</p>
<ul>

View File

@ -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:

View File

@ -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):

View File

@ -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

View File

@ -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"

View File

@ -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]):

View File

@ -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"})