From bb15b2d7082ff8bbb4c2f85cf9ee1f7e4884467a Mon Sep 17 00:00:00 2001 From: Sahil Batra Date: Mon, 30 Oct 2023 17:20:40 +0530 Subject: [PATCH] users: Send "update" events when deactivating or reactivating users. We now send "realm_user/update" (and "realm_bot/update" for bots) events with "is_active" field when deactivating and reactivating users, including bots. We would want to use "remove" event for a user losing access to another user for #10970, so it is better to use "update" event for deactivation as we only update "is_active" field in the user objects and the clients still have the data for deactivated users. Previously, we used to send "add" event for reactivation along with complete user objects, but clients should have the data for deactivated users as well, so an "update" event is enough like we do when deactivating users. --- api_docs/changelog.md | 12 +++++++++ version.py | 2 +- web/src/bot_data.ts | 4 --- web/src/server_events_dispatch.js | 16 ------------ web/src/user_events.js | 17 +++++++++++++ web/tests/bot_data.test.js | 19 +++++--------- web/tests/dispatch.test.js | 29 ---------------------- web/tests/lib/events.js | 18 -------------- web/tests/user_events.test.js | 35 +++++++++++++++++++++++--- zerver/actions/create_user.py | 16 ++++++++++-- zerver/actions/users.py | 16 ++++++------ zerver/lib/event_schema.py | 25 ++++++------------- zerver/lib/events.py | 18 +++++--------- zerver/openapi/zulip.yaml | 41 +++++++++++++++++++++++++++++-- zerver/tests/test_events.py | 14 +++++------ 15 files changed, 148 insertions(+), 134 deletions(-) diff --git a/api_docs/changelog.md b/api_docs/changelog.md index 4c5d5f3659..0103e5c1c4 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -20,6 +20,18 @@ format used by the Zulip server that they are interacting with. ## Changes in Zulip 8.0 +**Feature level 222** + +* [`GET /events`](/api/get-events): When a user is deactivated or + reactivated, the server uses `realm_user` events with `op: "update"` + updating the `is_active` field, instead of `realm_user` events with + `op: "remove"` and `op: "add"`, respectively. + +* [`GET /events`](/api/get-events): When a bot is deactivated or + reactivated, the server sends `realm_bot` events with `op: "update"` + updating the `is_active` field, instead of `realm_bot` events with + `op: "remove"` and `op: "add"`, respectively. + **Feature level 221** * [`POST /register`](/api/register-queue): Added `server_supported_permission_settings` diff --git a/version.py b/version.py index 62222385bf..4ce0f7054c 100644 --- a/version.py +++ b/version.py @@ -33,7 +33,7 @@ DESKTOP_WARNING_VERSION = "5.9.3" # Changes should be accompanied by documentation explaining what the # new level means in api_docs/changelog.md, as well as "**Changes**" # entries in the endpoint's documentation in `zulip.yaml`. -API_FEATURE_LEVEL = 221 +API_FEATURE_LEVEL = 222 # Bump the minor PROVISION_VERSION to indicate that folks should provision # only when going from an old version of the code to a newer version. Bump diff --git a/web/src/bot_data.ts b/web/src/bot_data.ts index c457577a7d..e88907b295 100644 --- a/web/src/bot_data.ts +++ b/web/src/bot_data.ts @@ -67,10 +67,6 @@ export function add(bot_data: ServerAddBotData): void { services.set(clean_bot.user_id, bot_services); } -export function deactivate(bot_id: number): void { - bots.get(bot_id)!.is_active = false; -} - export function del(bot_id: number): void { bots.delete(bot_id); services.delete(bot_id); diff --git a/web/src/server_events_dispatch.js b/web/src/server_events_dispatch.js index 8b2b71c1cd..28cf6b5738 100644 --- a/web/src/server_events_dispatch.js +++ b/web/src/server_events_dispatch.js @@ -8,7 +8,6 @@ import * as audible_notifications from "./audible_notifications"; import * as blueslip from "./blueslip"; import * as bot_data from "./bot_data"; import * as browser_history from "./browser_history"; -import {buddy_list} from "./buddy_list"; import * as compose_call from "./compose_call"; import * as compose_call_ui from "./compose_call_ui"; import * as compose_pm_pill from "./compose_pm_pill"; @@ -358,11 +357,6 @@ export function dispatch_normal_event(event) { bot_data.add(event.bot); settings_bots.render_bots(); break; - case "remove": - bot_data.deactivate(event.bot.user_id); - event.bot.is_active = false; - settings_bots.render_bots(); - break; case "delete": bot_data.del(event.bot.user_id); settings_bots.render_bots(); @@ -468,16 +462,6 @@ export function dispatch_normal_event(event) { settings_users.redraw_bots_list(); } break; - case "remove": - people.deactivate(event.person); - stream_events.remove_deactivated_user_from_all_streams(event.person.user_id); - settings_users.update_view_on_deactivate(event.person.user_id); - buddy_list.maybe_remove_key({key: event.person.user_id}); - settings_account.maybe_update_deactivate_account_button(); - if (people.user_is_bot(event.person.user_id)) { - settings_users.update_bot_data(event.person.user_id); - } - break; case "update": user_events.update_person(event.person); settings_account.maybe_update_deactivate_account_button(); diff --git a/web/src/user_events.js b/web/src/user_events.js index 5cc9c8aeab..eab38e266f 100644 --- a/web/src/user_events.js +++ b/web/src/user_events.js @@ -6,6 +6,7 @@ import $ from "jquery"; import * as activity_ui from "./activity_ui"; import * as blueslip from "./blueslip"; +import {buddy_list} from "./buddy_list"; import * as compose_state from "./compose_state"; import * as message_live_update from "./message_live_update"; import * as narrow_state from "./narrow_state"; @@ -21,6 +22,7 @@ import * as settings_profile_fields from "./settings_profile_fields"; import * as settings_realm_user_settings_defaults from "./settings_realm_user_settings_defaults"; import * as settings_streams from "./settings_streams"; import * as settings_users from "./settings_users"; +import * as stream_events from "./stream_events"; export const update_person = function update(person) { const person_obj = people.maybe_get_user_by_id(person.user_id); @@ -135,4 +137,19 @@ export const update_person = function update(person) { if (Object.hasOwn(person, "bot_owner_id")) { person_obj.bot_owner_id = person.bot_owner_id; } + + if (Object.hasOwn(person, "is_active")) { + if (person.is_active) { + people.add_active_user(person_obj); + } else { + people.deactivate(person_obj); + stream_events.remove_deactivated_user_from_all_streams(person.user_id); + settings_users.update_view_on_deactivate(person.user_id); + buddy_list.maybe_remove_key({key: person.user_id}); + } + settings_account.maybe_update_deactivate_account_button(); + if (people.user_is_bot(person.user_id)) { + settings_users.update_bot_data(person.user_id); + } + } }; diff --git a/web/tests/bot_data.test.js b/web/tests/bot_data.test.js index 0a1035f992..e6b6c03543 100644 --- a/web/tests/bot_data.test.js +++ b/web/tests/bot_data.test.js @@ -161,6 +161,12 @@ test("test_basics", () => { bot = bot_data.get(43); assert.equal(bot.owner_id, fred.user_id); + + bot_data.update(43, {...test_bot, is_active: false}); + assert.ok(!bot_data.get(43).is_active); + + bot_data.update(43, {...test_bot, is_active: true}); + assert.ok(bot_data.get(43).is_active); })(); (function test_embedded_bot_update() { @@ -176,19 +182,6 @@ test("test_basics", () => { assert.equal("embedded bot service", services[0].service_name); })(); - (function test_remove() { - let bot; - - bot_data.add({...test_bot, is_active: true}); - - bot = bot_data.get(43); - assert.equal("Bot 1", bot.full_name); - assert.ok(bot.is_active); - bot_data.deactivate(43); - bot = bot_data.get(43); - assert.equal(bot.is_active, false); - })(); - (function test_all_user_ids() { const all_ids = bot_data.all_user_ids(); all_ids.sort(); diff --git a/web/tests/dispatch.test.js b/web/tests/dispatch.test.js index 5a52e91ee1..ec11ccd050 100644 --- a/web/tests/dispatch.test.js +++ b/web/tests/dispatch.test.js @@ -67,7 +67,6 @@ const settings_user_groups_legacy = mock_esm("../src/settings_user_groups_legacy const settings_users = mock_esm("../src/settings_users"); const sidebar_ui = mock_esm("../src/sidebar_ui"); const stream_data = mock_esm("../src/stream_data"); -const stream_events = mock_esm("../src/stream_events"); const stream_list = mock_esm("../src/stream_list"); const stream_settings_ui = mock_esm("../src/stream_settings_ui"); const stream_list_sort = mock_esm("../src/stream_list_sort"); @@ -603,18 +602,6 @@ run_test("realm_bot add", ({override}) => { assert_same(args.bot, event.bot); }); -run_test("realm_bot remove", ({override}) => { - const event = event_fixtures.realm_bot__remove; - const bot_stub = make_stub(); - override(bot_data, "deactivate", bot_stub.f); - override(settings_bots, "render_bots", () => {}); - dispatch(event); - - assert.equal(bot_stub.num_calls, 1); - const args = bot_stub.get_args("user_id"); - assert_same(args.user_id, event.bot.user_id); -}); - run_test("realm_bot delete", ({override}) => { const event = event_fixtures.realm_bot__delete; const bot_stub = make_stub(); @@ -728,16 +715,6 @@ run_test("realm_user", ({override}) => { assert.ok(people.is_active_user_for_popover(event.person.user_id)); - event = event_fixtures.realm_user__remove; - override(stream_events, "remove_deactivated_user_from_all_streams", noop); - override(settings_users, "update_view_on_deactivate", noop); - dispatch(event); - - // We don't actually remove the person, we just deactivate them. - const removed_person = people.get_by_user_id(event.person.user_id); - assert.equal(removed_person.full_name, "Test User"); - assert.ok(!people.is_active_user_for_popover(event.person.user_id)); - event = event_fixtures.realm_user__update; const stub = make_stub(); override(user_events, "update_person", stub.f); @@ -753,12 +730,6 @@ run_test("realm_user", ({override}) => { dispatch({...event}); assert.equal(add_bot_stub.num_calls, 1); - const remove_bot_stub = make_stub(); - event = event_fixtures.realm_user__remove; - override(settings_users, "update_bot_data", remove_bot_stub.f); - dispatch(event); - assert.equal(remove_bot_stub.num_calls, 1); - const update_bot_stub = make_stub(); event = event_fixtures.realm_user__update; override(settings_users, "update_bot_data", update_bot_stub.f); diff --git a/web/tests/lib/events.js b/web/tests/lib/events.js index 2f53b369e3..521517768e 100644 --- a/web/tests/lib/events.js +++ b/web/tests/lib/events.js @@ -440,15 +440,6 @@ exports.fixtures = { }, }, - realm_bot__remove: { - type: "realm_bot", - op: "remove", - bot: { - user_id: 42, - full_name: "The Bot", - }, - }, - realm_bot__update: { type: "realm_bot", op: "update", @@ -568,15 +559,6 @@ exports.fixtures = { }, }, - realm_user__remove: { - type: "realm_user", - op: "remove", - person: { - full_name: test_user.full_name, - user_id: test_user.user_id, - }, - }, - realm_user__update: { type: "realm_user", op: "update", diff --git a/web/tests/user_events.test.js b/web/tests/user_events.test.js index 95dcf78bb2..27c5e808cb 100644 --- a/web/tests/user_events.test.js +++ b/web/tests/user_events.test.js @@ -10,10 +10,16 @@ const {page_params} = require("./lib/zpage_params"); const message_live_update = mock_esm("../src/message_live_update"); const settings_account = mock_esm("../src/settings_account", { + maybe_update_deactivate_account_button() {}, update_email() {}, update_full_name() {}, update_account_settings_display() {}, }); +const settings_users = mock_esm("../src/settings_users", { + update_user_data() {}, + update_view_on_deactivate() {}, +}); +const stream_events = mock_esm("../src/stream_events"); mock_esm("../src/activity_ui", { redraw() {}, @@ -42,9 +48,6 @@ mock_esm("../src/settings_realm_user_settings_defaults", { mock_esm("../src/settings_streams", { maybe_disable_widgets() {}, }); -mock_esm("../src/settings_users", { - update_user_data() {}, -}); page_params.is_admin = true; @@ -259,4 +262,30 @@ run_test("updates", () => { user_events.update_person({user_id: test_bot.user_id, bot_owner_id: me.user_id}); person = people.get_by_email(test_bot.email); assert.equal(person.bot_owner_id, me.user_id); + + let user_removed_from_streams = false; + stream_events.remove_deactivated_user_from_all_streams = (user_id) => { + assert.equal(user_id, isaac.user_id); + user_removed_from_streams = true; + }; + user_events.update_person({user_id: isaac.user_id, is_active: false}); + assert.ok(!people.is_person_active(isaac.user_id)); + assert.ok(user_removed_from_streams); + + user_events.update_person({user_id: isaac.user_id, is_active: true}); + assert.ok(people.is_person_active(isaac.user_id)); + + stream_events.remove_deactivated_user_from_all_streams = () => {}; + + let bot_data_updated = false; + settings_users.update_bot_data = (user_id) => { + assert.equal(user_id, test_bot.user_id); + bot_data_updated = true; + }; + user_events.update_person({user_id: test_bot.user_id, is_active: false}); + assert.equal(bot_data_updated, true); + + bot_data_updated = false; + user_events.update_person({user_id: test_bot.user_id, is_active: true}); + assert.ok(bot_data_updated); }); diff --git a/zerver/actions/create_user.py b/zerver/actions/create_user.py index 4c8ca644f2..a7cfde23e8 100644 --- a/zerver/actions/create_user.py +++ b/zerver/actions/create_user.py @@ -46,6 +46,7 @@ from zerver.models import ( UserGroupMembership, UserMessage, UserProfile, + active_user_ids, bot_owner_user_ids, get_system_bot, ) @@ -639,10 +640,21 @@ def do_reactivate_user(user_profile: UserProfile, *, acting_user: Optional[UserP if settings.BILLING_ENABLED: update_license_ledger_if_needed(user_profile.realm, event_time) - notify_created_user(user_profile) + event = dict( + type="realm_user", op="update", person=dict(user_id=user_profile.id, is_active=True) + ) + send_event_on_commit(user_profile.realm, event, active_user_ids(user_profile.realm_id)) if user_profile.is_bot: - notify_created_bot(user_profile) + event = dict( + type="realm_bot", + op="update", + bot=dict( + user_id=user_profile.id, + is_active=True, + ), + ) + send_event_on_commit(user_profile.realm, event, bot_owner_user_ids(user_profile)) if bot_owner_changed: from zerver.actions.bots import send_bot_owner_update_events diff --git a/zerver/actions/users.py b/zerver/actions/users.py index eea5c92a1f..3cf9964bb7 100644 --- a/zerver/actions/users.py +++ b/zerver/actions/users.py @@ -299,23 +299,23 @@ def do_deactivate_user( transaction.on_commit(lambda: delete_user_sessions(user_profile)) - event_remove_user = dict( + event_deactivate_user = dict( type="realm_user", - op="remove", - person=dict(user_id=user_profile.id, full_name=user_profile.full_name), + op="update", + person=dict(user_id=user_profile.id, is_active=False), ) send_event_on_commit( - user_profile.realm, event_remove_user, active_user_ids(user_profile.realm_id) + user_profile.realm, event_deactivate_user, active_user_ids(user_profile.realm_id) ) if user_profile.is_bot: - event_remove_bot = dict( + event_deactivate_bot = dict( type="realm_bot", - op="remove", - bot=dict(user_id=user_profile.id, full_name=user_profile.full_name), + op="update", + bot=dict(user_id=user_profile.id, is_active=False), ) send_event_on_commit( - user_profile.realm, event_remove_bot, bot_owner_user_ids(user_profile) + user_profile.realm, event_deactivate_bot, bot_owner_user_ids(user_profile) ) diff --git a/zerver/lib/event_schema.py b/zerver/lib/event_schema.py index 4d70a1dd80..2abd77a73c 100644 --- a/zerver/lib/event_schema.py +++ b/zerver/lib/event_schema.py @@ -644,15 +644,6 @@ bot_type_for_remove = DictType( ] ) -realm_bot_remove_event = event_dict_type( - required_keys=[ - ("type", Equals("realm_bot")), - ("op", Equals("remove")), - ("bot", bot_type_for_remove), - ] -) -check_realm_bot_remove = make_checker(realm_bot_remove_event) - bot_type_for_update = DictType( required_keys=[ ("user_id", int), @@ -664,6 +655,7 @@ bot_type_for_update = DictType( ("default_events_register_stream", OptionalType(str)), ("default_sending_stream", OptionalType(str)), ("full_name", str), + ("is_active", bool), ("owner_id", int), ("services", bot_services_type), ], @@ -1108,15 +1100,6 @@ removed_user_type = DictType( ] ) -realm_user_remove_event = event_dict_type( - required_keys=[ - ("type", Equals("realm_user")), - ("op", Equals("remove")), - ("person", removed_user_type), - ], -) -check_realm_user_remove = make_checker(realm_user_remove_event) - custom_profile_field_type = DictType( required_keys=[ ("id", int), @@ -1190,6 +1173,12 @@ realm_user_person_types = dict( ("timezone", str), ], ), + is_active=DictType( + required_keys=[ + ("user_id", int), + ("is_active", bool), + ], + ), ) realm_user_update_event = event_dict_type( diff --git a/zerver/lib/events.py b/zerver/lib/events.py index 3d5463d19a..63f17e48af 100644 --- a/zerver/lib/events.py +++ b/zerver/lib/events.py @@ -865,13 +865,6 @@ def apply_event( if not person["is_bot"]: person["profile_data"] = {} state["raw_users"][person_user_id] = person - elif event["op"] == "remove": - state["raw_users"][person_user_id]["is_active"] = False - if include_subscribers: - for sub in state["subscriptions"]: - sub["subscribers"] = [ - user_id for user_id in sub["subscribers"] if user_id != person_user_id - ] elif event["op"] == "update": is_me = person_user_id == user_profile.id @@ -980,16 +973,17 @@ def apply_event( if "new_email" in person: p["email"] = person["new_email"] + + if "is_active" in person and not person["is_active"] and include_subscribers: + for sub in state["subscriptions"]: + sub["subscribers"] = [ + user_id for user_id in sub["subscribers"] if user_id != person_user_id + ] else: raise AssertionError("Unexpected event type {type}/{op}".format(**event)) elif event["type"] == "realm_bot": if event["op"] == "add": state["realm_bots"].append(event["bot"]) - elif event["op"] == "remove": - user_id = event["bot"]["user_id"] - for bot in state["realm_bots"]: - if bot["user_id"] == user_id: - bot["is_active"] = False elif event["op"] == "delete": state["realm_bots"] = [ item for item in state["realm_bots"] if item["user_id"] != event["bot"]["user_id"] diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 7a23ff3c81..2b87427269 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -594,6 +594,25 @@ paths: should update any data structures associated with this user to use this new value as the user's Zulip API email address. + - type: object + additionalProperties: false + description: | + When a user is deactivated or reactivated. + + **Changes**: New in Zulip 8.0 (feature level + 222). Previously the server sent a `realm_user` event with + `op` field set to `remove` when deactivating a user and a + `realm_user` event with `op` field set to `add` when + reactivating a user. + properties: + user_id: + type: integer + description: | + The ID of the user affected by this change. + is_active: + type: boolean + description: | + A boolean describing whether the user account has been deactivated. additionalProperties: false example: { @@ -1048,8 +1067,12 @@ paths: } - type: object description: | - Event sent to all users in a Zulip organization when - a user is deactivated. + No longer sent by modern Zulip servers. + + **Changes**: Deprecated and no longer sent since Zulip 8.0 (feature level 222). + + Previously, this event was sent to all users in a Zulip organization when a + user was deactivated. properties: id: $ref: "#/components/schemas/EventIdSchema" @@ -3715,6 +3738,11 @@ paths: additionalProperties: false description: | Event sent to all users when a bot has been deactivated. + + **Changes**: Deprecated and no longer sent since Zulip 8.0 (feature level 222). + + Previously, this event was sent to all users in a Zulip organization when a + bot was deactivated. properties: id: $ref: "#/components/schemas/EventIdSchema" @@ -17982,6 +18010,15 @@ components: owner_id: nullable: true services: {} + is_active: + type: boolean + description: | + A boolean describing whether the user account has been deactivated. + + **Changes**: New in Zulip 8.0 (feature level 222). Previously + we sent `realm_user` event with `op` field set to `remove` + when deactivating a bot and `realm_user` event with `op` + field set to `add` when reactivating a bot. BasicBotBase: type: object properties: diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index 4821e3ed09..7c9ea8e7fb 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -153,7 +153,6 @@ from zerver.lib.event_schema import ( check_reaction_remove, check_realm_bot_add, check_realm_bot_delete, - check_realm_bot_remove, check_realm_bot_update, check_realm_deactivated, check_realm_default_update, @@ -167,7 +166,6 @@ from zerver.lib.event_schema import ( check_realm_update, check_realm_update_dict, check_realm_user_add, - check_realm_user_remove, check_realm_user_update, check_scheduled_message_add, check_scheduled_message_remove, @@ -2751,14 +2749,14 @@ class NormalActionsTest(BaseAction): bot = self.create_bot("test") action = lambda: do_deactivate_user(bot, acting_user=None) events = self.verify_action(action, num_events=2) - check_realm_user_remove("events[0]", events[0]) - check_realm_bot_remove("events[1]", events[1]) + check_realm_user_update("events[0]", events[0], "is_active") + check_realm_bot_update("events[1]", events[1], "is_active") def test_do_deactivate_user(self) -> None: user_profile = self.example_user("cordelia") action = lambda: do_deactivate_user(user_profile, acting_user=None) events = self.verify_action(action, num_events=1) - check_realm_user_remove("events[0]", events[0]) + check_realm_user_update("events[0]", events[0], "is_active") def test_do_reactivate_user(self) -> None: bot = self.create_bot("test") @@ -2768,7 +2766,7 @@ class NormalActionsTest(BaseAction): do_deactivate_user(bot, acting_user=None) action = lambda: do_reactivate_user(bot, acting_user=None) events = self.verify_action(action, num_events=3) - check_realm_bot_add("events[1]", events[1]) + check_realm_bot_update("events[1]", events[1], "is_active") check_subscription_peer_add("events[2]", events[2]) # Test 'peer_add' event for private stream is received only if user is subscribed to it. @@ -2776,7 +2774,7 @@ class NormalActionsTest(BaseAction): self.subscribe(self.example_user("hamlet"), "Test private stream") action = lambda: do_reactivate_user(bot, acting_user=None) events = self.verify_action(action, num_events=4) - check_realm_bot_add("events[1]", events[1]) + check_realm_bot_update("events[1]", events[1], "is_active") check_subscription_peer_add("events[2]", events[2]) check_subscription_peer_add("events[3]", events[3]) @@ -2789,7 +2787,7 @@ class NormalActionsTest(BaseAction): self.user_profile = self.example_user("iago") action = lambda: do_reactivate_user(bot, acting_user=self.example_user("iago")) events = self.verify_action(action, num_events=7) - check_realm_bot_add("events[1]", events[1]) + check_realm_bot_update("events[1]", events[1], "is_active") check_realm_bot_update("events[2]", events[2], "owner_id") check_realm_user_update("events[3]", events[3], "bot_owner_id") check_subscription_peer_remove("events[4]", events[4])