webhooks/github: Include title in message body if not in topic.

This is a follow-up in response to Tim's comments on #9951.

In instances where all messages from a GitHub integration are
grouped under one user specified topic (specified in the URL), we
should include the title of the issue/PR in the message body, since
the availability of a user-specified topic precludes us from
including it in the topic itself (which was the default behaviour).

zerver/webhooks/github/tests.py

@@ -117,10 +117,22 @@ class GithubWebhookTest(WebhookTestCase):
         expected_message = u"baxterthehacker [commented](https://github.com/baxterthehacker/public-repo/issues/2#issuecomment-99262140) on [Issue #2](https://github.com/baxterthehacker/public-repo/issues/2)\n\n~~~ quote\nYou are totally right! I'll get this fixed right away.\n~~~"
         self.send_and_test_stream_message('issue_comment', self.EXPECTED_SUBJECT_ISSUE_EVENTS, expected_message, HTTP_X_GITHUB_EVENT='issue_comment')
 
+    def test_issue_comment_msg_with_custom_topic_in_url(self) -> None:
+        self.url = self.build_webhook_url(topic='notifications')
+        expected_subject = u"notifications"
+        expected_message = u"baxterthehacker [commented](https://github.com/baxterthehacker/public-repo/issues/2#issuecomment-99262140) on [Issue #2 Spelling error in the README file](https://github.com/baxterthehacker/public-repo/issues/2)\n\n~~~ quote\nYou are totally right! I'll get this fixed right away.\n~~~"
+        self.send_and_test_stream_message('issue_comment', expected_subject, expected_message, HTTP_X_GITHUB_EVENT='issue_comment')
+
     def test_issue_msg(self) -> None:
         expected_message = u"baxterthehacker opened [Issue #2](https://github.com/baxterthehacker/public-repo/issues/2)\n\n~~~ quote\nIt looks like you accidently spelled 'commit' with two 't's.\n~~~"
         self.send_and_test_stream_message('issue', self.EXPECTED_SUBJECT_ISSUE_EVENTS, expected_message, HTTP_X_GITHUB_EVENT='issues')
 
+    def test_issue_msg_with_custom_topic_in_url(self) -> None:
+        self.url = self.build_webhook_url(topic='notifications')
+        expected_subject = u"notifications"
+        expected_message = u"baxterthehacker opened [Issue #2 Spelling error in the README file](https://github.com/baxterthehacker/public-repo/issues/2)\n\n~~~ quote\nIt looks like you accidently spelled 'commit' with two 't's.\n~~~"
+        self.send_and_test_stream_message('issue', expected_subject, expected_message, HTTP_X_GITHUB_EVENT='issues')
+
     def test_membership_msg(self) -> None:
         expected_message = u"baxterthehacker added [kdaigle](https://github.com/kdaigle) to Contractors team"
         self.send_and_test_stream_message('membership', self.EXPECTED_SUBJECT_ORGANIZATION_EVENTS, expected_message, HTTP_X_GITHUB_EVENT='membership')
@@ -130,19 +142,31 @@ class GithubWebhookTest(WebhookTestCase):
         self.send_and_test_stream_message('member', self.EXPECTED_SUBJECT_REPO_EVENTS, expected_message, HTTP_X_GITHUB_EVENT='member')
 
     def test_pull_request_opened_msg(self) -> None:
-        expected_message = u"baxterthehacker opened [PR](https://github.com/baxterthehacker/public-repo/pull/1)\nfrom `changes` to `master`\n\n~~~ quote\nThis is a pretty simple change that we need to pull into master.\n~~~"
+        expected_message = u"baxterthehacker opened [PR #1](https://github.com/baxterthehacker/public-repo/pull/1)\nfrom `changes` to `master`\n\n~~~ quote\nThis is a pretty simple change that we need to pull into master.\n~~~"
         self.send_and_test_stream_message('opened_pull_request', self.EXPECTED_SUBJECT_PR_EVENTS, expected_message, HTTP_X_GITHUB_EVENT='pull_request')
 
