Fix #1205: add phabricator attachment links to jira (#1206)

Author: segunfamisa

docs/actions.md

@@ -161,3 +161,4 @@ linked Jira issue status to "Closed". If the bug changes to a status not listed
 - `sync_whiteboard_labels`:
   Syncs the Bugzilla whitboard tags field to the Jira labels field.
 - `maybe_update_components`: looks at the component that's set on the bug (if any) and any components added to the project configuration with the `jira_components` parameter (see above). If those components are available on the Jira side as well, they're added to the Jira issue
+- `maybe_add_phabricator_link`: looks at an attachment and if it is a phabricator attachment, it gets added as a link or updated if the attachment was previously added.

jbi/bugzilla/models.py

@@ -1,5 +1,6 @@
 import datetime
 import logging
+import re
 from typing import Any, Optional, TypedDict
 from urllib.parse import ParseResult, urlparse
 
@@ -102,6 +103,36 @@ class WebhookAttachment(BaseModel, frozen=True):
     is_patch: bool
     is_private: bool
 
+    def is_phabricator_patch(self) -> bool:
+        """
+        Returns True if this attachment is a phabricator patch attachment.
+
+        We identify an attachment as a patch if the content type contains "phabricator-request"
+        """
+        return "phabricator-request" in self.content_type
+
+    def phabricator_url(self, base_url: str) -> str | None:
+        """
+        Returns the Phabricator patch URL from the file name if the attachment is a patch, otherwise, it returns None.
+        """
+        if not self.is_phabricator_patch():
+            return None
+
+        match = re.search(r'D\d+', self.file_name)
+        if not match:
+            logger.info(
+                "Expected that attachment with name %s is a patch, but we couldn't extract the phabricator id (e.g D1234)",
+                self.file_name,
+                extra={
+                    "bug": {
+                        "id": self.id,
+                    }
+                },
+            )
+            return None
+
+        revision_id = match.group(0)
+        return f"{base_url}/{revision_id}"
 
 class Bug(BaseModel, frozen=True):
     """Bugzilla Bug Object"""

jbi/environment.py

@@ -40,6 +40,9 @@ class Settings(BaseSettings):
     bugzilla_base_url: str = "https://bugzilla-dev.allizom.org"
     bugzilla_api_key: str
 
+    # Phabricator
+    phabricator_base_url: str = "https://phabricator.services.mozilla.com"
+
     # Logging
     log_level: str = "info"
     log_format: str = "json"  # set to "text" for human-readable logs

jbi/jira/service.py

@@ -132,6 +132,8 @@ def add_jira_comment(self, context: ActionContext):
                 formatted_comment += (
                     f"\n*Filename*: {att.file_name} ({att.content_type})"
                 )
+                if phabricator_url := att.phabricator_url(base_url=settings.phabricator_base_url):
+                    formatted_comment += f"\n*Phabricator URL*: {phabricator_url}"
 
         else:
             comment = context.bug.comment

jbi/models.py

@@ -45,6 +45,7 @@ class ActionSteps(BaseModel, frozen=True):
     ]
     attachment: list[str] = [
         "create_comment",
+        "maybe_add_phabricator_link",
     ]
 
     @field_validator("*")

jbi/steps.py

@@ -118,6 +118,51 @@ def add_link_to_bugzilla(
     context = context.append_responses(jira_response)
     return (StepStatus.SUCCESS, context)
 
+def maybe_add_phabricator_link(
+        context: ActionContext,
+        *,
+        jira_service: JiraService,
+) -> StepResult:
+    """Add a phabricator link to the Jira issue if an attachment is a phabricator attachment"""
+    if context.event.target != "attachment" or not context.bug.attachment:
+        return (StepStatus.NOOP, context)
+
+    attachment = context.bug.attachment
+
+    settings = get_settings()
+    phabricator_url = attachment.phabricator_url(base_url=settings.phabricator_base_url)
+
+    if not phabricator_url:
+        return (StepStatus.NOOP, context)
+
+    description = attachment.description
+    if attachment.is_obsolete:
+        description = f"{0} - {1}".format("Abandoned", attachment.description)
+
+    issue_key = context.jira.issue
+
+    jira_response = jira_service.client.create_or_update_issue_remote_links(
+        issue_key=issue_key,
+        global_id=f"{context.bug.id}-{attachment.id}",
+        link_url=phabricator_url,
+        title=description,
+    )
+
+    if jira_response:
+        logger.info(
+            "Phabricator patch added or updated in Jira issue %s",
+            issue_key,
+            extra=context.update(operation=Operation.LINK).model_dump(),
+        )
+        context = context.append_responses(jira_response)
+        return (StepStatus.SUCCESS, context)
+    else:
+        logger.info(
+            "Failed to add or update phabricator url in Jira issue %s",
+            issue_key,
+        )
+
+    return (StepStatus.NOOP, context)
 
 def maybe_delete_duplicate(
     context: ActionContext,

tests/unit/bugzilla/test_models.py

@@ -6,3 +6,31 @@ def test_attachments_flags(bug_factory):
         ],
     )
     # not raising is a success
