Repository: zulip/zulip-terminal

Issue:
Create extended user mention syntax when needed.
Recently, in zulip/zulip#10243 we merged support for the following mention syntax to zulip/zulip:

`@**Full Name|1234(id)**`

This is to allow us to mention users with the same full name, and is optional. The web app intelligently adds the new syntax only when the users have the same full name, and uses the simpler syntax for others.

Example: https://chat.zulip.org/#narrow/stream/7-test-here/topic/user.20mention

We should:

1. Add support for this syntax in the terminal client.
2. Ensure that it is behind a feature flag because it would break mentions in older servers/any non CZO server for now.

Discussion:
prah23: @neiljp I haven't removed the "`Tests updated.`" part of the commit text from the previous iteration, where I've appended `user_id`s to all the names in the list. I'm adding tests and will update you once the PR is ready. Also, I maintain a separate list of duplicate names, which I check with before appending `user_id`s to the final list that is passed to the typeahead, so the duplicates check does not rely on adjacency, or any particular order for that matter.

The duplicates list is formed by checking if the current name in the loop exists in the sub-list of elements leading to the current name. I've run this loop on the `user_names` list.
prah23: @neiljp I haven't removed the "`Tests updated.`" part of the commit text from the previous iteration, where I've appended `user_id`s to all the names in the list. I'm currently adding tests and will update you once the PR is ready. Also, I maintain a separate list of duplicate names, which I check with before appending `user_id`s to the final list that is passed to the typeahead, so the duplicates check does not rely on adjacency, or any particular order for that matter.

The duplicates list is formed by checking if the current name in the loop exists in the sub-list of elements leading to the current name. I've run this loop on the `user_names` list.
prah23: @neiljp I haven't removed the "`Tests updated.`" part of the commit text from the previous iteration, where I've appended `user_id`s to all the names in the list. I'm currently adding tests and will update you once the PR is ready. Also, I maintain a separate list of duplicate names, which I check with before appending `user_id`s to the final list that is passed to the typeahead, so the duplicates check does not rely on adjacency or any particular order for that matter.

The duplicates list is formed by checking if the current name in the loop exists in the sub-list of elements leading to the current name. I've run this loop on the `user_names` list.
neiljp: Re adjacency, I was referring to the sub-list check you indicated. If the list is sorted, can we rely upon the previous entry only? Alternatively, it might be clearer to use Counter. In any case, once we have tests then this will be easier to explore.
prah23: @zulipbot remove "PR awaiting update"
@zulipbot remove "PR needs review"
zulipbot: **ERROR:** Label "PR needs review" does not exist and was thus not removed from this pull request.
prah23: @zulipbot add "PR needs review"
prah23: @zulipbot remove "PR awaiting update"
prah23: @zulipbot remove "PR awaiting update"
@zulipbot add "PR needs review"
zulipbot: Hello @prah23, it seems like you have referenced #151 in your pull request description, but you have not referenced them in your commit message description(s). When you reference an issue in a commit message, it automatically closes the corresponding issue when the commit is merged.

Please run `git commit --amend` in your command line client to amend your commit message description with `Fixes #151.`.

An example of a correctly-formatted commit:
```
commit fabd5e450374c8dde65ec35f02140383940fe146
Author: zulipbot
Date:   Sat Mar 18 13:42:40 2017 -0700

    pull requests: Check PR commits reference when issue is referenced.

    Fixes #51.
```

Thank you for your contributions to Zulip!

<!-- fixCommitWarning -->
prah23: @neiljp this has been `black` rebased, is up-to-date, and ready for another review.
neiljp: @prah23 Thanks for working on this - I'm going to merge this now :tada:

We could debate the best test structure, but having the feature there and tested is good and we can always iterate from there :+1:

Patch:
diff --git a/zulipterminal/ui_tools/boxes.py b/zulipterminal/ui_tools/boxes.py
index 85a50a5..fab9cf9 100644
--- a/zulipterminal/ui_tools/boxes.py
+++ b/zulipterminal/ui_tools/boxes.py
@@ -1,7 +1,7 @@
 import re
 import typing
 import unicodedata
-from collections import OrderedDict, defaultdict
+from collections import Counter, OrderedDict, defaultdict
 from datetime import date, datetime, timedelta
 from sys import platform
 from time import sleep, time
@@ -457,12 +457,27 @@ class WriteBox(urwid.Pile):
         )
 
         user_names = [user["full_name"] for user in sorted_matching_users]