+    def test_pull_request_opened_msg_with_custom_topic_in_url(self) -> None:
+        self.url = self.build_webhook_url(topic='notifications')
+        expected_subject = u"notifications"
+        expected_message = u"baxterthehacker opened [PR #1 Update the README with new information](https://github.com/baxterthehacker/public-repo/pull/1)\nfrom `changes` to `master`\n\n~~~ quote\nThis is a pretty simple change that we need to pull into master.\n~~~"
+        self.send_and_test_stream_message('opened_pull_request', expected_subject, expected_message, HTTP_X_GITHUB_EVENT='pull_request')
+
     def test_pull_request_synchronized_msg(self) -> None:
-        expected_message = u"baxterthehacker updated [PR](https://github.com/baxterthehacker/public-repo/pull/1)\nfrom `changes` to `master`"
+        expected_message = u"baxterthehacker updated [PR #1](https://github.com/baxterthehacker/public-repo/pull/1)\nfrom `changes` to `master`"
         self.send_and_test_stream_message('synchronized_pull_request', self.EXPECTED_SUBJECT_PR_EVENTS, expected_message, HTTP_X_GITHUB_EVENT='pull_request')
 
     def test_pull_request_closed_msg(self) -> None:
-        expected_message = u"baxterthehacker closed without merge [PR](https://github.com/baxterthehacker/public-repo/pull/1)"
+        expected_message = u"baxterthehacker closed without merge [PR #1](https://github.com/baxterthehacker/public-repo/pull/1)"
         self.send_and_test_stream_message('closed_pull_request', self.EXPECTED_SUBJECT_PR_EVENTS, expected_message, HTTP_X_GITHUB_EVENT='pull_request')
 
+    def test_pull_request_closed_msg_with_custom_topic_in_url(self) -> None:
+        self.url = self.build_webhook_url(topic='notifications')
+        expected_subject = u"notifications"
+        expected_message = u"baxterthehacker closed without merge [PR #1 Update the README with new information](https://github.com/baxterthehacker/public-repo/pull/1)"
+        self.send_and_test_stream_message('closed_pull_request', expected_subject, expected_message, HTTP_X_GITHUB_EVENT='pull_request')
+
     def test_pull_request_merged_msg(self) -> None:
-        expected_message = u"baxterthehacker merged [PR](https://github.com/baxterthehacker/public-repo/pull/1)"
+        expected_message = u"baxterthehacker merged [PR #1](https://github.com/baxterthehacker/public-repo/pull/1)"
         self.send_and_test_stream_message('merged_pull_request', self.EXPECTED_SUBJECT_PR_EVENTS, expected_message, HTTP_X_GITHUB_EVENT='pull_request')
 
     def test_public_msg(self) -> None:
@@ -181,26 +205,45 @@ class GithubWebhookTest(WebhookTestCase):
         expected_message = u"baxterthehacker submitted [PR Review](https://github.com/baxterthehacker/public-repo/pull/1#pullrequestreview-2626884)"
         self.send_and_test_stream_message('pull_request_review', self.EXPECTED_SUBJECT_PR_EVENTS, expected_message, HTTP_X_GITHUB_EVENT='pull_request_review')
 
+    def test_pull_request_review_msg_with_custom_topic_in_url(self) -> None:
+        self.url = self.build_webhook_url(topic='notifications')
+        expected_subject = u"notifications"
+        expected_message = u"baxterthehacker submitted [PR Review for #1 Update the README with new information](https://github.com/baxterthehacker/public-repo/pull/1#pullrequestreview-2626884)"
+        self.send_and_test_stream_message('pull_request_review', expected_subject, expected_message, HTTP_X_GITHUB_EVENT='pull_request_review')
+
     def test_pull_request_review_comment_msg(self) -> None:
         expected_message = u"baxterthehacker created [PR Review Comment](https://github.com/baxterthehacker/public-repo/pull/1#discussion_r29724692)\n\n~~~ quote\nMaybe you should use more emojji on this line.\n~~~"
         self.send_and_test_stream_message('pull_request_review_comment', self.EXPECTED_SUBJECT_PR_EVENTS, expected_message, HTTP_X_GITHUB_EVENT='pull_request_review_comment')
 
