(Re)enable voting buttons for merged changes
Change I025b64 had to disable the voting buttons on merged changes to fix replying on merged changes. However having the voting buttons enabled for merged changes is desired after change Ie337b5 added backend support for voting on merged changes. To reenable them ChangeJson must set the 'permitted_labels' field for merged changes and make sure that each permitted label is contained in the 'labels' field. For changes which were auto-merged by Gerrit due to direct push of the commit there may be no approvals on the change although the change status is MERGED. This means for populating the 'labels' field for merged changes we cannot rely on the approvals. Instead of relying on the approvals set the 'labels' field for merged changes based on the submit records. This is the same way how the labels for open changes and permitted labels are computed. The voting range returned by 'permitted_labels' for merged changes is not always correct yet, since it returns the voting range that is allowed to the user, but it doesn't respect the fact that downgrading of votes is not possible for merged changes. This is a minor issue, since the user gets a proper error message if downgrading of a vote on a merged change is attempted. Hence it can be fixed in a follow-up change. Change-Id: Iba501e00bee77be3bd0ced72f88fd04ba0accaed Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
@@ -2123,7 +2123,7 @@ public class ChangeIT extends AbstractDaemonTest {
|
||||
.get();
|
||||
assertThat(change.status).isEqualTo(ChangeStatus.MERGED);
|
||||
assertThat(change.labels.keySet()).containsExactly("Code-Review");
|
||||
assertThat(change.permittedLabels).isEmpty();
|
||||
assertThat(change.permittedLabels.keySet()).containsExactly("Code-Review");
|
||||
}
|
||||
|
||||
@Test
|
||||
@@ -2138,8 +2138,8 @@ public class ChangeIT extends AbstractDaemonTest {
|
||||
.id(r.getChangeId())
|
||||
.get();
|
||||
assertThat(change.status).isEqualTo(ChangeStatus.MERGED);
|
||||
assertThat(change.labels).isEmpty();
|
||||
assertThat(change.permittedLabels).isEmpty();
|
||||
assertThat(change.labels.keySet()).containsExactly("Code-Review");
|
||||
assertThat(change.permittedLabels.keySet()).containsExactly("Code-Review");
|
||||
}
|
||||
|
||||
@Test
|
||||
|
||||
@@ -504,9 +504,10 @@ public class ChangeJson {
|
||||
// list permitted labels, since users can't vote on those patch sets.
|
||||
if (!limitToPsId.isPresent()
|
||||
|| limitToPsId.get().equals(in.currentPatchSetId())) {
|
||||
out.permittedLabels = cd.change().getStatus().isOpen()
|
||||
? permittedLabels(ctl, cd)
|
||||
: ImmutableMap.of();
|
||||
out.permittedLabels =
|
||||
cd.change().getStatus() != Change.Status.ABANDONED
|
||||
? permittedLabels(ctl, cd)
|
||||
: ImmutableMap.of();
|
||||
}
|
||||
out.removableReviewers = removableReviewers(ctl, out.labels.values());
|
||||
|
||||
@@ -785,16 +786,29 @@ public class ChangeJson {
|
||||
}
|
||||
}
|
||||
|
||||
// Don't use Maps.newTreeMap(Comparator) due to OpenJDK bug 100167.
|
||||
Map<String, LabelWithStatus> labels =
|
||||
new TreeMap<>(labelTypes.nameComparator());
|
||||
for (String name : labelNames) {
|
||||
LabelType type = labelTypes.byLabel(name);
|
||||
LabelWithStatus l = LabelWithStatus.create(new LabelInfo(), null);
|
||||
if (detailed) {
|
||||
setLabelValues(type, l);
|
||||
Map<String, LabelWithStatus> labels;
|
||||
if (cd.change().getStatus() == Change.Status.ABANDONED) {
|
||||
// For abandoned changes return only labels for which an approval exists.
|
||||
// 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(type.getName(), l);
|
||||
} 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()));
|
||||
}
|
||||
|
||||
for (Account.Id accountId : allUsers) {
|
||||
|
||||
Reference in New Issue
Block a user