From 5b14b86d7d81ac3690a7282009c2b42627164665 Mon Sep 17 00:00:00 2001 From: Connor Sheehan Date: Fri, 25 Oct 2024 10:57:13 -0400 Subject: [PATCH] checks: add a warning when a revision has diffs from multiple authors (#438) Add a warning that checks when a revision has diffs from multiple authors associated with it. First we add support to specify a diffs author in the test suite. Then we update `request_extended_revision_data` to pull diffs based on the `revisionPHID` instead of the diff `id`, which returns all diffs for a given revision. We update `gather_involved_phids` to take the list of diffs for a revision and gather all diff authors as involved in the revision. We then add the warning which gathers all the PHIDs that have authored diffs on a revision, and emit a warning when there are multiple authors, including the Phabricator username of each author. --- landoapi/api/stacks.py | 4 +++- landoapi/api/transplants.py | 4 +++- landoapi/revisions.py | 16 ++++++++++++--- landoapi/stacks.py | 15 ++++++++++---- landoapi/transplants.py | 39 +++++++++++++++++++++++++++++++++++-- tests/mocks.py | 8 +++++++- tests/test_stacks.py | 4 ++-- tests/test_transplants.py | 29 +++++++++++++++++++++++++++ 8 files changed, 105 insertions(+), 14 deletions(-) diff --git a/landoapi/api/stacks.py b/landoapi/api/stacks.py index 2b1c705b..448847c6 100644 --- a/landoapi/api/stacks.py +++ b/landoapi/api/stacks.py @@ -37,6 +37,7 @@ from landoapi.stacks import ( RevisionStack, build_stack_graph, + get_diffs_for_revision, request_extended_revision_data, ) from landoapi.transplants import build_stack_assessment_state, run_landing_checks @@ -107,7 +108,8 @@ def get(phab: PhabricatorClient, revision_id: str): involved_phids = set() for revision in stack_data.revisions.values(): - involved_phids.update(gather_involved_phids(revision)) + revision_diffs = get_diffs_for_revision(revision, stack_data.diffs) + involved_phids.update(gather_involved_phids(revision, revision_diffs)) involved_phids = list(involved_phids) diff --git a/landoapi/api/transplants.py b/landoapi/api/transplants.py index ac446a65..e3bc105b 100644 --- a/landoapi/api/transplants.py +++ b/landoapi/api/transplants.py @@ -48,6 +48,7 @@ from landoapi.stacks import ( RevisionStack, build_stack_graph, + get_diffs_for_revision, request_extended_revision_data, ) from landoapi.storage import db @@ -255,7 +256,8 @@ def post(phab: PhabricatorClient, data: dict): revisions = [r[0] for r in to_land] for revision in revisions: - involved_phids.update(gather_involved_phids(revision)) + revision_diffs = get_diffs_for_revision(revision, stack_data.diffs) + involved_phids.update(gather_involved_phids(revision, revision_diffs)) involved_phids = list(involved_phids) users = user_search(phab, involved_phids) diff --git a/landoapi/revisions.py b/landoapi/revisions.py index 6ff73663..53d03c1d 100644 --- a/landoapi/revisions.py +++ b/landoapi/revisions.py @@ -25,15 +25,25 @@ logger = logging.getLogger(__name__) -def gather_involved_phids(revision: dict) -> set[str]: +def gather_involved_phids(revision: dict, revision_diffs: list[dict]) -> set[str]: """Return the set of Phobject phids involved in a revision. - At the time of writing Users and Projects are the type of Phobjects - which may author or review a revision. + Receives a dict representing the revision, and a list of dicts representing every + diff that is associated with that revision. + + Gathers the PHID of the author of the revision, the set of all reviewers on the + revision, and any user who has pushed a diff to a revision. """ attachments = PhabricatorClient.expect(revision, "attachments") entities = {PhabricatorClient.expect(revision, "fields", "authorPHID")} + entities.update( + { + PhabricatorClient.expect(diff, "fields", "authorPHID") + for diff in revision_diffs + } + ) + entities.update( { PhabricatorClient.expect(r, "reviewerPHID") diff --git a/landoapi/stacks.py b/landoapi/stacks.py index 8426fd5a..58bed7d0 100644 --- a/landoapi/stacks.py +++ b/landoapi/stacks.py @@ -41,6 +41,16 @@ def build_stack_graph(revision: dict) -> tuple[set[str], set[tuple[str, str]]]: return phids, edges +def get_diffs_for_revision(revision: dict, all_diffs: dict[str, dict]) -> list[dict]: + """Return diffs associated with the given revision.""" + return [ + diff + for diff in all_diffs.values() + if PhabricatorClient.expect(revision, "phid") + == PhabricatorClient.expect(diff, "fields", "revisionPHID") + ] + + RevisionData = namedtuple("RevisionData", ("revisions", "diffs", "repositories")) @@ -74,11 +84,8 @@ def request_extended_revision_data( diffs = phab.call_conduit( "differential.diff.search", - constraints={ - "phids": [phab.expect(r, "fields", "diffPHID") for r in revs.values()] - }, + constraints={"revisionPHIDs": revision_phids}, attachments={"commits": True}, - limit=len(revs), ) phab.expect(diffs, "data", len(revision_phids) - 1) diffs = result_list_to_phid_dict(phab.expect(diffs, "data")) diff --git a/landoapi/transplants.py b/landoapi/transplants.py index 1ae3137e..de90f97e 100644 --- a/landoapi/transplants.py +++ b/landoapi/transplants.py @@ -52,6 +52,7 @@ from landoapi.stacks import ( RevisionData, RevisionStack, + get_diffs_for_revision, get_landable_repos_for_revision_data, ) from landoapi.transactions import get_inline_comments @@ -583,6 +584,30 @@ def warning_unresolved_comments( return "Revision has unresolved comments." +@RevisionWarningCheck(10, "Revision has multiple authors.") +def warning_multiple_authors( + revision: dict, diff: dict, stack_state: StackAssessmentState +): + """Warn the landing user when a revision has updates from multiple authors.""" + revision_phid = PhabricatorClient.expect(revision, "phid") + + # Get the author PHID for each diff associated with this revision. + author_phids = { + PhabricatorClient.expect(diff, "fields", "authorPHID") + for diff in stack_state.stack_data.diffs.values() + if PhabricatorClient.expect(diff, "fields", "revisionPHID") == revision_phid + } + + if len(author_phids) > 1: + author_usernames = sorted( + reviewer_identity( + author_phid, stack_state.users, stack_state.projects + ).identifier + for author_phid in author_phids + ) + return f"Revision has multiple authors: {', '.join(author_usernames)}." + + def blocker_user_no_auth0_email( stack_state: StackAssessmentState, ) -> Optional[str]: @@ -905,6 +930,7 @@ def blocker_try_task_config( warning_wip_commit_message, warning_code_freeze, warning_unresolved_comments, + warning_multiple_authors, ] @@ -1004,7 +1030,14 @@ def get_parsed_diffs( ) -> dict[int, dict]: """Return a mapping of diff PHID to `rs-parsepatch` parsed `diff --git` content.""" raw_diffs = {} - for diff in stack_data.diffs.values(): + + # Get the latest diffs for each revision. + latest_diffs = [ + stack_data.diffs[phab.expect(revision, "fields", "diffPHID")] + for revision in stack_data.revisions.values() + ] + + for diff in latest_diffs: diff_id = phab.expect(diff, "id") raw_diffs[diff_id] = get_raw_diff_by_id(phab, diff_id) @@ -1032,11 +1065,13 @@ def build_stack_assessment_state( involved_phids = set() reviewers = {} for revision in stack_data.revisions.values(): - involved_phids.update(gather_involved_phids(revision)) + revision_diffs = get_diffs_for_revision(revision, stack_data.diffs) + involved_phids.update(gather_involved_phids(revision, revision_diffs)) reviewers[revision["phid"]] = get_collated_reviewers(revision) # Get more Phabricator data. involved_phids = list(involved_phids) + users = user_search(phab, involved_phids) projects = project_search(phab, involved_phids) diff --git a/tests/mocks.py b/tests/mocks.py index 312990e7..a59eccbb 100644 --- a/tests/mocks.py +++ b/tests/mocks.py @@ -404,6 +404,7 @@ def diff( rawdiff=CANNED_RAW_DEFAULT_DIFF, changes=None, repo=None, + author=None, commits=[ { "identifier": "b15b8fbc79c2c3977aff9e17f0dfcc34c66ec29f", @@ -427,7 +428,12 @@ def diff( uri = "http://phabricator.test/differential/diff/{}/".format(diff_id) revision_id = revision["id"] if revision is not None else None revision_phid = revision["phid"] if revision is not None else None - author_phid = revision["authorPHID"] if revision is not None else None + + if author: + author_phid = author["phid"] + else: + author_phid = revision["authorPHID"] if revision is not None else None + repo_phid = ( repo["phid"] if repo is not None diff --git a/tests/test_stacks.py b/tests/test_stacks.py index 3c9e9eea..a636ebf2 100644 --- a/tests/test_stacks.py +++ b/tests/test_stacks.py @@ -170,7 +170,7 @@ def test_request_extended_revision_data_no_revisions(phabdouble): assert not data.repositories -def test_request_extended_revision_data_gets_latest_diff(phabdouble): +def test_request_extended_revision_data_gets_all_diffs(phabdouble): phab = phabdouble.get_phabricator_client() first_diff = phabdouble.diff() @@ -179,7 +179,7 @@ def test_request_extended_revision_data_gets_latest_diff(phabdouble): data = request_extended_revision_data(phab, [revision["phid"]]) assert revision["phid"] in data.revisions - assert first_diff["phid"] not in data.diffs + assert first_diff["phid"] in data.diffs assert latest_diff["phid"] in data.diffs diff --git a/tests/test_transplants.py b/tests/test_transplants.py index 8d00f4a5..a3ad4e92 100644 --- a/tests/test_transplants.py +++ b/tests/test_transplants.py @@ -30,6 +30,7 @@ blocker_revision_data_classification, blocker_try_task_config, blocker_uplift_approval, + warning_multiple_authors, warning_not_accepted, warning_previously_landed, warning_reviews_not_current, @@ -1881,3 +1882,31 @@ def test_blocker_try_task_config_landing_state_non_try( ) == "Revision introduces the `try_task_config.json` file." ), "`try_task_config.json` should be rejected." + + +def test_warning_multiple_authors(phabdouble, mocked_repo_config, create_state): + repo = phabdouble.repo() + + # Create two users. + alice = phabdouble.user(username="alice") + bob = phabdouble.user(username="bob") + + # Create one revision. + revision = phabdouble.revision(repo=repo, author=alice) + + # Create multiple diffs on the revision, one from each author. + phabdouble.diff(revision=revision, author=alice) + diff2 = phabdouble.diff(revision=revision, author=bob) + + phab_revision = phabdouble.api_object_for( + revision, + attachments={"reviewers": True, "reviewers-extra": True, "projects": True}, + ) + + stack_state = create_state(phab_revision) + + warning = warning_multiple_authors(phab_revision, diff2, stack_state) + assert warning is not None + assert ( + warning.details == "Revision has multiple authors: alice, bob." + ), "Multiple authors on a revision should return a warning."