+    def test_pull_request_review_comment_with_custom_topic_in_url(self) -> None:
+        self.url = self.build_webhook_url(topic='notifications')
+        expected_subject = u"notifications"
+        expected_message = u"baxterthehacker created [PR Review Comment on #1 Update the README with new information](https://github.com/baxterthehacker/public-repo/pull/1#discussion_r29724692)\n\n~~~ quote\nMaybe you should use more emojji on this line.\n~~~"
+        self.send_and_test_stream_message('pull_request_review_comment', expected_subject, expected_message, HTTP_X_GITHUB_EVENT='pull_request_review_comment')
+
     def test_push_tag_msg(self) -> None:
         expected_message = u"baxterthehacker pushed tag abc"
         self.send_and_test_stream_message('push_tag', self.EXPECTED_SUBJECT_REPO_EVENTS, expected_message, HTTP_X_GITHUB_EVENT='push')
 
     def test_pull_request_edited_msg(self) -> None:
-        expected_message = u"baxterthehacker edited [PR](https://github.com/baxterthehacker/public-repo/pull/1)\nfrom `changes` to `master`"
+        expected_message = u"baxterthehacker edited [PR #1](https://github.com/baxterthehacker/public-repo/pull/1)\nfrom `changes` to `master`"
         self.send_and_test_stream_message('edited_pull_request', self.EXPECTED_SUBJECT_PR_EVENTS, expected_message,
                                           HTTP_X_GITHUB_EVENT='pull_request')
 
     def test_pull_request_assigned_msg(self) -> None:
-        expected_message = u"baxterthehacker assigned [PR](https://github.com/baxterthehacker/public-repo/pull/1) to baxterthehacker"
+        expected_message = u"baxterthehacker assigned [PR #1](https://github.com/baxterthehacker/public-repo/pull/1) to baxterthehacker"
         self.send_and_test_stream_message('assigned_pull_request', self.EXPECTED_SUBJECT_PR_EVENTS, expected_message,
                                           HTTP_X_GITHUB_EVENT='pull_request')
 
+    def test_pull_request_assigned_msg_with_custom_topic_in_url(self) -> None:
+        self.url = self.build_webhook_url(topic='notifications')
+        expected_subject = u"notifications"
+        expected_message = u"baxterthehacker assigned [PR #1 Update the README with new information](https://github.com/baxterthehacker/public-repo/pull/1) to baxterthehacker"
+        self.send_and_test_stream_message('assigned_pull_request', expected_subject, expected_message,
+                                          HTTP_X_GITHUB_EVENT='pull_request')
+
     def test_pull_request_unassigned_msg(self) -> None:
-        expected_message = u"eeshangarg unassigned [PR](https://github.com/zulip-test-org/helloworld/pull/1)"
+        expected_message = u"eeshangarg unassigned [PR #1](https://github.com/zulip-test-org/helloworld/pull/1)"
         self.send_and_test_stream_message(
             'unassigned_pull_request',
             'helloworld / PR #1 Mention that Zulip rocks!',
@@ -222,6 +265,15 @@ class GithubWebhookTest(WebhookTestCase):
                                           expected_message,
                                           HTTP_X_GITHUB_EVENT='pull_request')
 
