ChangeJson: Only list increasing values as permitted post-submit

Change-Id: Iddffea00e263007d1284fb0337474a9263d986fd

gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java

@@ -20,6 +20,7 @@ import static com.google.gerrit.extensions.api.changes.SubmittedTogetherOption.N
 import static com.google.gerrit.reviewdb.client.Patch.COMMIT_MSG;
 import static com.google.gerrit.reviewdb.client.Patch.MERGE_LIST;
 import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
+import static java.util.stream.Collectors.toList;
 import static org.eclipse.jgit.lib.Constants.HEAD;
 
 import com.google.common.base.Strings;
@@ -139,6 +140,7 @@ import java.io.IOException;
 import java.io.InputStream;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.EnumSet;
 import java.util.HashMap;
@@ -1148,4 +1150,18 @@ public abstract class AbstractDaemonTest {
     grant(Permission.SUBMIT, project, "refs/for/refs/heads/*");
     return cloneProject(project);
   }
+
+  protected 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));
+    }
+  }
 }

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java

@@ -2304,6 +2304,8 @@ public class ChangeIT extends AbstractDaemonTest {
         .containsExactly("Code-Review", "Verified");
     assertThat(change.permittedLabels.keySet())
         .containsExactly("Code-Review", "Verified");
+    assertPermitted(change, "Code-Review", -2, -1, 0, 1, 2);
+    assertPermitted(change, "Verified", -1, 0, 1);
 
     // add an approval on the new label
     gApi.changes()
@@ -2344,6 +2346,7 @@ public class ChangeIT extends AbstractDaemonTest {
     assertThat(change.status).isEqualTo(ChangeStatus.MERGED);
     assertThat(change.labels.keySet()).containsExactly("Code-Review");
     assertThat(change.permittedLabels.keySet()).containsExactly("Code-Review");
+    assertPermitted(change, "Code-Review", 2);
 
     // add new label and assert that it's returned for existing changes
     ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
@@ -2363,6 +2366,8 @@ public class ChangeIT extends AbstractDaemonTest {
         .containsExactly("Code-Review", "Verified");
     assertThat(change.permittedLabels.keySet())
         .containsExactly("Code-Review", "Verified");
+    assertPermitted(change, "Code-Review", 2);
+    assertPermitted(change, "Verified", 0, 1);
 
     // ignore the new label by Prolog submit rule and assert that the label is
     // no longer returned
@@ -2378,8 +2383,8 @@ public class ChangeIT extends AbstractDaemonTest {
     change = gApi.changes()
         .id(r.getChangeId())
         .get();
-    assertThat(change.labels.keySet()).containsExactly("Code-Review");
+    assertPermitted(change, "Code-Review", 2);
-    assertThat(change.permittedLabels.keySet()).containsExactly("Code-Review");
+    assertPermitted(change, "Verified");
 
     // add an approval on the new label and assert that the label is now
     // returned although it is ignored by the Prolog submit rule and hence not
@@ -2395,7 +2400,8 @@ public class ChangeIT extends AbstractDaemonTest {
         .get();
     assertThat(change.labels.keySet())
         .containsExactly("Code-Review", "Verified");
-    assertThat(change.permittedLabels.keySet()).containsExactly("Code-Review");
+    assertPermitted(change, "Code-Review", 2);
+    assertPermitted(change, "Verified");
 
     // remove label and assert that it's no longer returned for existing
     // changes, even if there is an approval for it
@@ -2410,6 +2416,7 @@ public class ChangeIT extends AbstractDaemonTest {
         .get();
     assertThat(change.labels.keySet()).containsExactly("Code-Review");
     assertThat(change.permittedLabels.keySet()).containsExactly("Code-Review");
+    assertPermitted(change, "Code-Review", 2);
   }
 
   @Test
@@ -2425,7 +2432,7 @@ public class ChangeIT extends AbstractDaemonTest {
         .get();
     assertThat(change.status).isEqualTo(ChangeStatus.MERGED);
     assertThat(change.labels.keySet()).containsExactly("Code-Review");
-    assertThat(change.permittedLabels.keySet()).containsExactly("Code-Review");
+    assertPermitted(change, "Code-Review", 0, 1, 2);
   }
 
   @Test

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java

@@ -20,6 +20,7 @@ import static com.google.gerrit.acceptance.PushOneCommit.FILE_NAME;
 import static com.google.gerrit.acceptance.PushOneCommit.PATCH;
 import static com.google.gerrit.acceptance.PushOneCommit.PATCH_FILE_ONLY;
 import static com.google.gerrit.acceptance.PushOneCommit.SUBJECT;
+import static com.google.gerrit.extensions.client.ListChangesOption.DETAILED_LABELS;
 import static com.google.gerrit.reviewdb.client.Patch.COMMIT_MSG;
 import static com.google.gerrit.reviewdb.client.Patch.MERGE_LIST;
 import static java.nio.charset.StandardCharsets.UTF_8;
@@ -44,7 +45,6 @@ import com.google.gerrit.extensions.api.changes.ReviewInput.CommentInput;
 import com.google.gerrit.extensions.api.changes.RevisionApi;
 import com.google.gerrit.extensions.api.projects.BranchInput;
 import com.google.gerrit.extensions.client.ChangeStatus;
-import com.google.gerrit.extensions.client.ListChangesOption;
 import com.google.gerrit.extensions.client.SubmitType;
 import com.google.gerrit.extensions.common.ApprovalInfo;
 import com.google.gerrit.extensions.common.ChangeInfo;
@@ -166,6 +166,9 @@ public class RevisionIT extends AbstractDaemonTest {
     approval = getApproval(changeId, label);
     assertThat(approval.value).isEqualTo(1);
     assertThat(approval.postSubmit).isNull();
+    assertPermitted(
+        gApi.changes().id(changeId).get(EnumSet.of(DETAILED_LABELS)),
+        "Code-Review", 1, 2);
 
     // Repeating the current label is allowed. Does not flip the postSubmit bit
     // due to deduplication codepath.
@@ -200,6 +203,9 @@ public class RevisionIT extends AbstractDaemonTest {
     approval = getApproval(changeId, label);
     assertThat(approval.value).isEqualTo(2);
     assertThat(approval.postSubmit).isTrue();
+    assertPermitted(
+        gApi.changes().id(changeId).get(EnumSet.of(DETAILED_LABELS)),
+        "Code-Review", 2);
 
     // Decreasing to previous post-submit vote is still not allowed.
     try {
@@ -1090,7 +1096,7 @@ public class RevisionIT extends AbstractDaemonTest {
       throws Exception {
     ChangeInfo info = gApi.changes()
         .id(changeId)
-        .get(EnumSet.of(ListChangesOption.DETAILED_LABELS));
+        .get(EnumSet.of(DETAILED_LABELS));
     LabelInfo li = info.labels.get(label);
     assertThat(li).isNotNull();
     int accountId = atrScope.get().getUser().getAccountId().get();

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

@@ -18,7 +18,6 @@ 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;
@@ -44,9 +43,6 @@ 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 {
 
@@ -193,8 +189,7 @@ public class CustomLabelIT extends AbstractDaemonTest {
     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);
-    assertPermitted(info, "Code-Review", -2, -1, 0, 1, 2);
     assertPermitted(info, P.getName(), 0, 1);
     assertPermitted(info, label.getName());
 
@@ -210,20 +205,6 @@ public class CustomLabelIT extends AbstractDaemonTest {
     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-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java

@@ -36,6 +36,7 @@ import static com.google.gerrit.extensions.client.ListChangesOption.REVIEWER_UPD
 import static com.google.gerrit.extensions.client.ListChangesOption.SUBMITTABLE;
 import static com.google.gerrit.extensions.client.ListChangesOption.WEB_LINKS;
 import static com.google.gerrit.server.CommonConverters.toGitPerson;
+
 import static java.util.stream.Collectors.toList;
 
 import com.google.auto.value.AutoValue;
@@ -89,6 +90,7 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.AnonymousUser;
+import com.google.gerrit.server.ApprovalsUtil;
 import com.google.gerrit.server.ChangeMessagesUtil;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.GpgException;
@@ -192,6 +194,7 @@ public class ChangeJson {
   private final ChangeResource.Factory changeResourceFactory;
   private final ChangeKindCache changeKindCache;
   private final ChangeIndexCollection indexes;
+  private final ApprovalsUtil approvalsUtil;
 
   private boolean lazyLoad = true;
   private AccountLoader accountLoader;
@@ -221,6 +224,7 @@ public class ChangeJson {
       ChangeResource.Factory changeResourceFactory,
       ChangeKindCache changeKindCache,
       ChangeIndexCollection indexes,
+      ApprovalsUtil approvalsUtil,
       @Assisted Set<ListChangesOption> options) {
     this.db = db;
     this.labelNormalizer = ln;
@@ -244,6 +248,7 @@ public class ChangeJson {
     this.changeResourceFactory = changeResourceFactory;
     this.changeKindCache = changeKindCache;
     this.indexes = indexes;
+    this.approvalsUtil = approvalsUtil;
     this.options = options.isEmpty()
         ? EnumSet.noneOf(ListChangesOption.class)
         : EnumSet.copyOf(options);
@@ -891,10 +896,12 @@ public class ChangeJson {
 
   private Map<String, Collection<String>> permittedLabels(ChangeControl ctl, ChangeData cd)
       throws OrmException {
-    if (ctl == null) {
+    if (ctl == null || !ctl.getUser().isIdentifiedUser()) {
       return null;
     }
 
+    Map<String, Short> labels = null;
+    boolean isMerged = ctl.getChange().getStatus() == Change.Status.MERGED;
     LabelTypes labelTypes = ctl.getLabelTypes();
     SetMultimap<String, String> permitted = LinkedHashMultimap.create();
     for (SubmitRecord rec : submitRecords(cd)) {
@@ -903,16 +910,20 @@ public class ChangeJson {
       }
       for (SubmitRecord.Label r : rec.labels) {
         LabelType type = labelTypes.byLabel(r.label);
-        if (type == null) {
+        if (type == null || (isMerged && !type.allowPostSubmit())) {
-          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())) {
+          boolean ok = range.contains(v.getValue());
+          if (isMerged) {
+            if (labels == null) {
+              labels = currentLabels(ctl);
+            }
+            short prev = labels.getOrDefault(type.getName(), (short) 0);
+            ok &= v.getValue() >= prev;
+          }
+          if (ok) {
             permitted.put(r.label, v.formatValue());
           }
         }
@@ -932,6 +943,17 @@ public class ChangeJson {
     return permitted.asMap();
   }
 
+  private Map<String, Short> currentLabels(ChangeControl ctl)
+      throws OrmException {
+    Map<String, Short> result = new HashMap<>();
+    for (PatchSetApproval psa : approvalsUtil.byPatchSetUser(
+        db.get(), ctl, ctl.getChange().currentPatchSetId(),
+        ctl.getUser().getAccountId())) {
+      result.put(psa.getLabel(), psa.getValue());
+    }
+    return result;
+  }
+
   private Collection<ChangeMessageInfo> messages(ChangeControl ctl, ChangeData cd,
       Map<PatchSet.Id, PatchSet> map)
       throws OrmException {