Support disallowing voting on labels after submission

For common labels like Code-Review and Verified, it still makes sense
to allow voting after submission, so keep the default as true. For
others, such as a label to trigger CI, it doesn't.

Bug: Issue 4942
Change-Id: I01a6281c11ee13bec9a6c8a2568b18ddd4eaccf4

Documentation/config-labels.txt

@@ -230,6 +230,19 @@ accounts could lock the same change.
 Allowed range of values are 0 (Patch Set Unlocked) to 1 (Patch Set
 Locked).
 
+[[label_allowPostSubmit]]
+=== `label.Label-Name.allowPostSubmit`
+
+If true, the label may be voted on for changes that have already been
+submitted. If false, the label will not appear in the UI and will not
+be accepted when reviewing a closed change.
+
+In either case, voting on a label after submission is only permitted if
+the new vote is at least as high as the old vote by that user. This
+avoids creating the false impression that a post-submit vote can change
+the past and affect submission somehow.
+
+Defaults to true.
 
 [[label_copyMinScore]]
 === `label.Label-Name.copyMinScore`

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/CustomLabelIT.java

@@ -18,6 +18,7 @@ import static com.google.common.truth.Truth.assertThat;
 import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS;
 import static com.google.gerrit.server.project.Util.category;
 import static com.google.gerrit.server.project.Util.value;
+import static java.util.stream.Collectors.toList;
 
 import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.NoHttpd;
@@ -26,11 +27,13 @@ import com.google.gerrit.common.data.LabelType;
 import com.google.gerrit.common.data.Permission;
 import com.google.gerrit.extensions.api.changes.AddReviewerInput;
 import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.client.ListChangesOption;
 import com.google.gerrit.extensions.common.ChangeInfo;
 import com.google.gerrit.extensions.common.LabelInfo;
 import com.google.gerrit.extensions.events.CommentAddedListener;
 import com.google.gerrit.extensions.registration.DynamicSet;
 import com.google.gerrit.extensions.registration.RegistrationHandle;
+import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.server.git.ProjectConfig;
 import com.google.gerrit.server.group.SystemGroupBackend;
@@ -41,6 +44,9 @@ import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
 
