paulmataruso/zulip
1
0
Fork 0
mirror of https://github.com/zulip/zulip.git synced 2025-11-01 12:33:40 +00:00
Code Issues Packages Projects Releases Wiki Activity

reactions: Handle reactions by inaccessible users.

Browse Source 
Inaccessible user names are shown as "Unknown user" in the
reacted users list and tooltip.

This commit removes the call to `is_known_user_id` since we
may have reactions data from inaccessible users. And it is
fine to remove the `is_known_user_id` call anyways, because
it was added to ignore showing reactions from deactivated
users when the client did not have data about them, but now
the client has data about deactivated users.

So we anyways do not expect unknown user IDs in cases
other than inaccessible users for which we have to show
the reactions by showing their name as "Unknown user".

This commit also updates the comment for "is_known_user_id"
function accordingly.
This commit is contained in:
Sahil Batra
2023-12-01 19:12:00 +05:30
committed by Tim Abbott
parent ce6dc40a42
commit 2da3bd2813
3 changed files with 68 additions and 39 deletions
Download Patch File Download Diff File Expand all files Collapse all files

9
web/src/people.ts
View File

@@ -242,11 +242,10 @@ export function get_user_id(email: string): number | undefined {
export function is_known_user_id(user_id: number): boolean {
/*
For certain low-stakes operations, such as emoji reactions,
we may get a user_id that we don't know about, because the
user may have been deactivated. (We eventually want to track
deactivated users on the client, but until then, this is an
expedient thing we can check.)
We may get a user_id from mention syntax that we don't
know about if a user includes some random number in
the mention syntax by manually typing it instead of
selecting some user from typeahead.
*/
return people_by_user_id_dict.has(user_id);
}

5
web/src/reactions.js
View File

@@ -439,11 +439,6 @@ export function set_clean_reactions(message) {
const local_id = get_local_reaction_id(reaction);
const user_id = reaction.user_id;
if (!people.is_known_user_id(user_id)) {
blueslip.warn("Unknown user_id " + user_id + " in reaction for message " + message.id);
continue;
}
if (!distinct_reactions.has(local_id)) {
distinct_reactions.set(local_id, reaction);
user_map.set(local_id, []);

93
web/tests/reactions.test.js
View File

@@ -40,9 +40,7 @@ const sample_message = {
const channel = mock_esm("../src/channel");
const message_store = mock_esm("../src/message_store");
mock_esm("../src/settings_data", {
user_can_access_all_other_users: () => true,
});
const settings_data = mock_esm("../src/settings_data");
const spectators = mock_esm("../src/spectators", {
login_to_access() {},
});
@@ -109,7 +107,7 @@ function test(label, f) {
test("basics", () => {
const message = {...sample_message};
settings_data.user_can_access_all_other_users = () => true;
const result = reactions.get_message_reactions(message);
assert.ok(reactions.current_user_has_reacted_to_emoji(message, "unicode_emoji,1f642"));
assert.ok(!reactions.current_user_has_reacted_to_emoji(message, "bogus"));
@@ -201,6 +199,68 @@ test("basics", () => {
assert.deepEqual(result, expected_result);
});
test("reactions from unknown users", () => {
settings_data.user_can_access_all_other_users = () => false;
people.add_inaccessible_user(10);
const message = {
id: 1001,
reactions: [
{emoji_name: "smile", user_id: 5, reaction_type: "unicode_emoji", emoji_code: "1f642"},
{emoji_name: "smile", user_id: 9, reaction_type: "unicode_emoji", emoji_code: "1f642"},
{emoji_name: "frown", user_id: 9, reaction_type: "unicode_emoji", emoji_code: "1f641"},
{emoji_name: "tada", user_id: 6, reaction_type: "unicode_emoji", emoji_code: "1f389"},
{emoji_name: "tada", user_id: 10, reaction_type: "unicode_emoji", emoji_code: "1f389"},
],
};
const result = reactions.get_message_reactions(message);
result.sort((a, b) => a.count - b.count);
const expected_result = [
{
emoji_name: "frown",
reaction_type: "unicode_emoji",
emoji_code: "1f641",
local_id: "unicode_emoji,1f641",
count: 1,
vote_text: "1",
user_ids: [9],
label: "translated: translated: Unknown user reacted with :frown:",
emoji_alt_code: false,
class: "message_reaction",
is_realm_emoji: false,
},
{
emoji_name: "smile",
reaction_type: "unicode_emoji",
emoji_code: "1f642",
local_id: "unicode_emoji,1f642",
count: 2,
vote_text: "2",
user_ids: [5, 9],
label: "translated: You (click to remove) and translated: Unknown user reacted with :smile:",
emoji_alt_code: false,
class: "message_reaction reacted",
is_realm_emoji: false,
},
{
emoji_name: "tada",
reaction_type: "unicode_emoji",
emoji_code: "1f389",
local_id: "unicode_emoji,1f389",
count: 2,
vote_text: "2",
user_ids: [6, 10],
label: "translated: Bob van Roberts and translated: Unknown user reacted with :tada:",
emoji_alt_code: false,
class: "message_reaction",
is_realm_emoji: false,
},
];
assert.deepEqual(result, expected_result);
});
test("unknown realm emojis (add)", () => {
assert.throws(
() =>
@@ -1210,31 +1270,6 @@ test("process_reaction_click", ({override}) => {
assert.deepEqual(args.data, expected_reaction_info);
});
test("warnings", () => {
const message = {
id: 3001,
reactions: [
{emoji_name: "smile", user_id: 5, reaction_type: "unicode_emoji", emoji_code: "1f642"},
// add some bogus user_ids
{
emoji_name: "octopus",
user_id: 8888,
reaction_type: "unicode_emoji",
emoji_code: "1f419",
},
{
emoji_name: "frown",
user_id: 9999,
reaction_type: "unicode_emoji",
emoji_code: "1f641",
},
],
};
blueslip.expect("warn", "Unknown user_id 8888 in reaction for message 3001");
blueslip.expect("warn", "Unknown user_id 9999 in reaction for message 3001");
reactions.get_message_reactions(message);
});
test("code coverage", ({override}) => {
/*
We just silently fail in a few places in the reaction