From bd83dbfb4252506b6d57fe0339c1039162ae3877 Mon Sep 17 00:00:00 2001 From: Niloth P <20315308+Niloth-p@users.noreply.github.com> Date: Thu, 24 Oct 2024 07:26:41 +0530 Subject: [PATCH] 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 | 19 ++++++++++--------- zerver/webhooks/gitea/tests.py | 4 ++-- zerver/webhooks/gitea/view.py | 1 + zerver/webhooks/github/tests.py | 17 +++++++++-------- zerver/webhooks/github/view.py | 25 +++++++------------------ zerver/webhooks/gogs/tests.py | 4 ++-- zerver/webhooks/gogs/view.py | 11 ++++++++++- 7 files changed, 41 insertions(+), 40 deletions(-) diff --git a/zerver/lib/webhooks/git.py b/zerver/lib/webhooks/git.py index 660719b789..8a15e24b0c 100644 --- a/zerver/lib/webhooks/git.py +++ b/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_WITH_TITLE = ( - "{user_name} {action} [{type}{id} {title}]({url})" -) +PULL_REQUEST_OR_ISSUE_MESSAGE_TEMPLATE = "{user_name} {action}{assignee} [{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_WITH_TITLE.format(**kwargs) - else: - main_message = PULL_REQUEST_OR_ISSUE_MESSAGE_TEMPLATE.format(**kwargs) + 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, ) diff --git a/zerver/webhooks/gitea/tests.py b/zerver/webhooks/gitea/tests.py index c6825f7124..20733b6790 100644 --- a/zerver/webhooks/gitea/tests.py +++ b/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: diff --git a/zerver/webhooks/gitea/view.py b/zerver/webhooks/gitea/view.py index 43c0b775b3..e7e937d206 100644 --- a/zerver/webhooks/gitea/view.py +++ b/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, ) diff --git a/zerver/webhooks/github/tests.py b/zerver/webhooks/github/tests.py index 1fac82d5c1..b255ff6401 100644 --- a/zerver/webhooks/github/tests.py +++ b/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 = ( - "eeshangarg unassigned [PR #1](https://github.com/zulip-test-org/helloworld/pull/1)." - ) - self.check_webhook( - "pull_request__unassigned", - "helloworld / PR #1 Mention that Zulip rocks!", - expected_message, - ) + expected_message = "eeshangarg unassigned eeshangarg from [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) + + def test_pull_request_unassigned_msg_with_custom_topic_in_url(self) -> None: + self.url = self.build_webhook_url(topic="notifications") + 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." diff --git a/zerver/webhooks/github/view.py b/zerver/webhooks/github/view.py index 17d40ba03f..5ca7b660ce 100644 --- a/zerver/webhooks/github/view.py +++ b/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") - if assignee: - stringified_assignee = assignee["login"].tame(check_string) + assignee = payload["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 - base_message = get_issue_event_message( + return 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 diff --git a/zerver/webhooks/gogs/tests.py b/zerver/webhooks/gogs/tests.py index 3c98140e22..21c80059bf 100644 --- a/zerver/webhooks/gogs/tests.py +++ b/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: diff --git a/zerver/webhooks/gogs/view.py b/zerver/webhooks/gogs/view.py index eecfcd3e2e..7fe905a06f 100644 --- a/zerver/webhooks/gogs/view.py +++ b/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, )