Allow posting the same vote on a new patchset
Currently, we don't allow adding a new vote if the exact vote already
exists. This is a reasonable behavior for voting again on the same
patchset, but it is strange across multiple patchsets, example:
1. User votes +2 on Code-Review.
2. Change owner creates a new patchset, and the vote is carried over.
3. User wants to vote +2 again, but can't, as the user already voted.
This should also be fixed in the frontend, as the frontend doesn't allow
clicking "send" at all if there are no changes (and the vote +2 was
carried over).
This bug fix is a blocker for a follow-up feature that would add a
message and new content to the "submitted" email, such that users will
know the diff between the latest approved patchset and the submitted
patchset in case of sticky votes. We allow revoting on a label such that
the "latest approved patchset" will be up to date.
It's better to allow voting on the label even multiple times on the same
patch-set, as this is more correct to adjust the timestamp of the vote
when voting again. This has some side effects:
1. Change message will be posted that the user voted.
2. Email may be different (mentioning the vote as opposed to not
mentioning the vote).
3. Events will fire with the vote (as opposed to fire without the vote).
All of those side effects are okay.
Change-Id: Ide1b122b6ac8788b42c4cc8cdfb486ac8d544375
diff --git a/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java
index bf9990c..a360510 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReview.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReview.java
@@ -15,7 +15,6 @@
package com.google.gerrit.server.restapi.change;
import static com.google.common.base.MoreObjects.firstNonNull;
-import static com.google.common.base.Preconditions.checkState;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.gerrit.entities.Patch.PATCHSET_LEVEL;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
@@ -1273,24 +1272,28 @@
del.add(c);
update.putApproval(normName, (short) 0);
}
- } else if (c != null && c.value() != ent.getValue()) {
- 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) {
+ } 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 {
c =
ApprovalsUtil.newApproval(psId, user, lt.getLabelId(), ent.getValue(), ctx.getWhen())
.tag(Optional.ofNullable(in.tag))
@@ -1360,7 +1363,6 @@
if (prev == null) {
continue;
}
- checkState(prev != psa.value()); // Should be filtered out above.
if (prev > psa.value()) {
reduced.add(psa);
}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java b/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
index 036e531..0b2e0f7 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
@@ -52,6 +52,7 @@
import com.google.gerrit.extensions.common.ChangeMessageInfo;
import com.google.gerrit.extensions.common.RobotCommentInfo;
import com.google.gerrit.extensions.config.FactoryModule;
+import com.google.gerrit.extensions.events.CommentAddedListener;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.validators.CommentForValidation;
@@ -62,6 +63,7 @@
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;
@@ -595,7 +597,7 @@
input = new ReviewInput().label(LabelId.CODE_REVIEW, 2);
gApi.changes().id(r.getChangeId()).current().review(input);
testOnPostReview.assertApproval(
- LabelId.CODE_REVIEW, /* expectedOldValue= */ null, /* expectedNewValue= */ 2);
+ LabelId.CODE_REVIEW, /* expectedOldValue= */ 2, /* expectedNewValue= */ 2);
// Delete the vote.
input = new ReviewInput().label(LabelId.CODE_REVIEW, 0);
@@ -605,6 +607,88 @@
}
}
+ @Test
+ public void votingTheSameVoteSecondTime() throws Exception {
+ PushOneCommit.Result r = createChange();
+
+ gApi.changes().id(r.getChangeId()).addReviewer(user.email());
+ sender.clear();
+
+ // Add a new vote.
+ ReviewInput input = new ReviewInput().label(LabelId.CODE_REVIEW, 2);
+ gApi.changes().id(r.getChangeId()).current().review(input);
+ assertThat(r.getChange().approvals().values()).hasSize(1);
+
+ // Post without changing the vote.
+ input = new ReviewInput().label(LabelId.CODE_REVIEW, 2);
+ gApi.changes().id(r.getChangeId()).current().review(input);
+
+ // Second vote replaced the original vote, so still only one vote.
+ 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
+ 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");
+
+ // 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");
+ }
+ }
+
+ @Test
+ public void votingTheSameVoteSecondTimeExtendsOnPostReview() throws Exception {
+ PushOneCommit.Result r = createChange();
+
+ // Add a new vote.
+ ReviewInput input = new ReviewInput().label(LabelId.CODE_REVIEW, 2);
+ gApi.changes().id(r.getChangeId()).current().review(input);
+ assertThat(r.getChange().approvals().values()).hasSize(1);
+
+ TestOnPostReview testOnPostReview = new TestOnPostReview(/* message= */ null);
+ try (Registration registration = extensionRegistry.newRegistration().add(testOnPostReview)) {
+ // Post without changing the vote.
+ 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);
+ }
+ }
+
+ @Test
+ public void votingTheSameVoteSecondTimeFiresOnCommentAdded() throws Exception {
+ PushOneCommit.Result r = createChange();
+
+ // Add a new vote.
+ ReviewInput input = new ReviewInput().label(LabelId.CODE_REVIEW, 2);
+ gApi.changes().id(r.getChangeId()).current().review(input);
+ assertThat(r.getChange().approvals().values()).hasSize(1);
+
+ TestListener testListener = new TestListener();
+ try (Registration registration = extensionRegistry.newRegistration().add(testListener)) {
+ // Post without changing the vote.
+ 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");
+ }
+ }
+
+ private static class TestListener implements CommentAddedListener {
+ public CommentAddedListener.Event lastCommentAddedEvent;
+
+ @Override
+ public void onCommentAdded(Event event) {
+ lastCommentAddedEvent = event;
+ }
+ }
+
private List<RobotCommentInfo> getRobotComments(String changeId) throws RestApiException {
return gApi.changes().id(changeId).robotComments().values().stream()
.flatMap(Collection::stream)
diff --git a/javatests/com/google/gerrit/acceptance/api/change/StickyApprovalsIT.java b/javatests/com/google/gerrit/acceptance/api/change/StickyApprovalsIT.java
index 1e7271a..5c59129 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/StickyApprovalsIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/StickyApprovalsIT.java
@@ -43,6 +43,7 @@
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.LabelId;
import com.google.gerrit.entities.LabelType;
+import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.api.changes.CherryPickInput;
import com.google.gerrit.extensions.api.changes.ReviewInput;
@@ -574,6 +575,33 @@
assertVotes(detailedChange(changeId), admin, label, 0, REWORK);
}
+ @Test
+ public void canVoteMultipleTimesOnNewPatchsets() throws Exception {
+ // Code-Review will be sticky.
+ String label = LabelId.CODE_REVIEW;
+ try (ProjectConfigUpdate u = updateProject(project)) {
+ u.getConfig().updateLabelType(label, b -> b.setCopyAnyScore(true));
+ u.save();
+ }
+
+ PushOneCommit.Result r = createChange();
+
+ // Add a new vote.
+ ReviewInput input = new ReviewInput().label(LabelId.CODE_REVIEW, 2);
+ gApi.changes().id(r.getChangeId()).current().review(input);
+
+ // Make a new patchset, keeping the Code-Review +2 vote.
+ amendChange(r.getChangeId());
+
+ // Post without changing the vote.
+ input = new ReviewInput().label(LabelId.CODE_REVIEW, 2);
+ gApi.changes().id(r.getChangeId()).current().review(input);
+
+ // There is a vote both on patchset 1 and on patchset 2, although both votes are Code-Review +2.
+ assertThat(r.getChange().approvals().get(PatchSet.id(r.getChange().getId(), 1))).hasSize(1);
+ assertThat(r.getChange().approvals().get(PatchSet.id(r.getChange().getId(), 2))).hasSize(1);
+ }
+
private void assertChangeKindCacheContains(ObjectId prior, ObjectId next) {
ChangeKind kind =
changeKindCache.getIfPresent(ChangeKindCacheImpl.Key.create(prior, next, "recursive"));
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
index 6c53269..04e8706 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. Does not flip the postSubmit bit
- // due to deduplication codepath.
+ // Repeating the current label is allowed. Flips the postSubmit since technically this is a
+ // new vote.
gApi.changes().id(changeId).current().review(ReviewInput.recommend());
approval = getApproval(changeId, label);
assertThat(approval.value).isEqualTo(1);
- assertThat(approval.postSubmit).isNull();
+ assertThat(approval.postSubmit).isTrue();
// 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).isNull();
+ assertThat(approval.postSubmit).isTrue();
// Increasing vote is allowed.
gApi.changes().id(changeId).current().review(ReviewInput.approve());
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java b/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
index bc61fae..796ce38 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
@@ -1378,7 +1378,6 @@
ChangeInfo info = get(change.getChangeId(), ListChangesOption.MESSAGES);
assertThat(info.messages).isNotNull();
Iterable<String> messages = Iterables.transform(info.messages, i -> i.message);
- assertThat(messages).hasSize(3);
String last = Iterables.getLast(messages);
if (getSubmitType() == SubmitType.CHERRY_PICK) {
assertThat(last).startsWith("Change has been successfully cherry-picked as");
