Give details about ignored reviews

Depending on the configuration, not all reviews are considered for imports of PRs. This can be confusing for users. FIXED=179255584 PiperOrigin-RevId: 355443701 Change-Id: Ib5a4f5f5b173ad1798ea049a563b53db17b18d38
google · Feb 4, 2021 · 70b7735 · 70b7735
1 parent 8f7ce46
commit 70b7735
Show file tree

Hide file tree

Showing 3 changed files with 44 additions and 8 deletions.
diff --git a/java/com/google/copybara/git/BUILD b/java/com/google/copybara/git/BUILD
@@ -60,6 +60,7 @@ java_library(
         "//java/com/google/copybara/util",
         "//java/com/google/copybara/util:origin_util",
         "//java/com/google/copybara/util/console",
+        "//third_party:autovalue",
         "//third_party:flogger",
         "//third_party:google_http_client",
         "//third_party:guava",

diff --git a/java/com/google/copybara/git/GitHubPROrigin.java b/java/com/google/copybara/git/GitHubPROrigin.java
@@ -22,13 +22,16 @@
 import static com.google.copybara.git.github.util.GitHubUtil.asHeadRef;
 import static com.google.copybara.git.github.util.GitHubUtil.asMergeRef;
 
+import com.google.auto.value.AutoValue;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.CharMatcher;
+import com.google.common.base.Joiner;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Splitter;
 import com.google.common.collect.Collections2;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableListMultimap;
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSetMultimap;
 import com.google.common.collect.Iterables;
@@ -321,12 +324,22 @@ private GitRevision getRevisionForPR(String project, PullRequest prData)
     }
     if (reviewState != null) {
       ImmutableList<Review> reviews = api.getReviews(project, prNumber);
-      if (!gitHubPrOriginOptions.forceImport
-          && !reviewState.shouldMigrate(reviews, reviewApprovers, prData.getHead().getSha())) {
+      ApproverState shouldMigrate =
+          reviewState.shouldMigrate(reviews, reviewApprovers, prData.getHead().getSha());
+      if (!gitHubPrOriginOptions.forceImport && !shouldMigrate.shouldMigrate()) {
+        String rejected = "";
+        if (!shouldMigrate.rejectedReviews().isEmpty()) {
+          rejected = String.format("\nThe following reviews were ignored because they don't meet "
+                  + "the association requirement of %s:\n%s",
+              Joiner.on(", ").join(reviewApprovers),
+              shouldMigrate.rejectedReviews().entrySet().stream()
+                  .map(e -> String.format("User %s - Association: %s", e.getKey(), e.getValue()))
+                  .collect(Collectors.joining("\n")));
+        }
         throw new EmptyChangeException(String.format(
             "Cannot migrate http://github.com/%s/pull/%d because it is missing the required"
-                + " approvals (origin is configured as %s)",
-            project, prNumber, reviewState));
+                + " approvals (origin is configured as %s).%s",
+            project, prNumber, reviewState, rejected));
       }
       Set<String> approvers = new HashSet<>();
       Set<String> others = new HashSet<>();
@@ -670,16 +683,33 @@ boolean shouldMigrate(ImmutableList<Review> reviews, String sha) {
       }
     };
 
-    boolean shouldMigrate(ImmutableList<Review> reviews,
+    ApproverState shouldMigrate(ImmutableList<Review> reviews,
         ImmutableSet<AuthorAssociation> approvers, String sha) {
-      return shouldMigrate(
+      return ApproverState.create(shouldMigrate(
           reviews.stream()
-              // Only take into acccount reviews by valid approverTypes
+              // Only take into account reviews by valid approverTypes
               .filter(e -> approvers.contains(e.getAuthorAssociation()))
               .collect(toImmutableList()),
-          sha);
+          sha),
+          reviews.stream()
+              .filter(e -> !approvers.contains(e.getAuthorAssociation()))
+              .collect(toImmutableList()));
     }
 
     abstract boolean shouldMigrate(ImmutableList<Review> reviews, String sha);
   }
+
+  @AutoValue
+  abstract static class ApproverState {
+    public abstract boolean shouldMigrate();
+    public abstract ImmutableMap<String, String> rejectedReviews();
+
+    public static ApproverState create(
+        boolean shouldMigrate, ImmutableList<Review> rejectedReviews) {
+      return new AutoValue_GitHubPROrigin_ApproverState(
+          shouldMigrate,
+          rejectedReviews.stream().collect(ImmutableMap.toImmutableMap(
+              r -> r.getUser().getLogin(), r -> r.getAuthorAssociation().toString())));
+    }
+  }
 }
diff --git a/javatests/com/google/copybara/git/GitHubPrOriginTest.java b/javatests/com/google/copybara/git/GitHubPrOriginTest.java
@@ -771,6 +771,11 @@ public void testReviewApprovers() throws Exception {
                     "review_state = 'HEAD_COMMIT_APPROVED'",
                     "review_approvers = [\"MEMBER\", \"OWNER\"]"));
     assertThat(e).hasMessageThat().contains("missing the required approvals");
+    assertThat(e).hasMessageThat().contains("MEMBER");
+    assertThat(e).hasMessageThat().contains("OWNER");
+    assertThat(e).hasMessageThat()
+        .contains("User APPROVED_COLLABORATOR - Association: COLLABORATOR");
+    assertThat(e).hasMessageThat().contains("User COMMENTED_OTHER - Association: NONE");
 
     GitRevision hasReviewers = checkReviewApprovers("review_state = 'ANY_COMMIT_APPROVED'",
         "review_approvers = [\"MEMBER\", \"OWNER\"]");