+
+def test_patch_attachment_returns_valid_phabricator_url(bug_factory):
+    bug = bug_factory.build(
+        with_attachment=True,
+        attachment__is_patch=True,
+        attachment__file_name="phabricator-D262912-url.txt",
+        attachment__content_type="text/x-phabricator-request",
+    )
+    base_url = "https://phabricator.services.mozilla.com"
+    assert bug.attachment.phabricator_url(base_url=base_url) == "https://phabricator.services.mozilla.com/D262912"
+
+def test_non_patch_attachment_returns_no_phabricator_url(bug_factory):
+    bug = bug_factory.build(
+        with_attachment=True,
+        attachment__file_name="screenshot.png",
+        attachment__content_type="image/png",
+    )
+    base_url = "https://phabricator.services.mozilla.com"
+    assert bug.attachment.phabricator_url(base_url=base_url) is None
+
+def test_patch_attachment_with_unexpected_file_name_returns_no_phabricator_url(bug_factory):
+    bug = bug_factory.build(
+        with_attachment=True,
+        attachment__file_name="phabricator-F262912-url.txt", # normally should be "phabricator-D<>-url.txt"
+        attachment__content_type="text/x-phabricator-request",
+    )
+    base_url = "https://phabricator.services.mozilla.com"
+    assert bug.attachment.phabricator_url(base_url=base_url) is None

tests/unit/test_steps.py

@@ -25,6 +25,7 @@
         "maybe_assign_jira_user",
         "maybe_update_issue_status",
         "maybe_update_issue_resolution",
+        "maybe_add_phabricator_link",
     ],
     "comment": [
         "create_comment",
@@ -245,7 +246,36 @@ def test_added_attachment(
 
     mocked_jira.issue_add_comment.assert_called_once_with(
         issue_key="JBI-234",
-        comment="*[email protected]* created an attachment:\n*Description*: Bug 1337 - Stop war r?peace\n*Filename*: phabricator-D1234-url.txt (text/x-phabricator-request)",
+        comment="*[email protected]* created an attachment:\n*Description*: Bug 1337 - Stop war r?peace\n*Filename*: phabricator-D1234-url.txt (text/x-phabricator-request)\n*Phabricator URL*: https://phabricator.services.mozilla.com/D1234",
+    )
+
+def test_added_phabricator_attachment(
+    action_context_factory, mocked_jira, action_params_factory
+):
+    phabricator_attachment_context = action_context_factory(
+        operation=Operation.ATTACHMENT,
+        bug__with_attachment=True,
+        bug__id=5555,
+        bug__attachment__is_patch=True,
+        bug__attachment__is_obsolete=False,
+        bug__attachment__id=123456,
+        bug__attachment__file_name="phabricator-D1234-url.txt",
+        bug__attachment__description="Bug 1234 - Fix all the bugs",
+        bug__attachment__content_type="text/x-phabricator-request",
+        event__target="attachment",
+        jira__issue="JBI-234",
+    )
+    callable_object = Executor(
+        action_params_factory(jira_project_key=phabricator_attachment_context.jira.project)
+    )
+
+    callable_object(context=phabricator_attachment_context)
+
+    mocked_jira.create_or_update_issue_remote_links.assert_called_once_with(
+        issue_key="JBI-234",
+        link_url="https://phabricator.services.mozilla.com/D1234",
+        title="Bug 1234 - Fix all the bugs",
+        global_id="5555-123456",
     )