Merge "Fix bug about persisting copied votes on submit"
diff --git a/java/com/google/gerrit/server/change/PatchSetInserter.java b/java/com/google/gerrit/server/change/PatchSetInserter.java
index f093958..209901d 100644
--- a/java/com/google/gerrit/server/change/PatchSetInserter.java
+++ b/java/com/google/gerrit/server/change/PatchSetInserter.java
@@ -104,6 +104,7 @@
private boolean allowClosed;
private boolean sendEmail = true;
private String topic;
+ private boolean storeCopiedVotes = true;
// Fields set during some phase of BatchUpdate.Op.
private Change change;
@@ -203,6 +204,17 @@
return this;
}
+ /**
+ * We always want to store copied votes except when the change is getting submitted and a new
+ * patch-set is created on submit (using submit strategies such as "REBASE_ALWAYS"). In such
+ * cases, we already store the votes of the new patch-sets in SubmitStrategyOp#saveApprovals. We
+ * should not also store the copied votes.
+ */
+ public PatchSetInserter setStoreCopiedVotes(boolean storeCopiedVotes) {
+ this.storeCopiedVotes = storeCopiedVotes;
+ return this;
+ }
+
public Change getChange() {
checkState(change != null, "getChange() only valid after executing update");
return change;
@@ -288,7 +300,9 @@
// Approvals that are being set in the new patch-set during this operation are not available yet
// outside of the scope of this method. Only copied approvals are set here.
- approvalsUtil.byPatchSet(ctx.getNotes(), patchSet).forEach(a -> update.putCopiedApproval(a));
+ if (storeCopiedVotes) {
+ approvalsUtil.byPatchSet(ctx.getNotes(), patchSet).forEach(a -> update.putCopiedApproval(a));
+ }
return true;
}
diff --git a/java/com/google/gerrit/server/change/RebaseChangeOp.java b/java/com/google/gerrit/server/change/RebaseChangeOp.java
index 4952464..3e67cca 100644
--- a/java/com/google/gerrit/server/change/RebaseChangeOp.java
+++ b/java/com/google/gerrit/server/change/RebaseChangeOp.java
@@ -92,6 +92,7 @@
private boolean detailedCommitMessage;
private boolean postMessage = true;
private boolean sendEmail = true;
+ private boolean storeCopiedVotes = true;
private boolean matchAuthorToCommitterDate = false;
private CodeReviewCommit rebasedCommit;
@@ -169,6 +170,17 @@
return this;
}
+ /**
+ * We always want to store copied votes except when the change is getting submitted and a new
+ * patch-set is created on submit (using submit strategies such as "REBASE_ALWAYS"). In such
+ * cases, we already store the votes of the new patch-sets in SubmitStrategyOp#saveApprovals. We
+ * should not also store the copied votes.
+ */
+ public RebaseChangeOp setStoreCopiedVotes(boolean storeCopiedVotes) {
+ this.storeCopiedVotes = storeCopiedVotes;
+ return this;
+ }
+
public RebaseChangeOp setSendEmail(boolean sendEmail) {
this.sendEmail = sendEmail;
return this;
@@ -219,7 +231,10 @@
.setFireRevisionCreated(fireRevisionCreated)
.setCheckAddPatchSetPermission(checkAddPatchSetPermission)
.setValidate(validate)
- .setSendEmail(sendEmail);
+ .setSendEmail(sendEmail)
+ // The votes are automatically copied and they don't count as copied votes. See
+ // method's javadoc.
+ .setStoreCopiedVotes(storeCopiedVotes);
if (!rebasedCommit.getFilesWithGitConflicts().isEmpty()
&& !notes.getChange().isWorkInProgress()) {
diff --git a/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java b/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java
index cc3b75d..355d25f 100644
--- a/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java
+++ b/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java
@@ -211,7 +211,10 @@
.setPostMessage(false)
.setSendEmail(false)
.setMatchAuthorToCommitterDate(
- args.project.is(BooleanProjectConfig.MATCH_AUTHOR_TO_COMMITTER_DATE));
+ args.project.is(BooleanProjectConfig.MATCH_AUTHOR_TO_COMMITTER_DATE))
+ // The votes are automatically copied and they don't count as copied votes. See
+ // method's javadoc.
+ .setStoreCopiedVotes(/* storeCopiedVotes = */ false);
try {
rebaseOp.updateRepo(ctx);
} catch (MergeConflictException | NoSuchChangeException e) {
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java
index fff3cb6..b3592e3 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java
@@ -14,9 +14,11 @@
package com.google.gerrit.acceptance.rest.change;
+import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.extensions.client.ListChangesOption.CURRENT_REVISION;
import static com.google.gerrit.extensions.client.ListChangesOption.MESSAGES;
+import static java.util.Comparator.comparing;
import com.google.common.collect.Iterables;
import com.google.gerrit.acceptance.ExtensionRegistry;
@@ -26,6 +28,10 @@
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.common.FooterConstants;
import com.google.gerrit.entities.BranchNameKey;
+import com.google.gerrit.entities.LabelId;
+import com.google.gerrit.entities.PatchSetApproval;
+import com.google.gerrit.entities.Project.NameKey;
+import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.changes.SubmitInput;
import com.google.gerrit.extensions.client.ChangeStatus;
import com.google.gerrit.extensions.client.InheritableBoolean;
@@ -403,4 +409,54 @@
assertSubmittable(change2Result.getChangeId());
}
+
+ @Test
+ public void stickyVoteStoredOnSubmitOnNewPatchset_withoutCopyCondition() throws Exception {
+ try (ProjectConfigUpdate u = updateProject(NameKey.parse("All-Projects"))) {
+ u.getConfig()
+ .updateLabelType(
+ LabelId.CODE_REVIEW, b -> b.setCopyAllScoresIfListOfFilesDidNotChange(true));
+ u.save();
+ }
+ stickyVoteStoredOnSubmitOnNewPatchset();
+ }
+
+ @Test
+ public void stickyVoteStoredOnSubmitOnNewPatchset_withCopyCondition() throws Exception {
+ // Code-Review will be sticky.
+ try (ProjectConfigUpdate u = updateProject(NameKey.parse("All-Projects"))) {
+ u.getConfig()
+ .updateLabelType(LabelId.CODE_REVIEW, b -> b.setCopyCondition("has:unchanged-files"));
+ u.save();
+ }
+ stickyVoteStoredOnSubmitOnNewPatchset();
+ }
+
+ private void stickyVoteStoredOnSubmitOnNewPatchset() 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);
+
+ // Submit, also keeping the Code-Review +2 vote.
+ gApi.changes().id(r.getChangeId()).current().submit();
+
+ // The last approval is stored on the submitted patch-set which was created by cherry-pick
+ // during submit.
+ PatchSetApproval patchSetApprovals =
+ Iterables.getLast(
+ r.getChange().notes().getApprovalsWithCopied().values().stream()
+ .filter(a -> a.labelId().equals(LabelId.create(LabelId.CODE_REVIEW)))
+ .sorted(comparing(a -> a.patchSetId().get()))
+ .collect(toImmutableList()));
+
+ assertThat(patchSetApprovals.patchSetId().get()).isEqualTo(2);
+ assertThat(patchSetApprovals.label()).isEqualTo(LabelId.CODE_REVIEW);
+ assertThat(patchSetApprovals.value()).isEqualTo((short) 2);
+
+ // The approval is not copied since we don't need to persist copied votes on submit, only
+ // persist votes normally.
+ assertThat(patchSetApprovals.copied()).isFalse();
+ }
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByRebaseAlwaysIT.java b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByRebaseAlwaysIT.java
index eeeac2a..aa93815 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByRebaseAlwaysIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByRebaseAlwaysIT.java
@@ -14,20 +14,27 @@
package com.google.gerrit.acceptance.rest.change;
+import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.extensions.client.ListChangesOption.CURRENT_REVISION;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+import static java.util.Comparator.comparing;
import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
import com.google.gerrit.acceptance.ExtensionRegistry;
import com.google.gerrit.acceptance.ExtensionRegistry.Registration;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestProjectInput;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.common.FooterConstants;
+import com.google.gerrit.entities.LabelId;
import com.google.gerrit.entities.PatchSet;
+import com.google.gerrit.entities.PatchSetApproval;
+import com.google.gerrit.entities.Project.NameKey;
import com.google.gerrit.exceptions.InternalServerWithUserMessageException;
+import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.client.InheritableBoolean;
import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.extensions.common.ChangeInfo;
@@ -159,6 +166,56 @@
}
}
+ @Test
+ public void stickyVoteStoredOnSubmitOnNewPatchset_withoutCopyCondition() throws Exception {
+ try (ProjectConfigUpdate u = updateProject(NameKey.parse("All-Projects"))) {
+ u.getConfig()
+ .updateLabelType(
+ LabelId.CODE_REVIEW, b -> b.setCopyAllScoresIfListOfFilesDidNotChange(true));
+ u.save();
+ }
+ stickyVoteStoredOnSubmitOnNewPatchset();
+ }
+
+ @Test
+ public void stickyVoteStoredOnSubmitOnNewPatchset_withCopyCondition() throws Exception {
+ // Code-Review will be sticky.
+ try (ProjectConfigUpdate u = updateProject(NameKey.parse("All-Projects"))) {
+ u.getConfig()
+ .updateLabelType(LabelId.CODE_REVIEW, b -> b.setCopyCondition("has:unchanged-files"));
+ u.save();
+ }
+ stickyVoteStoredOnSubmitOnNewPatchset();
+ }
+
+ private void stickyVoteStoredOnSubmitOnNewPatchset() 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);
+
+ // Submit, also keeping the Code-Review +2 vote.
+ gApi.changes().id(r.getChangeId()).current().submit();
+
+ // The last approval is stored on the submitted patch-set which was created by rebase during
+ // submit.
+ PatchSetApproval patchSetApprovals =
+ Iterables.getLast(
+ r.getChange().notes().getApprovalsWithCopied().values().stream()
+ .filter(a -> a.labelId().equals(LabelId.create(LabelId.CODE_REVIEW)))
+ .sorted(comparing(a -> a.patchSetId().get()))
+ .collect(toImmutableList()));
+
+ assertThat(patchSetApprovals.patchSetId().get()).isEqualTo(2);
+ assertThat(patchSetApprovals.label()).isEqualTo(LabelId.CODE_REVIEW);
+ assertThat(patchSetApprovals.value()).isEqualTo((short) 2);
+
+ // The approval is not copied since we don't need to persist copied votes on submit, only
+ // persist votes normally.
+ assertThat(patchSetApprovals.copied()).isFalse();
+ }
+
private void assertLatestRevisionHasFooters(PushOneCommit.Result change) throws Throwable {
RevCommit c = getCurrentCommit(change);
assertThat(c.getFooterLines(FooterConstants.CHANGE_ID)).isNotEmpty();
