Merge changes I46073f40,I470cb8a4

* changes: ChangeJson: Ensure for merged changes that labels for approvals are included Test labels on changes if label type is added/removed
2016-10-28 15:23:40 +00:00
parent 8d43dde8db 1898caeeab
commit 173da9f3d9
3 changed files with 148 additions and 15 deletions
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -2141,6 +2141,56 @@ public class ChangeIT extends AbstractDaemonTest {
            .get(0).commit).isNotEqualTo(currentMaster.getCommit().getName());
  }

+  @Test
+  public void checkLabelsForOpenChange() throws Exception {
+    PushOneCommit.Result r = createChange();
+    ChangeInfo change = gApi.changes()
+        .id(r.getChangeId())
+        .get();
+    assertThat(change.status).isEqualTo(ChangeStatus.NEW);
+    assertThat(change.labels.keySet()).containsExactly("Code-Review");
+    assertThat(change.permittedLabels.keySet()).containsExactly("Code-Review");
+
+    // add new label and assert that it's returned for existing changes
+    ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
+    LabelType verified = Util.verified();
+    cfg.getLabelSections().put(verified.getName(), verified);
+    AccountGroup.UUID registeredUsers =
+        SystemGroupBackend.getGroup(REGISTERED_USERS).getUUID();
+    String heads = RefNames.REFS_HEADS + "*";
+    Util.allow(cfg, Permission.forLabel(verified.getName()), -1, 1,
+        registeredUsers, heads);
+    saveProjectConfig(project, cfg);
+
+    change = gApi.changes()
+        .id(r.getChangeId())
+        .get();
+    assertThat(change.labels.keySet())
+        .containsExactly("Code-Review", "Verified");
+    assertThat(change.permittedLabels.keySet())
+        .containsExactly("Code-Review", "Verified");
+
+    // add an approval on the new label
+    gApi.changes()
+        .id(r.getChangeId())
+        .revision(r.getCommit().name())
+        .review(new ReviewInput().label(
+            verified.getName(), verified.getMax().getValue()));
+
+    // remove label and assert that it's no longer returned for existing
+    // changes, even if there is an approval for it
+    cfg.getLabelSections().remove(verified.getName());
+    Util.remove(cfg, Permission.forLabel(verified.getName()), registeredUsers,
+        heads);
+    saveProjectConfig(project, cfg);
+
+    change = gApi.changes()
+        .id(r.getChangeId())
+        .get();
+    assertThat(change.labels.keySet()).containsExactly("Code-Review");
+    assertThat(change.permittedLabels.keySet()).containsExactly("Code-Review");
+  }
+
  @Test
  public void checkLabelsForMergedChange() throws Exception {
    PushOneCommit.Result r = createChange();
@@ -2159,6 +2209,72 @@ 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");
+
+    // add new label and assert that it's returned for existing changes
+    ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
+    LabelType verified = Util.verified();
+    cfg.getLabelSections().put(verified.getName(), verified);
+    AccountGroup.UUID registeredUsers =
+        SystemGroupBackend.getGroup(REGISTERED_USERS).getUUID();
+    String heads = RefNames.REFS_HEADS + "*";
+    Util.allow(cfg, Permission.forLabel(verified.getName()), -1, 1,
+        registeredUsers, heads);
+    saveProjectConfig(project, cfg);
+
+    change = gApi.changes()
+        .id(r.getChangeId())
+        .get();
+    assertThat(change.labels.keySet())
+        .containsExactly("Code-Review", "Verified");
+    assertThat(change.permittedLabels.keySet())
+        .containsExactly("Code-Review", "Verified");
+
+    // ignore the new label by Prolog submit rule and assert that the label is
+    // no longer returned
+    GitUtil.fetch(testRepo, RefNames.REFS_CONFIG + ":config");
+    testRepo.reset("config");
+    PushOneCommit push2 = pushFactory.create(db, admin.getIdent(), testRepo,
+        "Ignore Verified",
+        "rules.pl",
+        "submit_rule(submit(CR)) :-\n"
+            + "  gerrit:max_with_block(-2, 2, 'Code-Review', CR).");
+    push2.to(RefNames.REFS_CONFIG);
+
+    change = gApi.changes()
+        .id(r.getChangeId())
+        .get();
+    assertThat(change.labels.keySet()).containsExactly("Code-Review");
+    assertThat(change.permittedLabels.keySet()).containsExactly("Code-Review");
+
+    // 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
+    // included in the submit records
+    gApi.changes()
+        .id(r.getChangeId())
+        .revision(r.getCommit().name())
+        .review(new ReviewInput().label(
+            verified.getName(), verified.getMax().getValue()));
+
+    change = gApi.changes()
+        .id(r.getChangeId())
+        .get();
+    assertThat(change.labels.keySet())
+        .containsExactly("Code-Review", "Verified");
+    assertThat(change.permittedLabels.keySet()).containsExactly("Code-Review");
+
+    // remove label and assert that it's no longer returned for existing
+    // changes, even if there is an approval for it
+    cfg = projectCache.checkedGet(project).getConfig();
+    cfg.getLabelSections().remove(verified.getName());
+    Util.remove(cfg, Permission.forLabel(verified.getName()), registeredUsers,
+        heads);
+    saveProjectConfig(project, cfg);
+
+    change = gApi.changes()
+        .id(r.getChangeId())
+        .get();
+    assertThat(change.labels.keySet()).containsExactly("Code-Review");
+    assertThat(change.permittedLabels.keySet()).containsExactly("Code-Review");
  }

  @Test
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
@@ -771,8 +771,6 @@ public class ChangeJson {
      }
    }

