reactions: Rewrite code to use clean reactions.

Before this commit, the reactions code would
take the `message.reactions` structure from
the server and try to "collapse" all the reactions
for the same users into the same reactions,
but with each reaction having a list of user_ids.
It was a strangely denormalized structure that
was awkward to work with, and it made it really
hard to reason about whether the data was in
the original structure that the server sent or
the modified structure.

Now we use a cleaner, normalized Map to keep
each reaction (i.e. one per emoji), and we
write that to `message.clean_reactions`.

The `clean_reactions` structure is now the
authoritatize source for all reaction-related
operations.  As soon as you try to do anything
with reactions, we build the `clean_reactions`
data on the fly from the server data.

In particular, when we process events, we just
directly manipulate the `clean_reactions` data,
which is much easier to work with, since it's
a Map and doesn't duplicate any data.

This rewrite should avoid some obscure bugs.

I use `r` as shorthand for the clean reaction
structures, so as not to confuse it with
data from the server's message.reactions.

It also avoids some confusion where we use
`reaction` as a var name for the reaction
elements.

frontend_tests/node_tests/reactions.js

@@ -140,8 +140,8 @@ run_test('basics', () => {
     const result = reactions.get_message_reactions(message);
     assert.equal(blueslip.get_test_logs('warn').length, 2);
     blueslip.clear_test_data();
-    assert(reactions.current_user_has_reacted_to_emoji(message, '263a', 'unicode_emoji'));
+    assert(reactions.current_user_has_reacted_to_emoji(message, 'unicode_emoji,smile,263a'));
-    assert(!reactions.current_user_has_reacted_to_emoji(message, '1f641', 'unicode_emoji'));
+    assert(!reactions.current_user_has_reacted_to_emoji(message, 'bogus'));
 
     result.sort(function (a, b) { return a.count - b.count; });
 
@@ -451,12 +451,8 @@ run_test('add_and_remove_reaction', () => {
     reaction_element.prop = function () {};
     reactions.add_reaction(alice_event);
 
-    assert(reaction_element.hasClass('reacted'));
-    blueslip.set_test_data('warn', 'Unknown user_id 8888 in reaction for message 1001');
-    blueslip.set_test_data('warn', 'Unknown user_id 9999 in reaction for message 1001');
     const result = reactions.get_message_reactions(message);
-    assert.equal(blueslip.get_test_logs('warn').length, 2);
+    assert(reaction_element.hasClass('reacted'));
-    blueslip.clear_test_data();
     const realm_emoji_data = result.filter(v => v.emoji_name === 'realm_emoji')[0];
 
     assert.equal(realm_emoji_data.count, 2);
@@ -465,7 +461,6 @@ run_test('add_and_remove_reaction', () => {
     // And then remove Alice's reaction.
     reactions.remove_reaction(alice_event);
     assert(!reaction_element.hasClass('reacted'));
-
 });
 
 run_test('with_view_stubs', () => {
@@ -660,6 +655,59 @@ run_test('error_handling', () => {
     assert.equal(blueslip.get_test_logs('error').length, 0);
 });
 
+run_test('get_name_for_alias', () => {
+    const name = reactions.get_name_for_alias(
+        message,
+        7,
+        ['grumble', 'frown', 'scowl']
+    );
+
+    assert.equal(name, 'frown');
+});
+
+message_store.get = () => message;
+
+run_test('remove spurious user', () => {
+    // get coverage for removing non-user (it should just
+    // silently fail)
+
+    const event = {
+        reaction_type: 'unicode_emoji',
+        emoji_name: 'frown',
+        emoji_code: '1f641',
+        message_id: message.id,
+        user: {
+            user_id: alice.user_id,
+        },
+    };
+
+    reactions.remove_reaction(event);
+});
+
+run_test('remove last user', () => {
+    function assert_names(names) {
+        assert.deepEqual(
+            reactions.get_message_reactions(message).map((r) => r.emoji_name),
+            names
+        );
+    }
+
+    assert_names(['smile', 'frown', 'inactive_realm_emoji', 'realm_emoji']);
+
+    const event = {
+        reaction_type: 'unicode_emoji',
+        emoji_name: 'frown',
+        emoji_code: '1f641',
+        message_id: message.id,
+        user: {
+            user_id: cali.user_id,
+        },
+    };
+    reactions.remove_reaction(event);
+
+    assert_names(['smile', 'inactive_realm_emoji', 'realm_emoji']);
+});
+
 run_test('local_reaction_id', () => {
     const reaction_info = {
         reaction_type: 'unicode_emoji',
@@ -706,3 +754,50 @@ run_test('process_reaction_click', () => {
         assert.deepEqual(args.data, expected_reaction_info);
     });
 });
+
+run_test('warnings', () => {
+    // Clean the slate
+    delete message.clean_reactions;
+    blueslip.set_test_data('warn', 'Unknown user_id 8888 in reaction for message 1001');
+    blueslip.set_test_data('warn', 'Unknown user_id 9999 in reaction for message 1001');
+    reactions.get_message_reactions(message);
+});
+
+run_test('code coverage', () => {
+    /*
+        We just silently fail in a few places in the reaction
+        code, since events may come for messages that we don't
+        have yet, or reactions may be for deactivated users, etc.
+
+        Here we just cheaply ensure 100% line coverage to make
+        it easy to enforce 100% coverage for more significant
+        code additions.
+    */
+    message_store.get = (id) => {
+        assert.equal(id, 42);
+        return {
+            reactions: [],
+        };
+    };
+
+    reactions.remove_reaction({
+        message_id: 42,
+        user: {},
+    });
+});
+
+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'},
+        ],
+    };
+
+    blueslip.clear_test_data();
+    blueslip.set_test_data(
+        'error',
+        'server sent duplicate reactions for user 5 (key=unicode_emoji,smile,263a)');
+    reactions.set_clean_reactions(dup_reaction_message);
+});

static/js/emoji_picker.js

@@ -249,12 +249,13 @@ function get_alias_to_be_used(message_id, emoji_name) {
             return;
         }
     }
+
     const user_id = page_params.user_id;
-    const reaction = message.reactions.find(
+
-        reaction => reaction.user.id === user_id && aliases.includes(reaction.emoji_name)
+    const reaction_name = reactions.get_name_for_alias(message, user_id, aliases);
-    );
+
-    if (reaction) {
+    if (reaction_name) {
-        return reaction.emoji_name;
+        return reaction_name;
     }
     return emoji_name;
 }

static/js/reactions.js

@@ -30,11 +30,11 @@ exports.open_reactions_popover = function () {
     return true;
 };
 
-exports.current_user_has_reacted_to_emoji = function (message, emoji_code, type) {
+exports.current_user_has_reacted_to_emoji = function (message, local_id) {
-    const user_id = page_params.user_id;
+    exports.set_clean_reactions(message);
-    return message.reactions.some(r => r.user.id === user_id &&
+
-           r.reaction_type === type &&
+    const r = message.clean_reactions.get(local_id);
-           r.emoji_code === emoji_code);
+    return r && r.user_ids.includes(page_params.user_id);
 };
 
 function get_message(message_id) {
@@ -44,6 +44,7 @@ function get_message(message_id) {
         return;
     }
 
+    exports.set_clean_reactions(message);
     return message;
 }
 
@@ -63,10 +64,10 @@ function create_reaction(message_id, reaction_info) {
 
 function update_ui_and_send_reaction_ajax(message_id, reaction_info) {
     const message = get_message(message_id);
+    const local_id = exports.get_local_reaction_id(reaction_info);
     const has_reacted = exports.current_user_has_reacted_to_emoji(
         message,
-        reaction_info.emoji_code,
+        local_id
-        reaction_info.reaction_type
     );
     const operation = has_reacted ? 'remove' : 'add';
     const reaction = create_reaction(message_id, reaction_info);
@@ -99,12 +100,10 @@ function update_ui_and_send_reaction_ajax(message_id, reaction_info) {
 }
 
 function get_user_list_for_message_reaction(message, local_id) {
-    const matching_reactions = message.reactions.filter(function (reaction) {
+    exports.set_clean_reactions(message);
-        return reaction.local_id === local_id;
+
-    });
+    const r = message.clean_reactions.get(local_id);
-    return matching_reactions.map(function (reaction) {
+    return r.user_ids || [];
-        return reaction.user.id;
-    });
 }
 
 exports.toggle_emoji_reaction = function (message_id, emoji_name) {
@@ -170,6 +169,7 @@ function generate_title(emoji_name, user_ids) {
 // Add a tooltip showing who reacted to a message.
 exports.get_reaction_title_data = function (message_id, local_id) {
     const message = get_message(message_id);
+
     const user_list = get_user_list_for_message_reaction(message, local_id);
     const emoji_name = exports.get_reaction_info(local_id).emoji_name;
     const title = generate_title(emoji_name, user_list);
@@ -211,29 +211,41 @@ exports.add_reaction = function (event) {
         return;
     }
 
-    const reacted = exports.current_user_has_reacted_to_emoji(message,
+    exports.set_clean_reactions(message);
-                                                              event.emoji_code,
+
-                                                              event.reaction_type);
+    const local_id = exports.get_local_reaction_id(event);
-    if (reacted && event.user.user_id === page_params.user_id) {
+    const user_id = event.user.user_id;
+
+    const r = message.clean_reactions.get(local_id);
+
+    if (r && r.user_ids.includes(user_id)) {
         return;
     }
 
-    event.user.id = event.user.user_id;
+    if (r) {
-    event.local_id = exports.get_local_reaction_id(event);
+        r.user_ids.push(user_id);
-
+        exports.update_user_fields(r);
-    message.reactions.push(event);
+    } else {
-
+        exports.add_clean_reaction({
-    const user_list = get_user_list_for_message_reaction(message, event.local_id);
+            message: message,
-    const opts = {
+            local_id: local_id,
-        message_id: event.message_id,
+            user_ids: [user_id],
             reaction_type: event.reaction_type,
             emoji_name: event.emoji_name,
             emoji_code: event.emoji_code,
-        user_id: event.user.id,
+        });
+    }
+
+    const opts = {
+        message_id: message_id,
+        reaction_type: event.reaction_type,
+        emoji_name: event.emoji_name,
+        emoji_code: event.emoji_code,
+        user_id: user_id,
     };
 
-    if (user_list.length > 1) {
+    if (r) {
-        opts.user_list = user_list;
+        opts.user_list = r.user_ids;
         exports.view.update_existing_reaction(opts);
     } else {
         exports.view.insert_new_reaction(opts);
@@ -311,7 +323,6 @@ exports.remove_reaction = function (event) {
     const emoji_code = event.emoji_code;
     const message_id = event.message_id;
     const user_id = event.user.user_id;
-    let i = -1;
     const message = message_store.get(message_id);
     const local_id = exports.get_local_reaction_id(event);
 
@@ -322,34 +333,31 @@ exports.remove_reaction = function (event) {
         return;
     }
 
-    const not_reacted = !exports.current_user_has_reacted_to_emoji(message,
+    exports.set_clean_reactions(message);
-                                                                   emoji_code,
+
-                                                                   reaction_type);
+    const r = message.clean_reactions.get(local_id);
-    if (not_reacted && event.user.user_id === page_params.user_id) {
+
+    if (!r) {
         return;
     }
 
-    // Do the data part first:
+    if (!r.user_ids.includes(user_id)) {
-    // Remove reactions from our message object.
+        return;
-    for (const [index, reaction] of message.reactions.entries()) {
-        if (reaction.local_id === local_id && reaction.user.id === user_id) {
-            i = index;
-        }
     }
 
-    if (i !== -1) {
+    r.user_ids = r.user_ids.filter(id => id !== user_id);
-        message.reactions.splice(i, 1);
+    if (r.user_ids.length > 0) {
+        exports.update_user_fields(r);
+    } else {
+        message.clean_reactions.delete(local_id);
     }
 
-    // Compute the new user list for this reaction.
-    const user_list = get_user_list_for_message_reaction(message, local_id);
-
     exports.view.remove_reaction({
         message_id: message_id,
         reaction_type: reaction_type,
         emoji_name: emoji_name,
         emoji_code: emoji_code,
-        user_list: user_list,
+        user_list: r.user_ids,
         user_id: user_id,
     });
 };
@@ -389,56 +397,136 @@ exports.view.remove_reaction = function (opts) {
 exports.get_emojis_used_by_user_for_message_id = function (message_id) {
     const user_id = page_params.user_id;
     const message = message_store.get(message_id);
-    const reactions_by_user = message.reactions.filter(function (reaction) {
+    exports.set_clean_reactions(message);
-        return reaction.user.id === user_id;
+
-    });
+    const names = [];
-    return reactions_by_user.map(function (reaction) {
+    for (const r of message.clean_reactions.values()) {
-        return reaction.emoji_name;
+        if (r.user_ids.includes(user_id)) {
-    });
+            names.push(r.emoji_name);
+        }
+    }
+
+    return names;
+};
+
+exports.get_name_for_alias = function (message, user_id, aliases) {
+    exports.set_clean_reactions(message);
+
+    for (const r of message.clean_reactions.values()) {
+        if (aliases.includes(r.emoji_name)) {
+            if (r.user_ids.includes(user_id)) {
+                return r.emoji_name;
+            }
+        }
+    }
 };
 
 exports.get_message_reactions = function (message) {
-    const message_reactions = new Map();
+    exports.set_clean_reactions(message);
+    return Array.from(message.clean_reactions.values());
+};
+
+exports.set_clean_reactions = function (message) {
+    /*
+        The server sends us a single structure for
+        each reaction, even if two users are reacting
+        with the same emoji.  Our first loop creates
+        a map of distinct reactions and a map of
+        local_id -> user_ids.  The `local_id` is
+        basically a key for the emoji name.
+
+        Then in our second loop we build a more compact
+        data structure that's easier for our message
+        list view templates to work with.
+    */
+
+    if (message.clean_reactions) {
+        return;
+    }
+
+    const distinct_reactions = new Map();
+    const user_map = new Map();
 
     for (const reaction of message.reactions) {
+        const local_id = exports.get_local_reaction_id(reaction);
         const user_id = reaction.user.id;
-        reaction.local_id = exports.get_local_reaction_id(reaction);
+
         if (!people.is_known_user_id(user_id)) {
             blueslip.warn('Unknown user_id ' + user_id +
                           ' in reaction for message ' + message.id);
             continue;
         }
-        reaction.user_ids = [];
+
-        let collapsed_reaction = message_reactions.get(reaction.local_id);
+        if (!distinct_reactions.has(local_id)) {
-        if (collapsed_reaction === undefined) {
+            distinct_reactions.set(local_id, reaction);
-            collapsed_reaction = { ...reaction };
+            user_map.set(local_id, []);
-            delete collapsed_reaction.user;
-            message_reactions.set(reaction.local_id, collapsed_reaction);
-        }
-        collapsed_reaction.user_ids.push(user_id);
         }
 
-    const reactions = Array.from(message_reactions.values(), reaction => {
+        const user_ids = user_map.get(local_id);
-        reaction.local_id = reaction.local_id;
-        reaction.reaction_type = reaction.reaction_type;
-        reaction.emoji_name = reaction.emoji_name;
-        reaction.emoji_code = reaction.emoji_code;
-        reaction.count = reaction.user_ids.length;
-        reaction.label = generate_title(reaction.emoji_name, reaction.user_ids);
-        reaction.emoji_alt_code = page_params.emojiset === 'text';
 
-        if (reaction.reaction_type !== 'unicode_emoji') {
+        if (user_ids.includes(user_id)) {
-            reaction.is_realm_emoji = true;
+            blueslip.error('server sent duplicate reactions for user ' +
-            reaction.url = emoji.all_realm_emojis.get(reaction.emoji_code).emoji_url;
+                user_id + ' (key=' + local_id + ')');
+            continue;
         }
-        if (reaction.user_ids.includes(page_params.user_id)) {
+
-            reaction.class = "message_reaction reacted";
+        user_ids.push(user_id);
-        } else {
-            reaction.class = "message_reaction";
     }
-        return reaction;
+
+    /*
+        It might feel a little janky to attach clean_reactions
+        directly to the message object, but this allows the
+        server to send us a new copy of the message, and then
+        the next time we try to get reactions from it, we
+        won't have `clean_reactions`, and we will re-process
+        the server's latest copy of the reactions.
+    */
+    message.clean_reactions = new Map();
+
+    for (const local_id of distinct_reactions.keys()) {
+        const reaction = distinct_reactions.get(local_id);
+        const user_ids = user_map.get(local_id);
+
+        exports.add_clean_reaction({
+            message: message,
+            local_id: local_id,
+            user_ids: user_ids,
+            reaction_type: reaction.reaction_type,
+            emoji_name: reaction.emoji_name,
+            emoji_code: reaction.emoji_code,
         });
-    return reactions;
+    }
+};
+
+exports.add_clean_reaction = function (opts) {
+    const r = {};
+
+    r.reaction_type = opts.reaction_type;
+    r.emoji_name = opts.emoji_name;
+    r.emoji_code = opts.emoji_code;
+    r.local_id = opts.local_id;
+
+    r.user_ids = opts.user_ids;
+    exports.update_user_fields(r);
+
+    r.emoji_alt_code = page_params.emojiset === 'text';
+
+    if (r.reaction_type !== 'unicode_emoji') {
+        r.is_realm_emoji = true;
+        r.url = emoji.all_realm_emojis.get(r.emoji_code).emoji_url;
+    }
+
+    opts.message.clean_reactions.set(opts.local_id, r);
+};
+
+exports.update_user_fields = function (r) {
+    r.count = r.user_ids.length;
+    r.label = generate_title(r.emoji_name, r.user_ids);
+    if (r.user_ids.includes(page_params.user_id)) {
+        r.class = "message_reaction reacted";
+    } else {
+        r.class = "message_reaction";
+    }
 };
 
 window.reactions = exports;