+    def test_pull_request_review_requested_with_custom_topic_in_url(self) -> None:
+        self.url = self.build_webhook_url(topic='notifications')
+        expected_subject = u"notifications"
+        expected_message = u"**eeshangarg** requested [showell](https://github.com/showell) for a review on [PR #1 This is just a test commit](https://github.com/eeshangarg/Scheduler/pull/1)."
+        self.send_and_test_stream_message('pull_request_review_requested',
+                                          expected_subject,
+                                          expected_message,
+                                          HTTP_X_GITHUB_EVENT='pull_request')
+
     @patch('zerver.webhooks.github.view.check_send_webhook_message')
     def test_pull_request_labeled_ignore(
             self, check_send_webhook_message_mock: MagicMock) -> None:

zerver/webhooks/github/view.py

@@ -2,6 +2,7 @@ import logging
 import re
 from functools import partial
 from typing import Any, Callable, Dict, Optional
+from inspect import signature
 
 from django.http import HttpRequest, HttpResponse
 
@@ -20,7 +21,8 @@ from zerver.models import UserProfile
 class UnknownEventType(Exception):
     pass
 
-def get_opened_or_update_pull_request_body(payload: Dict[str, Any]) -> str:
+def get_opened_or_update_pull_request_body(payload: Dict[str, Any],
+                                           include_title: Optional[bool]=False) -> str:
     pull_request = payload['pull_request']
     action = payload['action']
     if action == 'synchronize':
@@ -36,10 +38,13 @@ def get_opened_or_update_pull_request_body(payload: Dict[str, Any]) -> str:
         target_branch=pull_request['head']['ref'],
         base_branch=pull_request['base']['ref'],
         message=pull_request['body'],
-        assignee=assignee
+        assignee=assignee,
+        number=pull_request['number'],
+        title=pull_request['title'] if include_title else None
     )
 
-def get_assigned_or_unassigned_pull_request_body(payload: Dict[str, Any]) -> str:
+def get_assigned_or_unassigned_pull_request_body(payload: Dict[str, Any],
+                                                 include_title: Optional[bool]=False) -> str:
     pull_request = payload['pull_request']
     assignee = pull_request.get('assignee')
     if assignee is not None:
@@ -49,18 +54,23 @@ def get_assigned_or_unassigned_pull_request_body(payload: Dict[str, Any]) -> str
         get_sender_name(payload),
         payload['action'],
         pull_request['html_url'],
+        number=pull_request['number'],
+        title=pull_request['title'] if include_title else None
     )
     if assignee is not None:
         return "{} to {}".format(base_message, assignee)
     return base_message
 
-def get_closed_pull_request_body(payload: Dict[str, Any]) -> str:
+def get_closed_pull_request_body(payload: Dict[str, Any],
+                                 include_title: Optional[bool]=False) -> str:
     pull_request = payload['pull_request']
     action = 'merged' if pull_request['merged'] else 'closed without merge'
     return get_pull_request_event_message(
         get_sender_name(payload),
         action,
         pull_request['html_url'],
+        number=pull_request['number'],
+        title=pull_request['title'] if include_title else None
     )
 
 def get_membership_body(payload: Dict[str, Any]) -> str:
@@ -88,7 +98,8 @@ def get_member_body(payload: Dict[str, Any]) -> str:
         payload['repository']['html_url']
     )
 
-def get_issue_body(payload: Dict[str, Any]) -> str:
+def get_issue_body(payload: Dict[str, Any],
+                   include_title: Optional[bool]=False) -> str:
     action = payload['action']
     issue = payload['issue']
     assignee = issue['assignee']
@@ -98,10 +109,12 @@ def get_issue_body(payload: Dict[str, Any]) -> str:
         issue['html_url'],
         issue['number'],
         issue['body'],
-        assignee=assignee['login'] if assignee else None
+        assignee=assignee['login'] if assignee else None,
+        title=issue['title'] if include_title else None
     )
 
-def get_issue_comment_body(payload: Dict[str, Any]) -> str:
+def get_issue_comment_body(payload: Dict[str, Any],
+                           include_title: Optional[bool]=False) -> str:
     action = payload['action']
     comment = payload['comment']
     issue = payload['issue']
