Treat 0 label value as a deleted vote in DeleteVoteOp.
Before the fix, the DeleteVoteOp sent e-mail each time when the
DeleteVoteOp is executed, even if the label had been already set to 0.
As a result, if the delete vote API was called multiple times for the
same vote, users received an e-mail for each API call, though actually
no changes were made.
This fix can break some scenarios, where the caller expect that
DeleteVoteOp is always succeed after the label was added at least once.
This should be fixed on the caller side.
Google-Bug-Id: b/264982770
Release-Notes: Delete vote now fails for an already deleted vote.
Change-Id: Ibfe6d2c4fed61a3fb002c748983e45b0a2766c5f
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 2810d1e..aceb38e 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -3799,6 +3799,10 @@
If another user removed a user's vote, the user with the deleted vote will be
added to the attention set.
+The request returns:
+ * '204 No Content' if the vote is deleted successfully;
+ * '404 Not Found' when the vote to be deleted is zero or not present.
+
.Request
----
DELETE /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/reviewers/John%20Doe/votes/Code-Review HTTP/1.0
diff --git a/java/com/google/gerrit/server/restapi/change/DeleteVoteOp.java b/java/com/google/gerrit/server/restapi/change/DeleteVoteOp.java
index 0e1a218..3ac4d22 100644
--- a/java/com/google/gerrit/server/restapi/change/DeleteVoteOp.java
+++ b/java/com/google/gerrit/server/restapi/change/DeleteVoteOp.java
@@ -153,10 +153,12 @@
}
// Set the approval to 0 if vote is being removed.
newApprovals.put(a.label(), (short) 0);
- found = true;
-
- // Set old value, as required by VoteDeleted.
- oldApprovals.put(a.label(), a.value());
+ // If the value is 0, we treat it as already deleted, so no additional actions is required
+ if (a.value() != 0) {
+ found = true;
+ // Set old value, as required by VoteDeleted.
+ oldApprovals.put(a.label(), a.value());
+ }
break;
}
if (!found) {
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/DeleteVoteIT.java b/javatests/com/google/gerrit/acceptance/rest/change/DeleteVoteIT.java
index 016b1e6..6491202 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/DeleteVoteIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/DeleteVoteIT.java
@@ -140,6 +140,33 @@
verifyCannotDeleteVote(true);
}
+ @Test
+ public void deleteAlreadyDeletedVote_returnsNotFoundAndWithoutEmails() throws Exception {
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allow(Permission.REMOVE_REVIEWER).ref("refs/*").group(REGISTERED_USERS))
+ .update();
+ PushOneCommit.Result r = createChange();
+ gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(ReviewInput.approve());
+ String deleteAdminVoteEndPoint =
+ "/changes/"
+ + r.getChangeId()
+ + "/reviewers/"
+ + admin.id().toString()
+ + "/votes/Code-Review";
+
+ sender.clear();
+ RestResponse response = userRestSession.delete(deleteAdminVoteEndPoint);
+ response.assertNoContent();
+ assertThat(sender.getMessages()).hasSize(1);
+
+ sender.clear();
+ response = userRestSession.delete(deleteAdminVoteEndPoint);
+ response.assertNotFound();
+ assertThat(sender.getMessages()).isEmpty();
+ }
+
private void verifyDeleteVote(boolean onRevisionLevel) throws Exception {
PushOneCommit.Result r = createChange();
gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(ReviewInput.approve());
