webhooks/gitlab: Use information about all assignees.

Previously, the GitLab webhook code, namely the `get_objects_assignee`
method first tried to get a single assignee and if that failed then it
looks for multiple assignees and then it would return the first
assignee that it found (there's actually a code smell here - a loop
which would always return on the first iteration).

Instead, this commit will change that behavior to first check for
multiple assignees first then for a single assignee if we can't find
multiple assignees. Ultimately it will return a list of all of the
assignees (however many that might be [0, n]). This method has then
aptly been renamed to `get_assignees`.

Finally, we tweked the code using this method to always use it's
output as an "assignees" parameter to templates (there's also an
assignee parameter which we want to avoid here for consistency).

Signed-off-by: Hemanth V. Alluri <hdrive1999@gmail.com>

zerver/webhooks/gitlab/tests.py

@@ -123,7 +123,7 @@ class GitlabHookTests(WebhookTestCase):
 
     def test_create_issue_with_two_assignees_event_message(self) -> None:
         expected_subject = "Zulip GitLab Test / Issue #2 Zulip Test Issue 2"
-        expected_message = "Adam Birds created [Issue #2](https://gitlab.com/adambirds/zulip-gitlab-test/issues/2) (assigned to adambirds and eeshangarg):\n\n~~~ quote\nZulip Test Issue 2\n~~~"
+        expected_message = "Adam Birds created [Issue #2](https://gitlab.com/adambirds/zulip-gitlab-test/issues/2) (assigned to Adam Birds and Eeshan Garg):\n\n~~~ quote\nZulip Test Issue 2\n~~~"
 
         self.check_webhook(
             "issue_hook__issue_created_with_two_assignees", expected_subject, expected_message
@@ -131,7 +131,7 @@ class GitlabHookTests(WebhookTestCase):
 
     def test_create_issue_with_three_assignees_event_message(self) -> None:
         expected_subject = "Zulip GitLab Test / Issue #2 Zulip Test Issue 2"
-        expected_message = "Adam Birds created [Issue #2](https://gitlab.com/adambirds/zulip-gitlab-test/issues/2) (assigned to adambirds, eeshangarg and timabbott):\n\n~~~ quote\nZulip Test Issue 2\n~~~"
+        expected_message = "Adam Birds created [Issue #2](https://gitlab.com/adambirds/zulip-gitlab-test/issues/2) (assigned to Adam Birds, Eeshan Garg and Tim Abbott):\n\n~~~ quote\nZulip Test Issue 2\n~~~"
 
         self.check_webhook(
             "issue_hook__issue_created_with_three_assignees", expected_subject, expected_message
@@ -139,7 +139,7 @@ class GitlabHookTests(WebhookTestCase):
 
     def test_create_confidential_issue_with_assignee_event_message(self) -> None:
         expected_subject = "testing / Issue #2 Testing"
-        expected_message = "Joe Bloggs created [Issue #2](https://gitlab.example.co.uk/joe.bloggs/testing/issues/2) (assigned to joe.bloggs):\n\n~~~ quote\nTesting\n~~~"
+        expected_message = "Joe Bloggs created [Issue #2](https://gitlab.example.co.uk/joe.bloggs/testing/issues/2) (assigned to Joe Bloggs):\n\n~~~ quote\nTesting\n~~~"
 
         self.check_webhook(
             "issue_hook__confidential_issue_created_with_assignee",
@@ -306,7 +306,7 @@ class GitlabHookTests(WebhookTestCase):
         expected_topic = "my-awesome-project / MR #3 New Merge Request"
         expected_message = "Tomasz Kolek created [MR #3](https://gitlab.com/tomaszkolek0/my-awesome-project/merge_requests/3) (assigned to Tomasz Kolek) from `tomek` to `master`:\n\n~~~ quote\ndescription of merge request\n~~~"
         self.check_webhook(
-            "merge_request_hook__merge_request_created_with_assignee",
+            'merge_request_hook__merge_request_created_with_assignee',
             expected_topic,
             expected_message,
         )
@@ -356,8 +356,9 @@ class GitlabHookTests(WebhookTestCase):
         expected_topic = "my-awesome-project / MR #3 New Merge Request"
         expected_message = "Tomasz Kolek updated [MR #3](https://gitlab.com/tomaszkolek0/my-awesome-project/merge_requests/3) (assigned to Tomasz Kolek) from `tomek` to `master`:\n\n~~~ quote\nupdated desc\n~~~"
         self.check_webhook(
-            "merge_request_hook__merge_request_updated", expected_topic, expected_message
-        )
+            'merge_request_hook__merge_request_updated',
+            expected_topic,
+            expected_message)
 
     def test_merge_request_added_commit_event_message(self) -> None:
         expected_topic = "my-awesome-project / MR #3 New Merge Request"
@@ -529,8 +530,9 @@ class GitlabHookTests(WebhookTestCase):
         expected_topic = "my-awesome-project / MR #3 New Merge Request"
         expected_message = "Tomasz Kolek created [MR #3](https://gitlab.com/tomaszkolek0/my-awesome-project/merge_requests/3) (assigned to Tomasz Kolek) from `tomek` to `master`:\n\n~~~ quote\ndescription of merge request\n~~~"
         self.check_webhook(
-            "system_hook__merge_request_created_with_assignee", expected_topic, expected_message
-        )
+            'system_hook__merge_request_created_with_assignee',
+            expected_topic,
+            expected_message)
 
     def test_system_merge_request_closed_event_message(self) -> None:
         expected_topic = "my-awesome-project / MR #2 NEW MR"

zerver/webhooks/gitlab/view.py

@@ -1,7 +1,7 @@
 import re
 from functools import partial
 from inspect import signature
-from typing import Any, Dict, Optional
+from typing import Any, Dict, List, Optional
 
 from django.http import HttpRequest, HttpResponse
 
@@ -91,8 +91,7 @@ def get_issue_created_event_body(payload: Dict[str, Any],
         get_object_url(payload),
         payload['object_attributes'].get('iid'),
         description,
-        get_objects_assignee(payload),
-        payload.get('assignees'),
+        assignees=replace_assignees_username_with_name(get_assignees(payload)),
         title=payload['object_attributes'].get('title') if include_title else None,
     )
 
@@ -142,22 +141,31 @@ def get_merge_request_open_or_updated_body(payload: Dict[str, Any], action: str,
         pull_request.get('source_branch'),
         pull_request.get('target_branch'),
         pull_request.get('description'),
-        get_objects_assignee(payload),
+        assignees=replace_assignees_username_with_name(get_assignees(payload)),
         type='MR',
         title=payload['object_attributes'].get('title') if include_title else None,
     )
 
-def get_objects_assignee(payload: Dict[str, Any]) -> Optional[str]:
-    assignee_object = payload.get('assignee')
-    if assignee_object:
-        return assignee_object.get('name')
+def get_assignees(payload: Dict[str, Any]) -> List[Dict[str, str]]:
+    assignee_details = payload.get('assignees')
+    if assignee_details is None:
+        single_assignee_details = payload.get('assignee')
+        if single_assignee_details is None:
+            assignee_details = []
         else:
-        assignee_object = payload.get('assignees')
-        if assignee_object:
-            for assignee in assignee_object:
-                return assignee['name']
+            assignee_details = [single_assignee_details]
+    return assignee_details
 
-    return None
+def replace_assignees_username_with_name(assignees: List[Dict[str, str]]) -> List[Dict[str, str]]:
+    """Replace the username of each assignee with their (full) name.
+
+    This is a hack-like adaptor so that when assignees are passed to
+    `get_pull_request_event_message` we can use the assignee's name
+    and not their username (for more consistency).
+    """
+    for assignee in assignees:
+        assignee["username"] = assignee["name"]
+    return assignees
 
 def get_commented_commit_event_body(payload: Dict[str, Any]) -> str:
     comment = payload['object_attributes']