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

api: Improve consistency of reactions API.

Browse Source 
Previously, the message and event APIs represented the user differently
for the same reaction data. To make this more consistent, I added a
user_id field to the reaction dict for both messages and events. I
updated the front end to use the user_id field rather than the user
dict. Lastly, I updated front end and back end tests that used user
info.

I primarily tested this by running my local Zulip build and
adding/removing reactions from messages.

Fixes #12049.
This commit is contained in:
rebtung
2020-04-22 17:24:28 -04:00
committed by Tim Abbott
parent 2a65be2bf5
commit f7fbe3419f
8 changed files with 57 additions and 48 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
frontend_tests/node_tests/dispatch.js
View File

@@ -190,6 +190,7 @@ const event_fixtures = {
op: 'add',
message_id: 128,
emoji_name: 'anguished_pig',
user_id: "1",
user: {
id: "1",
},
@@ -200,6 +201,7 @@ const event_fixtures = {
op: 'remove',
message_id: 256,
emoji_name: 'angery',
user_id: "1",
user: {
id: "1",
},

39
frontend_tests/node_tests/reactions.js
View File

@@ -74,15 +74,15 @@ people.add(cali);
const message = {
id: 1001,
reactions: [
{emoji_name: 'smile', user: {id: 5}, reaction_type: 'unicode_emoji', emoji_code: '263a'},
{emoji_name: 'smile', user: {id: 6}, reaction_type: 'unicode_emoji', emoji_code: '263a'},
{emoji_name: 'frown', user: {id: 7}, reaction_type: 'unicode_emoji', emoji_code: '1f641'},
{emoji_name: 'inactive_realm_emoji', user: {id: 5}, reaction_type: 'realm_emoji',
{emoji_name: 'smile', user_id: 5, reaction_type: 'unicode_emoji', emoji_code: '263a'},
{emoji_name: 'smile', user_id: 6, reaction_type: 'unicode_emoji', emoji_code: '263a'},
{emoji_name: 'frown', user_id: 7, reaction_type: 'unicode_emoji', emoji_code: '1f641'},
{emoji_name: 'inactive_realm_emoji', user_id: 5, reaction_type: 'realm_emoji',
emoji_code: '992'},
// 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'},
{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'},
],
};
@@ -298,9 +298,7 @@ run_test('add_and_remove_reaction', () => {
reaction_type: 'unicode_emoji',
emoji_name: '8ball',
emoji_code: '1f3b1',
user: {
user_id: alice.user_id,
},
};
const message_reactions = $.create('our-reactions');
@@ -355,9 +353,7 @@ run_test('add_and_remove_reaction', () => {
reaction_type: 'unicode_emoji',
emoji_name: '8ball',
emoji_code: '1f3b1',
user: {
user_id: bob.user_id,
},
};
const count_element = $.create('count-element');
@@ -402,9 +398,7 @@ run_test('add_and_remove_reaction', () => {
reaction_type: 'realm_emoji',
emoji_name: 'realm_emoji',
emoji_code: '991',
user: {
user_id: cali.user_id,
},
};
template_called = false;
@@ -430,9 +424,7 @@ run_test('add_and_remove_reaction', () => {
reaction_type: 'realm_emoji',
emoji_name: 'realm_emoji',
emoji_code: '991',
user: {
user_id: alice.user_id,
},
};
message_reactions.find = function (selector) {
@@ -495,9 +487,7 @@ run_test('with_view_stubs', () => {
reaction_type: 'unicode_emoji',
emoji_name: '8ball',
emoji_code: '1f3b1',
user: {
user_id: alice.user_id,
},
};
const bob_8ball_event = {
@@ -505,9 +495,7 @@ run_test('with_view_stubs', () => {
reaction_type: 'unicode_emoji',
emoji_name: '8ball',
emoji_code: '1f3b1',
user: {
user_id: bob.user_id,
},
};
const cali_airplane_event = {
@@ -515,9 +503,7 @@ run_test('with_view_stubs', () => {
reaction_type: 'unicode_emoji',
emoji_name: 'airplane',
emoji_code: '2708',
user: {
user_id: cali.user_id,
},
};
test_view_calls({
@@ -627,9 +613,7 @@ run_test('error_handling', () => {
reaction_type: 'realm_emoji',
emoji_name: 'realm_emoji',
emoji_code: '991',
user: {
user_id: 99,
},
};
const original_func = reactions.current_user_has_reacted_to_emoji;
@@ -653,9 +637,7 @@ run_test('remove spurious user', () => {
emoji_name: 'frown',
emoji_code: '1f641',
message_id: message.id,
user: {
user_id: alice.user_id,
},
};
reactions.remove_reaction(event);
@@ -676,9 +658,7 @@ run_test('remove last user', () => {
emoji_name: 'frown',
emoji_code: '1f641',
message_id: message.id,
user: {
user_id: cali.user_id,
},
};
reactions.remove_reaction(event);
@@ -745,8 +725,7 @@ run_test('code coverage', () => {
};
reactions.remove_reaction({
message_id: 42,
user: {},
message_id: 42, // TODO: REACTIONS API
});
});
@@ -754,8 +733,8 @@ run_test('duplicates', () => {
const dup_reaction_message = {
id: 1001,
reactions: [
{emoji_name: 'smile', user: {id: 5}, reaction_type: 'unicode_emoji', emoji_code: '263a'},
{emoji_name: 'smile', user: {id: 5}, reaction_type: 'unicode_emoji', emoji_code: '263a'},
{emoji_name: 'smile', user_id: 5, reaction_type: 'unicode_emoji', emoji_code: '263a'},
{emoji_name: 'smile', user_id: 5, reaction_type: 'unicode_emoji', emoji_code: '263a'},
],
};

9
static/js/reactions.js
View File

@@ -41,10 +41,7 @@ function get_message(message_id) {
function create_reaction(message_id, reaction_info) {
return {
message_id: message_id,
user: {
user_id: page_params.user_id,
id: page_params.user_id,
},
local_id: exports.get_local_reaction_id(reaction_info),
reaction_type: reaction_info.reaction_type,
emoji_name: reaction_info.emoji_name,
@@ -218,7 +215,7 @@ exports.add_reaction = function (event) {
exports.set_clean_reactions(message);
const local_id = exports.get_local_reaction_id(event);
const user_id = event.user.user_id;
const user_id = event.user_id;
const r = message.clean_reactions.get(local_id);
@@ -326,7 +323,7 @@ exports.remove_reaction = function (event) {
const emoji_name = event.emoji_name;
const emoji_code = event.emoji_code;
const message_id = event.message_id;
const user_id = event.user.user_id;
const user_id = event.user_id;
const message = message_store.get(message_id);
const local_id = exports.get_local_reaction_id(event);
@@ -441,7 +438,7 @@ exports.set_clean_reactions = function (message) {
for (const reaction of message.reactions) {
const local_id = exports.get_local_reaction_id(reaction);
const user_id = reaction.user.id;
const user_id = reaction.user_id;
if (!people.is_known_user_id(user_id)) {
blueslip.warn('Unknown user_id ' + user_id +

14
templates/zerver/api/get-messages.md
View File

@@ -103,6 +103,20 @@ present in all Zulip API responses).
displayed sorted by ID.
* `is_me_message`: Whether the message is a [/me status message][status-messages]
* `reactions`: Data on any reactions to the message.
* `emoji_code`: An encoded version of the emoji's unicode codepoint.
* `emoji_name`: Name of the emoji.
* `reaction_type`: If the reaction uses a [custom emoji](/help/add-custom-emoji),
`reaction_type` will be set to `realm_emoji`.
* `user_id`: The ID of the user who added the reaction.
**Changes**: New in Zulip 2.2. The `user` object is
deprecated and will be removed in the future.
* `user`: Dictionary with data on the user who added the reaction, including
the user ID as the `id` field. **Note**: In the [events
API](/api/get-events-from-queue), this `user` dictionary
confusing had the user ID in a field called `user_id`
instead. We recommend ignoring fields other than the user
ID. **Deprecated** and to be removed in a future release
once core clients have migrated to use the `user_id` field.
* `recipient_id`: A unique ID for the set of users receiving the
message (either a stream or group of users). Useful primarily
for hashing.

4
zerver/lib/actions.py
View File

@@ -1670,6 +1670,10 @@ def notify_reaction_update(user_profile: UserProfile, message: Message,
event: Dict[str, Any] = {
'type': 'reaction',
'op': op,
'user_id': user_profile.id,
# TODO: We plan to remove this redundant user_dict object once
# clients are updated to support accessing use user_id. See
# https://github.com/zulip/zulip/pull/14711 for details.
'user': user_dict,
'message_id': message.id,
'emoji_name': reaction.emoji_name,

10
zerver/lib/message.py
View File

@@ -556,9 +556,17 @@ class ReactionDict:
return {'emoji_name': row['emoji_name'],
'emoji_code': row['emoji_code'],
'reaction_type': row['reaction_type'],
# TODO: We plan to remove this redundant user dictionary once
# clients are updated to support accessing use user_id. See
# https://github.com/zulip/zulip/pull/14711 for details.
#
# When we do that, we can likely update the `.values()` query to
# not fetch the extra user_profile__* fields from the database
# as a small performance optimization.
'user': {'email': row['user_profile__email'],
'id': row['user_profile__id'],
'full_name': row['user_profile__full_name']}}
'full_name': row['user_profile__full_name']},
'user_id': row['user_profile__id']}
def access_message(user_profile: UserProfile, message_id: int) -> Tuple[Message, Optional[UserMessage]]:

4
zerver/tests/test_events.py
View File

@@ -846,6 +846,7 @@ class EventsRegisterTest(ZulipTestCase):
('emoji_name', check_string),
('emoji_code', check_string),
('reaction_type', check_string),
('user_id', check_int),
('user', check_dict_only([
('email', check_string),
('full_name', check_string),
@@ -871,6 +872,7 @@ class EventsRegisterTest(ZulipTestCase):
('emoji_name', check_string),
('emoji_code', check_string),
('reaction_type', check_string),
('user_id', check_int),
('user', check_dict_only([
('email', check_string),
('full_name', check_string),
@@ -897,6 +899,7 @@ class EventsRegisterTest(ZulipTestCase):
('emoji_name', check_string),
('emoji_code', check_string),
('reaction_type', check_string),
('user_id', check_int),
('user', check_dict_only([
('email', check_string),
('full_name', check_string),
@@ -951,6 +954,7 @@ class EventsRegisterTest(ZulipTestCase):
('emoji_name', check_string),
('emoji_code', check_string),
('reaction_type', check_string),
('user_id', check_int),
('user', check_dict_only([
('email', check_string),
('full_name', check_string),

1
zerver/tests/test_messages.py
View File

@@ -1527,6 +1527,7 @@ class MessageDictTest(ZulipTestCase):
msg_dict = MessageDict.build_dict_from_raw_db_row(row)
self.assertEqual(msg_dict['reactions'][0]['emoji_name'],
reaction.emoji_name)
self.assertEqual(msg_dict['reactions'][0]['user_id'], sender.id)
self.assertEqual(msg_dict['reactions'][0]['user']['id'],
sender.id)
self.assertEqual(msg_dict['reactions'][0]['user']['email'],