From fb63c47ea6d07e3de67f40a21df068c53857ef8f Mon Sep 17 00:00:00 2001 From: Sahil Batra Date: Thu, 12 Sep 2024 15:16:48 +0530 Subject: [PATCH] user_groups: Add client capability to handle deactivated groups. This commit adds a client capability to not receive data about deactivated groups. --- api_docs/changelog.md | 17 +++++++++++++++++ zerver/actions/user_groups.py | 9 +++++++++ zerver/lib/events.py | 14 +++++++++++++- zerver/openapi/zulip.yaml | 31 +++++++++++++++++++++++++++++++ zerver/tests/test_events.py | 20 ++++++++++++++++++++ zerver/tornado/django_api.py | 2 ++ zerver/tornado/event_queue.py | 34 ++++++++++++++++++++++++++++++++++ zerver/tornado/views.py | 5 +++++ 8 files changed, 131 insertions(+), 1 deletion(-) diff --git a/api_docs/changelog.md b/api_docs/changelog.md index 3ea445ff5b..8af8dba39d 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -20,6 +20,23 @@ format used by the Zulip server that they are interacting with. ## Changes in Zulip 10.0 +**Feature level 294** + +* [`POST /register`](/api/register-queue): Clients that do not + support the `include_deactivated_groups` + [client capability](/api/register-queue#parameter-client_capabilities) + do not receive deactivated user groups in the response. +* [`GET /events`](/api/get-events): Clients that do not support the + `include_deactivated_groups` + [client capability](/api/register-queue#parameter-client_capabilities) + receive `remove` event on user group deactivation instead of `update` + event. +* [`GET /events`](/api/get-events): Clients that do not support the + `include_deactivated_groups` + [client capability](/api/register-queue#parameter-client_capabilities) + do not receive `update` event when name of a deactivated user group + is updated. + **Feature level 293** * [`POST /register`](/api/register-queue), [`PATCH /settings`](/api/update-settings): diff --git a/zerver/actions/user_groups.py b/zerver/actions/user_groups.py index 484ba9a403..a855f70217 100644 --- a/zerver/actions/user_groups.py +++ b/zerver/actions/user_groups.py @@ -224,6 +224,12 @@ def do_send_user_group_update_event( user_group: NamedUserGroup, data: dict[str, str | int | AnonymousSettingGroupDict] ) -> None: event = dict(type="user_group", op="update", group_id=user_group.id, data=data) + if "name" in data: + # This field will be popped eventually before sending the event + # to client, but is needed to make sure we do not send the + # name update event for deactivated groups to client with + # 'include_deactivated_groups' client capability set to false. + event["deactivated"] = user_group.deactivated send_event_on_commit(user_group.realm, event, active_user_ids(user_group.realm_id)) @@ -463,6 +469,9 @@ def do_deactivate_user_group( do_send_user_group_update_event(user_group, dict(deactivated=True)) + event = dict(type="user_group", op="remove", group_id=user_group.id) + send_event_on_commit(user_group.realm, event, active_user_ids(user_group.realm_id)) + @transaction.atomic(savepoint=False) def do_change_user_group_permission_setting( diff --git a/zerver/lib/events.py b/zerver/lib/events.py index 04a7e58bc6..ab9232af13 100644 --- a/zerver/lib/events.py +++ b/zerver/lib/events.py @@ -141,6 +141,7 @@ def fetch_initial_state_data( pronouns_field_type_supported: bool = True, linkifier_url_template: bool = False, user_list_incomplete: bool = False, + include_deactivated_groups: bool = False, ) -> dict[str, Any]: """When `event_types` is None, fetches the core data powering the web app's `page_params` and `/api/v1/register` (for mobile/terminal @@ -507,7 +508,9 @@ def fetch_initial_state_data( state["realm_playgrounds"] = get_realm_playgrounds(realm) if want("realm_user_groups"): - state["realm_user_groups"] = user_groups_in_realm_serialized(realm, allow_deactivated=True) + state["realm_user_groups"] = user_groups_in_realm_serialized( + realm, allow_deactivated=include_deactivated_groups + ) if user_profile is not None: settings_user = user_profile @@ -782,6 +785,7 @@ def apply_events( include_subscribers: bool, linkifier_url_template: bool, user_list_incomplete: bool, + include_deactivated_groups: bool, ) -> None: for event in events: if fetch_event_types is not None and event["type"] not in fetch_event_types: @@ -803,6 +807,7 @@ def apply_events( include_subscribers=include_subscribers, linkifier_url_template=linkifier_url_template, user_list_incomplete=user_list_incomplete, + include_deactivated_groups=include_deactivated_groups, ) @@ -816,6 +821,7 @@ def apply_event( include_subscribers: bool, linkifier_url_template: bool, user_list_incomplete: bool, + include_deactivated_groups: bool, ) -> None: if event["type"] == "message": state["max_message_id"] = max(state["max_message_id"], event["message"]["id"]) @@ -1662,6 +1668,7 @@ class ClientCapabilities(TypedDict): user_settings_object: NotRequired[bool] linkifier_url_template: NotRequired[bool] user_list_incomplete: NotRequired[bool] + include_deactivated_groups: NotRequired[bool] def do_events_register( @@ -1698,6 +1705,7 @@ def do_events_register( user_settings_object = client_capabilities.get("user_settings_object", False) linkifier_url_template = client_capabilities.get("linkifier_url_template", False) user_list_incomplete = client_capabilities.get("user_list_incomplete", False) + include_deactivated_groups = client_capabilities.get("include_deactivated_groups", False) if fetch_event_types is not None: event_types_set: set[str] | None = set(fetch_event_types) @@ -1739,6 +1747,7 @@ def do_events_register( # Force include_streams=False for security reasons. include_streams=include_streams, spectator_requested_language=spectator_requested_language, + include_deactivated_groups=include_deactivated_groups, ) post_process_state(user_profile, ret, notification_settings_null=False) @@ -1767,6 +1776,7 @@ def do_events_register( pronouns_field_type_supported=pronouns_field_type_supported, linkifier_url_template=linkifier_url_template, user_list_incomplete=user_list_incomplete, + include_deactivated_groups=include_deactivated_groups, ) if queue_id is None: @@ -1788,6 +1798,7 @@ def do_events_register( pronouns_field_type_supported=pronouns_field_type_supported, linkifier_url_template=linkifier_url_template, user_list_incomplete=user_list_incomplete, + include_deactivated_groups=include_deactivated_groups, ) # Apply events that came in while we were fetching initial data @@ -1802,6 +1813,7 @@ def do_events_register( include_subscribers=include_subscribers, linkifier_url_template=linkifier_url_template, user_list_incomplete=user_list_incomplete, + include_deactivated_groups=include_deactivated_groups, ) post_process_state(user_profile, ret, notification_settings_null) diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 25c123eb40..22f440bbe4 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -3163,6 +3163,14 @@ paths: description: | Event sent to all users in a Zulip organization when a property of a user group is changed. + + For group deactivation, this event is only sent + if `include_deactivated_groups` client capability + is set to `true`. + + **Changes**: Prior to Zulip 10.0 (feature level 294), + this event was sent to all clients when a user group + was deactivated. properties: id: $ref: "#/components/schemas/EventIdSchema" @@ -3410,6 +3418,13 @@ paths: additionalProperties: false description: | Event sent to all users when a user group has been deleted. + + This event is also sent when a user group is deactivated + but only to clients with `include_deactivated_groups` client + capability set to `false`. + + **Changes**: Prior to Zulip 10.0 (feature level 294), this + event was only sent when a user group was deleted. properties: id: $ref: "#/components/schemas/EventIdSchema" @@ -13485,6 +13500,16 @@ paths: **Changes**: New in Zulip 8.0 (feature level 232). This capability is for backwards-compatibility. + - `include_deactivated_groups`: Boolean for whether the client can handle + deactivated user groups by themselves. If false, then the `realm_user_groups` + array in the `/register` response will only include active groups, clients + will receive a `remove` event instead of `update` event when a group is + deactivated and no `update` event will be sent to the client if a deactivated + user group is renamed. +
+ **Changes**: New in Zulip 10.0 (feature level 294). This + capability is for backwards-compatibility. + [help-linkifiers]: /help/add-a-custom-linkifier [rfc6570]: https://www.rfc-editor.org/rfc/rfc6570.html [events-linkifiers]: /api/get-events#realm_linkifiers @@ -14021,6 +14046,12 @@ paths: An array of dictionaries where each dictionary describes a [user group](/help/user-groups) in the Zulip organization. + + Deactivated groups will only be included if `include_deactivated_groups` + client capability is set to `true`. + + **Changes**: Prior to Zulip 10.0 (feature level 294), deactivated + groups were included for all the clients. realm_bots: type: array items: diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index 62afb2f7c4..bfec4cff48 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -299,6 +299,7 @@ class BaseAction(ZulipTestCase): linkifier_url_template: bool = True, user_list_incomplete: bool = False, client_is_old: bool = False, + include_deactivated_groups: bool = False, ) -> Iterator[list[dict[str, Any]]]: """ Make sure we have a clean slate of client descriptors for these tests. @@ -329,6 +330,7 @@ class BaseAction(ZulipTestCase): pronouns_field_type_supported=pronouns_field_type_supported, linkifier_url_template=linkifier_url_template, user_list_incomplete=user_list_incomplete, + include_deactivated_groups=include_deactivated_groups, ) ) @@ -346,6 +348,7 @@ class BaseAction(ZulipTestCase): pronouns_field_type_supported=pronouns_field_type_supported, linkifier_url_template=linkifier_url_template, user_list_incomplete=user_list_incomplete, + include_deactivated_groups=include_deactivated_groups, ) if client_is_old: @@ -386,6 +389,7 @@ class BaseAction(ZulipTestCase): include_subscribers=include_subscribers, linkifier_url_template=linkifier_url_template, user_list_incomplete=user_list_incomplete, + include_deactivated_groups=include_deactivated_groups, ) post_process_state(self.user_profile, hybrid_state, notification_settings_null) after = orjson.dumps(hybrid_state) @@ -415,6 +419,7 @@ class BaseAction(ZulipTestCase): pronouns_field_type_supported=pronouns_field_type_supported, linkifier_url_template=linkifier_url_template, user_list_incomplete=user_list_incomplete, + include_deactivated_groups=include_deactivated_groups, ) post_process_state(self.user_profile, normal_state, notification_settings_null) self.match_states(hybrid_state, normal_state, events) @@ -1924,8 +1929,23 @@ class NormalActionsTest(BaseAction): # Test deactivate event with self.verify_action() as events: do_deactivate_user_group(backend, acting_user=None) + check_user_group_remove("events[0]", events[0]) + + with self.verify_action(include_deactivated_groups=True) as events: + do_deactivate_user_group(api_design, acting_user=None) check_user_group_update("events[0]", events[0], "deactivated") + with self.verify_action(num_events=0, state_change_expected=False): + do_update_user_group_name(api_design, "api-deisgn-team", acting_user=None) + + with self.verify_action(include_deactivated_groups=True) as events: + do_update_user_group_name(api_design, "api-deisgn", acting_user=None) + check_user_group_update("events[0]", events[0], "name") + + # Reactivate the group to test "remove" event. + backend.deactivated = False + backend.save() + # Test remove event with self.verify_action() as events: check_delete_user_group(backend, acting_user=othello) diff --git a/zerver/tornado/django_api.py b/zerver/tornado/django_api.py index 8d5c088301..fbff9253eb 100644 --- a/zerver/tornado/django_api.py +++ b/zerver/tornado/django_api.py @@ -90,6 +90,7 @@ def request_event_queue( pronouns_field_type_supported: bool = True, linkifier_url_template: bool = False, user_list_incomplete: bool = False, + include_deactivated_groups: bool = False, ) -> str | None: if not settings.USING_TORNADO: return None @@ -113,6 +114,7 @@ def request_event_queue( "pronouns_field_type_supported": orjson.dumps(pronouns_field_type_supported), "linkifier_url_template": orjson.dumps(linkifier_url_template), "user_list_incomplete": orjson.dumps(user_list_incomplete), + "include_deactivated_groups": orjson.dumps(include_deactivated_groups), } if event_types is not None: diff --git a/zerver/tornado/event_queue.py b/zerver/tornado/event_queue.py index 9ab0a6e031..6b0f246424 100644 --- a/zerver/tornado/event_queue.py +++ b/zerver/tornado/event_queue.py @@ -78,6 +78,7 @@ class ClientDescriptor: pronouns_field_type_supported: bool = True, linkifier_url_template: bool = False, user_list_incomplete: bool = False, + include_deactivated_groups: bool = False, ) -> None: # TODO: We eventually want to upstream this code to the caller, but # serialization concerns make it a bit difficult. @@ -108,6 +109,7 @@ class ClientDescriptor: self.pronouns_field_type_supported = pronouns_field_type_supported self.linkifier_url_template = linkifier_url_template self.user_list_incomplete = user_list_incomplete + self.include_deactivated_groups = include_deactivated_groups # Default for lifespan_secs is DEFAULT_EVENT_QUEUE_TIMEOUT_SECS; # but users can set it as high as MAX_QUEUE_TIMEOUT_SECS. @@ -138,6 +140,7 @@ class ClientDescriptor: pronouns_field_type_supported=self.pronouns_field_type_supported, linkifier_url_template=self.linkifier_url_template, user_list_incomplete=self.user_list_incomplete, + include_deactivated_groups=self.include_deactivated_groups, ) @override @@ -174,6 +177,7 @@ class ClientDescriptor: d.get("pronouns_field_type_supported", True), d.get("linkifier_url_template", False), d.get("user_list_incomplete", False), + d.get("include_deactivated_groups", False), ) ret.last_connection_time = d["last_connection_time"] return ret @@ -231,6 +235,16 @@ class ClientDescriptor: # events are sent only if user_settings_object is False, # otherwise only 'user_settings' event is sent. return False + if event["type"] == "user_group": + if event["op"] == "remove": + # 'user_group/remove' events are only sent if the client + # cannot filter out deactivated groups by themselves. + return not self.include_deactivated_groups + if event["op"] == "update" and "deactivated" in event["data"]: + # 'update' events for group deactivation are only sent to + # clients who can filter out deactivated groups by themselves. + # Other clients receive 'remove' event. + return self.include_deactivated_groups return True # TODO: Refactor so we don't need this function @@ -1556,6 +1570,21 @@ def reformat_legacy_send_message_event( return (modern_event, user_dicts) +def process_user_group_name_update_event(event: Mapping[str, Any], users: Iterable[int]) -> None: + user_group_event = dict(event) + # 'deactivated' field is no longer needed and can be popped, as we now + # know whether the group that was renamed is deactivated or not and can + # avoid sending the event to client with 'include_deactivated_groups' + # client capability set to false. + event_for_deactivated_group = user_group_event.pop("deactivated", False) + for user_profile_id in users: + for client in get_client_descriptors_for_user(user_profile_id): + if client.accepts_event(user_group_event): + if event_for_deactivated_group and not client.include_deactivated_groups: + continue + client.add_event(user_group_event) + + def process_notification(notice: Mapping[str, Any]) -> None: event: Mapping[str, Any] = notice["event"] users: list[int] | list[Mapping[str, Any]] = notice["users"] @@ -1579,6 +1608,11 @@ def process_notification(notice: Mapping[str, Any]) -> None: process_custom_profile_fields_event(event, cast(list[int], users)) elif event["type"] == "realm_user" and event["op"] == "add": process_realm_user_add_event(event, cast(list[int], users)) + elif event["type"] == "user_group" and event["op"] == "update" and "name" in event["data"]: + # Only name can be changed for deactivated groups, so we handle the + # event sent for updating name separately for clients with different + # capabilities. + process_user_group_name_update_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. diff --git a/zerver/tornado/views.py b/zerver/tornado/views.py index 94e4710ce3..f2a9060bdd 100644 --- a/zerver/tornado/views.py +++ b/zerver/tornado/views.py @@ -206,6 +206,10 @@ def get_events_backend( Json[bool], ApiParamConfig(documentation_status=DocumentationStatus.INTENTIONALLY_UNDOCUMENTED), ] = False, + include_deactivated_groups: Annotated[ + Json[bool], + ApiParamConfig(documentation_status=DocumentationStatus.INTENTIONALLY_UNDOCUMENTED), + ] = False, ) -> HttpResponse: if narrow is None: narrow = [] @@ -243,6 +247,7 @@ def get_events_backend( pronouns_field_type_supported=pronouns_field_type_supported, linkifier_url_template=linkifier_url_template, user_list_incomplete=user_list_incomplete, + include_deactivated_groups=include_deactivated_groups, ) result = in_tornado_thread(fetch_events)(