git webhooks: Handle assignment events better.

With the introduction of `assignee_updated` parameter in the library,
- Github, Gitea, Gogs can display the assignee in assignment events.
- Github can display the user unassigned in unassignment events.

Fixes https://chat.zulip.org/#narrow/channel/127-integrations/near/1965136

zerver/lib/webhooks/git.py

@@ -58,10 +58,7 @@ ISSUE_LABELED_OR_UNLABELED_MESSAGE_TEMPLATE_WITH_TITLE = "[{user_name}]({user_ur
 ISSUE_MILESTONED_OR_DEMILESTONED_MESSAGE_TEMPLATE = "[{user_name}]({user_url}) {action} milestone [{milestone_name}]({milestone_url}) {preposition} [issue #{id}]({url})."
 ISSUE_MILESTONED_OR_DEMILESTONED_MESSAGE_TEMPLATE_WITH_TITLE = "[{user_name}]({user_url}) {action} milestone [{milestone_name}]({milestone_url}) {preposition} [issue #{id} {title}]({url})."
 
-PULL_REQUEST_OR_ISSUE_MESSAGE_TEMPLATE = "{user_name} {action} [{type}{id}]({url})"
+PULL_REQUEST_OR_ISSUE_MESSAGE_TEMPLATE = "{user_name} {action}{assignee} [{type}{id}{title}]({url})"
-PULL_REQUEST_OR_ISSUE_MESSAGE_TEMPLATE_WITH_TITLE = (
-    "{user_name} {action} [{type}{id} {title}]({url})"
-)
 PULL_REQUEST_OR_ISSUE_ASSIGNEE_INFO_TEMPLATE = "(assigned to {assignee})"
 PULL_REQUEST_REVIEWER_INFO_TEMPLATE = "(assigned reviewers: {reviewer})"
 PULL_REQUEST_BRANCH_INFO_TEMPLATE = "from `{target}` to `{base}`"