+
+        # Counter holds a count of each name in the list of users' names in a
+        # dict-like manner, which is a more efficient approach when compared to
+        # slicing the original list on each name.
+        # FIXME: Use a persistent counter rather than generate one on each autocomplete.
+        user_names_counter = Counter(user_names)
+
+        # Append user_id's to users with the same names.
+        user_names_with_distinct_duplicates = [
+            f"{user['full_name']}|{user['user_id']}"
+            if user_names_counter[user["full_name"]] > 1
+            else user["full_name"]
+            for user in sorted_matching_users
+        ]
+
         extra_prefix = "{}{}".format(
             "*" if prefix_string[-1] != "*" else "",
             "*" if prefix_string[-2:] != "**" else "",
         )
         user_typeahead = format_string(
-            user_names, prefix_string + extra_prefix + "{}**"
+            user_names_with_distinct_duplicates, prefix_string + extra_prefix + "{}**"
         )
 
         return user_typeahead, user_names


Test patch:
diff --git a/tests/conftest.py b/tests/conftest.py
index 04e70ef..bec5fa7 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -105,6 +105,20 @@ def users_fixture(logged_on_user):
                 "is_admin": False,
             }
         )
+    # Also add two users with the same name.
+    for i in range(1, 3):
+        users.append(
+            {
+                "user_id": 12 + i,
+                "full_name": "Human Duplicate",
+                "email": f"personduplicate{i}@example.com",
+                "avatar_url": None,
+                "is_active": True,
+                "bot_type": None,
+                "is_bot": False,
+                "is_admin": False,
+            }
+        )
     return users
 
 
@@ -811,6 +825,18 @@ def user_dict(logged_on_user):
             "user_id": 12,
             "status": "inactive",
         },
+        "personduplicate1@example.com": {
+            "full_name": "Human Duplicate",
+            "email": "personduplicate1@example.com",
+            "user_id": 13,
+            "status": "inactive",
+        },
+        "personduplicate2@example.com": {
+            "full_name": "Human Duplicate",
+            "email": "personduplicate2@example.com",
+            "user_id": 14,
+            "status": "inactive",
+        },
         "emailgateway@zulip.com": {
             "email": "emailgateway@zulip.com",
             "full_name": "Email Gateway",
@@ -870,6 +896,18 @@ def user_list(logged_on_user):
             "user_id": 12,
             "status": "inactive",
         },
+        {
+            "full_name": "Human Duplicate",
+            "email": "personduplicate1@example.com",
+            "user_id": 13,
+            "status": "inactive",
+        },
+        {
+            "full_name": "Human Duplicate",
+            "email": "personduplicate2@example.com",
+            "user_id": 14,
+            "status": "inactive",
+        },
         {
             "email": "notification-bot@zulip.com",
             "full_name": "Notification Bot",
diff --git a/tests/ui_tools/test_boxes.py b/tests/ui_tools/test_boxes.py
index e83242e..e95f8d5 100644
--- a/tests/ui_tools/test_boxes.py
+++ b/tests/ui_tools/test_boxes.py
@@ -101,6 +101,8 @@ class TestWriteBox:
                     "Human Myself",
                     "Human 1",
                     "Human 2",
+                    "Human Duplicate",
+                    "Human Duplicate",
                     "Group 1",
                     "Group 2",
                     "Group 3",
@@ -108,14 +110,74 @@ class TestWriteBox:
                 ],
             ),
             ("@*", 0, ["Group 1", "Group 2", "Group 3", "Group 4"]),
-            ("@**", 0, ["Human Myself", "Human 1", "Human 2"]),
+            (
+                "@**",
+                0,
+                [
+                    "Human Myself",
+                    "Human 1",
+                    "Human 2",
+                    "Human Duplicate",
+                    "Human Duplicate",
+                ],
+            ),
             # mentions
-            ("@Human", 0, ["Human Myself", "Human 1", "Human 2"]),
-            ("@**Human", 0, ["Human Myself", "Human 1", "Human 2"]),
-            ("@_Human", 0, ["Human Myself", "Human 1", "Human 2"]),
+            (
+                "@Human",
+                0,
+                [
+                    "Human Myself",
+                    "Human 1",
+                    "Human 2",
+                    "Human Duplicate",
+                    "Human Duplicate",
+                ],
+            ),
+            (
+                "@**Human",
+                0,
+                [
+                    "Human Myself",
+                    "Human 1",
+                    "Human 2",
+                    "Human Duplicate",
+                    "Human Duplicate",
+                ],
+            ),
+            (
+                "@_Human",
+                0,
+                [
+                    "Human Myself",
+                    "Human 1",
+                    "Human 2",
+                    "Human Duplicate",
+                    "Human Duplicate",
+                ],
+            ),
             ("@_*Human", None, []),  # NOTE: Optional single star fails
