markdown: Clean up userMentionHandler().

This mostly moves logic into people.js.
The people functions added here are glorified
two-liners.

One thing that changes here is that we
are a bit more rigorous about duplicate
names.

The code is slightly awkward, because this
commit preserves the strange behavior
that if 'alice|42' doesn't match on
the user with the name "alice" and user_id
"42", we instead look for a user whose
name is "alice|42".  That seems like a
misfeature to me, but there's a test for
it, so I want to check with Tim that it's not
intentional behavior before I simplify
the code.

frontend_tests/node_tests/people.js

@@ -56,15 +56,23 @@ run_test('basics', () => {
     const email = 'isaac@example.com';
 
     assert(!people.is_known_user_id(32));
+    assert(!people.is_valid_full_name_and_user_id(full_name, 32));
+    assert.equal(people.get_user_id_from_name(full_name), undefined);
+
     people.add(isaac);
 
+    assert.equal(
+        people.get_actual_name_from_user_id(32),
+        full_name
+    );
+
+    assert(people.is_valid_full_name_and_user_id(full_name, 32));
     assert(people.is_known_user_id(32));
     assert.equal(people.get_realm_count(), 1);
 
-    let person = people.get_by_name(full_name);
+    assert.equal(people.get_user_id_from_name(full_name), 32);
-    assert.equal(people.get_user_id(email), 32);
+
-    assert.equal(person.email, email);
+    let person = people.get_by_email(email);
-    person = people.get_by_email(email);
     assert.equal(person.full_name, full_name);
     person = people.get_active_user_for_email(email);
     assert(!person);
@@ -231,7 +239,7 @@ run_test('updates', () => {
     people.set_full_name(person, 'Me the Third');
     assert.equal(people.my_full_name(), 'Me the Third');
     assert.equal(person.full_name, 'Me the Third');
-    assert.equal(people.get_by_name('Me the Third').email, 'me@example.com');
+    assert.equal(people.get_user_id_from_name('Me the Third'), me.user_id);
 });
 
 run_test('get_by_user_id', () => {
@@ -810,9 +818,27 @@ run_test('track_duplicate_full_names', () => {
         user_id: 603,
         full_name: 'Maria Athens',
     };
-    people.add(stephen1);
+
-    people.add(stephen2);
     people.add(maria);
+    people.add(stephen1);
+
+    assert(!people.is_duplicate_full_name('Stephen King'));
+    assert.equal(
+        people.get_user_id_from_name('Stephen King'),
+        stephen1.user_id
+    );
+
+    // Now duplicate the Stephen King name.
+    people.add(stephen2);
+
+    // For duplicate names we won't try to guess which
+    // user_id the person means; the UI should use
+    // other codepaths for disambiguation.
+    assert.equal(
+        people.get_user_id_from_name('Stephen King'),
+        undefined
+    );
+
     assert(people.is_duplicate_full_name('Stephen King'));
     assert(!people.is_duplicate_full_name('Maria Athens'));
     assert(!people.is_duplicate_full_name('Some Random Name'));
@@ -989,6 +1015,14 @@ run_test('email_for_user_settings', () => {
     assert.equal(email(isaac), isaac.email);
 });
 
+initialize();
+
+run_test('is_valid_full_name_and_user_id', () => {
+    assert(!people.is_valid_full_name_and_user_id('bogus', 99));
+    assert(!people.is_valid_full_name_and_user_id(me.full_name, 99));
+    assert(people.is_valid_full_name_and_user_id(me.full_name, me.user_id));
+});
+
 run_test('emails_strings_to_user_ids_array', function () {
     const steven = {
         email: 'steven@example.com',

frontend_tests/node_tests/people_errors.js

@@ -52,6 +52,11 @@ run_test('blueslip', () => {
     assert.equal(blueslip.get_test_logs('debug').length, 1);
     blueslip.clear_test_data();
 
+    blueslip.set_test_data('error', 'Unknown user_id: 9999');
+    people.get_actual_name_from_user_id(9999);
+    assert.equal(blueslip.get_test_logs('error').length, 1);
+    blueslip.clear_test_data();
+
     blueslip.set_test_data('error', 'Unknown email for get_user_id: ' + unknown_email);
     people.get_user_id(unknown_email);
     assert.equal(blueslip.get_test_logs('error').length, 1);

static/js/markdown.js

@@ -90,29 +90,52 @@ exports.apply_markdown = function (message) {
     message_store.init_booleans(message);
 
     const options = {
-        userMentionHandler: function (name, silently) {
+        userMentionHandler: function (mention, silently) {
-            if (name === 'all' || name === 'everyone' || name === 'stream') {
+            if (mention === 'all' || mention === 'everyone' || mention === 'stream') {
                 message.mentioned = true;
                 return '<span class="user-mention" data-user-id="*">' +
-                       '@' + name +
+                       '@' + mention +
                        '</span>';
             }
 
-            let person = people.get_by_name(name);
+            let full_name;
+            let user_id;
 
             const id_regex = /(.+)\|(\d+)$/g; // For @**user|id** syntax
-            const match = id_regex.exec(name);
+            const match = id_regex.exec(mention);
+
             if (match) {
-                const user_id = parseInt(match[2], 10);
+                /*
-                if (people.is_known_user_id(user_id)) {
+                    If we have two users named Alice, we want
-                    person = people.get_by_user_id(user_id);
+                    users to provide mentions like this:
-                    if (person.full_name !== match[1]) { // Invalid Syntax
+
-                        return;
+                        alice|42
-                    }
+                        alice|99
+
+                    The autocomplete feature will help users
+                    send correct mentions for duplicate names,
+                    but we also have to consider the possibility
+                    that the user will hand-type something
+                    incorrectly, in which case we'll fall
+                    through to the other code (which may be a
+                    misfeature).
+                */
+                full_name = match[1];
+                user_id = parseInt(match[2], 10);
+
+                if (!people.is_valid_full_name_and_user_id(full_name, user_id)) {
+                    user_id = undefined;
+                    full_name = undefined;
                 }
             }
 
-            if (!person) {
+            if (user_id === undefined) {
+                // Handle normal syntax
+                full_name = mention;
+                user_id = people.get_user_id_from_name(full_name);
+            }
+
+            if (user_id === undefined) {
                 // This is nothing to be concerned about--the users
                 // are allowed to hand-type mentions and they may
                 // have had a typo in the name.
@@ -124,17 +147,21 @@ exports.apply_markdown = function (message) {
             // flags on the message itself that get used by the message
             // view code and possibly our filtering code.
 
-            if (people.my_current_user_id() === person.user_id && !silently) {
+            if (people.my_current_user_id() === user_id && !silently) {
                 message.mentioned = true;
                 message.mentioned_me_directly = true;
             }
             let str = '';
             if (silently) {
-                str += '<span class="user-mention silent" data-user-id="' + person.user_id + '">';
+                str += '<span class="user-mention silent" data-user-id="' + user_id + '">';
             } else {
-                str += '<span class="user-mention" data-user-id="' + person.user_id + '">@';
+                str += '<span class="user-mention" data-user-id="' + user_id + '">@';
             }
-            return str + _.escape(person.full_name) + '</span>';
+
+            // If I mention "@aLiCe sMITH", I still want "Alice Smith" to
+            // show in the pill.
+            const actual_full_name = people.get_actual_name_from_user_id(user_id);
+            return str + _.escape(actual_full_name) + '</span>';
         },
         groupMentionHandler: function (name) {
             const group = user_groups.get_user_group_from_name(name);

static/js/people.js

@@ -836,8 +836,59 @@ exports.filter_people_by_search_terms = function (users, search_terms) {
     return filtered_users;
 };
 
-exports.get_by_name = function (name) {
+exports.is_valid_full_name_and_user_id = (full_name, user_id) => {
-    return people_by_name_dict.get(name);
+    const person = people_by_user_id_dict.get(user_id);
+
+    if (!person) {
+        return false;
+    }
+
+    return person.full_name === full_name;
+};
+
+exports.get_actual_name_from_user_id = (user_id) => {
+    /*
+        If you are dealing with user-entered data, you
+        should validate the user_id BEFORE calling
+        this function.
+    */
+    const person = people_by_user_id_dict.get(user_id);
+
+    if (!person) {
+        blueslip.error('Unknown user_id: ' + user_id);
+        return;
+    }
+
+    return person.full_name;
+};
+
+exports.get_user_id_from_name = function (full_name) {
+    // get_user_id_from_name('Alice Smith') === 42
+
+    /*
+        This function is intended to be called
+        with a full name that is user-entered, such
+        a full name from a user mention.
+
+        We will only return a **unique** user_id
+        here.  For duplicate names, our UI should
+        force users to disambiguate names with a
+        user_id and then call is_valid_full_name_and_user_id
+        to make sure the combo is valid.  This is
+        exactly what we do with mentions.
+    */
+
+    const person = people_by_name_dict.get(full_name);
+
+    if (!person) {
+        return;
+    }
+
+    if (exports.is_duplicate_full_name(full_name)) {
+        return;
+    }
+
+    return person.user_id;
 };
 
 function people_cmp(person1, person2) {