diff --git a/docs/actions.md b/docs/actions.md index 38f72683..6616698d 100644 --- a/docs/actions.md +++ b/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. diff --git a/jbi/bugzilla/models.py b/jbi/bugzilla/models.py index 3b79fe93..4b127373 100644 --- a/jbi/bugzilla/models.py +++ b/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""" diff --git a/jbi/environment.py b/jbi/environment.py index 3fe44e3c..16a25d3f 100644 --- a/jbi/environment.py +++ b/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 diff --git a/jbi/jira/service.py b/jbi/jira/service.py index ef50dfc0..44b30a5d 100644 --- a/jbi/jira/service.py +++ b/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 diff --git a/jbi/models.py b/jbi/models.py index 4e7ef088..fd607adf 100644 --- a/jbi/models.py +++ b/jbi/models.py @@ -45,6 +45,7 @@ class ActionSteps(BaseModel, frozen=True): ] attachment: list[str] = [ "create_comment", + "maybe_add_phabricator_link", ] @field_validator("*") diff --git a/jbi/steps.py b/jbi/steps.py index ffc2c555..cbf44274 100644 --- a/jbi/steps.py +++ b/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, diff --git a/tests/unit/bugzilla/test_models.py b/tests/unit/bugzilla/test_models.py index bd5f50ae..efb7262d 100644 --- a/tests/unit/bugzilla/test_models.py +++ b/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 diff --git a/tests/unit/test_steps.py b/tests/unit/test_steps.py index 4f286304..93edf2c3 100644 --- a/tests/unit/test_steps.py +++ b/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="*phab-bot@bmo.tld* created an attachment:\n*Description*: Bug 1337 - Stop war r?peace\n*Filename*: phabricator-D1234-url.txt (text/x-phabricator-request)", + comment="*phab-bot@bmo.tld* 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", )