Disallow voting on the same patch-set twice with the same label
We shouldn't allow voting twice on the same label since the new behavior
breaks some users.
However, we should still allow re-voting on a sticky vote.
This change partly reverts Ide1b122b6.
Change-Id: Ia8c03c865a89b226eb86f686578921f8ec7922f5
diff --git a/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java
index a360510..562bdf8 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReview.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReview.java
@@ -1272,28 +1272,27 @@
del.add(c);
update.putApproval(normName, (short) 0);
}
- } else if (c != null) {
- // Check if the label exists in the request input (the user voted again). If the user
- // hadn't voted again, there is no need to re-apply the vote.
- if (inLabels.keySet().contains(c.label())) {
- PatchSetApproval.Builder b =
- c.toBuilder()
- .value(ent.getValue())
- .granted(ctx.getWhen())
- .tag(Optional.ofNullable(in.tag));
- ctx.getUser().updateRealAccountId(b::realAccountId);
- c = b.build();
- ups.add(c);
- addLabelDelta(normName, c.value());
- oldApprovals.put(normName, previous.get(normName));
- approvals.put(normName, c.value());
- update.putApproval(normName, ent.getValue());
- } else {
- current.put(normName, c);
- oldApprovals.put(normName, null);
- approvals.put(normName, c.value());
- }
- } else {
+ // Only allow voting again if the vote is copied over from a past patch-set, or the
+ // values are different.
+ } else if (c != null
+ && (c.value() != ent.getValue() || isApprovalCopiedOver(c, ctx.getNotes()))) {
+ PatchSetApproval.Builder b =
+ c.toBuilder()
+ .value(ent.getValue())
+ .granted(ctx.getWhen())
+ .tag(Optional.ofNullable(in.tag));
+ ctx.getUser().updateRealAccountId(b::realAccountId);
+ c = b.build();
+ ups.add(c);
+ addLabelDelta(normName, c.value());
+ oldApprovals.put(normName, previous.get(normName));
+ approvals.put(normName, c.value());
+ update.putApproval(normName, ent.getValue());
+ } else if (c != null && c.value() == ent.getValue()) {
+ current.put(normName, c);
+ oldApprovals.put(normName, null);
+ approvals.put(normName, c.value());
+ } else if (c == null) {
c =
ApprovalsUtil.newApproval(psId, user, lt.getLabelId(), ent.getValue(), ctx.getWhen())
.tag(Optional.ofNullable(in.tag))
@@ -1319,6 +1318,17 @@
return !del.isEmpty() || !ups.isEmpty();
}
+ /**
+ * Approval is copied over if it doesn't exist in the approvals of the current patch-set
+ * according to change notes (which means it was computed in {@link
+ * com.google.gerrit.server.ApprovalInference})
+ */
+ private boolean isApprovalCopiedOver(
+ PatchSetApproval patchSetApproval, ChangeNotes changeNotes) {
+ return !changeNotes.getApprovals().get(changeNotes.getChange().currentPatchSetId()).stream()
+ .anyMatch(p -> p.equals(patchSetApproval));
+ }
+
private void validatePostSubmitLabels(
ChangeContext ctx,
LabelTypes labelTypes,
diff --git a/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java b/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
index 0b2e0f7..c8b1715 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
@@ -63,7 +63,6 @@
import com.google.gerrit.server.restapi.change.OnPostReview;
import com.google.gerrit.server.restapi.change.PostReview;
import com.google.gerrit.server.update.CommentsRejectedException;
-import com.google.gerrit.testing.FakeEmailSender;
import com.google.gerrit.testing.TestCommentHelper;
import com.google.inject.Inject;
import com.google.inject.Module;
@@ -597,7 +596,7 @@
input = new ReviewInput().label(LabelId.CODE_REVIEW, 2);
gApi.changes().id(r.getChangeId()).current().review(input);
testOnPostReview.assertApproval(
- LabelId.CODE_REVIEW, /* expectedOldValue= */ 2, /* expectedNewValue= */ 2);
+ LabelId.CODE_REVIEW, /* expectedOldValue= */ null, /* expectedNewValue= */ 2);
// Delete the vote.
input = new ReviewInput().label(LabelId.CODE_REVIEW, 0);
@@ -627,21 +626,19 @@
assertThat(r.getChange().approvals().values()).hasSize(1);
List<ChangeMessageInfo> changeMessages = gApi.changes().id(r.getChangeId()).messages();
- // The two latest change messages are both about Code-Review+2
+ // Only the last change message is about Code-Review+2
assertThat(Iterables.getLast(changeMessages).message).isEqualTo("Patch Set 1: Code-Review+2");
changeMessages.remove(changeMessages.size() - 1);
- assertThat(Iterables.getLast(changeMessages).message).isEqualTo("Patch Set 1: Code-Review+2");
+ assertThat(Iterables.getLast(changeMessages).message)
+ .isNotEqualTo("Patch Set 1: Code-Review+2");
- // The two latest emails are about Code-Review +2.
- List<FakeEmailSender.Message> messages = sender.getMessages();
- assertThat(messages).hasSize(2);
- for (FakeEmailSender.Message message : messages) {
- assertThat(message.body()).contains("Patch Set 1: Code-Review+2");
- }
+ // Only one email is about Code-Review +2 was sent.
+ assertThat(Iterables.getOnlyElement(sender.getMessages()).body())
+ .contains("Patch Set 1: Code-Review+2");
}
@Test
- public void votingTheSameVoteSecondTimeExtendsOnPostReview() throws Exception {
+ public void votingTheSameVoteSecondTimeExtendsOnPostReviewWithOldNullValue() throws Exception {
PushOneCommit.Result r = createChange();
// Add a new vote.
@@ -656,12 +653,12 @@
gApi.changes().id(r.getChangeId()).current().review(input);
testOnPostReview.assertApproval(
- LabelId.CODE_REVIEW, /* expectedOldValue= */ 2, /* expectedNewValue= */ 2);
+ LabelId.CODE_REVIEW, /* expectedOldValue= */ null, /* expectedNewValue= */ 2);
}
}
@Test
- public void votingTheSameVoteSecondTimeFiresOnCommentAdded() throws Exception {
+ public void votingTheSameVoteSecondTimeDoesNotFireOnCommentAdded() throws Exception {
PushOneCommit.Result r = createChange();
// Add a new vote.
@@ -675,8 +672,8 @@
input = new ReviewInput().label(LabelId.CODE_REVIEW, 2);
gApi.changes().id(r.getChangeId()).current().review(input);
- assertThat(testListener.lastCommentAddedEvent.getComment())
- .isEqualTo("Patch Set 1: Code-Review+2");
+ // Event not fired.
+ assertThat(testListener.lastCommentAddedEvent).isNull();
}
}
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
index 0d92c60..9bdbd20 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
@@ -180,12 +180,12 @@
assertThat(approval.postSubmit).isNull();
assertPermitted(gApi.changes().id(changeId).get(DETAILED_LABELS), LabelId.CODE_REVIEW, 1, 2);
- // Repeating the current label is allowed. Flips the postSubmit since technically this is a
- // new vote.
+ // Repeating the current label is allowed. Does not flip the postSubmit bit due to
+ // deduplication codepath.
gApi.changes().id(changeId).current().review(ReviewInput.recommend());
approval = getApproval(changeId, label);
assertThat(approval.value).isEqualTo(1);
- assertThat(approval.postSubmit).isTrue();
+ assertThat(approval.postSubmit).isNull();
// Reducing vote is not allowed.
ResourceConflictException thrown =
@@ -197,7 +197,7 @@
.isEqualTo("Cannot reduce vote on labels for closed change: Code-Review");
approval = getApproval(changeId, label);
assertThat(approval.value).isEqualTo(1);
- assertThat(approval.postSubmit).isTrue();
+ assertThat(approval.postSubmit).isNull();
// Increasing vote is allowed.
gApi.changes().id(changeId).current().review(ReviewInput.approve());
