Include copied votes into change message when voting on outdated patch set Since change I4d9b71616 votes that are applied on outdated patch sets are copied to follow-up patch sets if possible (follow-up patch sets are all patch sets that are newer than the outdated patch set on which the vote was applied). When votes are copied to follow-up patch sets they are applied in NoteDb, but users are not informed about them. This means if a vote is applied on an outdated patch set and is then copied forward to follow-up patch sets, including the current patch set, users can see the copied vote on the change, but they can't find out how it appeared there. With this change we are now informing in the change message that is posted when a vote is applied on an outdated patch set about the follow-up patch sets to which the vote is copied forward. In other words we are extending the existing change message that informs about the user vote on the outdated patch set with information about the copied votes on the follow-up patch sets that have been updated as result of the user voting on the outdated patch set. Informing the user about the follow-up patch sets that have been updated as result of the user voting on the outdated patch set via the change message is consistent with how we inform users about copied votes when a new patch set is created (also in this case we post this information as a change message). When the user votes on an outdated patch sets there are 3 possibilities how copied votes on follow-up patch sets are updated: 1. a copied approval is added on a follow-up patch set (no copied approval existing on this patch set yet) 2. an existing copied approval is updated on a follow-up patch set (e.g. if Code-Review-2 on PS1 was sticky and got copied to PS2, but now the vote on PS1 is changed to Code-Review+2 which is also sticky the copied vote on PS2 gets updated from Code-Review-2 to Code-Review+2) 3. an existing copied approval is removed from a follow-up patch set (example 1: if Code-Review-2 on PS1 was sticky and got copied to PS2, but now the vote on PS1 is changed to Code-Review-1 which is not sticky the existing copied vote on PS2 gets removed; example 2: if Code-Review-2 on PS1 was sticky and got copied to PS2, but now the vote on PS1 is removed the existing copied vote on PS2 gets removed) Note: The approval copying that is done when a user votes on an outdated patch set only affects copied approvals on follow-up patch sets, but never non-copied approvals nor explicit approval deletions. The existing change message already informs about the user action (which vote was applied on which patch sets), e.g. "Patch Set 1: Code-Review+2". In addition we include now information about the updates on the follow-up patch sets. If label votes have been copied to follow-up patch sets (case 1.) the additional information in the change message is: * <label-vote> has been copied to patch sets: 3, 4 (copy condition: "<copy-condition>"). If existing copied votes on follow-up patch sets have been updated (case 2.), the old copied votes are included into the message: * <label-vote> has been copied to patch sets: 3 (was <old-label-vote>), 4 (was <old-label-vote>) (copy condition: "<copy-condition>"). If existing copied votes on follow-up patch sets have been removed (e.g. because the new vote is not copyable) (case 3.) the message is: * Copied <label> vote has been removed from patch set 3 (was <old-label-vote>), 4 (was <old-label-vote>) (copy condition: "<copy-condition>"). If copied votes have been both added/updated and removed, 2 messages are included: * <label-vote> has been copied to patch sets: 3 (was <old-label-vote>), 4 (was <old-label-vote>) (copy condition: "<copy-condition>"). * Copied <label> vote has been removed from patch set 5 (was <old-label-vote>) (copy condition: "<copy-condition>"). Passing atoms in copy conditions are not highlighted. This is because the passing atoms can be different for different follow-up patch sets (e.g. 'changekind:TRIVIAL_REBASE OR changekind:NO_CODE_CHANGE' can have 'changekind:TRIVIAL_REBASE' passing for one follow-up patch set and 'changekind:NO_CODE_CHANGE' passing for another follow-up patch set). Including the copy condition once per follow-up patch set with differently highlighted passing atoms would make the message unreadable. Hence we don't highlight passing atoms here. Signed-off-by: Edwin Kempin <ekempin@google.com> Change-Id: I7ca035f668ce5dadc8403ebe9195a3aac27e354d Bug: Google b/235818646 Release-Notes: Votes that are copied to follow-up patch sets when a vote on an outdated patch set is applied are now posted as a change message.
diff --git a/java/com/google/gerrit/server/restapi/change/PostReviewOp.java b/java/com/google/gerrit/server/restapi/change/PostReviewOp.java index 3c31aad..8a92046 100644 --- a/java/com/google/gerrit/server/restapi/change/PostReviewOp.java +++ b/java/com/google/gerrit/server/restapi/change/PostReviewOp.java
@@ -18,15 +18,19 @@ 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; +import static java.util.Comparator.comparing; import static java.util.stream.Collectors.joining; import static java.util.stream.Collectors.toList; import static java.util.stream.Collectors.toSet; +import com.google.auto.value.AutoValue; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Joiner; import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableTable; +import com.google.common.collect.MultimapBuilder; +import com.google.common.collect.SortedSetMultimap; import com.google.common.collect.Streams; import com.google.common.collect.Table.Cell; import com.google.gerrit.entities.Account; @@ -34,7 +38,6 @@ import com.google.gerrit.entities.FixReplacement; import com.google.gerrit.entities.FixSuggestion; import com.google.gerrit.entities.HumanComment; -import com.google.gerrit.entities.LabelId; import com.google.gerrit.entities.LabelType; import com.google.gerrit.entities.LabelTypes; import com.google.gerrit.entities.PatchSet; @@ -98,6 +101,81 @@ PostReviewOp create(ProjectState projectState, PatchSet.Id psId, ReviewInput in); } + /** + * Update of a copied label that has been performed on a follow-up patch set after a vote has been + * applied on an outdated patch set (follow-up patch sets = all patch sets that are newer than the + * outdated patch set on which the user voted). + */ + @AutoValue + abstract static class CopiedLabelUpdate { + /** + * Type of the update that has been performed for a copied vote on a follow-up patch set. + * + * <p>Whether the copied vote has been added + * + * <ul> + * <li>added to + * <li>updated on + * <li>removed from + * </ul> + * + * a follow-up patch set. + */ + enum Type { + /** A copied vote was added. No copied vote existed for this label yet. */ + ADDED, + + /** An existing copied vote has been updated. */ + UPDATED, + + /** An existing copied vote has been removed. */ + REMOVED; + } + + /** The ID of the (follow-up) patch set on which the copied label update has been performed. */ + abstract PatchSet.Id patchSetId(); + + /** + * The old copied label vote that has been updated or that has been removed. + * + * <p>Not set if {@link #type()} is {@link Type#ADDED}. + */ + abstract Optional<LabelVote> oldLabelVote(); + + /** + * The type of the update that has been performed for the copied vote on the (follow-up) patch + * set. + */ + abstract Type type(); + + /** Returns a string with the patch set number and if present the old label vote. */ + private String formatPatchSetWithOldLabelVote() { + StringBuilder b = new StringBuilder(); + b.append(patchSetId().get()); + if (oldLabelVote().isPresent()) { + b.append(" (was ").append(oldLabelVote().get().format()).append(")"); + } + return b.toString(); + } + + private static CopiedLabelUpdate added(PatchSet.Id patchSetId) { + return create(patchSetId, Optional.empty(), Type.ADDED); + } + + private static CopiedLabelUpdate updated(PatchSet.Id patchSetId, LabelVote oldLabelVote) { + return create(patchSetId, Optional.of(oldLabelVote), Type.UPDATED); + } + + private static CopiedLabelUpdate removed(PatchSet.Id patchSetId, LabelVote oldLabelVote) { + return create(patchSetId, Optional.of(oldLabelVote), Type.REMOVED); + } + + private static CopiedLabelUpdate create( + PatchSet.Id patchSetId, Optional<LabelVote> oldLabelVote, Type type) { + return new AutoValue_PostReviewOp_CopiedLabelUpdate(patchSetId, oldLabelVote, type); + } + } + @VisibleForTesting public static final String START_REVIEW_MESSAGE = "This change is ready for review."; @@ -123,6 +201,8 @@ private String mailMessage; private List<Comment> comments = new ArrayList<>(); private List<LabelVote> labelDelta = new ArrayList<>(); + private SortedSetMultimap<LabelVote, CopiedLabelUpdate> labelUpdatesOnFollowUpPatchSets = + MultimapBuilder.hashKeys().treeSetValues(comparing(CopiedLabelUpdate::patchSetId)).build(); private Map<String, Short> approvals = new HashMap<>(); private Map<String, Short> oldApprovals = new HashMap<>(); @@ -743,32 +823,25 @@ ImmutableTable<String, Account.Id, Optional<PatchSetApproval>> newApprovals = ctx.getUpdate(psId).getApprovals(); for (Cell<String, Account.Id, Optional<PatchSetApproval>> cell : newApprovals.cellSet()) { - String label = cell.getRowKey(); - Account.Id approverId = cell.getColumnKey(); - PatchSetApproval.Key psaKey = PatchSetApproval.key(psId, approverId, LabelId.create(label)); + PatchSetApproval psaOrig = cell.getValue().get(); if (isRemoval(cell)) { - if (removeCopies(ctx, followUpPatchSets, psaKey)) { + if (removeCopies(ctx, followUpPatchSets, psaOrig)) { dirty = true; } continue; } PatchSet patchSet = psUtil.get(ctx.getNotes(), psId); - PatchSetApproval psaOrig = cell.getValue().get(); // Target patch sets to which the approval is copyable. ImmutableList<PatchSet.Id> targetPatchSets = approvalCopier.forApproval( - ctx.getNotes(), - patchSet, - psaKey.accountId(), - psaKey.labelId().get(), - psaOrig.value()); + ctx.getNotes(), patchSet, psaOrig.accountId(), psaOrig.label(), psaOrig.value()); // Iterate over all follow-up patch sets, in patch set order. for (PatchSet.Id followUpPatchSetId : followUpPatchSets) { - if (hasOverrideOf(ctx, followUpPatchSetId, psaKey)) { + if (hasOverrideOf(ctx, followUpPatchSetId, psaOrig.key())) { // a non-copied approval exists that overrides any copied approval // -> do not copy the approval to this patch set nor to any follow-up patch sets break; @@ -777,21 +850,28 @@ if (targetPatchSets.contains(followUpPatchSetId)) { // The approval is copyable to the new patch set. - if (hasCopyOfWithValue(ctx, followUpPatchSetId, psaKey, psaOrig.value())) { + if (hasCopyOfWithValue(ctx, followUpPatchSetId, psaOrig)) { // a copy approval with the exact value already exists continue; } // add/update the copied approval on the target patch set + Optional<PatchSetApproval> copiedPsa = getCopyOf(ctx, followUpPatchSetId, psaOrig.key()); PatchSetApproval copiedPatchSetApproval = psaOrig.copyWithPatchSet(followUpPatchSetId); ctx.getUpdate(followUpPatchSetId).putCopiedApproval(copiedPatchSetApproval); + labelUpdatesOnFollowUpPatchSets.put( + LabelVote.createFrom(psaOrig), + copiedPsa.isPresent() + ? CopiedLabelUpdate.updated( + followUpPatchSetId, LabelVote.createFrom(copiedPsa.get())) + : CopiedLabelUpdate.added(followUpPatchSetId)); dirty = true; } else { // The approval is not copyable to the new patch set. - - if (hasCopyOf(ctx, followUpPatchSetId, psaKey)) { + Optional<PatchSetApproval> copiedPsa = getCopyOf(ctx, followUpPatchSetId, psaOrig.key()); + if (copiedPsa.isPresent()) { // a copy approval exists and should be removed - removeCopy(ctx, followUpPatchSetId, psaKey); + removeCopy(ctx, psaOrig, copiedPsa.get()); dirty = true; } } @@ -818,18 +898,17 @@ * @param ctx the change context * @param followUpPatchSets the follow-up patch sets of the patch set on which the review is * posted - * @param psaKey the key of the patch set approval for which copies should be removed from all + * @param psaOrig the original patch set approval for which copies should be removed from all * follow-up patch sets * @return whether any copy approval has been removed */ private boolean removeCopies( - ChangeContext ctx, - ImmutableList<PatchSet.Id> followUpPatchSets, - PatchSetApproval.Key psaKey) { + ChangeContext ctx, ImmutableList<PatchSet.Id> followUpPatchSets, PatchSetApproval psaOrig) { boolean dirty = false; for (PatchSet.Id followUpPatchSet : followUpPatchSets) { - if (hasCopyOf(ctx, followUpPatchSet, psaKey)) { - removeCopy(ctx, followUpPatchSet, psaKey); + Optional<PatchSetApproval> copiedPsa = getCopyOf(ctx, followUpPatchSet, psaOrig.key()); + if (copiedPsa.isPresent()) { + removeCopy(ctx, psaOrig, copiedPsa.get()); } else { // Do not remove copy from this follow-up patch sets and also not from any further follow-up // patch sets (if the further follow-up patch sets have copies they are copies of a @@ -844,32 +923,37 @@ * Removes the copy approval with the given key from the given patch set. * * @param ctx the change context - * @param patchSet patch set from which the copy approval with the given key should be removed - * @param psaKey the key of the patch set approval for which copies should be removed from the + * @param psaOrig the original patch set approval for which copies should be removed from the * given patch set + * @param copiedPsa the copied patch set approval that should be removed */ - private void removeCopy(ChangeContext ctx, PatchSet.Id patchSet, PatchSetApproval.Key psaKey) { - ctx.getUpdate(patchSet) + private void removeCopy(ChangeContext ctx, PatchSetApproval psaOrig, PatchSetApproval copiedPsa) { + ctx.getUpdate(copiedPsa.patchSetId()) .removeCopiedApprovalFor( ctx.getIdentifiedUser().getRealUser().isIdentifiedUser() ? ctx.getIdentifiedUser().getRealUser().getAccountId() : null, - psaKey.accountId(), - psaKey.labelId().get()); + copiedPsa.accountId(), + copiedPsa.labelId().get()); + labelUpdatesOnFollowUpPatchSets.put( + LabelVote.createFrom(psaOrig), + CopiedLabelUpdate.removed(copiedPsa.patchSetId(), LabelVote.createFrom(copiedPsa))); } /** - * Whether the given patch set has a copy approval with the given key. + * Retrieves the copy of the given approval from the given patch set if it exists. * * @param ctx the change context - * @param patchSetId the ID of the patch for which it should be checked whether it has a copy - * approval with the given key - * @param psaKey the key of the patch set approval + * @param patchSetId the ID of the patch from which it the copied approval should be returned + * @param psaKey the key of the patch set approval for which the copied approval should be + * returned + * @return the copy of the given approval from the given patch set if it exists */ - private boolean hasCopyOf( + private Optional<PatchSetApproval> getCopyOf( ChangeContext ctx, PatchSet.Id patchSetId, PatchSetApproval.Key psaKey) { return ctx.getNotes().getApprovals().onlyCopied().get(patchSetId).stream() - .anyMatch(psa -> areAccountAndLabelTheSame(psa.key(), psaKey)); + .filter(psa -> areAccountAndLabelTheSame(psa.key(), psaKey)) + .findAny(); } /** @@ -878,12 +962,15 @@ * @param ctx the change context * @param patchSetId the ID of the patch for which it should be checked whether it has a copy * approval with the given key and value - * @param psaKey the key of the patch set approval + * @param psaOrig the original patch set approval */ private boolean hasCopyOfWithValue( - ChangeContext ctx, PatchSet.Id patchSetId, PatchSetApproval.Key psaKey, short value) { + ChangeContext ctx, PatchSet.Id patchSetId, PatchSetApproval psaOrig) { return ctx.getNotes().getApprovals().onlyCopied().get(patchSetId).stream() - .anyMatch(psa -> areAccountAndLabelTheSame(psa.key(), psaKey) && psa.value() == value); + .anyMatch( + psa -> + areAccountAndLabelTheSame(psa.key(), psaOrig.key()) + && psa.value() == psaOrig.value()); } /** @@ -915,6 +1002,21 @@ labelDelta.stream().map(LabelVote::format).sorted().collect(toImmutableList())) { buf.append(" ").append(formattedLabelVote); } + if (!labelUpdatesOnFollowUpPatchSets.isEmpty()) { + buf.append("\n\nCopied votes on follow-up patch sets have been updated:"); + for (Map.Entry<LabelVote, Collection<CopiedLabelUpdate>> e : + labelUpdatesOnFollowUpPatchSets.asMap().entrySet().stream() + .sorted(Map.Entry.comparingByKey(comparing(LabelVote::label))) + .collect(toImmutableList())) { + Optional<String> copyCondition = + projectState + .getLabelTypes(ctx.getNotes()) + .byLabel(e.getKey().label()) + .map(LabelType::getCopyCondition) + .map(Optional::get); + buf.append(formatVotesCopiedToFollowUpPatchSets(e.getKey(), e.getValue(), copyCondition)); + } + } if (comments.size() == 1) { buf.append("\n\n(1 comment)"); } else if (comments.size() > 1) { @@ -951,6 +1053,88 @@ return true; } + /** + * Given a label vote that has been applied on an outdated patch set, this method formats the + * updates to the copied labels on the follow-up patch sets that have been performed for that + * label vote. + * + * <p>If label votes have been copied to follow-up patch sets the formatted message is + * "<label-vote> has been copied to patch sets: 3, 4 (copy condition: "<copy-condition>").". + * + * <p>If existing copied votes on follow-up patch sets have been updated, the old copied votes are + * included into the message: "<label-vote> has been copied to patch sets: 3 (was + * <old-label-vote>), 4 (was <old-label-vote>) (copy condition: "<copy-condition>").". + * + * <p>If existing copied votes on follow-up patch sets have been removed (because the new vote is + * not copyable) the message is: "Copied <label> vote has been removed from patch set 3 (was + * <old-label-vote>), 4 (was <old-label-vote>) (copy condition: "<copy-condition>").". + * + * <p>If copied votes have been both added/updated and removed, 2 messages are returned. + * + * <p>Each returned message is formatted as a list item (prefixed with '* '). + * + * <p>Passing atoms in copy conditions are not highlighted. This is because the passing atoms can + * be different for different follow-up patch sets (e.g. 'changekind:TRIVIAL_REBASE OR + * changekind:NO_CODE_CHANGE' can have 'changekind:TRIVIAL_REBASE' passing for one follow-up patch + * set and 'changekind:NO_CODE_CHANGE' passing for another follow-up patch set). Including the + * copy condition once per follow-up patch set with differently highlighted passing atoms would + * make the message unreadable. Hence we don't highlight passing atoms here. + * + * @param labelVote the label vote that has been applied on an outdated patch set + * @param followUpPatchSetUpdates updates to copied votes on follow-up patch sets that have been + * done by copying the label vote on the outdated patch set to follow-up patch sets + * @param copyCondition the copy condition of the label for which a vote was applied on an + * outdated patch set + * @return formatted string to be included into a change message + */ + private String formatVotesCopiedToFollowUpPatchSets( + LabelVote labelVote, + Collection<CopiedLabelUpdate> followUpPatchSetUpdates, + Optional<String> copyCondition) { + StringBuilder b = new StringBuilder(); + + // Add line for added/updated copied approvals. + ImmutableList<CopiedLabelUpdate> additionsAndUpdates = + followUpPatchSetUpdates.stream() + .filter( + copiedLabelUpdate -> + copiedLabelUpdate.type() == CopiedLabelUpdate.Type.ADDED + || copiedLabelUpdate.type() == CopiedLabelUpdate.Type.UPDATED) + .collect(toImmutableList()); + if (!additionsAndUpdates.isEmpty()) { + b.append("\n* "); + b.append(labelVote.format()); + b.append(" has been copied to patch set "); + b.append( + additionsAndUpdates.stream() + .map(CopiedLabelUpdate::formatPatchSetWithOldLabelVote) + .collect(joining(", "))); + copyCondition.ifPresent(cc -> b.append(" (copy condition: \"" + cc + "\")")); + b.append("."); + } + + // Add line for removed copied approvals. + ImmutableList<CopiedLabelUpdate> removals = + followUpPatchSetUpdates.stream() + .filter(copiedLabelUpdate -> copiedLabelUpdate.type() == CopiedLabelUpdate.Type.REMOVED) + .collect(toImmutableList()); + if (!removals.isEmpty()) { + b.append("\n* Copied "); + b.append(labelVote.label()); + b.append(" vote has been removed from patch set "); + b.append( + removals.stream() + .map(CopiedLabelUpdate::formatPatchSetWithOldLabelVote) + .collect(joining(", "))); + b.append(" since the new "); + b.append(labelVote.value() != 0 ? labelVote.format() : labelVote.formatWithEquals()); + b.append(" vote is not copyable"); + copyCondition.ifPresent(cc -> b.append(" (copy condition: \"" + cc + "\")")); + b.append("."); + } + return b.toString(); + } + private void addLabelDelta(String name, short value) { labelDelta.add(LabelVote.create(name, value)); }
diff --git a/java/com/google/gerrit/server/util/LabelVote.java b/java/com/google/gerrit/server/util/LabelVote.java index 038fe2c..fbcf3ce 100644 --- a/java/com/google/gerrit/server/util/LabelVote.java +++ b/java/com/google/gerrit/server/util/LabelVote.java
@@ -19,6 +19,7 @@ import com.google.auto.value.AutoValue; import com.google.common.base.Strings; import com.google.gerrit.entities.LabelType; +import com.google.gerrit.entities.PatchSetApproval; /** A single vote on a label, consisting of a label name and a value. */ @AutoValue @@ -68,6 +69,10 @@ return new AutoValue_LabelVote(LabelType.checkNameInternal(label), value); } + public static LabelVote createFrom(PatchSetApproval psa) { + return create(psa.label(), psa.value()); + } + public abstract String label(); public abstract short value();
diff --git a/javatests/com/google/gerrit/acceptance/api/change/CopyApprovalsToFollowUpPatchSetsIT.java b/javatests/com/google/gerrit/acceptance/api/change/CopyApprovalsToFollowUpPatchSetsIT.java index 04759e8..a055201 100644 --- a/javatests/com/google/gerrit/acceptance/api/change/CopyApprovalsToFollowUpPatchSetsIT.java +++ b/javatests/com/google/gerrit/acceptance/api/change/CopyApprovalsToFollowUpPatchSetsIT.java
@@ -25,6 +25,7 @@ import static com.google.gerrit.server.project.testing.TestLabels.labelBuilder; import static com.google.gerrit.server.project.testing.TestLabels.value; +import com.google.common.collect.Iterables; import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.acceptance.TestAccount; @@ -38,6 +39,7 @@ import com.google.gerrit.extensions.api.changes.ReviewInput; import com.google.gerrit.extensions.common.ApprovalInfo; import com.google.gerrit.extensions.common.ChangeInfo; +import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.project.testing.TestLabels; import com.google.inject.Inject; @@ -106,9 +108,11 @@ r.assertOkStatus(); PatchSet patchSet2 = r.getChange().currentPatchSet(); - // Vote on the first patch set. + // Vote on the first patch set and verify the change messages. vote(admin, changeId, patchSet1.number(), 2, 1); + assertLastChangeMessage(r.getChangeId(), "Patch Set 1: Code-Review+2 Verified+1"); vote(user, changeId, patchSet1.number(), -2, -1); + assertLastChangeMessage(r.getChangeId(), "Patch Set 1: Code-Review-2 Verified-1"); // Verify that no votes have been copied to the current patch set. ChangeInfo c = detailedChange(changeId); @@ -139,9 +143,23 @@ r.assertOkStatus(); PatchSet patchSet2 = r.getChange().currentPatchSet(); - // Vote on the first patch set. + // Vote on the first patch set and verify the change messages. vote(admin, changeId, patchSet1.number(), 2, 1); + assertLastChangeMessage( + r.getChangeId(), + String.format( + "Patch Set 1: Code-Review+2 Verified+1\n\n" + + "Copied votes on follow-up patch sets have been updated:\n" + + "* Code-Review+2 has been copied to patch set 2 (copy condition: \"is:ANY\").\n" + + "* Verified+1 has been copied to patch set 2 (copy condition: \"is:ANY\").")); vote(user, changeId, patchSet1.number(), -2, -1); + assertLastChangeMessage( + r.getChangeId(), + String.format( + "Patch Set 1: Code-Review-2 Verified-1\n\n" + + "Copied votes on follow-up patch sets have been updated:\n" + + "* Code-Review-2 has been copied to patch set 2 (copy condition: \"is:ANY\").\n" + + "* Verified-1 has been copied to patch set 2 (copy condition: \"is:ANY\").")); // Verify that the votes have been copied to the current patch set. ChangeInfo c = detailedChange(changeId); @@ -176,9 +194,13 @@ vote(admin, changeId, patchSet2.number(), 2, 1); vote(user, changeId, patchSet2.number(), -2, -1); - // Vote on the first patch set. + // Vote on the first patch set and verify change message. vote(admin, changeId, patchSet1.number(), 1, -1); + assertLastChangeMessage( + r.getChangeId(), String.format("Patch Set 1: Code-Review+1 Verified-1")); vote(user, changeId, patchSet1.number(), -1, 1); + assertLastChangeMessage( + r.getChangeId(), String.format("Patch Set 1: Code-Review-1 Verified+1")); // Verify that the votes have not been copied to the current patch set (since a current vote // already exists). @@ -219,9 +241,13 @@ deleteCurrentVotes(admin, changeId); deleteCurrentVotes(user, changeId); - // Vote on the first patch set. + // Vote on the first patch set and verify the change messages. vote(admin, changeId, patchSet1.number(), 1, -1); + assertLastChangeMessage( + r.getChangeId(), String.format("Patch Set 1: Code-Review+1 Verified-1")); vote(user, changeId, patchSet1.number(), -1, 1); + assertLastChangeMessage( + r.getChangeId(), String.format("Patch Set 1: Code-Review-1 Verified+1")); // Verify that the votes have not been copied to the current patch set (since a deletion vote // already exists on the current patch set). @@ -254,9 +280,11 @@ r.assertOkStatus(); PatchSet patchSet2 = r.getChange().currentPatchSet(); - // Update the votes on the first patch set. + // Update the votes on the first patch set and verify the change messages. vote(admin, changeId, patchSet1.number(), 2, -1); + assertLastChangeMessage(r.getChangeId(), "Patch Set 1: Code-Review+2 Verified-1"); vote(user, changeId, patchSet1.number(), -1, 1); + assertLastChangeMessage(r.getChangeId(), "Patch Set 1: Code-Review-1 Verified+1"); // Verify that no votes have been copied to the current patch set. ChangeInfo c = detailedChange(changeId); @@ -297,9 +325,30 @@ assertCurrentVotes(c, admin, 1, 1); assertCurrentVotes(c, user, 2, 1); - // Update the votes on the first patch set with votes that are not copied + // Update the votes on the first patch set with votes that are not copied and verify the change + // messages. vote(admin, changeId, patchSet1.number(), -1, -1); + assertLastChangeMessage( + r.getChangeId(), + String.format( + "Patch Set 1: Code-Review-1 Verified-1\n\n" + + "Copied votes on follow-up patch sets have been updated:\n" + + "* Copied Code-Review vote has been removed from patch set 2 (was Code-Review+1)" + + " since the new Code-Review-1 vote is not copyable" + + " (copy condition: \"is:1 OR is:2\").\n" + + "* Copied Verified vote has been removed from patch set 2 (was Verified+1)" + + " since the new Verified-1 vote is not copyable (copy condition: \"is:MAX\").")); vote(user, changeId, patchSet1.number(), -2, -1); + assertLastChangeMessage( + r.getChangeId(), + String.format( + "Patch Set 1: Code-Review-2 Verified-1\n\n" + + "Copied votes on follow-up patch sets have been updated:\n" + + "* Copied Code-Review vote has been removed from patch set 2 (was Code-Review+2)" + + " since the new Code-Review-2 vote is not copyable" + + " (copy condition: \"is:1 OR is:2\").\n" + + "* Copied Verified vote has been removed from patch set 2 (was Verified+1)" + + " since the new Verified-1 vote is not copyable (copy condition: \"is:MAX\").")); // Verify that the copied votes on the current patch set have been unset. c = detailedChange(changeId); @@ -339,9 +388,26 @@ assertCurrentVotes(c, admin, 0, 0); assertCurrentVotes(c, user, 0, 0); - // Update the votes on the first patch set with votes that are copied. + // Update the votes on the first patch set with votes that are copied and verify the change + // messages. vote(admin, changeId, patchSet1.number(), 2, 1); + assertLastChangeMessage( + r.getChangeId(), + String.format( + "Patch Set 1: Code-Review+2 Verified+1\n\n" + + "Copied votes on follow-up patch sets have been updated:\n" + + "* Code-Review+2 has been copied to patch set 2" + + " (copy condition: \"is:1 OR is:2\").\n" + + "* Verified+1 has been copied to patch set 2 (copy condition: \"is:MAX\").")); vote(user, changeId, patchSet1.number(), 1, 1); + assertLastChangeMessage( + r.getChangeId(), + String.format( + "Patch Set 1: Code-Review+1 Verified+1\n\n" + + "Copied votes on follow-up patch sets have been updated:\n" + + "* Code-Review+1 has been copied to patch set 2" + + " (copy condition: \"is:1 OR is:2\").\n" + + "* Verified+1 has been copied to patch set 2 (copy condition: \"is:MAX\").")); // Verify that the votes have been copied to the current patch set. c = detailedChange(changeId); @@ -380,9 +446,11 @@ vote(admin, changeId, patchSet2.number(), 2, 1); vote(user, changeId, patchSet2.number(), -2, -1); - // Update the votes on the first patch set. + // Update the votes on the first patch set and verify the change messages. vote(admin, changeId, patchSet1.number(), -1, 1); + assertLastChangeMessage(r.getChangeId(), "Patch Set 1: Code-Review-1 Verified+1"); vote(user, changeId, patchSet1.number(), 1, -1); + assertLastChangeMessage(r.getChangeId(), "Patch Set 1: Code-Review+1 Verified-1"); // Verify that the votes have not been copied to the current patch set (since a current vote // already exists). @@ -427,9 +495,11 @@ deleteCurrentVotes(admin, changeId); deleteCurrentVotes(user, changeId); - // Update the votes on the first patch set. + // Update the votes on the first patch set and verify the change messages. vote(admin, changeId, patchSet1.number(), -1, 1); + assertLastChangeMessage(r.getChangeId(), "Patch Set 1: Code-Review-1 Verified+1"); vote(user, changeId, patchSet1.number(), 1, -1); + assertLastChangeMessage(r.getChangeId(), "Patch Set 1: Code-Review+1 Verified-1"); // Verify that the votes have not been copied to the current patch set (since a deletion vote // already exists on the current patch set). @@ -471,9 +541,28 @@ assertCurrentVotes(c, admin, -2, -1); assertCurrentVotes(c, user, 2, 1); - // Update the votes on the first patch set with votes that are copied. + // Update the votes on the first patch set with votes that are copied and verify the change + // messages. vote(admin, changeId, patchSet1.number(), 2, 1); + assertLastChangeMessage( + r.getChangeId(), + String.format( + "Patch Set 1: Code-Review+2 Verified+1\n\n" + + "Copied votes on follow-up patch sets have been updated:\n" + + "* Code-Review+2 has been copied to patch set 2 (was Code-Review-2)" + + " (copy condition: \"is:ANY\").\n" + + "* Verified+1 has been copied to patch set 2 (was Verified-1)" + + " (copy condition: \"is:ANY\").")); vote(user, changeId, patchSet1.number(), -2, -1); + assertLastChangeMessage( + r.getChangeId(), + String.format( + "Patch Set 1: Code-Review-2 Verified-1\n\n" + + "Copied votes on follow-up patch sets have been updated:\n" + + "* Code-Review-2 has been copied to patch set 2 (was Code-Review+2)" + + " (copy condition: \"is:ANY\").\n" + + "* Verified-1 has been copied to patch set 2 (was Verified+1)" + + " (copy condition: \"is:ANY\").")); // Verify that the votes have been copied to the current patch set. c = detailedChange(changeId); @@ -509,9 +598,11 @@ vote(admin, changeId, patchSet2.number(), -2, -1); vote(user, changeId, patchSet2.number(), 2, 1); - // Delete the votes on the first patch set. + // Delete the votes on the first patch set and verify the change messages. vote(admin, changeId, patchSet1.number(), 0, 0); + assertLastChangeMessage(r.getChangeId(), "Patch Set 1: -Code-Review -Verified"); vote(user, changeId, patchSet1.number(), 0, 0); + assertLastChangeMessage(r.getChangeId(), "Patch Set 1: -Code-Review -Verified"); // Verify that the vote deletions have not been copied to the current patch set. ChangeInfo c = detailedChange(changeId); @@ -551,9 +642,11 @@ assertCurrentVotes(c, admin, 0, 0); assertCurrentVotes(c, user, 0, 0); - // Delete the votes on the first patch set. + // Delete the votes on the first patch set and verify the change messages. vote(admin, changeId, patchSet1.number(), 0, 0); + assertLastChangeMessage(r.getChangeId(), "Patch Set 1: -Code-Review -Verified"); vote(user, changeId, patchSet1.number(), 0, 0); + assertLastChangeMessage(r.getChangeId(), "Patch Set 1: -Code-Review -Verified"); // Verify that there are still no votes on the current patch set. c = detailedChange(changeId); @@ -592,9 +685,11 @@ vote(admin, changeId, patchSet2.number(), 2, 1); vote(user, changeId, patchSet2.number(), -2, -1); - // Delete the votes on the first patch set. + // Delete the votes on the first patch set and verify the change messages. vote(admin, changeId, patchSet1.number(), 0, 0); + assertLastChangeMessage(r.getChangeId(), "Patch Set 1: -Code-Review -Verified"); vote(user, changeId, patchSet1.number(), 0, 0); + assertLastChangeMessage(r.getChangeId(), "Patch Set 1: -Code-Review -Verified"); // Verify that the vote deletions have not been copied to the current patch set (since a current // vote already exists). @@ -639,9 +734,11 @@ deleteCurrentVotes(admin, changeId); deleteCurrentVotes(user, changeId); - // Delete the votes on the first patch set. + // Delete the votes on the first patch set and verify the change messages. vote(admin, changeId, patchSet1.number(), 0, 0); + assertLastChangeMessage(r.getChangeId(), "Patch Set 1: -Code-Review -Verified"); vote(user, changeId, patchSet1.number(), 0, 0); + assertLastChangeMessage(r.getChangeId(), "Patch Set 1: -Code-Review -Verified"); // Verify that there are still no votes on the current patch set. ChangeInfo c = detailedChange(changeId); @@ -682,9 +779,29 @@ assertCurrentVotes(c, admin, -2, -1); assertCurrentVotes(c, user, 2, 1); - // Delete the votes on the first patch set. + // Delete the votes on the first patch set and verify the change messages. vote(admin, changeId, patchSet1.number(), 0, 0); + assertLastChangeMessage( + r.getChangeId(), + String.format( + "Patch Set 1: -Code-Review -Verified\n\n" + + "Copied votes on follow-up patch sets have been updated:\n" + + "* Copied Code-Review vote has been removed from patch set 2 (was Code-Review-2)" + + " since the new Code-Review=0 vote is not copyable" + + " (copy condition: \"is:ANY\").\n" + + "* Copied Verified vote has been removed from patch set 2 (was Verified-1)" + + " since the new Verified=0 vote is not copyable (copy condition: \"is:ANY\").")); vote(user, changeId, patchSet1.number(), 0, 0); + assertLastChangeMessage( + r.getChangeId(), + String.format( + "Patch Set 1: -Code-Review -Verified\n\n" + + "Copied votes on follow-up patch sets have been updated:\n" + + "* Copied Code-Review vote has been removed from patch set 2 (was Code-Review+2)" + + " since the new Code-Review=0 vote is not copyable" + + " (copy condition: \"is:ANY\").\n" + + "* Copied Verified vote has been removed from patch set 2 (was Verified+1)" + + " since the new Verified=0 vote is not copyable (copy condition: \"is:ANY\").")); // Verify that the vote deletions have been copied to the current patch set. c = detailedChange(changeId); @@ -720,9 +837,27 @@ r.assertOkStatus(); PatchSet patchSet4 = r.getChange().currentPatchSet(); - // Vote on the first patch set. + // Vote on the first patch set and verify the change messages. vote(admin, changeId, patchSet1.number(), 2, 1); + assertLastChangeMessage( + r.getChangeId(), + String.format( + "Patch Set 1: Code-Review+2 Verified+1\n\n" + + "Copied votes on follow-up patch sets have been updated:\n" + + "* Code-Review+2 has been copied to patch set 2, 3, 4" + + " (copy condition: \"is:ANY\").\n" + + "* Verified+1 has been copied to patch set 2, 3, 4" + + " (copy condition: \"is:ANY\").")); vote(user, changeId, patchSet1.number(), -2, -1); + assertLastChangeMessage( + r.getChangeId(), + String.format( + "Patch Set 1: Code-Review-2 Verified-1\n\n" + + "Copied votes on follow-up patch sets have been updated:\n" + + "* Code-Review-2 has been copied to patch set 2, 3, 4" + + " (copy condition: \"is:ANY\").\n" + + "* Verified-1 has been copied to patch set 2, 3, 4" + + " (copy condition: \"is:ANY\").")); // Verify that votes have been copied to the current patch set. ChangeInfo c = detailedChange(changeId); @@ -777,9 +912,25 @@ assertCurrentVotes(c, admin, 0, -1); assertCurrentVotes(c, user, 0, -1); - // Vote on the first patch set with copyable votes. + // Vote on the first patch set with copyable votes and verify the change messages. vote(admin, changeId, patchSet1.number(), 2, 1); + assertLastChangeMessage( + r.getChangeId(), + String.format( + "Patch Set 1: Code-Review+2 Verified+1\n\n" + + "Copied votes on follow-up patch sets have been updated:\n" + + "* Code-Review+2 has been copied to patch set 2 " + + "(copy condition: \"is:1 OR is:2\").\n" + + "* Verified+1 has been copied to patch set 2 (copy condition: \"is:ANY\").")); vote(user, changeId, patchSet1.number(), 1, 1); + assertLastChangeMessage( + r.getChangeId(), + String.format( + "Patch Set 1: Code-Review+1 Verified+1\n\n" + + "Copied votes on follow-up patch sets have been updated:\n" + + "* Code-Review+1 has been copied to patch set 2" + + " (copy condition: \"is:1 OR is:2\").\n" + + "* Verified+1 has been copied to patch set 2 (copy condition: \"is:ANY\").")); // Verify that votes have been not copied to the current patch set. c = detailedChange(changeId); @@ -832,9 +983,33 @@ assertCurrentVotes(c, admin, 2, 1); assertCurrentVotes(c, user, -2, -1); - // Delete the votes on the first patch set. + // Delete the votes on the first patch set and verify the change messages. vote(admin, changeId, patchSet1.number(), 0, 0); + assertLastChangeMessage( + r.getChangeId(), + String.format( + "Patch Set 1: -Code-Review -Verified\n\n" + + "Copied votes on follow-up patch sets have been updated:\n" + + "* Copied Code-Review vote has been removed from patch set" + + " 2 (was Code-Review+2), 3 (was Code-Review+2), 4 (was Code-Review+2)" + + " since the new Code-Review=0 vote is not copyable" + + " (copy condition: \"is:ANY\").\n" + + "* Copied Verified vote has been removed from patch set" + + " 2 (was Verified+1), 3 (was Verified+1), 4 (was Verified+1)" + + " since the new Verified=0 vote is not copyable (copy condition: \"is:ANY\").")); vote(user, changeId, patchSet1.number(), 0, 0); + assertLastChangeMessage( + r.getChangeId(), + String.format( + "Patch Set 1: -Code-Review -Verified\n\n" + + "Copied votes on follow-up patch sets have been updated:\n" + + "* Copied Code-Review vote has been removed from patch set" + + " 2 (was Code-Review-2), 3 (was Code-Review-2), 4 (was Code-Review-2)" + + " since the new Code-Review=0 vote is not copyable" + + " (copy condition: \"is:ANY\").\n" + + "* Copied Verified vote has been removed from patch set" + + " 2 (was Verified-1), 3 (was Verified-1), 4 (was Verified-1)" + + " since the new Verified=0 vote is not copyable (copy condition: \"is:ANY\").")); // Verify that the votes has been copied to the current patch set. c = detailedChange(changeId); @@ -894,7 +1069,27 @@ // Delete the votes on the first patch set. vote(admin, changeId, patchSet1.number(), 0, 0); + assertLastChangeMessage( + r.getChangeId(), + String.format( + "Patch Set 1: -Code-Review -Verified\n\n" + + "Copied votes on follow-up patch sets have been updated:\n" + + "* Copied Code-Review vote has been removed from patch set 2 (was Code-Review+2)" + + " since the new Code-Review=0 vote is not copyable" + + " (copy condition: \"is:1 OR is:2\").\n" + + "* Copied Verified vote has been removed from patch set 2 (was Verified+1)" + + " since the new Verified=0 vote is not copyable (copy condition: \"is:ANY\").")); vote(user, changeId, patchSet1.number(), 0, 0); + assertLastChangeMessage( + r.getChangeId(), + String.format( + "Patch Set 1: -Code-Review -Verified\n\n" + + "Copied votes on follow-up patch sets have been updated:\n" + + "* Copied Code-Review vote has been removed from patch set 2 (was Code-Review+1)" + + " since the new Code-Review=0 vote is not copyable" + + " (copy condition: \"is:1 OR is:2\").\n" + + "* Copied Verified vote has been removed from patch set 2 (was Verified-1)" + + " since the new Verified=0 vote is not copyable (copy condition: \"is:ANY\").")); // Verify that the vote deletions have been not copied to the current patch set. c = detailedChange(changeId); @@ -936,9 +1131,23 @@ r.assertOkStatus(); PatchSet patchSet4 = r.getChange().currentPatchSet(); - // Vote on the third patch set. + // Vote on the third patch set and verify the change messages. vote(admin, changeId, patchSet3.number(), 2, 1); + assertLastChangeMessage( + r.getChangeId(), + String.format( + "Patch Set 3: Code-Review+2 Verified+1\n\n" + + "Copied votes on follow-up patch sets have been updated:\n" + + "* Code-Review+2 has been copied to patch set 4 (copy condition: \"is:ANY\").\n" + + "* Verified+1 has been copied to patch set 4 (copy condition: \"is:ANY\").")); vote(user, changeId, patchSet3.number(), -2, -1); + assertLastChangeMessage( + r.getChangeId(), + String.format( + "Patch Set 3: Code-Review-2 Verified-1\n\n" + + "Copied votes on follow-up patch sets have been updated:\n" + + "* Code-Review-2 has been copied to patch set 4 (copy condition: \"is:ANY\").\n" + + "* Verified-1 has been copied to patch set 4 (copy condition: \"is:ANY\").")); // Verify that votes have been copied to the current patch set. ChangeInfo c = detailedChange(changeId); @@ -1057,4 +1266,10 @@ assertThat(patchSetApproval.get().value()).isEqualTo((short) expectedVote); assertThat(patchSetApproval.get().copied()).isEqualTo(expectedToBeCopied); } + + private void assertLastChangeMessage(String changeId, String expectedMessage) + throws RestApiException { + assertThat(Iterables.getLast(gApi.changes().id(changeId).get().messages).message) + .isEqualTo(expectedMessage); + } }