diff --git a/bugbug/tools/core/platforms/phabricator.py b/bugbug/tools/core/platforms/phabricator.py index 4b99842d85..e6836d775a 100644 --- a/bugbug/tools/core/platforms/phabricator.py +++ b/bugbug/tools/core/platforms/phabricator.py @@ -262,6 +262,8 @@ def _sanitize_comments(comments: list, users_info: dict[str, dict]) -> tuple[lis # Mark that this comment's content has been redacted so downstream # code doesn't need to rely on string comparisons. comment_copy.content_redacted = True + if isinstance(comment_copy, PhabricatorInlineComment): + comment_copy.suggestion_text = None sanitized_comments.append(comment_copy) @@ -277,10 +279,16 @@ def __init__(self, transaction: dict): self.date_modified: int = comment["dateModified"] self.content: str = comment["content"]["raw"] self.author_phid: str = transaction["authorPHID"] + self.removed: bool = comment.get("removed", False) # Whether this comment's content has been redacted due to trust rules. # Set by the sanitizer; used by renderers (e.g., to_md()). self.content_redacted: bool = False + @property + def is_renderable(self) -> bool: + """Whether the comment carries anything worth showing to a reader.""" + return not self.removed and bool(self.content.strip()) + class PhabricatorGeneralComment(PhabricatorComment): """Representation of a general comment posted on a Phabricator revision.""" @@ -299,10 +307,26 @@ def __init__(self, transaction: dict): self.line_length = inline_fields["length"] self.is_reply = inline_fields["replyToCommentPHID"] is not None self.is_done = inline_fields["isDone"] + self.suggestion_text: str | None = ( + inline_fields.get("suggestionText") + if inline_fields.get("hasSuggestion") + else None + ) + + # `isNewFile` says which side of the changeset `start_line` indexes + # into. Older Phabricator versions omit it, leaving the side unknown. + is_new_file = inline_fields.get("isNewFile") + self.on_removed_code = None if is_new_file is None else not is_new_file + + @property + def is_renderable(self) -> bool: + """Whether the comment carries anything worth showing to a reader. - # Unfortunately, we do not have this information for a limitation - # in Phabricator's API. - self.on_removed_code = None + An inline comment always does: it points at a file and a line even + when the reviewer left the text empty and put everything into a + suggestion. + """ + return not self.removed @property def end_line(self) -> int: @@ -740,7 +764,7 @@ def patch_stack(self) -> list[PatchSet]: @cached_property def _all_comments(self) -> list: - return [c for c in self.get_comments() if c.content.strip()] + return [c for c in self.get_comments() if c.is_renderable] @cached_property def _users_info(self) -> dict[str, dict]: @@ -938,8 +962,21 @@ def to_md(self) -> str: ) md_lines.append("") - md_lines.append(final_comment_content) - md_lines.append("") + if final_comment_content: + md_lines.append(final_comment_content) + md_lines.append("") + + if ( + isinstance(comment, PhabricatorInlineComment) + and comment.suggestion_text + ): + md_lines.append("Suggested replacement:") + md_lines.append("") + md_lines.append("```suggestion") + md_lines.append(comment.suggestion_text) + md_lines.append("```") + md_lines.append("") + md_lines.append("---") md_lines.append("") diff --git a/tests/test_phabricator.py b/tests/test_phabricator.py index 57cb3ab1dd..235f1dfdb8 100644 --- a/tests/test_phabricator.py +++ b/tests/test_phabricator.py @@ -5,6 +5,7 @@ from datetime import timedelta from unittest.mock import MagicMock +from unittest.mock import patch as mock_patch import pytest @@ -530,3 +531,138 @@ async def test_github_repo_unmapped_callsign(monkeypatch) -> None: async def test_github_repo_no_repository_phid() -> None: patch = _FakePatchWithRepo(None) assert await patch.github_repo_ref() is None + + +def _inline_transaction(content: str = "", **inline_fields) -> dict: + fields = { + "diff": {"id": 1351682}, + "path": "browser/components/tabbrowser/docs/gbrowser.md", + "line": 13, + "length": 1, + "replyToCommentPHID": None, + "isDone": True, + } + fields.update(inline_fields) + return { + "id": 11098175, + "type": "inline", + "authorPHID": "PHID-USER-testauthor", + "comments": [ + { + "id": 1710710, + "dateCreated": 1787105886, + "dateModified": 1787133683, + "removed": False, + "content": {"raw": content}, + } + ], + "fields": fields, + } + + +def _to_md(comments: list) -> str: + """Render a revision whose comment timeline is the only thing that varies.""" + revision_metadata = { + "id": 319190, + "phid": "PHID-DREV-test", + "fields": { + "title": "A revision", + "authorPHID": "PHID-USER-testauthor", + "status": {"name": "Needs Review"}, + "uri": "https://phabricator.services.mozilla.com/D319190", + "bugzilla.bug-id": "123456", + "summary": "", + "testPlan": "", + "stackGraph": {}, + }, + } + diff_metadata = { + "id": 1351682, + "dateCreated": 1787105886, + "dateModified": 1787105886, + "baseRevision": "abc123", + "authorPHID": "PHID-USER-testauthor", + } + users_info = { + "PHID-USER-testauthor": { + "email": "author@mozilla.com", + "real_name": "Test Author", + "is_trusted": True, + "is_trusted_bot": False, + } + } + + with ( + mock_patch.object( + phab_platform.PhabricatorPatch, "_revision_metadata", revision_metadata + ), + mock_patch.object( + phab_platform.PhabricatorPatch, "_diff_metadata", diff_metadata + ), + mock_patch.object( + phab_platform.PhabricatorPatch, "get_comments", return_value=comments + ), + mock_patch.object(phab_platform.PhabricatorPatch, "raw_diff", "diff content"), + mock_patch( + "bugbug.tools.core.platforms.phabricator._get_users_info_batch", + return_value=users_info, + ), + ): + return phab_platform.PhabricatorPatch(diff_id=1351682).to_md() + + +def test_to_md_renders_suggestion_only_inline_comment() -> None: + markdown = _to_md( + [ + phab_platform.PhabricatorInlineComment( + _inline_transaction( + hasSuggestion=True, suggestionText="the replacement" + ) + ) + ] + ) + + assert "gbrowser.md` at Line 13" in markdown + assert "```suggestion\nthe replacement\n```" in markdown + + +def test_to_md_keeps_inline_comment_with_neither_text_nor_suggestion() -> None: + # The file and line alone tell a reader a review comment was left there. + markdown = _to_md([phab_platform.PhabricatorInlineComment(_inline_transaction())]) + + assert "gbrowser.md` at Line 13" in markdown + + +def test_to_md_drops_removed_inline_and_empty_general_comments() -> None: + removed_inline = _inline_transaction(hasSuggestion=True, suggestionText="gone") + removed_inline["comments"][0]["removed"] = True + empty_general = _inline_transaction() + empty_general["type"] = "comment" + empty_general["fields"] = {} + + markdown = _to_md( + [ + phab_platform.PhabricatorInlineComment(removed_inline), + phab_platform.PhabricatorGeneralComment(empty_general), + ] + ) + + assert "*No comments*" in markdown + assert "gone" not in markdown + + +def test_sanitizing_untrusted_inline_comment_drops_its_suggestion() -> None: + comment = phab_platform.PhabricatorInlineComment( + _inline_transaction("some text", hasSuggestion=True, suggestionText="payload") + ) + users_info = { + comment.author_phid: {"is_trusted": False, "is_trusted_bot": False}, + } + + sanitized, filtered_count = phab_platform._sanitize_comments([comment], users_info) + + assert filtered_count == 1 + assert sanitized[0].content == phab_platform.UNTRUSTED_CONTENT_REDACTED + assert sanitized[0].suggestion_text is None + # The sanitizer works on a copy, so the original keeps its suggestion. + assert comment.suggestion_text == "payload"