-    // We can only approximately reconstruct what the submit rule evaluator
-    // would have done. These should really come from a stored submit record.
    Set<String> labelNames = new HashSet<>();
    Multimap<Account.Id, PatchSetApproval> current = HashMultimap.create();
    for (PatchSetApproval a : cd.currentApprovals()) {
@@ -787,25 +785,35 @@ public class ChangeJson {
    }

    Map<String, LabelWithStatus> labels;
-    if (cd.change().getStatus() == Change.Status.ABANDONED) {
-      // For abandoned changes return only labels for which an approval exists.
+    if (cd.change().getStatus() == Change.Status.MERGED) {
+      // Since voting on merged changes is allowed all labels which apply to
+      // the change must be returned. All applying labels can be retrieved from
+      // the submit records, which is what initLabels does.
+      // It's not possible to only compute the labels based on the approvals
+      // since merged changes may not have approvals for all labels (e.g. if not
+      // all labels are required for submit or if the change was auto-closed due
+      // to direct push or if new labels were defined after the change was
+      // merged).
+      labels = initLabels(cd, labelTypes, standard);
+
+      // Also include all labels for which approvals exists. E.g. there can be
+      // approvals for labels that are ignored by a Prolog submit rule and hence
+      // it wouldn't be included in the submit records.
+      for (String name : labelNames) {
+        if (!labels.containsKey(name)) {
+          labels.put(name, LabelWithStatus.create(new LabelInfo(), null));
+        }
+      }
+    } else {
+      // For abandoned changes return only labels for which approvals exist.
      // Other labels are not needed since voting on abandoned changes is not
      // allowed.
      labels = new TreeMap<>(labelTypes.nameComparator());
      for (String name : labelNames) {
-        labels.put(labelTypes.byLabel(name).getName(),
-            LabelWithStatus.create(new LabelInfo(), null));
+        labels.put(name, LabelWithStatus.create(new LabelInfo(), null));
      }
-    } else {
-      // Since voting on merged changes is allowed all labels which apply to
-      // the change must be returned. All applying labels can be retrieved from
-      // the submit records, which is what initLabels does.
-      // It's not possible to compute the labels based on the approvals since
-      // merged changes may not have approvals for all labels (e.g. if not all
-      // labels are required for submit or if the change was auto-closed due to
-      // direct push or if new labels were defined after the change was merged).
-      labels = initLabels(cd, labelTypes, standard);
    }
+
    if (detailed) {
      labels.entrySet().stream().forEach(
          e -> setLabelValues(labelTypes.byLabel(e.getKey()), e.getValue()));
--- a/gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java
@@ -128,6 +128,15 @@ public class Util {
    return rule;
  }

+  public static PermissionRule remove(ProjectConfig project,
+      String permissionName, AccountGroup.UUID group, String ref) {
+    PermissionRule rule = newRule(project, group);
+    project.getAccessSection(ref, true)
+        .getPermission(permissionName, true)
+        .remove(rule);
+    return rule;
+  }
+
  public static PermissionRule block(ProjectConfig project,
      String capabilityName, AccountGroup.UUID group) {
    PermissionRule rule = newRule(project, group);