From a0fbc05a1ed58d15beb52cd5050fb370349f3834 Mon Sep 17 00:00:00 2001 From: Morgan Rae Reschenberg Date: Fri, 15 May 2026 15:16:09 -0700 Subject: [PATCH] Add accessibility-frontend-reviewers workflow --- bot/code_review_bot/report/phabricator.py | 4 + bot/code_review_bot/tasks/a11y_frontend.py | 165 ++++++++++++++++++ bot/tests/conftest.py | 8 + bot/tests/test_a11y_frontend.py | 192 +++++++++++++++++++++ 4 files changed, 369 insertions(+) create mode 100644 bot/code_review_bot/tasks/a11y_frontend.py create mode 100644 bot/tests/test_a11y_frontend.py diff --git a/bot/code_review_bot/report/phabricator.py b/bot/code_review_bot/report/phabricator.py index f0c7cf0f8..a1cdb2faf 100644 --- a/bot/code_review_bot/report/phabricator.py +++ b/bot/code_review_bot/report/phabricator.py @@ -13,6 +13,7 @@ from code_review_bot.backend import BackendAPI from code_review_bot.report.base import Reporter from code_review_bot.revisions import PhabricatorRevision +from code_review_bot.tasks.a11y_frontend import handle_a11y_review_group from code_review_bot.tasks.clang_tidy_external import ExternalTidyIssue from code_review_bot.tasks.coverage import CoverageIssue from code_review_bot.tools import treeherder @@ -194,6 +195,9 @@ def publish(self, issues, revision, task_failures, notices, reviewers): ], ) + # Handle the accessibility-frontend-reviewers group workflow. + handle_a11y_review_group(self.api, revision) + # Use only new and publishable issues and patches # Avoid publishing a patch from a de-activated analyzer publishable_issues = [ diff --git a/bot/code_review_bot/tasks/a11y_frontend.py b/bot/code_review_bot/tasks/a11y_frontend.py new file mode 100644 index 000000000..563e2d65e --- /dev/null +++ b/bot/code_review_bot/tasks/a11y_frontend.py @@ -0,0 +1,165 @@ +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, You can obtain one at http://mozilla.org/MPL/2.0/. + +import structlog + +logger = structlog.get_logger(__name__) + +A11Y_GROUP_SLUG = "accessibility-frontend-reviewers" + +# Used to detect whether the bot has already handled this revision, +# by searching prior comments for this marker text. +COMMENT_MARKER = "You've tagged the accessibility team for review." + +COMMENT_A11Y_REVIEW = """You've tagged the accessibility team for review. + +If you've already spoken to a team member about your request, please request review from the specific team member you spoke to. + +Otherwise, please do ALL of the following: + * Post a comment describing the specific accessibility behaviour you're looking for feedback about + * Attach screenshots showing behaviour before and after your patch (if this is a visual change) + * Post a description of screen reader output/behaviour before and after your patch (if this change affects assistive technology users) + +Once you have posted all of the above, please re-add the group. The group will not be removed again after you re-add it if the above conditions are met. +""" + + +def handle_a11y_review_group(api, revision): + """ + Workflow for the #accessibility-frontend-reviewers group: + + 1. If the group is not currently tagged on the revision, do nothing. + 2. If the group was added by an accessibility team member (i.e. a member of the + accessibility-frontend-reviewers project in Phabricator), leave it alone. + 3. If this is the FIRST time a non-team-member has tagged the group (no prior bot comment): + - Post the instructions comment. + - Remove the group from reviewers. + 4. If the author re-added the group after the bot removed it, check whether they + have followed the instructions by verifying at least one of: + - A comment was posted by the revision author after the bot's removal. + - An individual accessibility team member has been tagged as a reviewer. + If either condition holds, leave the group in place. + If neither holds, re-post the comment and remove the group. + + Prior bot handling is detected by searching the revision's transaction history + for a comment authored by this bot that contains COMMENT_MARKER. + """ + # Resolve the group's PHID and fetch the group's current member list. + data = api.search_projects(slugs=[A11Y_GROUP_SLUG], attachments={"members": True}) + if not data or "phid" not in data[0]: + logger.warning( + "Unable to find PHID for accessibility reviewer group", + slug=A11Y_GROUP_SLUG, + ) + return + + group_phid = data[0]["phid"] + team_member_phids = { + m["phid"] + for m in data[0].get("attachments", {}).get("members", {}).get("members", []) + } + + # Verify the group is tagged for the current revision. + revision_data = api.load_revision( + rev_id=revision.phabricator_id, attachments={"reviewers": True} + ) + group_reviewer_entry = next( + ( + r + for r in revision_data.get("attachments", {}) + .get("reviewers", {}) + .get("reviewers", []) + if r["reviewerPHID"] == group_phid + ), + None, + ) + + if group_reviewer_entry is None: + return + + # If an accessibility team member added the group, leave the group assigned and exit the workflow. + actor_phid = group_reviewer_entry.get("actorPHID") + if actor_phid and actor_phid in team_member_phids: + logger.info( + "Accessibility reviewer group added by team member, taking no action", + revision_id=revision.phabricator_id, + actor_phid=actor_phid, + ) + return + + # Group is present and was added by a non-team-member. Search the revision's + # transaction history for a prior bot comment containing COMMENT_MARKER. + # If found, the bot has already handled this revision once (this is a re-add). + bot_phid = api.user["phid"] + all_reviewers = ( + revision_data.get("attachments", {}).get("reviewers", {}).get("reviewers", []) + ) + transactions = api.request( + "transaction.search", objectIdentifier=revision.phabricator_phid + ) + transaction_data = transactions.get("data", []) + + bot_comment_timestamp = None + for transaction in transaction_data: + if transaction.get("authorPHID") != bot_phid: + continue + for comment in transaction.get("comments", []): + if COMMENT_MARKER in comment.get("content", {}).get("raw", ""): + bot_comment_timestamp = transaction["dateCreated"] + break + if bot_comment_timestamp is not None: + break + + if bot_comment_timestamp is not None: + # This is a re-add by a non-team-member (team member adds are handled above). + # Verify the author has followed the instructions before allowing the group to stay. + + # Check 1: did the revision author post a comment after the bot's removal? + author_phid = revision_data["fields"]["authorPHID"] + has_followup_comment = any( + any(c.get("content", {}).get("raw") for c in tx.get("comments", [])) + for tx in transaction_data + if tx.get("authorPHID") == author_phid + and tx.get("dateCreated", 0) > bot_comment_timestamp + ) + + # Check 2: has an individual accessibility team member been tagged? + current_reviewer_phids = {r["reviewerPHID"] for r in all_reviewers} + has_individual_team_reviewer = bool(current_reviewer_phids & team_member_phids) + + if has_followup_comment or has_individual_team_reviewer: + logger.info( + "Accessibility reviewer group legitimately re-added, taking no action", + revision_id=revision.phabricator_id, + has_followup_comment=has_followup_comment, + has_individual_team_reviewer=has_individual_team_reviewer, + ) + return + + # Premature re-add: re-post the comment so the author knows why the group + # is being removed again, then remove it. + logger.info( + "Accessibility reviewer group re-added without required context, removing group again", + revision_id=revision.phabricator_id, + ) + api.comment(revision.phabricator_id, COMMENT_A11Y_REVIEW) + api.edit_revision( + revision.phabricator_id, + [{"type": "reviewers.remove", "value": [group_phid]}], + ) + return + + # First time the group has been tagged by a non-team-member: post the + # instructions comment and remove the group. + logger.info( + "Accessibility reviewer group tagged for first time, posting comment and removing group", + revision_id=revision.phabricator_id, + ) + + api.comment(revision.phabricator_id, COMMENT_A11Y_REVIEW) + + api.edit_revision( + revision.phabricator_id, + [{"type": "reviewers.remove", "value": [group_phid]}], + ) diff --git a/bot/tests/conftest.py b/bot/tests/conftest.py index 35f6a2552..a8df9d8b8 100644 --- a/bot/tests/conftest.py +++ b/bot/tests/conftest.py @@ -969,6 +969,14 @@ def search_projects(self, request): result = [ {"phid": "PHID-123456789-TGReviewers", "slug": "taskgraph-reviewers"} ] + elif params["constraints"]["slugs"] == ["accessibility-frontend-reviewers"]: + result = [ + { + "phid": "PHID-123456789-A11yReviewers", + "slug": "accessibility-frontend-reviewers", + "attachments": {"members": {"members": []}}, + } + ] return ( 201, diff --git a/bot/tests/test_a11y_frontend.py b/bot/tests/test_a11y_frontend.py new file mode 100644 index 000000000..605450f14 --- /dev/null +++ b/bot/tests/test_a11y_frontend.py @@ -0,0 +1,192 @@ +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, You can obtain one at http://mozilla.org/MPL/2.0/. + +from unittest.mock import MagicMock + +import pytest + +from code_review_bot.tasks.a11y_frontend import ( + A11Y_GROUP_SLUG, + COMMENT_A11Y_REVIEW, + COMMENT_MARKER, + handle_a11y_review_group, +) + +GROUP_PHID = "PHID-PROJ-A11yReviewers" +TEAM_MEMBER_PHID = "PHID-USER-TeamMember" +NON_MEMBER_PHID = "PHID-USER-NonMember" +BOT_PHID = "PHID-USER-Bot" +REVISION_ID = 51 +REVISION_PHID = "PHID-DREV-test" + + +@pytest.fixture +def api(): + mock = MagicMock() + mock.user = {"phid": BOT_PHID} + return mock + + +@pytest.fixture +def revision(): + mock = MagicMock() + mock.phabricator_id = REVISION_ID + mock.phabricator_phid = REVISION_PHID + return mock + + +def _group_project(members=None): + """Return a mock search_projects result for the a11y group.""" + return [ + { + "phid": GROUP_PHID, + "slug": A11Y_GROUP_SLUG, + "attachments": { + "members": {"members": [{"phid": p} for p in (members or [])]} + }, + } + ] + + +def _revision_with_reviewer(reviewer_phid, actor_phid, extra_reviewers=None): + """Return a mock load_revision result with the given reviewer.""" + reviewers = [{"reviewerPHID": reviewer_phid, "actorPHID": actor_phid}] + for phid in extra_reviewers or []: + reviewers.append({"reviewerPHID": phid, "actorPHID": phid}) + return { + "fields": {"authorPHID": NON_MEMBER_PHID}, + "attachments": {"reviewers": {"reviewers": reviewers}}, + } + + +def _revision_without_reviewer(): + """Return a mock load_revision result with no reviewers.""" + return { + "fields": {"authorPHID": NON_MEMBER_PHID}, + "attachments": {"reviewers": {"reviewers": []}}, + } + + +def _transactions(bot_comment_ts=None, author_comment_ts=None): + """Build a minimal transaction.search result.""" + data = [] + if bot_comment_ts is not None: + data.append( + { + "authorPHID": BOT_PHID, + "dateCreated": bot_comment_ts, + "comments": [{"content": {"raw": COMMENT_MARKER + " extra text"}}], + } + ) + if author_comment_ts is not None: + data.append( + { + "authorPHID": NON_MEMBER_PHID, + "dateCreated": author_comment_ts, + "comments": [ + {"content": {"raw": "Here is my accessibility description."}} + ], + } + ) + return {"data": data} + + +def test_first_time_tagging(api, revision): + """First-time tag by a non-team-member: post comment and remove group.""" + api.search_projects.return_value = _group_project(members=[]) + api.load_revision.return_value = _revision_with_reviewer( + GROUP_PHID, NON_MEMBER_PHID + ) + api.request.return_value = _transactions() + + handle_a11y_review_group(api, revision) + + api.comment.assert_called_once_with(REVISION_ID, COMMENT_A11Y_REVIEW) + api.edit_revision.assert_called_once_with( + REVISION_ID, [{"type": "reviewers.remove", "value": [GROUP_PHID]}] + ) + + +def test_team_member_tagging(api, revision): + """Group added by an accessibility team member: leave it in place.""" + api.search_projects.return_value = _group_project(members=[TEAM_MEMBER_PHID]) + api.load_revision.return_value = _revision_with_reviewer( + GROUP_PHID, TEAM_MEMBER_PHID + ) + + handle_a11y_review_group(api, revision) + + api.comment.assert_not_called() + api.edit_revision.assert_not_called() + + +def test_legitimate_readd_via_comment(api, revision): + """Re-add after the author posted a followup comment: group stays.""" + api.search_projects.return_value = _group_project(members=[]) + api.load_revision.return_value = _revision_with_reviewer( + GROUP_PHID, NON_MEMBER_PHID + ) + # Bot commented at t=100, author commented at t=200 (after removal) + api.request.return_value = _transactions(bot_comment_ts=100, author_comment_ts=200) + + handle_a11y_review_group(api, revision) + + api.comment.assert_not_called() + api.edit_revision.assert_not_called() + + +def test_legitimate_readd_via_individual_team_reviewer(api, revision): + """Re-add after the author tagged an individual team member: group stays.""" + api.search_projects.return_value = _group_project(members=[TEAM_MEMBER_PHID]) + # Group is tagged (by non-member), AND individual team member is also a reviewer + api.load_revision.return_value = _revision_with_reviewer( + GROUP_PHID, NON_MEMBER_PHID, extra_reviewers=[TEAM_MEMBER_PHID] + ) + # Bot previously commented, but no author followup comment + api.request.return_value = _transactions(bot_comment_ts=100) + + handle_a11y_review_group(api, revision) + + api.comment.assert_not_called() + api.edit_revision.assert_not_called() + + +def test_premature_readd(api, revision): + """Re-add without followup comment or individual team member: remove group again.""" + api.search_projects.return_value = _group_project(members=[]) + api.load_revision.return_value = _revision_with_reviewer( + GROUP_PHID, NON_MEMBER_PHID + ) + # Bot previously commented, no author followup, no individual team reviewer + api.request.return_value = _transactions(bot_comment_ts=100) + + handle_a11y_review_group(api, revision) + + api.comment.assert_called_once_with(REVISION_ID, COMMENT_A11Y_REVIEW) + api.edit_revision.assert_called_once_with( + REVISION_ID, [{"type": "reviewers.remove", "value": [GROUP_PHID]}] + ) + + +def test_group_not_present(api, revision): + """Group not on the revision: do nothing.""" + api.search_projects.return_value = _group_project(members=[]) + api.load_revision.return_value = _revision_without_reviewer() + + handle_a11y_review_group(api, revision) + + api.comment.assert_not_called() + api.edit_revision.assert_not_called() + api.request.assert_not_called() + + +def test_group_phid_not_found(api, revision): + """Project search returns nothing: log warning and do nothing.""" + api.search_projects.return_value = [] + + handle_a11y_review_group(api, revision) + + api.load_revision.assert_not_called() + api.comment.assert_not_called() + api.edit_revision.assert_not_called()