Merge "Get project list for guessing groups from in memory project cache"
diff --git a/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java
index a8ba052..22eb32c 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReview.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReview.java
@@ -379,7 +379,8 @@
// Add the review ops.
logger.atFine().log("posting review");
PostReviewOp postReviewOp =
- postReviewOpFactory.create(projectState, revision.getPatchSet().id(), input);
+ postReviewOpFactory.create(
+ projectState, revision.getPatchSet().id(), input, revision.getAccountId());
bu.addOp(revision.getChange().getId(), postReviewOp);
// Adjust the attention set based on the input
diff --git a/java/com/google/gerrit/server/restapi/change/PostReviewOp.java b/java/com/google/gerrit/server/restapi/change/PostReviewOp.java
index b7d17f2..29e453b 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReviewOp.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReviewOp.java
@@ -97,7 +97,8 @@
public class PostReviewOp implements BatchUpdateOp {
interface Factory {
- PostReviewOp create(ProjectState projectState, PatchSet.Id psId, ReviewInput in);
+ PostReviewOp create(
+ ProjectState projectState, PatchSet.Id psId, ReviewInput in, Account.Id reviewerId);
}
/**
@@ -192,6 +193,7 @@
private final ProjectState projectState;
private final PatchSet.Id psId;
private final ReviewInput in;
+ private final Account.Id reviewerId;
private final boolean publishPatchSetLevelComment;
private IdentifiedUser user;
@@ -220,7 +222,8 @@
PluginSetContext<OnPostReview> onPostReviews,
@Assisted ProjectState projectState,
@Assisted PatchSet.Id psId,
- @Assisted ReviewInput in) {
+ @Assisted ReviewInput in,
+ @Assisted Account.Id reviewerId) {
this.approvalCopier = approvalCopier;
this.approvalsUtil = approvalsUtil;
this.publishCommentUtil = publishCommentUtil;
@@ -237,6 +240,7 @@
this.projectState = projectState;
this.psId = psId;
this.in = in;
+ this.reviewerId = reviewerId;
}
@Override
@@ -645,10 +649,11 @@
del.add(c);
update.putApproval(normName, (short) 0);
}
- // Only allow voting again if the vote is copied over from a past patch-set, or the
- // values are different.
+ // Only allow voting again the values are different, if the real account differs or if the
+ // vote is copied over from a past patch-set.
} else if (c != null
&& (c.value() != ent.getValue()
+ || !c.realAccountId().equals(reviewerId)
|| (inLabels.containsKey(c.label()) && isApprovalCopiedOver(c, ctx.getNotes())))) {
PatchSetApproval.Builder b =
c.toBuilder()
diff --git a/javatests/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java b/javatests/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java
index 804723b..3531d1c 100644
--- a/javatests/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java
@@ -166,6 +166,202 @@
}
@Test
+ public void overrideImpersonatedVoteWithOtherImpersonatedVote_sameValue() throws Exception {
+ allowCodeReviewOnBehalfOf();
+ TestAccount realUser = admin;
+ TestAccount realUser2 = admin2;
+ TestAccount impersonatedUser = user;
+ PushOneCommit.Result r = createChange();
+ RevisionApi revision = gApi.changes().id(r.getChangeId()).current();
+
+ // realUser votes Code-Review+1 on behalf of impersonatedUser
+ ReviewInput in = ReviewInput.recommend();
+ in.onBehalfOf = impersonatedUser.id().toString();
+ in.message = "Message on behalf of";
+ revision.review(in);
+
+ PatchSetApproval psa = Iterables.getOnlyElement(r.getChange().approvals().values());
+ assertThat(psa.patchSetId().get()).isEqualTo(1);
+ assertThat(psa.label()).isEqualTo("Code-Review");
+ assertThat(psa.accountId()).isEqualTo(impersonatedUser.id());
+ assertThat(psa.value()).isEqualTo(1);
+ assertThat(psa.realAccountId()).isEqualTo(realUser.id());
+
+ ChangeData cd = r.getChange();
+ ChangeMessage m = Iterables.getLast(cmUtil.byChange(cd.notes()));
+ assertThat(m.getMessage()).endsWith(in.message);
+ assertThat(m.getAuthor()).isEqualTo(impersonatedUser.id());
+ assertThat(m.getRealAuthor()).isEqualTo(realUser.id());
+
+ // realUser2 votes Code-Review+1 on behalf of impersonatedUser, this should override the
+ // impersonated Code-Review+1 of realUser with an impersonated Code-Review+1 of realUser2
+ requestScopeOperations.setApiUser(realUser2.id());
+ in = ReviewInput.recommend();
+ in.onBehalfOf = impersonatedUser.id().toString();
+ in.message = "Another message on behalf of";
+ gApi.changes().id(r.getChangeId()).current().review(in);
+
+ psa = Iterables.getOnlyElement(r.getChange().approvals().values());
+ assertThat(psa.patchSetId().get()).isEqualTo(1);
+ assertThat(psa.label()).isEqualTo("Code-Review");
+ assertThat(psa.accountId()).isEqualTo(impersonatedUser.id());
+ assertThat(psa.value()).isEqualTo(1);
+ assertThat(psa.realAccountId()).isEqualTo(realUser2.id());
+
+ cd = r.getChange();
+ m = Iterables.getLast(cmUtil.byChange(cd.notes()));
+ assertThat(m.getMessage()).endsWith(in.message);
+ assertThat(m.getAuthor()).isEqualTo(impersonatedUser.id());
+ assertThat(m.getRealAuthor()).isEqualTo(realUser2.id());
+ }
+
+ @Test
+ public void overrideImpersonatedVoteWithOtherImpersonatedVote_differentValue() throws Exception {
+ allowCodeReviewOnBehalfOf();
+ TestAccount realUser = admin;
+ TestAccount realUser2 = admin2;
+ TestAccount impersonatedUser = user;
+ PushOneCommit.Result r = createChange();
+ RevisionApi revision = gApi.changes().id(r.getChangeId()).current();
+
+ // realUser votes Code-Review+1 on behalf of impersonatedUser
+ ReviewInput in = ReviewInput.recommend();
+ in.onBehalfOf = impersonatedUser.id().toString();
+ in.message = "Message on behalf of";
+ revision.review(in);
+
+ PatchSetApproval psa = Iterables.getOnlyElement(r.getChange().approvals().values());
+ assertThat(psa.patchSetId().get()).isEqualTo(1);
+ assertThat(psa.label()).isEqualTo("Code-Review");
+ assertThat(psa.accountId()).isEqualTo(impersonatedUser.id());
+ assertThat(psa.value()).isEqualTo(1);
+ assertThat(psa.realAccountId()).isEqualTo(realUser.id());
+
+ ChangeData cd = r.getChange();
+ ChangeMessage m = Iterables.getLast(cmUtil.byChange(cd.notes()));
+ assertThat(m.getMessage()).endsWith(in.message);
+ assertThat(m.getAuthor()).isEqualTo(impersonatedUser.id());
+ assertThat(m.getRealAuthor()).isEqualTo(realUser.id());
+
+ // realUser2 votes Code-Review-1 on behalf of impersonatedUser, this should override the
+ // impersonated Code-Review+1 of realUser with an impersonated Code-Review-1 of realUser2
+ requestScopeOperations.setApiUser(realUser2.id());
+ in = ReviewInput.dislike();
+ in.onBehalfOf = impersonatedUser.id().toString();
+ in.message = "Another message on behalf of";
+ gApi.changes().id(r.getChangeId()).current().review(in);
+
+ psa = Iterables.getOnlyElement(r.getChange().approvals().values());
+ assertThat(psa.patchSetId().get()).isEqualTo(1);
+ assertThat(psa.label()).isEqualTo("Code-Review");
+ assertThat(psa.accountId()).isEqualTo(impersonatedUser.id());
+ assertThat(psa.value()).isEqualTo(-1);
+ assertThat(psa.realAccountId()).isEqualTo(realUser2.id());
+
+ cd = r.getChange();
+ m = Iterables.getLast(cmUtil.byChange(cd.notes()));
+ assertThat(m.getMessage()).endsWith(in.message);
+ assertThat(m.getAuthor()).isEqualTo(impersonatedUser.id());
+ assertThat(m.getRealAuthor()).isEqualTo(realUser2.id());
+ }
+
+ @Test
+ public void overrideImpersonatedVoteWithNonImpersonatedVote_sameValue() throws Exception {
+ allowCodeReviewOnBehalfOf();
+ TestAccount realUser = admin;
+ TestAccount impersonatedUser = user;
+ PushOneCommit.Result r = createChange();
+ RevisionApi revision = gApi.changes().id(r.getChangeId()).current();
+
+ // realUser votes Code-Review+1 on behalf of impersonatedUser
+ ReviewInput in = ReviewInput.recommend();
+ in.onBehalfOf = impersonatedUser.id().toString();
+ in.message = "Message on behalf of";
+ revision.review(in);
+
+ PatchSetApproval psa = Iterables.getOnlyElement(r.getChange().approvals().values());
+ assertThat(psa.patchSetId().get()).isEqualTo(1);
+ assertThat(psa.label()).isEqualTo("Code-Review");
+ assertThat(psa.accountId()).isEqualTo(impersonatedUser.id());
+ assertThat(psa.value()).isEqualTo(1);
+ assertThat(psa.realAccountId()).isEqualTo(realUser.id());
+
+ ChangeData cd = r.getChange();
+ ChangeMessage m = Iterables.getLast(cmUtil.byChange(cd.notes()));
+ assertThat(m.getMessage()).endsWith(in.message);
+ assertThat(m.getAuthor()).isEqualTo(impersonatedUser.id());
+ assertThat(m.getRealAuthor()).isEqualTo(realUser.id());
+
+ // impersonatedUser votes Code-Review+1 themselves, this should override the impersonated
+ // Code-Review+1 with a non-impersonated Code-Review+1
+ requestScopeOperations.setApiUser(impersonatedUser.id());
+ in = ReviewInput.recommend();
+ in.message = "Message";
+ gApi.changes().id(r.getChangeId()).current().review(in);
+
+ psa = Iterables.getOnlyElement(r.getChange().approvals().values());
+ assertThat(psa.patchSetId().get()).isEqualTo(1);
+ assertThat(psa.label()).isEqualTo("Code-Review");
+ assertThat(psa.accountId()).isEqualTo(impersonatedUser.id());
+ assertThat(psa.value()).isEqualTo(1);
+ assertThat(psa.realAccountId()).isEqualTo(impersonatedUser.id());
+
+ cd = r.getChange();
+ m = Iterables.getLast(cmUtil.byChange(cd.notes()));
+ assertThat(m.getMessage()).endsWith(in.message);
+ assertThat(m.getAuthor()).isEqualTo(impersonatedUser.id());
+ assertThat(m.getRealAuthor()).isEqualTo(impersonatedUser.id());
+ }
+
+ @Test
+ public void overrideImpersonatedVoteWithNonImpersonatedVote_differentValue() throws Exception {
+ allowCodeReviewOnBehalfOf();
+ TestAccount realUser = admin;
+ TestAccount impersonatedUser = user;
+ PushOneCommit.Result r = createChange();
+ RevisionApi revision = gApi.changes().id(r.getChangeId()).current();
+
+ // realUser votes Code-Review+1 on behalf of impersonatedUser
+ ReviewInput in = ReviewInput.recommend();
+ in.onBehalfOf = impersonatedUser.id().toString();
+ in.message = "Message on behalf of";
+ revision.review(in);
+
+ PatchSetApproval psa = Iterables.getOnlyElement(r.getChange().approvals().values());
+ assertThat(psa.patchSetId().get()).isEqualTo(1);
+ assertThat(psa.label()).isEqualTo("Code-Review");
+ assertThat(psa.accountId()).isEqualTo(impersonatedUser.id());
+ assertThat(psa.value()).isEqualTo(1);
+ assertThat(psa.realAccountId()).isEqualTo(realUser.id());
+
+ ChangeData cd = r.getChange();
+ ChangeMessage m = Iterables.getLast(cmUtil.byChange(cd.notes()));
+ assertThat(m.getMessage()).endsWith(in.message);
+ assertThat(m.getAuthor()).isEqualTo(impersonatedUser.id());
+ assertThat(m.getRealAuthor()).isEqualTo(realUser.id());
+
+ // impersonatedUser votes Code-Review-1 themselves, this should override the impersonated
+ // Code-Review+1 with a non-impersonated Code-Review-1
+ requestScopeOperations.setApiUser(impersonatedUser.id());
+ in = ReviewInput.dislike();
+ in.message = "Message";
+ gApi.changes().id(r.getChangeId()).current().review(in);
+
+ psa = Iterables.getOnlyElement(r.getChange().approvals().values());
+ assertThat(psa.patchSetId().get()).isEqualTo(1);
+ assertThat(psa.label()).isEqualTo("Code-Review");
+ assertThat(psa.accountId()).isEqualTo(impersonatedUser.id());
+ assertThat(psa.value()).isEqualTo(-1);
+ assertThat(psa.realAccountId()).isEqualTo(impersonatedUser.id());
+
+ cd = r.getChange();
+ m = Iterables.getLast(cmUtil.byChange(cd.notes()));
+ assertThat(m.getMessage()).endsWith(in.message);
+ assertThat(m.getAuthor()).isEqualTo(impersonatedUser.id());
+ assertThat(m.getRealAuthor()).isEqualTo(impersonatedUser.id());
+ }
+
+ @Test
public void voteOnBehalfOfRequiresLabel() throws Exception {
allowCodeReviewOnBehalfOf();
PushOneCommit.Result r = createChange();
