Append all approvals info when posting review with REST endpoint
Change Ibc1c64d70 added all approval info to every Gerrit event however there was a bug in that change. When a review is posted using the REST endpoint and SSH command only the label that was provided by the review appears in the event. This change fixes that by appending all labels and votes to the event in that scenario. Change-Id: If39a09b28a41718957873f9293f1cafedcfb0035
This commit is contained in:
Khai Do
Marco Miller
parent
63b5eaf815
commit
82d3bfa84c
@ -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 org.junit.Assert.fail;
|
||||
|
||||
import com.google.gerrit.acceptance.AbstractDaemonTest;
|
||||
import com.google.gerrit.acceptance.NoHttpd;
|
||||
@ -105,6 +104,20 @@ public class CommentAddedEventIT extends AbstractDaemonTest {
|
||||
saveProjectConfig(project, cfg);
|
||||
}
|
||||
|
||||
/* Need to lookup info for the label under test since there can be multiple
|
||||
* labels defined. By default Gerrit already has a Code-Review label.
|
||||
*/
|
||||
private ApprovalAttribute getApprovalAttribute(LabelType label) {
|
||||
ApprovalAttribute[] aa = lastCommentAddedEvent.approvals.get();
|
||||
ApprovalAttribute res = null;
|
||||
for (int i=0; i < aa.length; i++) {
|
||||
if (aa[i].description.equals(label.getName())) {
|
||||
res = aa[i];
|
||||
}
|
||||
}
|
||||
return res;
|
||||
}
|
||||
|
||||
@Test
|
||||
public void newChangeWithVote() throws Exception {
|
||||
saveLabelConfig();
|
||||
@ -114,10 +127,9 @@ public class CommentAddedEventIT extends AbstractDaemonTest {
|
||||
ReviewInput reviewInput = new ReviewInput().label(
|
||||
label.getName(), (short)-1);
|
||||
revision(r).review(reviewInput);
|
||||
String newVote = lastCommentAddedEvent.approvals.get()[0].value;
|
||||
String oldVote = lastCommentAddedEvent.approvals.get()[0].oldValue;
|
||||
assertThat(oldVote).isEqualTo("0");
|
||||
assertThat(newVote).isEqualTo("-1");
|
||||
ApprovalAttribute attr = getApprovalAttribute(label);
|
||||
assertThat(attr.oldValue).isEqualTo("0");
|
||||
assertThat(attr.value).isEqualTo("-1");
|
||||
assertThat(lastCommentAddedEvent.comment).isEqualTo(
|
||||
String.format("Patch Set 1: %s-1", label.getName()));
|
||||
}
|
||||
@ -137,10 +149,9 @@ public class CommentAddedEventIT extends AbstractDaemonTest {
|
||||
reviewInput = new ReviewInput().label(
|
||||
label.getName(), (short)1);
|
||||
revision(r).review(reviewInput);
|
||||
String newVote = lastCommentAddedEvent.approvals.get()[0].value;
|
||||
String oldVote = lastCommentAddedEvent.approvals.get()[0].oldValue;
|
||||
assertThat(oldVote).isEqualTo("0");
|
||||
assertThat(newVote).isEqualTo("1");
|
||||
ApprovalAttribute attr = getApprovalAttribute(label);
|
||||
assertThat(attr.oldValue).isEqualTo("0");
|
||||
assertThat(attr.value).isEqualTo("1");
|
||||
assertThat(lastCommentAddedEvent.comment).isEqualTo(
|
||||
String.format("Patch Set 2: %s+1", label.getName()));
|
||||
}
|
||||
@ -155,58 +166,55 @@ public class CommentAddedEventIT extends AbstractDaemonTest {
|
||||
// review with message only, do not apply votes
|
||||
ReviewInput reviewInput = new ReviewInput().message(label.getName());
|
||||
revision(r).review(reviewInput);
|
||||
// reply message only so votes are excluded from comment
|
||||
assertThat(lastCommentAddedEvent.approvals.get()).isNull();
|
||||
// reply message only so vote is shown as 0
|
||||
ApprovalAttribute attr = getApprovalAttribute(label);
|
||||
assertThat(attr.oldValue).isNull();
|
||||
assertThat(attr.value).isEqualTo("0");
|
||||
assertThat(lastCommentAddedEvent.comment).isEqualTo(
|
||||
String.format("Patch Set 1:\n\n%s", label.getName()));
|
||||
|
||||
// transition from un-voted to -1 vote
|
||||
reviewInput = new ReviewInput().label(label.getName(), -1);
|
||||
revision(r).review(reviewInput);
|
||||
String newVote = lastCommentAddedEvent.approvals.get()[0].value;
|
||||
String oldVote = lastCommentAddedEvent.approvals.get()[0].oldValue;
|
||||
assertThat(oldVote).isEqualTo("0");
|
||||
assertThat(newVote).isEqualTo("-1");
|
||||
attr = getApprovalAttribute(label);
|
||||
assertThat(attr.oldValue).isEqualTo("0");
|
||||
assertThat(attr.value).isEqualTo("-1");
|
||||
assertThat(lastCommentAddedEvent.comment).isEqualTo(
|
||||
String.format("Patch Set 1: %s-1", label.getName()));
|
||||
|
||||
// transition vote from -1 to 0
|
||||
reviewInput = new ReviewInput().label(label.getName(), 0);
|
||||
revision(r).review(reviewInput);
|
||||
newVote = lastCommentAddedEvent.approvals.get()[0].value;
|
||||
oldVote = lastCommentAddedEvent.approvals.get()[0].oldValue;
|
||||
assertThat(oldVote).isEqualTo("-1");
|
||||
assertThat(newVote).isEqualTo("0");
|
||||
attr = getApprovalAttribute(label);
|
||||
assertThat(attr.oldValue).isEqualTo("-1");
|
||||
assertThat(attr.value).isEqualTo("0");
|
||||
assertThat(lastCommentAddedEvent.comment).isEqualTo(
|
||||
String.format("Patch Set 1: -%s", label.getName()));
|
||||
|
||||
// transition vote from 0 to 1
|
||||
reviewInput = new ReviewInput().label(label.getName(), 1);
|
||||
revision(r).review(reviewInput);
|
||||
newVote = lastCommentAddedEvent.approvals.get()[0].value;
|
||||
oldVote = lastCommentAddedEvent.approvals.get()[0].oldValue;
|
||||
assertThat(oldVote).isEqualTo("0");
|
||||
assertThat(newVote).isEqualTo("1");
|
||||
attr = getApprovalAttribute(label);
|
||||
assertThat(attr.oldValue).isEqualTo("0");
|
||||
assertThat(attr.value).isEqualTo("1");
|
||||
assertThat(lastCommentAddedEvent.comment).isEqualTo(
|
||||
String.format("Patch Set 1: %s+1", label.getName()));
|
||||
|
||||
// transition vote from 1 to -1
|
||||
reviewInput = new ReviewInput().label(label.getName(), -1);
|
||||
revision(r).review(reviewInput);
|
||||
newVote = lastCommentAddedEvent.approvals.get()[0].value;
|
||||
oldVote = lastCommentAddedEvent.approvals.get()[0].oldValue;
|
||||
assertThat(oldVote).isEqualTo("1");
|
||||
assertThat(newVote).isEqualTo("-1");
|
||||
attr = getApprovalAttribute(label);
|
||||
assertThat(attr.oldValue).isEqualTo("1");
|
||||
assertThat(attr.value).isEqualTo("-1");
|
||||
assertThat(lastCommentAddedEvent.comment).isEqualTo(
|
||||
String.format("Patch Set 1: %s-1", label.getName()));
|
||||
|
||||
// review with message only, do not apply votes
|
||||
reviewInput = new ReviewInput().message(label.getName());
|
||||
revision(r).review(reviewInput);
|
||||
newVote = lastCommentAddedEvent.approvals.get()[0].value;
|
||||
oldVote = lastCommentAddedEvent.approvals.get()[0].oldValue;
|
||||
assertThat(oldVote).isEqualTo(null); // no vote change so not included
|
||||
assertThat(newVote).isEqualTo("-1");
|
||||
attr = getApprovalAttribute(label);
|
||||
assertThat(attr.oldValue).isEqualTo(null); // no vote change so not included
|
||||
assertThat(attr.value).isEqualTo("-1");
|
||||
assertThat(lastCommentAddedEvent.comment).isEqualTo(
|
||||
String.format("Patch Set 1:\n\n%s", label.getName()));
|
||||
}
|
||||
@ -222,10 +230,27 @@ public class CommentAddedEventIT extends AbstractDaemonTest {
|
||||
ChangeInfo c = get(r.getChangeId());
|
||||
LabelInfo q = c.labels.get(label.getName());
|
||||
assertThat(q.all).hasSize(1);
|
||||
ApprovalAttribute labelAttr = getApprovalAttribute(label);
|
||||
assertThat(labelAttr.oldValue).isEqualTo("0");
|
||||
assertThat(labelAttr.value).isEqualTo("-1");
|
||||
assertThat(lastCommentAddedEvent.comment).isEqualTo(
|
||||
String.format("Patch Set 1: %s-1\n\n%s",
|
||||
label.getName(), label.getName()));
|
||||
|
||||
// there should be 3 approval labels (label, pLabel, and CRVV)
|
||||
assertThat(lastCommentAddedEvent.approvals.get()).hasLength(3);
|
||||
|
||||
// check the approvals that were not voted on
|
||||
ApprovalAttribute pLabelAttr = getApprovalAttribute(pLabel);
|
||||
assertThat(pLabelAttr.oldValue).isNull();
|
||||
assertThat(pLabelAttr.value).isEqualTo("0");
|
||||
|
||||
LabelType crLabel = LabelType.withDefaultValues("Code-Review");
|
||||
ApprovalAttribute crlAttr = getApprovalAttribute(crLabel);
|
||||
assertThat(crlAttr.oldValue).isNull();
|
||||
assertThat(crlAttr.value).isEqualTo("0");
|
||||
|
||||
// update pLabel approval
|
||||
reviewInput = new ReviewInput().label(pLabel.getName(), 1);
|
||||
reviewInput.message = pLabel.getName();
|
||||
revision(r).review(reviewInput);
|
||||
@ -233,21 +258,20 @@ public class CommentAddedEventIT extends AbstractDaemonTest {
|
||||
c = get(r.getChangeId());
|
||||
q = c.labels.get(label.getName());
|
||||
assertThat(q.all).hasSize(1);
|
||||
pLabelAttr = getApprovalAttribute(pLabel);
|
||||
assertThat(pLabelAttr.oldValue).isEqualTo("0");
|
||||
assertThat(pLabelAttr.value).isEqualTo("1");
|
||||
assertThat(lastCommentAddedEvent.comment).isEqualTo(
|
||||
String.format("Patch Set 1: %s+1\n\n%s",
|
||||
pLabel.getName(), pLabel.getName()));
|
||||
|
||||
assertThat(lastCommentAddedEvent.approvals.get()).hasLength(2);
|
||||
for (ApprovalAttribute approval : lastCommentAddedEvent.approvals.get()) {
|
||||
if (approval.type.equals(label.getName())) {
|
||||
assertThat(approval.value).isEqualTo("-1");
|
||||
assertThat(approval.oldValue).isNull();
|
||||
} else if (approval.type.equals(pLabel.getName())) {
|
||||
assertThat(approval.value).isEqualTo("1");
|
||||
assertThat(approval.oldValue).isEqualTo("0");
|
||||
} else {
|
||||
fail("Unexpected label: " + approval.type);
|
||||
}
|
||||
}
|
||||
// check the approvals that were not voted on
|
||||
labelAttr = getApprovalAttribute(label);
|
||||
assertThat(labelAttr.oldValue).isNull();
|
||||
assertThat(labelAttr.value).isEqualTo("-1");
|
||||
|
||||
crlAttr = getApprovalAttribute(crLabel);
|
||||
assertThat(crlAttr.oldValue).isNull();
|
||||
assertThat(crlAttr.value).isEqualTo("0");
|
||||
}
|
||||
}
|
||||
|
||||
@ -553,6 +553,37 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
|
||||
}
|
||||
}
|
||||
|
||||
private Map<String, Short> getAllApprovals(LabelTypes labelTypes,
|
||||
Map<String, Short> current, Map<String, Short> input) {
|
||||
Map<String, Short> allApprovals = new HashMap<>();
|
||||
for (LabelType lt : labelTypes.getLabelTypes()) {
|
||||
allApprovals.put(lt.getName(), (short) 0);
|
||||
}
|
||||
// set approvals to existing votes
|
||||
if (current != null) {
|
||||
allApprovals.putAll(current);
|
||||
}
|
||||
// set approvals to new votes
|
||||
if (input != null) {
|
||||
allApprovals.putAll(input);
|
||||
}
|
||||
return allApprovals;
|
||||
}
|
||||
|
||||
private Map<String, Short> getPreviousApprovals(
|
||||
Map<String, Short> allApprovals, Map<String, Short> current) {
|
||||
Map<String, Short> previous = new HashMap<>();
|
||||
for (Map.Entry<String, Short> approval : allApprovals.entrySet()) {
|
||||
// assume vote is 0 if there is no vote
|
||||
if (!current.containsKey(approval.getKey())) {
|
||||
previous.put(approval.getKey(), (short) 0);
|
||||
} else {
|
||||
previous.put(approval.getKey(), current.get(approval.getKey()));
|
||||
}
|
||||
}
|
||||
return previous;
|
||||
}
|
||||
|
||||
private boolean updateLabels(ChangeContext ctx)
|
||||
throws OrmException, ResourceConflictException {
|
||||
Map<String, Short> inLabels = MoreObjects.firstNonNull(in.labels,
|
||||
@ -568,27 +599,13 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
|
||||
List<PatchSetApproval> del = Lists.newArrayList();
|
||||
List<PatchSetApproval> ups = Lists.newArrayList();
|
||||
Map<String, PatchSetApproval> current = scanLabels(ctx, del);
|
||||
|
||||
// get all approvals in cases of quick approve vote
|
||||
Map<String, Short> allApprovals = approvalsByKey(current.values());
|
||||
allApprovals.putAll(inLabels);
|
||||
|
||||
// get previous label votes
|
||||
Map<String, Short> currentLabels = new HashMap<>();
|
||||
for (Map.Entry<String, PatchSetApproval> ent : current.entrySet()) {
|
||||
currentLabels.put(ent.getValue().getLabel(), ent.getValue().getValue());
|
||||
}
|
||||
Map<String, Short> previous = new HashMap<>();
|
||||
for (Map.Entry<String, Short> ent : allApprovals.entrySet()) {
|
||||
if (!currentLabels.containsKey(ent.getKey())) {
|
||||
previous.put(ent.getKey(), (short)0);
|
||||
} else {
|
||||
previous.put(ent.getKey(), currentLabels.get(ent.getKey()));
|
||||
}
|
||||
}
|
||||
LabelTypes labelTypes = ctx.getControl().getLabelTypes();
|
||||
Map<String, Short> allApprovals = getAllApprovals(labelTypes,
|
||||
approvalsByKey(current.values()), inLabels);
|
||||
Map<String, Short> previous = getPreviousApprovals(allApprovals,
|
||||
approvalsByKey(current.values()));
|
||||
|
||||
ChangeUpdate update = ctx.getUpdate(psId);
|
||||
LabelTypes labelTypes = ctx.getControl().getLabelTypes();
|
||||
for (Map.Entry<String, Short> ent : allApprovals.entrySet()) {
|
||||
String name = ent.getKey();
|
||||
LabelType lt = checkNotNull(labelTypes.byLabel(name), name);
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user