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

@@ -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)
 

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)

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")

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"))

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)
 

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

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
 
 

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."