streams: Fix events send when archiving and unarchiving streams.

This commit is contained in:
Sahil Batra
2025-03-31 12:31:30 +05:30
committed by Tim Abbott
parent ae579aa25a
commit 7c470f0161
9 changed files with 317 additions and 112 deletions

View File

@@ -0,0 +1,5 @@
* [`GET /events`](/api/get-events): Archiving and unarchiving
streams now send `update` events to clients that declared
the `archived_channels` client capability. `delete` and `create`
events are still sent to clients that did not declare
`archived_channels` client capability.

View File

@@ -159,7 +159,17 @@ def do_deactivate_stream(stream: Stream, *, acting_user: UserProfile | None) ->
for group in default_stream_groups_for_stream:
do_remove_streams_from_default_stream_group(stream.realm, group, [stream])
send_stream_deletion_event(stream.realm, affected_user_ids, [stream])
event = dict(
type="stream",
op="update",
stream_id=stream.id,
name=stream.name,
property="is_archived",
value=True,
)
send_event_on_commit(stream.realm, event, affected_user_ids)
send_stream_deletion_event(stream.realm, affected_user_ids, [stream], for_archiving=True)
event_time = timezone_now()
RealmAuditLog.objects.create(
@@ -271,9 +281,25 @@ def do_unarchive_stream(stream: Stream, new_name: str, *, acting_user: UserProfi
recent_traffic = get_streams_traffic({stream.id}, realm)
notify_user_ids = list(can_access_stream_metadata_user_ids(stream))
event = dict(
type="stream",
op="update",
stream_id=stream.id,
name=stream.name,
property="is_archived",
value=False,
)
send_event_on_commit(stream.realm, event, notify_user_ids)
anonymous_group_membership = get_anonymous_group_membership_dict_for_streams([stream])
send_stream_creation_event(
realm, stream, notify_user_ids, recent_traffic, anonymous_group_membership
realm,
stream,
notify_user_ids,
recent_traffic,
anonymous_group_membership,
for_unarchiving=True,
)
sender = get_system_bot(settings.NOTIFICATION_BOT, stream.realm_id)

View File

@@ -529,6 +529,7 @@ def check_stream_update(
"name",
"stream_id",
"first_message_id",
"is_archived",
}
if prop == "description":
@@ -565,6 +566,9 @@ def check_stream_update(
elif prop == "is_announcement_only":
assert extra_keys == set()
assert isinstance(value, bool)
elif prop == "is_archived":
assert extra_keys == set()
assert isinstance(value, bool)
else:
raise AssertionError(f"Unknown property: {prop}")

View File

@@ -1309,71 +1309,46 @@ def apply_event(
if event["op"] == "delete":
deleted_stream_ids = {stream["stream_id"] for stream in event["streams"]}
updated_first_message_ids: dict[int, int] = dict()
if archived_channels:
for stream in state["subscriptions"]:
if stream["stream_id"] in deleted_stream_ids:
stream["is_archived"] = True
state["subscriptions"] = [
stream
for stream in state["subscriptions"]
if stream["stream_id"] not in deleted_stream_ids
]
for stream in state["unsubscribed"]:
if stream["stream_id"] in deleted_stream_ids:
stream["is_archived"] = True
if stream["first_message_id"] is None:
new_first_message_id = Stream.objects.get(
id=stream["stream_id"]
).first_message_id
assert new_first_message_id is not None
stream["first_message_id"] = new_first_message_id
updated_first_message_ids[stream["stream_id"]] = new_first_message_id
state["unsubscribed"] = [
stream
for stream in state["unsubscribed"]
if stream["stream_id"] not in deleted_stream_ids
]
for stream in state["never_subscribed"]:
if stream["stream_id"] in deleted_stream_ids:
stream["is_archived"] = True
if stream["first_message_id"] is None:
new_first_message_id = Stream.objects.get(
id=stream["stream_id"]
).first_message_id
assert new_first_message_id is not None
stream["first_message_id"] = new_first_message_id
updated_first_message_ids[stream["stream_id"]] = new_first_message_id
state["never_subscribed"] = [
stream
for stream in state["never_subscribed"]
if stream["stream_id"] not in deleted_stream_ids
]
if "streams" in state:
for stream in state["streams"]:
if stream["stream_id"] in deleted_stream_ids:
stream["is_archived"] = True
if stream["stream_id"] in updated_first_message_ids:
stream["first_message_id"] = updated_first_message_ids[
stream["stream_id"]
]
else:
state["subscriptions"] = [
stream
for stream in state["subscriptions"]
if stream["stream_id"] not in deleted_stream_ids
if "streams" in state:
state["streams"] = [
s for s in state["streams"] if s["stream_id"] not in deleted_stream_ids
]
state["unsubscribed"] = [
stream
for stream in state["unsubscribed"]
if stream["stream_id"] not in deleted_stream_ids
]
state["never_subscribed"] = [
stream
for stream in state["never_subscribed"]
if stream["stream_id"] not in deleted_stream_ids
]
if "streams" in state:
state["streams"] = [
s for s in state["streams"] if s["stream_id"] not in deleted_stream_ids
]
if event["op"] == "update":
# For legacy reasons, we call stream data 'subscriptions' in
# the state var here, for the benefit of the JS code.
for obj in state["subscriptions"]:
if obj["name"].lower() == event["name"].lower():
obj[event["property"]] = event["value"]
if event["property"] == "description":
obj["rendered_description"] = event["rendered_description"]
if event.get("history_public_to_subscribers") is not None:
obj["history_public_to_subscribers"] = event[
"history_public_to_subscribers"
]
if event.get("is_web_public") is not None:
obj["is_web_public"] = event["is_web_public"]
updated_first_message_ids = dict()
for sub_list in [
state["subscriptions"],
state["unsubscribed"],
state["never_subscribed"],
]:
@@ -1388,6 +1363,17 @@ def apply_event(
]
if event.get("is_web_public") is not None:
obj["is_web_public"] = event["is_web_public"]
if (
event["property"] == "is_archived"
and event["value"]
and obj["first_message_id"] is None
):
new_first_message_id = Stream.objects.get(
id=obj["stream_id"]
).first_message_id
assert new_first_message_id is not None
obj["first_message_id"] = new_first_message_id
updated_first_message_ids[obj["stream_id"]] = new_first_message_id
# Also update the pure streams data
if "streams" in state:
for stream in state["streams"]:
@@ -1403,6 +1389,13 @@ def apply_event(
]
if event.get("is_web_public") is not None:
stream["is_web_public"] = event["is_web_public"]
if (
event["property"] == "is_archived"
and stream["stream_id"] in updated_first_message_ids
):
stream["first_message_id"] = updated_first_message_ids[
stream["stream_id"]
]
elif event["type"] == "default_streams":
state["realm_default_streams"] = event["default_streams"]

View File

@@ -148,11 +148,13 @@ def send_stream_creation_event(
user_ids: list[int],
recent_traffic: dict[int, int] | None = None,
anonymous_group_membership: dict[int, UserGroupMembersData] | None = None,
for_unarchiving: bool = False,
) -> None:
event = dict(
type="stream",
op="create",
streams=[stream_to_dict(stream, recent_traffic, anonymous_group_membership)],
for_unarchiving=for_unarchiving,
)
send_event_on_commit(realm, event, user_ids)
@@ -1758,7 +1760,7 @@ def check_update_all_streams_active_status(
def send_stream_deletion_event(
realm: Realm, user_ids: Iterable[int], streams: list[Stream]
realm: Realm, user_ids: Iterable[int], streams: list[Stream], for_archiving: bool = False
) -> None:
stream_deletion_event = dict(
type="stream",
@@ -1766,6 +1768,7 @@ def send_stream_deletion_event(
# "streams" is deprecated, kept only for compatibility.
streams=[dict(stream_id=stream.id) for stream in streams],
stream_ids=[stream.id for stream in streams],
for_archiving=for_archiving,
)
send_event_on_commit(realm, stream_deletion_event, user_ids)

View File

@@ -1296,11 +1296,19 @@ paths:
private channel is made public, or a guest user is subscribed
to a public (or private) channel.
This event is also sent when a channel is unarchived but only
to clients that did not declare the `archived_channels` [client
capability][client-capabilities].
Note that organization administrators who are not subscribed will
not be able to see content on the channel; just that it exists.
**Changes**: Prior to Zulip 8.0 (feature level 220), this event was
incorrectly not sent to guest users a web-public channel was created.
**Changes**: Prior to Zulip 11.0 (feature level ZF-960986), this
event was sent to all the users who could see the channel when it
was unarchived.
Prior to Zulip 8.0 (feature level 220), this event was incorrectly
not sent to guest users a web-public channel was created.
Prior to Zulip 8.0 (feature level 205), this event was not sent
when a user gained access to a channel due to their role changing.
@@ -1363,16 +1371,23 @@ paths:
}
- type: object
description: |
Event sent to all users who can see a channel when it is deactivated.
This event is also sent when a user loses access to a channel they
previously [could access](/help/channel-permissions) because they
are unsubscribed from a private channel or their [role](/help/user-roles)
Event sent when a user loses access to a channel they previously
[could access](/help/channel-permissions) because they are
unsubscribed from a private channel or their [role](/help/user-roles)
has changed.
**Changes**: Prior to Zulip 8.0 (feature level 205), this event was
not sent when a user lost access to a channel due to their role
changing.
This event is also sent when a channel is archived but only
to clients that did not declare the `archived_channels` [client
capability][client-capabilities].
**Changes**: Prior to Zulip 11.0 (feature level ZF-960986), this
event was sent to all the users who could see the channel when it
was archived.
Prior to Zulip 8.0 (feature level 205), this event was not sent
when a user lost access to a channel due to their role changing.
[client-capabilities]: /api/register-queue#parameter-client_capabilities
properties:
id:
$ref: "#/components/schemas/EventIdSchema"
@@ -1429,10 +1444,21 @@ paths:
[GET /streams](/api/get-streams#response) response
for details on the various properties of a channel.
**Changes**: Before Zulip 9.0 (feature level 256), this event
was never sent when the `first_message_id` property of a channel
was updated because the oldest message that had been sent to it
This event is also sent when archiving or unarchiving a
channel to all the users who can see that channel exists
but only to the clients that declared the `archived_channels`
[client capability][client-capabilities].
**Changes**: Prior to Zulip 11.0 (feature level ZF-960986),
this event was never sent when archiving or unarchiving
a channel.
Before Zulip 9.0 (feature level 256), this event was never
sent when the `first_message_id` property of a channel was
updated because the oldest message that had been sent to it
changed.
[client-capabilities]: /api/register-queue#parameter-client_capabilities
properties:
id:
$ref: "#/components/schemas/EventIdSchema"

View File

@@ -112,6 +112,7 @@ from zerver.actions.streams import (
do_change_subscription_property,
do_deactivate_stream,
do_rename_stream,
do_unarchive_stream,
)
from zerver.actions.submessage import do_add_submessage
from zerver.actions.typing import (
@@ -363,6 +364,7 @@ class BaseAction(ZulipTestCase):
linkifier_url_template=linkifier_url_template,
user_list_incomplete=user_list_incomplete,
include_deactivated_groups=include_deactivated_groups,
archived_channels=archived_channels,
)
)
@@ -3771,7 +3773,19 @@ class NormalActionsTest(BaseAction):
include_streams=include_streams, archived_channels=True
) as events:
do_deactivate_stream(stream, acting_user=None)
check_stream_update("events[0]", events[0])
self.assertEqual(events[0]["stream_id"], stream.id)
self.assertEqual(events[0]["property"], "is_archived")
self.assertEqual(events[0]["value"], True)
do_unarchive_stream(stream, stream.name, acting_user=None)
with self.verify_action(
include_streams=include_streams, archived_channels=False
) as events:
do_deactivate_stream(stream, acting_user=None)
check_stream_delete("events[0]", events[0])
self.assertEqual(events[0]["stream_ids"], [stream.id])
def test_admin_deactivate_unsubscribed_stream(self) -> None:
self.set_up_db_for_testing_user_access()
@@ -3788,7 +3802,37 @@ class NormalActionsTest(BaseAction):
with self.verify_action(num_events=1, archived_channels=True) as events:
do_deactivate_stream(stream, acting_user=iago)
check_stream_update("events[0]", events[0])
self.assertEqual(events[0]["stream_id"], stream.id)
self.assertEqual(events[0]["property"], "is_archived")
self.assertEqual(events[0]["value"], True)
do_unarchive_stream(stream, stream.name, acting_user=iago)
with self.verify_action(num_events=1, archived_channels=False) as events:
do_deactivate_stream(stream, acting_user=iago)
check_stream_delete("events[0]", events[0])
self.assertEqual(events[0]["stream_ids"], [stream.id])
def test_unarchiving_stream(self) -> None:
iago = self.example_user("iago")
stream = self.make_stream("test_stream")
do_deactivate_stream(stream, acting_user=iago)
with self.verify_action(num_events=1, archived_channels=False) as events:
do_unarchive_stream(stream, stream.name, acting_user=iago)
check_stream_create("events[0]", events[0])
self.assert_length(events[0]["streams"], 1)
self.assertEqual(events[0]["streams"][0]["stream_id"], stream.id)
do_deactivate_stream(stream, acting_user=iago)
with self.verify_action(num_events=1, archived_channels=True) as events:
do_unarchive_stream(stream, stream.name, acting_user=iago)
check_stream_update("events[0]", events[0])
self.assertEqual(events[0]["stream_id"], stream.id)
self.assertEqual(events[0]["property"], "is_archived")
self.assertEqual(events[0]["value"], False)
def test_user_losing_access_on_deactivating_stream(self) -> None:
self.set_up_db_for_testing_user_access()
@@ -3804,7 +3848,7 @@ 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_stream_update("events[0]", events[0])
# Test that if the subscribers of deactivated stream are involved in
# DMs with guest, then the guest does not get "remove" event for them.
@@ -3818,7 +3862,7 @@ 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_stream_update("events[0]", events[0])
def test_subscribe_other_user_never_subscribed(self) -> None:
for i, include_streams in enumerate([True, False]):
@@ -4939,6 +4983,38 @@ class SubscribeActionTest(BaseAction):
acting_user=iago,
)
# Check updating description, stream permission for an unsubscribed streams.
self.user_profile = self.example_user("hamlet")
self.unsubscribe(self.example_user("hamlet"), stream.name)
with self.verify_action(include_subscribers=include_subscribers, num_events=1) as events:
do_change_stream_description(
stream, "description", acting_user=self.example_user("hamlet")
)
check_stream_update("events[0]", events[0])
with self.verify_action(include_subscribers=include_subscribers, num_events=1) as events:
do_change_stream_permission(
stream,
invite_only=False,
history_public_to_subscribers=True,
is_web_public=True,
acting_user=iago,
)
check_stream_update("events[0]", events[0])
with self.verify_action(include_subscribers=include_subscribers, num_events=1) as events:
do_change_stream_permission(
stream,
invite_only=True,
history_public_to_subscribers=False,
is_web_public=False,
acting_user=iago,
)
check_stream_update("events[0]", events[0])
# Subscribe the user again for further tests.
self.subscribe(self.example_user("hamlet"), stream.name)
self.user_profile = self.example_user("hamlet")
with self.verify_action(include_subscribers=include_subscribers, num_events=2) as events:
do_change_stream_message_retention_days(stream, self.example_user("hamlet"), -1)

View File

@@ -1925,34 +1925,47 @@ class StreamAdminTest(ZulipTestCase):
acting_user=zoe,
)
self.subscribe(self.example_user("cordelia"), "stream_private_name1")
with self.capture_send_event_calls(expected_num_events=2) as events:
with self.capture_send_event_calls(expected_num_events=3) as events:
do_unarchive_stream(stream, new_name="private", acting_user=None)
stream = Stream.objects.get(id=stream.id)
self.assertFalse(stream.is_web_public)
# Clients will get this event only if they support
# archived_channels client capability.
self.assertEqual(events[0]["event"]["op"], "update")
self.assertEqual(events[0]["event"]["stream_id"], stream.id)
self.assertEqual(events[0]["event"]["property"], "is_archived")
self.assertEqual(events[0]["event"]["value"], False)
# Tell all users with metadata access that the stream exists.
self.assertEqual(events[0]["event"]["op"], "create")
self.assertEqual(events[0]["event"]["streams"][0]["name"], "private")
self.assertEqual(events[0]["event"]["streams"][0]["stream_id"], stream.id)
notified_user_ids = set(events[0]["users"])
self.assertEqual(
notified_user_ids,
can_access_stream_metadata_user_ids(stream),
)
self.assertIn(self.example_user("cordelia").id, notified_user_ids)
# An important corner case is that all organization admins are notified.
self.assertIn(self.example_user("iago").id, notified_user_ids)
# The current user, Hamlet was made an admin and thus should be notified too.
self.assertIn(aaron.id, notified_user_ids)
# Channel admin should be notified.
self.assertIn(self.example_user("aaron").id, notified_user_ids)
# User belonging to `can_add_subscribers_group` should be notified.
self.assertIn(prospero.id, notified_user_ids)
# User belonging to `can_subscribe_group` should be notified.
self.assertIn(zoe.id, notified_user_ids)
# Guest user should not be notified.
self.assertNotIn(self.example_user("polonius").id, notified_user_ids)
# This event will only be sent to clients that do not support
# archived_channels client capability, as clients supporting
# archived_channels client capability will already know that
# the stream exists.
self.assertEqual(events[1]["event"]["op"], "create")
self.assertEqual(events[1]["event"]["streams"][0]["name"], "private")
self.assertEqual(events[1]["event"]["streams"][0]["stream_id"], stream.id)
for event in [events[0], events[1]]:
notified_user_ids = set(event["users"])
self.assertEqual(
notified_user_ids,
can_access_stream_metadata_user_ids(stream),
)
self.assertIn(self.example_user("cordelia").id, notified_user_ids)
# An important corner case is that all organization admins are notified.
self.assertIn(self.example_user("iago").id, notified_user_ids)
# The current user, Hamlet was made an admin and thus should be notified too.
self.assertIn(aaron.id, notified_user_ids)
# Channel admin should be notified.
self.assertIn(self.example_user("aaron").id, notified_user_ids)
# User belonging to `can_add_subscribers_group` should be notified.
self.assertIn(prospero.id, notified_user_ids)
# User belonging to `can_subscribe_group` should be notified.
self.assertIn(zoe.id, notified_user_ids)
# Guest user should not be notified.
self.assertNotIn(self.example_user("polonius").id, notified_user_ids)
def test_unarchive_stream(self) -> None:
hamlet = self.example_user("hamlet")
@@ -1966,20 +1979,33 @@ class StreamAdminTest(ZulipTestCase):
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=2) as events:
with self.capture_send_event_calls(expected_num_events=3) as events:
do_unarchive_stream(stream, new_name="new_stream", acting_user=None)
# Clients will get this event only if they support
# archived_channels client capability.
self.assertEqual(events[0]["event"]["op"], "update")
self.assertEqual(events[0]["event"]["stream_id"], stream.id)
self.assertEqual(events[0]["event"]["property"], "is_archived")
self.assertEqual(events[0]["event"]["value"], False)
# Tell all users with metadata access 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)
notified_user_ids = set(events[0]["users"])
self.assertCountEqual(
notified_user_ids,
set(active_non_guest_user_ids(stream.realm_id)),
)
# Guest user should not be notified.
self.assertNotIn(self.example_user("polonius").id, notified_user_ids)
# This event will only be sent to clients that do not support
# archived_channels client capability, as clients supporting
# archived_channels client capability will already know that
# the stream exists.
self.assertEqual(events[1]["event"]["op"], "create")
self.assertEqual(events[1]["event"]["streams"][0]["name"], "new_stream")
self.assertEqual(events[1]["event"]["streams"][0]["stream_id"], stream.id)
for event in [events[0], events[1]]:
notified_user_ids = set(event["users"])
self.assertCountEqual(
notified_user_ids,
set(active_non_guest_user_ids(stream.realm_id)),
)
# Guest user should not be notified.
self.assertNotIn(self.example_user("polonius").id, notified_user_ids)
stream = Stream.objects.get(id=stream.id)
self.assertFalse(stream.deactivated)
@@ -3206,7 +3232,7 @@ class StreamAdminTest(ZulipTestCase):
realm = stream.realm
stream_id = stream.id
with self.capture_send_event_calls(expected_num_events=2) as events:
with self.capture_send_event_calls(expected_num_events=3) as events:
result = self.client_delete("/json/streams/" + str(stream_id))
self.assert_json_success(result)
@@ -3217,10 +3243,21 @@ class StreamAdminTest(ZulipTestCase):
self.assertEqual(sub_events, [])
stream_events = [e for e in events if e["event"]["type"] == "stream"]
self.assert_length(stream_events, 1)
event = stream_events[0]["event"]
self.assertEqual(event["op"], "delete")
self.assertEqual(event["streams"][0]["stream_id"], stream.id)
self.assert_length(stream_events, 2)
# Clients will get this event only if they support
# archived_channels client capability.
update_event = stream_events[0]["event"]
self.assertEqual(update_event["op"], "update")
self.assertEqual(update_event["stream_id"], stream.id)
self.assertEqual(update_event["property"], "is_archived")
self.assertEqual(update_event["value"], True)
# This event will only be sent to clients that do not support
# archived_channels client capability.
delete_event = stream_events[1]["event"]
self.assertEqual(delete_event["op"], "delete")
self.assertEqual(delete_event["streams"][0]["stream_id"], stream.id)
hashed_stream_id = hashlib.sha512(str(stream_id).encode()).hexdigest()[0:7]
old_deactivated_stream_name = hashed_stream_id + "!DEACTIVATED:" + active_name

View File

@@ -259,6 +259,15 @@ class ClientDescriptor:
# groups by themselves. Other clients receive 'remove' and
# 'add' event.
return self.include_deactivated_groups
if (
event["type"] == "stream"
and event["op"] == "update"
and event["property"] == "is_archived"
):
# 'update' events for archiving and unarchiving streams are
# only sent to clients that can process archived channels.
# Other clients receive "create" and "delete" events.
return self.archived_channels
return True
# TODO: Refactor so we don't need this function
@@ -1575,6 +1584,28 @@ def process_user_group_name_update_event(event: Mapping[str, Any], users: Iterab
client.add_event(user_group_event)
def process_stream_creation_event(event: Mapping[str, Any], users: Iterable[int]) -> None:
stream_create_event = dict(event)
event_for_unarchiving_stream = stream_create_event.pop("for_unarchiving", False)
for user_profile_id in users:
for client in get_client_descriptors_for_user(user_profile_id):
if client.accepts_event(stream_create_event):
if event_for_unarchiving_stream and client.archived_channels:
continue
client.add_event(stream_create_event)
def process_stream_deletion_event(event: Mapping[str, Any], users: Iterable[int]) -> None:
stream_delete_event = dict(event)
event_for_archiving_stream = stream_delete_event.pop("for_archiving", False)
for user_profile_id in users:
for client in get_client_descriptors_for_user(user_profile_id):
if client.accepts_event(stream_delete_event):
if event_for_archiving_stream and client.archived_channels:
continue
client.add_event(stream_delete_event)
def process_user_topic_event(event: Mapping[str, Any], users: Iterable[int]) -> None:
empty_topic_name_fallback_event: Mapping[str, Any] | dict[str, Any]
if event.get("topic_name") == "":
@@ -1668,6 +1699,10 @@ def process_notification(notice: Mapping[str, Any]) -> None:
and event["flag"] == "read"
):
process_mark_message_unread_event(event, cast(list[int], users))
elif event["type"] == "stream" and event["op"] == "create":
process_stream_creation_event(event, cast(list[int], users))
elif event["type"] == "stream" and event["op"] == "delete":
process_stream_deletion_event(event, cast(list[int], users))
elif event["type"] == "cleanup_queue":
# cleanup_event_queue may generate this event to forward cleanup
# requests to the right shard.