@@ -118,6 +131,7 @@ def get_issue_comment_body(payload: Dict[str, Any]) -> str:
         issue['html_url'],
         issue['number'],
         comment['body'],
+        title=issue['title'] if include_title else None
     )
 
 def get_fork_body(payload: Dict[str, Any]) -> str:
@@ -257,34 +271,52 @@ def get_status_body(payload: Dict[str, Any]) -> str:
         status
     )
 
-def get_pull_request_review_body(payload: Dict[str, Any]) -> str:
+def get_pull_request_review_body(payload: Dict[str, Any],
+                                 include_title: Optional[bool]=False) -> str:
+    title = "for #{} {}".format(
+        payload['pull_request']['number'],
+        payload['pull_request']['title']
+    )
     return get_pull_request_event_message(
         get_sender_name(payload),
         'submitted',
         payload['review']['html_url'],
-        type='PR Review'
+        type='PR Review',
+        title=title if include_title else None
     )
 
-def get_pull_request_review_comment_body(payload: Dict[str, Any]) -> str:
+def get_pull_request_review_comment_body(payload: Dict[str, Any],
+                                         include_title: Optional[bool]=False) -> str:
     action = payload['action']
     message = None
     if action == 'created':
         message = payload['comment']['body']
 
+    title = "on #{} {}".format(
+        payload['pull_request']['number'],
+        payload['pull_request']['title']
+    )
+
     return get_pull_request_event_message(
         get_sender_name(payload),
         action,
         payload['comment']['html_url'],
         message=message,
-        type='PR Review Comment'
+        type='PR Review Comment',
+        title=title if include_title else None
     )
 
-def get_pull_request_review_requested_body(payload: Dict[str, Any]) -> str:
+def get_pull_request_review_requested_body(payload: Dict[str, Any],
+                                           include_title: Optional[bool]=False) -> str:
     requested_reviewers = payload['pull_request']['requested_reviewers']
     sender = get_sender_name(payload)
     pr_number = payload['pull_request']['number']
     pr_url = payload['pull_request']['html_url']
     message = "**{sender}** requested {reviewers} for a review on [PR #{pr_number}]({pr_url})."
+    message_with_title = ("**{sender}** requested {reviewers} for a review on "
+                          "[PR #{pr_number} {title}]({pr_url}).")
+    body = message_with_title if include_title else message
+
     reviewers = ""
     if len(requested_reviewers) == 1:
         reviewers = "[{login}]({html_url})".format(**requested_reviewers[0])
@@ -293,11 +325,12 @@ def get_pull_request_review_requested_body(payload: Dict[str, Any]) -> str:
             reviewers += "[{login}]({html_url}), ".format(**reviewer)
         reviewers += "and [{login}]({html_url})".format(**requested_reviewers[-1])
 
-    return message.format(
+    return body.format(
         sender=sender,
         reviewers=reviewers,
         pr_number=pr_number,
         pr_url=pr_url,
+        title=payload['pull_request']['title'] if include_title else None
     )
 
 def get_ping_body(payload: Dict[str, Any]) -> str:
@@ -393,11 +426,19 @@ EVENT_FUNCTION_MAPPER = {
 def api_github_webhook(
         request: HttpRequest, user_profile: UserProfile,
         payload: Dict[str, Any]=REQ(argument_type='body'),
-        branches: str=REQ(default=None)) -> HttpResponse:
+        branches: str=REQ(default=None),
+        user_specified_topic: Optional[str]=REQ("topic", default=None)) -> HttpResponse:
     event = get_event(request, payload, branches)
     if event is not None:
         subject = get_subject_based_on_type(payload, event)
-        body = get_body_function_based_on_type(event)(payload)
+        body_function = get_body_function_based_on_type(event)
+        if 'include_title' in signature(body_function).parameters:
+            body = body_function(
+                payload,
+                include_title=user_specified_topic is not None
+            )
+        else:
+            body = body_function(payload)
         check_send_webhook_message(request, user_profile, subject, body)
     return json_success()