Merge "MergeMetrics: Fix change/submitted_with_rebaser_approval metric"

java/com/google/gerrit/server/submit/MergeMetrics.java

diff --git a/java/com/google/gerrit/server/submit/MergeMetrics.java b/java/com/google/gerrit/server/submit/MergeMetrics.java
index 4c7a197..4c11925 100644
--- a/java/com/google/gerrit/server/submit/MergeMetrics.java
+++ b/java/com/google/gerrit/server/submit/MergeMetrics.java
@@ -106,9 +106,18 @@
             submitRequirementChangequeryBuilder
                 .get()
                 .parse(submitRequirement.submittabilityExpression().expressionString());
-        return ignoresCodeReviewApprovalsOfUploader(predicate);
+        boolean ignoresCodeReviewApprovalsOfUploader =
+            ignoresCodeReviewApprovalsOfUploader(predicate);
+        logger.atFine().log(
+            "ignoresCodeReviewApprovalsOfUploader = %s", ignoresCodeReviewApprovalsOfUploader);
+        if (ignoresCodeReviewApprovalsOfUploader) {
+          return true;
+        }
       } catch (QueryParseException e) {
-        return false;
+        logger.atFine().log(
+            "Failed to parse submit requirement expression %s: %s",
+            submitRequirement.submittabilityExpression().expressionString(), e.getMessage());
+        // ignore and inspect the next submit requirement
       }
     }
     return false;

javatests/com/google/gerrit/acceptance/api/change/RebaseOnBehalfOfUploaderIT.java

diff --git a/javatests/com/google/gerrit/acceptance/api/change/RebaseOnBehalfOfUploaderIT.java b/javatests/com/google/gerrit/acceptance/api/change/RebaseOnBehalfOfUploaderIT.java
index 8565ced..1b6956c 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/RebaseOnBehalfOfUploaderIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/RebaseOnBehalfOfUploaderIT.java
@@ -21,6 +21,8 @@
 import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.block;
 import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
 import static com.google.gerrit.server.notedb.ChangeNoteFooters.FOOTER_REAL_USER;
+import static com.google.gerrit.server.project.testing.TestLabels.labelBuilder;
+import static com.google.gerrit.server.project.testing.TestLabels.value;
 import static com.google.gerrit.testing.GerritJUnit.assertThrows;
 
 import com.google.common.collect.Iterables;
@@ -37,6 +39,8 @@
 import com.google.gerrit.entities.Account;
 import com.google.gerrit.entities.AccountGroup;
 import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.LabelId;
+import com.google.gerrit.entities.LabelType;
 import com.google.gerrit.entities.PatchSet;
 import com.google.gerrit.entities.Permission;
 import com.google.gerrit.entities.RefNames;
@@ -1025,11 +1029,25 @@
 
   @Test
   public void testSubmittedWithRebaserApprovalMetric() throws Exception {
+    allowVotingOnCodeReviewToAllUsers();
+
+    createVerifiedLabel();
+    allowVotingOnVerifiedToAllUsers();
+
     // Require a Code-Review approval from a non-uploader for submit.
     try (ProjectConfigUpdate u = updateProject(project)) {
       u.getConfig()
           .upsertSubmitRequirement(
               SubmitRequirement.builder()
+                  .setName(TestLabels.verified().getName())
+                  .setSubmittabilityExpression(
+                      SubmitRequirementExpression.create(
+                          String.format("label:%s=MAX", TestLabels.verified().getName())))
+                  .setAllowOverrideInChildProjects(false)
+                  .build());
+      u.getConfig()
+          .upsertSubmitRequirement(
+              SubmitRequirement.builder()
                   .setName(TestLabels.codeReview().getName())
                   .setSubmittabilityExpression(
                       SubmitRequirementExpression.create(
@@ -1056,7 +1074,10 @@
 
     // Approve and submit the change that will be the new base for the change that will be rebased.
     requestScopeOperations.setApiUser(approver);
-    gApi.changes().id(changeToBeTheNewBase.get()).current().review(ReviewInput.approve());
+    gApi.changes()
+        .id(changeToBeTheNewBase.get())
+        .current()
+        .review(ReviewInput.approve().label(TestLabels.verified().getName(), 1));
     testMetricMaker.reset();
     gApi.changes().id(changeToBeTheNewBase.get()).current().submit();
     assertThat(testMetricMaker.getCount("change/submitted_with_rebaser_approval")).isEqualTo(0);
@@ -1068,8 +1089,10 @@
     gApi.changes().id(changeToBeRebased.get()).rebase(rebaseInput);
 
     // Approve the change as the rebaser.
-    allowVotingOnCodeReviewToAllUsers();
-    gApi.changes().id(changeToBeRebased.get()).current().review(ReviewInput.approve());
+    gApi.changes()
+        .id(changeToBeRebased.get())
+        .current()
+        .review(ReviewInput.approve().label(TestLabels.verified().getName(), 1));
 
     // The change is submittable because the approval is from a user (the rebaser) that is not the
     // uploader.
@@ -1136,6 +1159,29 @@
         .update();
   }
 
+  private void createVerifiedLabel() throws Exception {
+    try (ProjectConfigUpdate u = updateProject(project)) {
+      LabelType.Builder verified =
+          labelBuilder(
+                  LabelId.VERIFIED, value(1, "Passes"), value(0, "No score"), value(-1, "Failed"))
+              .setCopyCondition("is:MIN");
+      u.getConfig().upsertLabelType(verified.build());
+      u.save();
+    }
+  }
+
+  private void allowVotingOnVerifiedToAllUsers() {
+    projectOperations
+        .project(project)
+        .forUpdate()
+        .add(
+            allowLabel(TestLabels.verified().getName())
+                .ref("refs/*")
+                .group(REGISTERED_USERS)
+                .range(-1, 1))
+        .update();
+  }
+
   private void blockPermissionForAllUsers(String permission) {
     blockPermission(permission, REGISTERED_USERS);
   }