+import java.util.Arrays;
+import java.util.Collection;
+
 @NoHttpd
 public class CustomLabelIT extends AbstractDaemonTest {
 
@@ -175,6 +181,49 @@ public class CustomLabelIT extends AbstractDaemonTest {
     assertThat(q.blocking).isTrue();
   }
 
+  @Test
+  public void customLabel_DisallowPostSubmit() throws Exception {
+    label.setFunctionName("NoOp");
+    label.setAllowPostSubmit(false);
+    P.setFunctionName("NoOp");
+    saveLabelConfig();
+
+    PushOneCommit.Result r = createChange();
+    revision(r).review(ReviewInput.approve());
+    revision(r).submit();
+
+    ChangeInfo info = get(r.getChangeId(), ListChangesOption.DETAILED_LABELS);
+    // TODO(dborowitz): Don't claim reducing vote is allowed.
+    assertPermitted(info, "Code-Review", -2, -1, 0, 1, 2);
+    assertPermitted(info, P.getName(), 0, 1);
+    assertPermitted(info, label.getName());
+
+    ReviewInput in = new ReviewInput();
+    in.label(P.getName(), P.getMax().getValue());
+    revision(r).review(in);
+
+    in = new ReviewInput();
+    in.label(label.getName(), label.getMax().getValue());
+    exception.expect(ResourceConflictException.class);
+    exception.expectMessage(
+        "Voting on labels disallowed after submit: " + label.getName());
+    revision(r).review(in);
+  }
+
+  private void assertPermitted(ChangeInfo info, String label,
+      Integer... expected) {
+    assertThat(info.permittedLabels).isNotNull();
+    Collection<String> strs = info.permittedLabels.get(label);
+    if (expected.length == 0) {
+      assertThat(strs).isNull();
+    } else {
+      assertThat(
+              strs.stream().map(s -> Integer.valueOf(s.trim()))
+                  .collect(toList()))
+          .containsExactlyElementsIn(Arrays.asList(expected));
+    }
+  }
+
   private void saveLabelConfig() throws Exception {
     ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
     cfg.getLabelSections().put(label.getName(), label);

gerrit-common/src/main/java/com/google/gerrit/common/data/LabelType.java

@@ -25,6 +25,7 @@ import java.util.List;
 import java.util.Map;
 
 public class LabelType {
+  public static final boolean DEF_ALLOW_POST_SUBMIT = true;
   public static final boolean DEF_CAN_OVERRIDE = true;
   public static final boolean DEF_COPY_ALL_SCORES_IF_NO_CHANGE = true;
   public static final boolean DEF_COPY_ALL_SCORES_IF_NO_CODE_CHANGE = false;
@@ -104,6 +105,7 @@ public class LabelType {
   protected boolean copyAllScoresOnTrivialRebase;
   protected boolean copyAllScoresIfNoCodeChange;
   protected boolean copyAllScoresIfNoChange;
+  protected boolean allowPostSubmit;
   protected short defaultValue;
 
   protected List<LabelValue> values;
@@ -144,6 +146,7 @@ public class LabelType {
         DEF_COPY_ALL_SCORES_ON_MERGE_FIRST_PARENT_UPDATE);
     setCopyMaxScore(DEF_COPY_MAX_SCORE);
     setCopyMinScore(DEF_COPY_MIN_SCORE);
+    setAllowPostSubmit(DEF_ALLOW_POST_SUBMIT);
   }
 
   public String getName() {
@@ -174,6 +177,14 @@ public class LabelType {
     this.canOverride = canOverride;
   }
 
+  public boolean allowPostSubmit() {
+    return allowPostSubmit;
+  }
+
+  public void setAllowPostSubmit(boolean allowPostSubmit) {
+    this.allowPostSubmit = allowPostSubmit;
+  }
+
   public void setRefPatterns(List<String> refPatterns) {
     this.refPatterns = refPatterns;
   }

gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java

@@ -906,6 +906,10 @@ public class ChangeJson {
         if (type == null) {
           continue;
         }
+        if (ctl.getChange().getStatus() == Change.Status.MERGED
+            && !type.allowPostSubmit()) {
+          continue;
+        }
         PermissionRange range = ctl.getRange(Permission.forLabel(r.label));
         for (LabelValue v : type.getValues()) {
           if (range.contains(v.getValue())) {

gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java

@@ -900,10 +900,15 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
       // make it possible to take a merged change and make it no longer
       // submittable.
       List<PatchSetApproval> reduced = new ArrayList<>(ups.size() + del.size());
+      List<String> disallowed =
+          new ArrayList<>(labelTypes.getLabelTypes().size());
       reduced.addAll(del);
       for (PatchSetApproval psa : ups) {
         LabelType lt = checkNotNull(labelTypes.byLabel(psa.getLabel()));
         String normName = lt.getName();
+        if (!lt.allowPostSubmit()) {
+          disallowed.add(normName);
+        }
         Short prev = previous.get(normName);
         if (prev == null) {
           continue;
@@ -918,6 +923,12 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
         }
       }
 
+      if (!disallowed.isEmpty()) {
+        throw new ResourceConflictException(
+            "Voting on labels disallowed after submit: "
+                + disallowed.stream().distinct().sorted()
+                    .collect(joining(", ")));
+      }
       if (!reduced.isEmpty()) {
         throw new ResourceConflictException(
             "Cannot reduce vote on labels for closed change: "

gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java

@@ -144,6 +144,7 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
   private static final String KEY_FUNCTION = "function";
   private static final String KEY_DEFAULT_VALUE = "defaultValue";
   private static final String KEY_COPY_MIN_SCORE = "copyMinScore";
+  private static final String KEY_ALLOW_POST_SUBMIT = "allowPostSubmit";
   private static final String KEY_COPY_MAX_SCORE = "copyMaxScore";
   private static final String KEY_COPY_ALL_SCORES_ON_MERGE_FIRST_PARENT_UPDATE = "copyAllScoresOnMergeFirstParentUpdate";
   private static final String KEY_COPY_ALL_SCORES_ON_TRIVIAL_REBASE = "copyAllScoresOnTrivialRebase";
@@ -802,6 +803,9 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
               KEY_DEFAULT_VALUE, dv, name)));
         }
       }
+      label.setAllowPostSubmit(
+          rc.getBoolean(LABEL, name, KEY_ALLOW_POST_SUBMIT,
+              LabelType.DEF_ALLOW_POST_SUBMIT));
       label.setCopyMinScore(
           rc.getBoolean(LABEL, name, KEY_COPY_MIN_SCORE,
               LabelType.DEF_COPY_MIN_SCORE));
@@ -1197,6 +1201,8 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
       rc.setString(LABEL, name, KEY_FUNCTION, label.getFunctionName());
       rc.setInt(LABEL, name, KEY_DEFAULT_VALUE, label.getDefaultValue());
 
+      setBooleanConfigKey(rc, name, KEY_ALLOW_POST_SUBMIT, label.allowPostSubmit(),
+          LabelType.DEF_ALLOW_POST_SUBMIT);
       setBooleanConfigKey(rc, name, KEY_COPY_MIN_SCORE, label.isCopyMinScore(),
           LabelType.DEF_COPY_MIN_SCORE);
       setBooleanConfigKey(rc, name, KEY_COPY_MAX_SCORE, label.isCopyMaxScore(),