@@ -204,6 +201,7 @@ def get_pull_request_event_message(
     message: str | None = None,
     assignee: str | None = None,
     assignees: list[dict[str, Any]] | None = None,
+    assignee_updated: str | None = None,
     reviewer: str | None = None,
     type: str = "PR",
     title: str | None = None,
@@ -214,13 +212,14 @@ def get_pull_request_event_message(
         "type": type,
         "url": url,
         "id": f" #{number}" if number is not None else "",
-        "title": title,
+        "title": f" {title}" if title is not None else "",
+        "assignee": {
+            "assigned": f" {assignee_updated} to",
+            "unassigned": f" {assignee_updated} from",
+        }.get(action, ""),
     }
 
-    if title is not None:
+    main_message = PULL_REQUEST_OR_ISSUE_MESSAGE_TEMPLATE.format(**kwargs)
-        main_message = PULL_REQUEST_OR_ISSUE_MESSAGE_TEMPLATE_WITH_TITLE.format(**kwargs)
-    else:
-        main_message = PULL_REQUEST_OR_ISSUE_MESSAGE_TEMPLATE.format(**kwargs)
 
     if target_branch and base_branch:
         branch_info = PULL_REQUEST_BRANCH_INFO_TEMPLATE.format(
@@ -270,6 +269,7 @@ def get_issue_event_message(
     message: str | None = None,
     assignee: str | None = None,
     assignees: list[dict[str, Any]] | None = None,
+    assignee_updated: str | None = None,
     title: str | None = None,
 ) -> str:
     return get_pull_request_event_message(
@@ -280,6 +280,7 @@ def get_issue_event_message(
         message=message,
         assignee=assignee,
         assignees=assignees,
+        assignee_updated=assignee_updated,
         type="issue",
         title=title,
     )

zerver/webhooks/gitea/tests.py

@@ -53,7 +53,7 @@ class GiteaHookTests(WebhookTestCase):
 
     def test_pull_request_assigned(self) -> None:
         expected_topic_name = "test / PR #1906 test 2"
-        expected_message = """kostekIV assigned [PR #5](https://try.gitea.io/kostekIV/test/pulls/5) from `d` to `master` (assigned to kostekIV)."""
+        expected_message = """kostekIV assigned kostekIV to [PR #5](https://try.gitea.io/kostekIV/test/pulls/5) from `d` to `master` (assigned to kostekIV)."""
         self.check_webhook("pull_request__assigned", expected_topic_name, expected_message)
 
     def test_issues_opened(self) -> None:
@@ -73,7 +73,7 @@ class GiteaHookTests(WebhookTestCase):
 
     def test_issues_assigned(self) -> None:
         expected_topic_name = "test / issue #3 Test issue"
-        expected_message = """kostekIV assigned [issue #3](https://try.gitea.io/kostekIV/test/issues/3) (assigned to kostekIV):\n\n~~~ quote\nTest body\n~~~"""
+        expected_message = """kostekIV assigned kostekIV to [issue #3](https://try.gitea.io/kostekIV/test/issues/3) (assigned to kostekIV):\n\n~~~ quote\nTest body\n~~~"""
         self.check_webhook("issues__assigned", expected_topic_name, expected_message)
 
     def test_issues_reopened(self) -> None:

zerver/webhooks/gitea/view.py

@@ -46,6 +46,7 @@ def format_pull_request_event(payload: WildValue, include_title: bool = False) -
         base_branch=base_branch,
         title=title,
         assignee=stringified_assignee,
+        assignee_updated=stringified_assignee if action == "assigned" else None,
     )
 
 

zerver/webhooks/github/tests.py

@@ -386,14 +386,15 @@ class GitHubWebhookTest(WebhookTestCase):
         self.check_webhook("pull_request__assigned", expected_topic_name, expected_message)
 
     def test_pull_request_unassigned_msg(self) -> None:
-        expected_message = (
+        expected_message = "eeshangarg unassigned eeshangarg from [PR #1](https://github.com/zulip-test-org/helloworld/pull/1)."
-            "eeshangarg unassigned [PR #1](https://github.com/zulip-test-org/helloworld/pull/1)."
+        expected_topic_name = "helloworld / PR #1 Mention that Zulip rocks!"
-        )
+        self.check_webhook("pull_request__unassigned", expected_topic_name, expected_message)
-        self.check_webhook(
+
-            "pull_request__unassigned",
+    def test_pull_request_unassigned_msg_with_custom_topic_in_url(self) -> None:
-            "helloworld / PR #1 Mention that Zulip rocks!",
+        self.url = self.build_webhook_url(topic="notifications")
-            expected_message,
+        expected_topic_name = "notifications"
-        )
+        expected_message = "eeshangarg unassigned eeshangarg from [PR #1 Mention that Zulip rocks!](https://github.com/zulip-test-org/helloworld/pull/1)"
+        self.check_webhook("pull_request__unassigned", expected_topic_name, expected_message)
 
     def test_pull_request_ready_for_review_msg(self) -> None:
         expected_message = "**Hypro999** has marked [PR #2](https://github.com/Hypro999/temp-test-github-webhook/pull/2) as ready for review."

zerver/webhooks/github/view.py

@@ -92,20 +92,16 @@ def get_assigned_or_unassigned_pull_request_body(helper: Helper) -> str:
     payload = helper.payload
     include_title = helper.include_title
     pull_request = payload["pull_request"]
-    assignee = pull_request.get("assignee")
+    assignee = payload["assignee"]["login"].tame(check_string)
-    if assignee:
-        stringified_assignee = assignee["login"].tame(check_string)
 
-    base_message = get_pull_request_event_message(
+    return get_pull_request_event_message(
         user_name=get_sender_name(payload),
         action=payload["action"].tame(check_string),
         url=pull_request["html_url"].tame(check_string),
         number=pull_request["number"].tame(check_int),
         title=pull_request["title"].tame(check_string) if include_title else None,
+        assignee_updated=assignee,
     )
-    if assignee:
-        return base_message.replace("assigned", f"assigned {stringified_assignee} to", 1)
-    return base_message
 
 
 def get_closed_pull_request_body(helper: Helper) -> str:
@@ -155,8 +151,7 @@ def get_issue_body(helper: Helper) -> str:
     include_title = helper.include_title
     action = payload["action"].tame(check_string)
     issue = payload["issue"]
-    has_assignee = "assignee" in payload
+    return get_issue_event_message(
-    base_message = get_issue_event_message(
         user_name=get_sender_name(payload),
         action=action,
         url=issue["html_url"].tame(check_string),
@@ -167,17 +162,11 @@ def get_issue_body(helper: Helper) -> str:
             else issue["body"].tame(check_none_or(check_string))
         ),
         title=issue["title"].tame(check_string) if include_title else None,
+        assignee_updated=payload["assignee"]["login"].tame(check_string)
+        if "assignee" in payload
+        else None,
     )
 
-    if has_assignee:
-        stringified_assignee = payload["assignee"]["login"].tame(check_string)
-        if action == "assigned":
-            return base_message.replace("assigned", f"assigned {stringified_assignee} to", 1)
-        elif action == "unassigned":
-            return base_message.replace("unassigned", f"unassigned {stringified_assignee} from", 1)
-
-    return base_message
-
 
 def get_issue_comment_body(helper: Helper) -> str:
     payload = helper.payload

zerver/webhooks/gogs/tests.py

@@ -94,7 +94,7 @@ class GogsHookTests(WebhookTestCase):
 
     def test_pull_request_assigned(self) -> None:
         expected_topic_name = "test / PR #1349 Test"
-        expected_message = """kostekIV assigned [PR #2](https://try.gogs.io/kostekIV/test/pulls/2) from `c` to `master`."""
+        expected_message = """kostekIV assigned kostekIV to [PR #2](https://try.gogs.io/kostekIV/test/pulls/2) from `c` to `master`."""
         self.check_webhook("pull_request__assigned", expected_topic_name, expected_message)
 
     def test_pull_request_synchronized(self) -> None:
@@ -119,7 +119,7 @@ class GogsHookTests(WebhookTestCase):
 
     def test_issues_assignee(self) -> None:
         expected_topic_name = "test / issue #3 New test issue"
-        expected_message = """kostekIV assigned [issue #3](https://try.gogs.io/kostekIV/test/issues/3) (assigned to kostekIV):\n\n~~~ quote\nTest\n~~~"""
+        expected_message = """kostekIV assigned kostekIV to [issue #3](https://try.gogs.io/kostekIV/test/issues/3) (assigned to kostekIV):\n\n~~~ quote\nTest\n~~~"""
         self.check_webhook("issues__assigned", expected_topic_name, expected_message)
 
     def test_issues_closed(self) -> None:

zerver/webhooks/gogs/view.py

@@ -86,6 +86,11 @@ def format_pull_request_event(payload: WildValue, include_title: bool = False) -
         target_branch = payload["pull_request"]["head_branch"].tame(check_string)
         base_branch = payload["pull_request"]["base_branch"].tame(check_string)
     title = payload["pull_request"]["title"].tame(check_string) if include_title else None
+    stringified_assignee = (
+        payload["pull_request"]["assignee"]["login"].tame(check_string)
+        if payload["action"] and payload["pull_request"]["assignee"]
+        else None
+    )
 
     return get_pull_request_event_message(
         user_name=user_name,
@@ -95,20 +100,24 @@ def format_pull_request_event(payload: WildValue, include_title: bool = False) -
         target_branch=target_branch,
         base_branch=base_branch,
         title=title,
+        assignee_updated=stringified_assignee,
     )
 
 
 def format_issues_event(payload: WildValue, include_title: bool = False) -> str:
     issue_nr = payload["issue"]["number"].tame(check_int)
     assignee = payload["issue"]["assignee"]
+    stringified_assignee = assignee["login"].tame(check_string) if assignee else None
+    action = payload["action"].tame(check_string)
     return get_issue_event_message(
         user_name=payload["sender"]["login"].tame(check_string),
         action=payload["action"].tame(check_string),
         url=get_issue_url(payload["repository"]["html_url"].tame(check_string), issue_nr),
         number=issue_nr,
         message=payload["issue"]["body"].tame(check_string),
-        assignee=assignee["login"].tame(check_string) if assignee else None,
+        assignee=stringified_assignee,
         title=payload["issue"]["title"].tame(check_string) if include_title else None,
+        assignee_updated=stringified_assignee if action == "assigned" else None,
     )