Skip to content

Commit

Permalink
users: Send "update" events when deactivating or reactivating users.
Browse files Browse the repository at this point in the history
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 zulip#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.
  • Loading branch information
sahil839 authored and timabbott committed Nov 2, 2023
1 parent 7eb24ff commit 4a0fdf9
Show file tree
Hide file tree
Showing 15 changed files with 148 additions and 134 deletions.
12 changes: 12 additions & 0 deletions api_docs/changelog.md
Expand Up @@ -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`
Expand Down
2 changes: 1 addition & 1 deletion version.py
Expand Up @@ -33,7 +33,7 @@
# 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
Expand Down
4 changes: 0 additions & 4 deletions web/src/bot_data.ts
Expand Up @@ -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);
Expand Down
16 changes: 0 additions & 16 deletions web/src/server_events_dispatch.js
Expand Up @@ -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";
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand Down
17 changes: 17 additions & 0 deletions web/src/user_events.js
Expand Up @@ -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";
Expand All @@ -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);
Expand Down Expand Up @@ -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);
}
}
};
19 changes: 6 additions & 13 deletions web/tests/bot_data.test.js
Expand Up @@ -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() {
Expand All @@ -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();
Expand Down
29 changes: 0 additions & 29 deletions web/tests/dispatch.test.js
Expand Up @@ -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");
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down
18 changes: 0 additions & 18 deletions web/tests/lib/events.js
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down
35 changes: 32 additions & 3 deletions web/tests/user_events.test.js
Expand Up @@ -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() {},
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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);
});
16 changes: 14 additions & 2 deletions zerver/actions/create_user.py
Expand Up @@ -46,6 +46,7 @@
UserGroupMembership,
UserMessage,
UserProfile,
active_user_ids,
bot_owner_user_ids,
get_system_bot,
)
Expand Down Expand Up @@ -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
Expand Down
16 changes: 8 additions & 8 deletions zerver/actions/users.py
Expand Up @@ -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)
)


Expand Down

0 comments on commit 4a0fdf9

Please sign in to comment.