-            ("@_**Human", 0, ["Human Myself", "Human 1", "Human 2"]),
-            ("@Human", None, ["Human Myself", "Human 1", "Human 2"]),
+            (
+                "@_**Human",
+                0,
+                [
+                    "Human Myself",
+                    "Human 1",
+                    "Human 2",
+                    "Human Duplicate",
+                    "Human Duplicate",
+                ],
+            ),
+            (
+                "@Human",
+                None,
+                [
+                    "Human Myself",
+                    "Human 1",
+                    "Human 2",
+                    "Human Duplicate",
+                    "Human Duplicate",
+                ],
+            ),
             ("@NoMatch", None, []),
             # streams
             (
@@ -159,13 +221,19 @@ class TestWriteBox:
             ("@Human", 0, "@**Human Myself**"),
             ("@Human", 1, "@**Human 1**"),
             ("@Human", 2, "@**Human 2**"),
-            ("@Human", -1, "@**Human 2**"),
-            ("@Human", -2, "@**Human 1**"),
-            ("@Human", -3, "@**Human Myself**"),
-            ("@Human", -4, None),
+            ("@Human", 3, "@**Human Duplicate|13**"),
+            ("@Human", 4, "@**Human Duplicate|14**"),
+            ("@Human", -1, "@**Human Duplicate|14**"),
+            ("@Human", -2, "@**Human Duplicate|13**"),
+            ("@Human", -3, "@**Human 2**"),
+            ("@Human", -4, "@**Human 1**"),
+            ("@Human", -5, "@**Human Myself**"),
+            ("@Human", -6, None),
             ("@_Human", 0, "@_**Human Myself**"),
             ("@_Human", 1, "@_**Human 1**"),
             ("@_Human", 2, "@_**Human 2**"),
+            ("@_Human", 3, "@_**Human Duplicate|13**"),
+            ("@_Human", 4, "@_**Human Duplicate|14**"),
             ("@H", 1, "@**Human 1**"),
             ("@Hu", 1, "@**Human 1**"),
             ("@Hum", 1, "@**Human 1**"),
@@ -192,18 +260,22 @@ class TestWriteBox:
             ("@", 0, "@**Human Myself**"),
             ("@", 1, "@**Human 1**"),
             ("@", 2, "@**Human 2**"),
-            ("@", 3, "@*Group 1*"),
-            ("@", 4, "@*Group 2*"),
-            ("@", 5, "@*Group 3*"),
-            ("@", 6, "@*Group 4*"),
-            ("@", 7, None),  # Reached last match
-            ("@", 8, None),  # Beyond end
+            ("@", 3, "@**Human Duplicate|13**"),
+            ("@", 4, "@**Human Duplicate|14**"),
+            ("@", 5, "@*Group 1*"),
+            ("@", 6, "@*Group 2*"),
+            ("@", 7, "@*Group 3*"),
+            ("@", 8, "@*Group 4*"),
+            ("@", 9, None),  # Reached last match
+            ("@", 10, None),  # Beyond end
             # Expected sequence of autocompletes from '@**' (no groups)
             ("@**", 0, "@**Human Myself**"),
             ("@**", 1, "@**Human 1**"),
             ("@**", 2, "@**Human 2**"),
-            ("@**", 3, None),  # Reached last match
-            ("@**", 4, None),  # Beyond end
+            ("@", 3, "@**Human Duplicate|13**"),
+            ("@", 4, "@**Human Duplicate|14**"),
+            ("@**", 5, None),  # Reached last match
+            ("@**", 6, None),  # Beyond end
             # Expected sequence of autocompletes from '@*' (only groups)
             ("@*", 0, "@*Group 1*"),
             ("@*", 1, "@*Group 2*"),
@@ -215,9 +287,11 @@ class TestWriteBox:
             ("@_", 0, "@_**Human Myself**"),  # NOTE: No silent group mention
             ("@_", 1, "@_**Human 1**"),
             ("@_", 2, "@_**Human 2**"),
-            ("@_", 3, None),  # Reached last match
-            ("@_", 4, None),  # Beyond end
-            ("@_", -1, "@_**Human 2**"),
+            ("@_", 3, "@_**Human Duplicate|13**"),
+            ("@_", 4, "@_**Human Duplicate|14**"),
+            ("@_", 5, None),  # Reached last match
+            ("@_", 6, None),  # Beyond end
+            ("@_", -1, "@_**Human Duplicate|14**"),
             # Complex autocomplete prefixes.
             ("(@H", 0, "(@**Human Myself**"),
             ("(@H", 1, "(@**Human 1**"),
@@ -266,6 +340,41 @@ class TestWriteBox:
         typeahead_string = write_box.generic_autocomplete(text, state)
         assert typeahead_string == required_typeahead
 
+    @pytest.mark.parametrize(
+        "text, expected_distinct_prefix",
+        # Add 3 different lists of tuples, with each tuple containing a combination
+        # of the text to be autocompleted and the corresponding typeahead prefix to
+        # be added to the typeahead suggestions. Only the "@" case has to be ignored
+        # while building the parameters, because it includes group suggestions too.
+        [("@" + "Human"[: index + 1], "@") for index in range(len("Human"))]
+        + [("@**" + "Human"[:index], "@") for index in range(len("Human") + 1)]
+        + [("@_" + "Human"[:index], "@_") for index in range(len("Human") + 1)],
+    )
+    def test_generic_autocomplete_user_mentions(
+        self, write_box, mocker, text, expected_distinct_prefix, state=1
+    ):
+        _process_typeaheads = mocker.patch(BOXES + ".WriteBox._process_typeaheads")
+
+        write_box.generic_autocomplete(text, state)
+
+        matching_users = [
+            "Human Myself",
+            "Human 1",
+            "Human 2",
+            "Human Duplicate",
+            "Human Duplicate",
+        ]
+        distinct_matching_users = [
+            expected_distinct_prefix + "**Human Myself**",
+            expected_distinct_prefix + "**Human 1**",
+            expected_distinct_prefix + "**Human 2**",
+            expected_distinct_prefix + "**Human Duplicate|13**",
+            expected_distinct_prefix + "**Human Duplicate|14**",
+        ]
+        _process_typeaheads.assert_called_once_with(
+            distinct_matching_users, state, matching_users
+        )
+
     @pytest.mark.parametrize(
         "text, state, required_typeahead, to_pin",
         [
@@ -383,11 +492,19 @@ class TestWriteBox:
         [
             (
                 "",
-                ["Human Myself", "Human 1", "Human 2"],
+                [
+                    "Human Myself",
+                    "Human 1",
+                    "Human 2",
+                    "Human Duplicate",
+                    "Human Duplicate",
+                ],
                 [
                     "Human Myself <FOOBOO@gmail.com>",
                     "Human 1 <person1@example.com>",
                     "Human 2 <person2@example.com>",
+                    "Human Duplicate <personduplicate1@example.com>",
+                    "Human Duplicate <personduplicate2@example.com>",
                 ],
             ),
             ("My", ["Human Myself"], ["Human Myself <FOOBOO@gmail.com>"]),
@@ -436,7 +553,13 @@ class TestWriteBox:
         [
             (
                 "Welcome Bot <welcome-bot@zulip.com>, Human",
-                ["Human Myself", "Human 1", "Human 2"],
+                [
+                    "Human Myself",
+                    "Human 1",
+                    "Human 2",
+                    "Human Duplicate",
+                    "Human Duplicate",
+                ],
                 [
                     "Welcome Bot <welcome-bot@zulip.com>, "
                     "Human Myself <FOOBOO@gmail.com>",
@@ -444,6 +567,10 @@ class TestWriteBox:
                     "Human 1 <person1@example.com>",
                     "Welcome Bot <welcome-bot@zulip.com>, "
                     "Human 2 <person2@example.com>",
+                    "Welcome Bot <welcome-bot@zulip.com>, "
+                    "Human Duplicate <personduplicate1@example.com>",
+                    "Welcome Bot <welcome-bot@zulip.com>, "
+                    "Human Duplicate <personduplicate2@example.com>",
                 ],
             ),
             (
@@ -457,7 +584,13 @@ class TestWriteBox:
             ),
             (
                 "Email Gateway <emailgateway@zulip.com>,Human",
-                ["Human Myself", "Human 1", "Human 2"],
+                [
+                    "Human Myself",
+                    "Human 1",
+                    "Human 2",
+                    "Human Duplicate",
+                    "Human Duplicate",
+                ],
                 [
                     "Email Gateway <emailgateway@zulip.com>, "
                     "Human Myself <FOOBOO@gmail.com>",
@@ -465,6 +598,10 @@ class TestWriteBox:
                     "Human 1 <person1@example.com>",
                     "Email Gateway <emailgateway@zulip.com>, "
                     "Human 2 <person2@example.com>",
+                    "Email Gateway <emailgateway@zulip.com>, "
+                    "Human Duplicate <personduplicate1@example.com>",
+                    "Email Gateway <emailgateway@zulip.com>, "
+                    "Human Duplicate <personduplicate2@example.com>",
                 ],
             ),
             (