diff --git a/frontend_tests/node_tests/people.js b/frontend_tests/node_tests/people.js index 7415cf3985..c6c2e5cf0e 100644 --- a/frontend_tests/node_tests/people.js +++ b/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(email), 32); - assert.equal(person.email, email); - person = people.get_by_email(email); + assert.equal(people.get_user_id_from_name(full_name), 32); + + let 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', diff --git a/frontend_tests/node_tests/people_errors.js b/frontend_tests/node_tests/people_errors.js index 06ccf23f6a..4e346dca34 100644 --- a/frontend_tests/node_tests/people_errors.js +++ b/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); diff --git a/static/js/markdown.js b/static/js/markdown.js index 5faf00a405..90cdcd79be 100644 --- a/static/js/markdown.js +++ b/static/js/markdown.js @@ -90,29 +90,52 @@ exports.apply_markdown = function (message) { message_store.init_booleans(message); const options = { - userMentionHandler: function (name, silently) { - if (name === 'all' || name === 'everyone' || name === 'stream') { + userMentionHandler: function (mention, silently) { + if (mention === 'all' || mention === 'everyone' || mention === 'stream') { message.mentioned = true; return '' + - '@' + name + + '@' + mention + ''; } - 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)) { - person = people.get_by_user_id(user_id); - if (person.full_name !== match[1]) { // Invalid Syntax - return; - } + /* + If we have two users named Alice, we want + users to provide mentions like this: + + 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 += ''; + str += ''; } else { - str += '@'; + str += '@'; } - return str + _.escape(person.full_name) + ''; + + // 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) + ''; }, groupMentionHandler: function (name) { const group = user_groups.get_user_group_from_name(name); diff --git a/static/js/people.js b/static/js/people.js index b22a4a4c7a..758595e85f 100644 --- a/static/js/people.js +++ b/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) { - return people_by_name_dict.get(name); +exports.is_valid_full_name_and_user_id = (full_name, user_id) => { + 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) {