Merge "Highlight matching atoms of copy conditions in change messages"
diff --git a/java/com/google/gerrit/server/approval/ApprovalCopier.java b/java/com/google/gerrit/server/approval/ApprovalCopier.java
index 059445e..cd2d79e 100644
--- a/java/com/google/gerrit/server/approval/ApprovalCopier.java
+++ b/java/com/google/gerrit/server/approval/ApprovalCopier.java
@@ -32,6 +32,7 @@
import com.google.gerrit.entities.Project;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.client.ChangeKind;
+import com.google.gerrit.index.query.Predicate;
import com.google.gerrit.index.query.QueryParseException;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.change.ChangeKindCache;
@@ -85,7 +86,7 @@
* <li>the approval is not overridden by a current approval on the patch set
* </ul>
*/
- public abstract ImmutableSet<PatchSetApproval> copiedApprovals();
+ public abstract ImmutableSet<PatchSetApprovalData> copiedApprovals();
/**
* Approvals on the previous patch set that have not been copied to the patch set.
@@ -96,7 +97,7 @@
* <p>Only returns non-copied approvals of the previous patch set. Approvals from earlier patch
* sets that were outdated before are not included.
*/
- public abstract ImmutableSet<PatchSetApproval> outdatedApprovals();
+ public abstract ImmutableSet<PatchSetApprovalData> outdatedApprovals();
static Result empty() {
return create(
@@ -105,10 +106,68 @@
@VisibleForTesting
public static Result create(
- ImmutableSet<PatchSetApproval> copiedApprovals,
- ImmutableSet<PatchSetApproval> outdatedApprovals) {
+ ImmutableSet<PatchSetApprovalData> copiedApprovals,
+ ImmutableSet<PatchSetApprovalData> outdatedApprovals) {
return new AutoValue_ApprovalCopier_Result(copiedApprovals, outdatedApprovals);
}
+
+ /**
+ * A {@link PatchSetApproval} with information about which atoms of the copy condition are
+ * passing/failing.
+ */
+ @AutoValue
+ public abstract static class PatchSetApprovalData {
+ /** The approval. */
+ public abstract PatchSetApproval patchSetApproval();
+
+ /**
+ * Lists the leaf predicates of the copy condition that are fulfilled.
+ *
+ * <p>Example: The expression
+ *
+ * <pre>
+ * changekind:TRIVIAL_REBASE OR is:MIN
+ * </pre>
+ *
+ * has two leaf predicates:
+ *
+ * <ul>
+ * <li>changekind:TRIVIAL_REBASE
+ * <li>is:MIN
+ * </ul>
+ *
+ * This method will return the leaf predicates that are fulfilled, for example if only the
+ * first predicate is fulfilled, the returned list will be equal to
+ * ["changekind:TRIVIAL_REBASE"].
+ *
+ * <p>Empty if the label type is missing, if there is no copy condition or if the copy
+ * condition is not parseable.
+ */
+ public abstract ImmutableSet<String> passingAtoms();
+
+ /**
+ * Lists the leaf predicates of the copy condition that are not fulfilled. See {@link
+ * #passingAtoms()} for more details.
+ *
+ * <p>Empty if the label type is missing, if there is no copy condition or if the copy
+ * condition is not parseable.
+ */
+ public abstract ImmutableSet<String> failingAtoms();
+
+ @VisibleForTesting
+ public static PatchSetApprovalData create(
+ PatchSetApproval approval,
+ ImmutableSet<String> passingAtoms,
+ ImmutableSet<String> failingAtoms) {
+ return new AutoValue_ApprovalCopier_Result_PatchSetApprovalData(
+ approval, passingAtoms, failingAtoms);
+ }
+
+ private static PatchSetApprovalData createForMissingLabelType(PatchSetApproval approval) {
+ return new AutoValue_ApprovalCopier_Result_PatchSetApprovalData(
+ approval, ImmutableSet.of(), ImmutableSet.of());
+ }
+ }
}
private final GitRepositoryManager repoManager;
@@ -227,17 +286,18 @@
followUpPatchSet.commitId());
boolean isMerge = isMerge(changeNotes.getProjectName(), revWalk, followUpPatchSet);
- if (canCopy(
- changeNotes,
- priorPatchSet.id(),
- followUpPatchSet,
- approverId,
- labelType.get(),
- approvalValue,
- changeKind,
- isMerge,
- revWalk,
- repo.getConfig())) {
+ if (computeCopyResult(
+ changeNotes,
+ priorPatchSet.id(),
+ followUpPatchSet,
+ approverId,
+ labelType.get(),
+ approvalValue,
+ changeKind,
+ isMerge,
+ revWalk,
+ repo.getConfig())
+ .canCopy()) {
targetPatchSetsBuilder.add(followUpPatchSetId);
} else {
// The approval is not copyable to this follow-up patch set.
@@ -251,7 +311,14 @@
return targetPatchSetsBuilder.build();
}
- private boolean canCopy(
+ /**
+ * Checks whether a given approval can be copied from the given source patch set to the given
+ * target patch set.
+ *
+ * <p>The returned result also informs about which atoms of the copy condition are
+ * passing/failing.
+ */
+ private ApprovalCopyResult computeCopyResult(
ChangeNotes changeNotes,
PatchSet.Id sourcePatchSetId,
PatchSet targetPatchSet,
@@ -263,7 +330,7 @@
RevWalk revWalk,
Config repoConfig) {
if (!labelType.getCopyCondition().isPresent()) {
- return false;
+ return ApprovalCopyResult.createForMissingCopyCondition();
}
ApprovalContext ctx =
ApprovalContext.create(
@@ -283,15 +350,18 @@
// request (e.g. a group used in this query might not be visible to the person sending this
// request).
try (ManualRequestContext ignored = requestContext.open()) {
- return approvalQueryBuilder
- .parse(labelType.getCopyCondition().get())
- .asMatchable()
- .match(ctx);
+ Predicate<ApprovalContext> copyConditionPredicate =
+ approvalQueryBuilder.parse(labelType.getCopyCondition().get());
+ boolean canCopy = copyConditionPredicate.asMatchable().match(ctx);
+ ImmutableSet.Builder<String> passingAtoms = ImmutableSet.builder();
+ ImmutableSet.Builder<String> failingAtoms = ImmutableSet.builder();
+ evaluateAtoms(copyConditionPredicate, ctx, passingAtoms, failingAtoms);
+ return ApprovalCopyResult.create(canCopy, passingAtoms.build(), failingAtoms.build());
}
} catch (QueryParseException e) {
logger.atWarning().withCause(e).log(
"Unable to copy label because config is invalid. This should have been caught before.");
- return false;
+ return ApprovalCopyResult.createForNonParseableCopyCondition();
}
}
@@ -321,8 +391,10 @@
nonCopiedApprovalsForGivenPatchSet.forEach(
psa -> currentApprovalsByUser.put(psa.label(), psa.accountId(), psa));
- Table<String, Account.Id, PatchSetApproval> copiedApprovalsByUser = HashBasedTable.create();
- ImmutableSet.Builder<PatchSetApproval> outdatedApprovalsBuilder = ImmutableSet.builder();
+ Table<String, Account.Id, Result.PatchSetApprovalData> copiedApprovalsByUser =
+ HashBasedTable.create();
+ ImmutableSet.Builder<Result.PatchSetApprovalData> outdatedApprovalsBuilder =
+ ImmutableSet.builder();
ImmutableList<PatchSetApproval> priorApprovals =
notes.load().getApprovals().all().get(priorPatchSet.getKey());
@@ -362,35 +434,55 @@
priorPsa.key().patchSetId().changeId().get(),
targetPsId.get(),
projectName);
- outdatedApprovalsBuilder.add(priorPsa);
+ outdatedApprovalsBuilder.add(
+ Result.PatchSetApprovalData.createForMissingLabelType(priorPsa));
continue;
}
- if (canCopy(
- notes,
- priorPsa.patchSetId(),
- targetPatchSet,
- priorPsa.accountId(),
- labelType.get(),
- priorPsa.value(),
- changeKind,
- isMerge,
- rw,
- repoConfig)) {
+ ApprovalCopyResult approvalCopyResult =
+ computeCopyResult(
+ notes,
+ priorPsa.patchSetId(),
+ targetPatchSet,
+ priorPsa.accountId(),
+ labelType.get(),
+ priorPsa.value(),
+ changeKind,
+ isMerge,
+ rw,
+ repoConfig);
+ if (approvalCopyResult.canCopy()) {
if (!currentApprovalsByUser.contains(priorPsa.label(), priorPsa.accountId())) {
+ PatchSetApproval copiedApproval = priorPsa.copyWithPatchSet(targetPatchSet.id());
+
+ // Normalize the copied approval.
+ Optional<PatchSetApproval> copiedApprovalNormalized =
+ labelNormalizer.normalize(notes, copiedApproval);
+ logger.atFine().log(
+ "Copied approval %s has been normalized to %s",
+ copiedApproval,
+ copiedApprovalNormalized.map(PatchSetApproval::toString).orElse("n/a"));
+ if (!copiedApprovalNormalized.isPresent()) {
+ continue;
+ }
+
copiedApprovalsByUser.put(
priorPsa.label(),
priorPsa.accountId(),
- priorPsa.copyWithPatchSet(targetPatchSet.id()));
+ Result.PatchSetApprovalData.create(
+ copiedApprovalNormalized.get(),
+ approvalCopyResult.passingAtoms(),
+ approvalCopyResult.failingAtoms()));
}
} else {
- outdatedApprovalsBuilder.add(priorPsa);
+ outdatedApprovalsBuilder.add(
+ Result.PatchSetApprovalData.create(
+ priorPsa, approvalCopyResult.passingAtoms(), approvalCopyResult.failingAtoms()));
continue;
}
}
- ImmutableSet<PatchSetApproval> copiedApprovals =
- labelNormalizer.normalize(notes, copiedApprovalsByUser.values()).getNormalized();
- return Result.create(copiedApprovals, outdatedApprovalsBuilder.build());
+ return Result.create(
+ ImmutableSet.copyOf(copiedApprovalsByUser.values()), outdatedApprovalsBuilder.build());
}
private boolean isMerge(Project.NameKey project, RevWalk rw, PatchSet patchSet) {
@@ -404,4 +496,72 @@
e);
}
}
+
+ /**
+ * Evaluates a predicate of the copy condition and adds its passing and failing atoms to the given
+ * builders.
+ *
+ * @param predicate a predicate of the copy condition that should be evaluated
+ * @param approvalContext the approval context against which the predicate should be evaluated
+ * @param passingAtoms a builder to which passing atoms should be added
+ * @param failingAtoms a builder to which failing atoms should be added
+ */
+ private static void evaluateAtoms(
+ Predicate<ApprovalContext> predicate,
+ ApprovalContext approvalContext,
+ ImmutableSet.Builder<String> passingAtoms,
+ ImmutableSet.Builder<String> failingAtoms) {
+ if (predicate.isLeaf()) {
+ boolean isPassing = predicate.asMatchable().match(approvalContext);
+ (isPassing ? passingAtoms : failingAtoms).add(predicate.getPredicateString());
+ return;
+ }
+ predicate
+ .getChildren()
+ .forEach(
+ childPredicate ->
+ evaluateAtoms(childPredicate, approvalContext, passingAtoms, failingAtoms));
+ }
+
+ /** Result for checking if an approval can be copied to the next patch set. */
+ @AutoValue
+ abstract static class ApprovalCopyResult {
+ /** Whether the approval can be copied to the next patch set. */
+ abstract boolean canCopy();
+
+ /**
+ * Lists the leaf predicates of the copy condition that are fulfilled. See {@link
+ * Result.PatchSetApprovalData#passingAtoms()} for more details.
+ *
+ * <p>Empty if there is no copy condition or if the copy condition is not parseable.
+ */
+ abstract ImmutableSet<String> passingAtoms();
+
+ /**
+ * Lists the leaf predicates of the copy condition that are not fulfilled. See {@link
+ * Result.PatchSetApprovalData#passingAtoms()} for more details.
+ *
+ * <p>Empty if there is no copy condition or if the copy condition is not parseable.
+ */
+ abstract ImmutableSet<String> failingAtoms();
+
+ private static ApprovalCopyResult create(
+ boolean canCopy, ImmutableSet<String> passingAtoms, ImmutableSet<String> failingAtoms) {
+ return new AutoValue_ApprovalCopier_ApprovalCopyResult(canCopy, passingAtoms, failingAtoms);
+ }
+
+ private static ApprovalCopyResult createForMissingCopyCondition() {
+ return new AutoValue_ApprovalCopier_ApprovalCopyResult(
+ /* canCopy= */ false,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of());
+ }
+
+ private static ApprovalCopyResult createForNonParseableCopyCondition() {
+ return new AutoValue_ApprovalCopier_ApprovalCopyResult(
+ /* canCopy= */ false,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of());
+ }
+ }
}
diff --git a/java/com/google/gerrit/server/approval/ApprovalsUtil.java b/java/com/google/gerrit/server/approval/ApprovalsUtil.java
index bd31356f..09820b1 100644
--- a/java/com/google/gerrit/server/approval/ApprovalsUtil.java
+++ b/java/com/google/gerrit/server/approval/ApprovalsUtil.java
@@ -15,8 +15,10 @@
package com.google.gerrit.server.approval;
import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkState;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.collect.ImmutableListMultimap.toImmutableListMultimap;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.CC;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
import static com.google.gerrit.server.project.ProjectCache.illegalState;
@@ -25,6 +27,7 @@
import static java.util.stream.Collectors.joining;
import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Function;
import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
@@ -85,6 +88,7 @@
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
+import java.util.StringTokenizer;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.revwalk.RevWalk;
@@ -399,12 +403,17 @@
ChangeUpdate changeUpdate) {
ApprovalCopier.Result approvalCopierResult =
approvalCopier.forPatchSet(notes, patchSet, revWalk, repoConfig);
- approvalCopierResult.copiedApprovals().forEach(a -> changeUpdate.putCopiedApproval(a));
+ approvalCopierResult
+ .copiedApprovals()
+ .forEach(approvalData -> changeUpdate.putCopiedApproval(approvalData.patchSetApproval()));
if (!notes.getChange().isWorkInProgress()) {
// The attention set should not be updated when the change is work-in-progress.
addAttentionSetUpdatesForOutdatedApprovals(
- changeUpdate, approvalCopierResult.outdatedApprovals());
+ changeUpdate,
+ approvalCopierResult.outdatedApprovals().stream()
+ .map(ApprovalCopier.Result.PatchSetApprovalData::patchSetApproval)
+ .collect(toImmutableSet()));
}
return approvalCopierResult;
@@ -511,31 +520,40 @@
* "is:FOO")}
* </ul>
*
- * @param approvals the approvals that should be formatted
+ * @param approvalDatas the approvals that should be formatted, with approval meta data
* @param labelTypes the label types
* @return bullet list with the formatted approvals
*/
private String formatApprovalListWithCopyCondition(
- ImmutableSet<PatchSetApproval> approvals, LabelTypes labelTypes) {
+ ImmutableSet<ApprovalCopier.Result.PatchSetApprovalData> approvalDatas,
+ LabelTypes labelTypes) {
StringBuilder message = new StringBuilder();
// sort approvals by label vote so that we list them in a deterministic order
- ImmutableList<PatchSetApproval> approvalsSortedByLabelVote =
- approvals.stream()
- .sorted(comparing(psa -> LabelVote.create(psa.label(), psa.value()).format()))
+ ImmutableList<ApprovalCopier.Result.PatchSetApprovalData> approvalsSortedByLabelVote =
+ approvalDatas.stream()
+ .sorted(
+ comparing(
+ approvalData ->
+ LabelVote.create(
+ approvalData.patchSetApproval().label(),
+ approvalData.patchSetApproval().value())
+ .format()))
.collect(toImmutableList());
- ImmutableListMultimap<String, PatchSetApproval> approvalsByLabel =
- Multimaps.index(approvalsSortedByLabelVote, PatchSetApproval::label);
+ ImmutableListMultimap<String, ApprovalCopier.Result.PatchSetApprovalData> approvalsByLabel =
+ Multimaps.index(
+ approvalsSortedByLabelVote, approvalData -> approvalData.patchSetApproval().label());
- for (Map.Entry<String, Collection<PatchSetApproval>> approvalsByLabelEntry :
- approvalsByLabel.asMap().entrySet()) {
+ for (Map.Entry<String, Collection<ApprovalCopier.Result.PatchSetApprovalData>>
+ approvalsByLabelEntry : approvalsByLabel.asMap().entrySet()) {
String label = approvalsByLabelEntry.getKey();
- Collection<PatchSetApproval> approvalsForSameLabel = approvalsByLabelEntry.getValue();
+ Collection<ApprovalCopier.Result.PatchSetApprovalData> approvalsForSameLabel =
+ approvalsByLabelEntry.getValue();
- message.append("* ");
if (!labelTypes.byLabel(label).isPresent()) {
message
+ .append("* ")
.append(formatApprovalsAsLabelVotesList(approvalsForSameLabel))
.append(" (label type is missing)\n");
continue;
@@ -543,22 +561,65 @@
LabelType labelType = labelTypes.byLabel(label).get();
if (!labelType.getCopyCondition().isPresent()) {
- message.append(formatApprovalsAsLabelVotesList(approvalsForSameLabel)).append("\n");
+ message
+ .append("* ")
+ .append(formatApprovalsAsLabelVotesList(approvalsForSameLabel))
+ .append("\n");
continue;
}
- message
- .append(
- formatApprovalsWithCopyCondition(
- approvalsForSameLabel, labelType.getCopyCondition().get()))
- .append("\n");
+ // Group the approvals that have the same label by the passing atoms. If approvals have the
+ // same label, but have different passing atoms, we need to list them in separate lines
+ // (because in each line we will highlight different passing atoms that matched). Approvals
+ // with the same label and the same passing atoms are formatted as a single line.
+ ImmutableListMultimap<ImmutableSet<String>, ApprovalCopier.Result.PatchSetApprovalData>
+ approvalsForSameLabelByPassingAndFailingAtoms =
+ Multimaps.index(
+ approvalsForSameLabel, ApprovalCopier.Result.PatchSetApprovalData::passingAtoms);
+
+ // Approvals with the same label that have the same passing atoms should have the same failing
+ // atoms (since the label is the same they have the same copy condition).
+ approvalsForSameLabelByPassingAndFailingAtoms
+ .asMap()
+ .values()
+ .forEach(
+ approvalsForSameLabelAndSamePassingAtoms ->
+ checkThatPropertyIsTheSameForAllApprovals(
+ approvalsForSameLabelAndSamePassingAtoms,
+ "failing atoms",
+ approvalData -> approvalData.failingAtoms()));
+
+ // The order in which we add lines for approvals with the same label but different passing
+ // atoms needs to be deterministic for tests. Just sort them by the string representation of
+ // the passing atoms.
+ for (Collection<ApprovalCopier.Result.PatchSetApprovalData>
+ approvalsForSameLabelWithSamePassingAndFailingAtoms :
+ approvalsForSameLabelByPassingAndFailingAtoms.asMap().entrySet().stream()
+ .sorted(
+ comparing(
+ (Map.Entry<
+ ImmutableSet<String>,
+ Collection<ApprovalCopier.Result.PatchSetApprovalData>>
+ e) -> e.getKey().toString()))
+ .map(Map.Entry::getValue)
+ .collect(toImmutableList())) {
+ message
+ .append("* ")
+ .append(
+ formatApprovalsWithCopyCondition(
+ approvalsForSameLabelWithSamePassingAndFailingAtoms,
+ labelType.getCopyCondition().get()))
+ .append("\n");
+ }
}
return message.toString();
}
/**
- * Formats the given approvals of the same label with the given copy condition.
+ * Formats the given approvals with the given copy condition.
+ *
+ * <p>The given approvals must have the same label and the same passing and failing atoms.
*
* <p>E.g.: {Code-Review+1, Code-Review+2 (copy condition: "is:MIN")}
*
@@ -578,12 +639,29 @@
* "is:FOO")}
* </ul>
*
- * @param approvalsForSameLabel the approvals that should be formatted, must be for the same label
+ * @param approvalsWithSameLabelAndSamePassingAndFailingAtoms the approvals that should be
+ * formatted, must be for the same label
* @param copyCondition the copy condition of the label
* @return the formatted approvals
*/
private String formatApprovalsWithCopyCondition(
- Collection<PatchSetApproval> approvalsForSameLabel, String copyCondition) {
+ Collection<ApprovalCopier.Result.PatchSetApprovalData>
+ approvalsWithSameLabelAndSamePassingAndFailingAtoms,
+ String copyCondition) {
+ // Check that all given approvals have the same label and the same passing and failing atoms.
+ checkThatPropertyIsTheSameForAllApprovals(
+ approvalsWithSameLabelAndSamePassingAndFailingAtoms,
+ "label",
+ approvalData -> approvalData.patchSetApproval().label());
+ checkThatPropertyIsTheSameForAllApprovals(
+ approvalsWithSameLabelAndSamePassingAndFailingAtoms,
+ "passing atoms",
+ approvalData -> approvalData.passingAtoms());
+ checkThatPropertyIsTheSameForAllApprovals(
+ approvalsWithSameLabelAndSamePassingAndFailingAtoms,
+ "failing atoms",
+ approvalData -> approvalData.failingAtoms());
+
StringBuilder message = new StringBuilder();
boolean containsUserInPredicate;
@@ -591,7 +669,8 @@
containsUserInPredicate = containsUserInPredicate(copyCondition);
} catch (QueryParseException e) {
logger.atWarning().withCause(e).log("Non-parsable query condition");
- message.append(formatApprovalsAsLabelVotesList(approvalsForSameLabel));
+ message.append(
+ formatApprovalsAsLabelVotesList(approvalsWithSameLabelAndSamePassingAndFailingAtoms));
message.append(String.format(" (non-parseable copy condition: \"%s\")", copyCondition));
return message.toString();
}
@@ -618,26 +697,35 @@
// sort the approvals by their approvers name-email so that the approvers always appear in a
// deterministic order
- ImmutableList<PatchSetApproval> approvalsSortedByLabelVoteAndApprover =
- approvalsForSameLabel.stream()
- .sorted(
- comparing(
- (PatchSetApproval psa) ->
- LabelVote.create(psa.label(), psa.value()).format())
- .thenComparing(
- psa ->
- accountCache
- .getEvenIfMissing(psa.accountId())
- .account()
- .getNameEmail(anonymousCowardName)))
- .collect(toImmutableList());
+ ImmutableList<ApprovalCopier.Result.PatchSetApprovalData>
+ approvalsSortedByLabelVoteAndApprover =
+ approvalsWithSameLabelAndSamePassingAndFailingAtoms.stream()
+ .sorted(
+ comparing(
+ (ApprovalCopier.Result.PatchSetApprovalData approvalData) ->
+ LabelVote.create(
+ approvalData.patchSetApproval().label(),
+ approvalData.patchSetApproval().value())
+ .format())
+ .thenComparing(
+ approvalData ->
+ accountCache
+ .getEvenIfMissing(approvalData.patchSetApproval().accountId())
+ .account()
+ .getNameEmail(anonymousCowardName)))
+ .collect(toImmutableList());
ImmutableListMultimap<LabelVote, Account.Id> approversByLabelVote =
Multimaps.index(
approvalsSortedByLabelVoteAndApprover,
- psa -> LabelVote.create(psa.label(), psa.value()))
+ approvalData ->
+ LabelVote.create(
+ approvalData.patchSetApproval().label(),
+ approvalData.patchSetApproval().value()))
.entries().stream()
- .collect(toImmutableListMultimap(e -> e.getKey(), e -> e.getValue().accountId()));
+ .collect(
+ toImmutableListMultimap(
+ e -> e.getKey(), e -> e.getValue().patchSetApproval().accountId()));
message.append(
approversByLabelVote.asMap().entrySet().stream()
.map(
@@ -647,12 +735,64 @@
.collect(joining(", ")));
} else {
// copy condition doesn't contain a UserInPredicate
- message.append(formatApprovalsAsLabelVotesList(approvalsForSameLabel));
+ message.append(
+ formatApprovalsAsLabelVotesList(approvalsWithSameLabelAndSamePassingAndFailingAtoms));
}
- message.append(String.format(" (copy condition: \"%s\")", copyCondition));
+ ImmutableSet<String> passingAtoms =
+ !approvalsWithSameLabelAndSamePassingAndFailingAtoms.isEmpty()
+ ? approvalsWithSameLabelAndSamePassingAndFailingAtoms.iterator().next().passingAtoms()
+ : ImmutableSet.of();
+ message.append(
+ String.format(
+ " (copy condition: \"%s\")",
+ formatCopyConditionAsMarkdown(copyCondition, passingAtoms)));
return message.toString();
}
+ /** Checks that all given approvals have the same value for a given property. */
+ private void checkThatPropertyIsTheSameForAllApprovals(
+ Collection<ApprovalCopier.Result.PatchSetApprovalData> approvals,
+ String propertyName,
+ Function<ApprovalCopier.Result.PatchSetApprovalData, ?> propertyExtractor) {
+ if (approvals.isEmpty()) {
+ return;
+ }
+
+ Object propertyOfFirstEntry = propertyExtractor.apply(approvals.iterator().next());
+ approvals.forEach(
+ approvalData ->
+ checkState(
+ propertyExtractor.apply(approvalData).equals(propertyOfFirstEntry),
+ "property %s of approval %s does not match, expected value: %s",
+ propertyName,
+ approvalData,
+ propertyOfFirstEntry));
+ }
+
+ /**
+ * Formats the given copy condition as a Markdown string.
+ *
+ * <p>Passing atoms are formatted as bold.
+ *
+ * @param copyCondition the copy condition that should be formatted
+ * @param passingAtoms atoms of the copy conditions which are passing/matching
+ * @return the formatted copy condition as a Markdown string
+ */
+ private String formatCopyConditionAsMarkdown(
+ String copyCondition, ImmutableSet<String> passingAtoms) {
+ StringBuilder formattedCopyCondition = new StringBuilder();
+ StringTokenizer tokenizer = new StringTokenizer(copyCondition, " ()", /* returnDelims= */ true);
+ while (tokenizer.hasMoreTokens()) {
+ String token = tokenizer.nextToken();
+ if (passingAtoms.contains(token)) {
+ formattedCopyCondition.append("**" + token.replace("*", "\\*") + "**");
+ } else {
+ formattedCopyCondition.append(token);
+ }
+ }
+ return formattedCopyCondition.toString();
+ }
+
private boolean containsUserInPredicate(String copyCondition) throws QueryParseException {
// Use a request context to run checks as an internal user with expanded visibility. This is
// so that the output of the copy condition does not depend on who is running the current
@@ -675,8 +815,9 @@
* @return the given approvals as a comma-separated list of label votes
*/
private String formatApprovalsAsLabelVotesList(
- Collection<PatchSetApproval> sortedApprovalsForSameLabel) {
+ Collection<ApprovalCopier.Result.PatchSetApprovalData> sortedApprovalsForSameLabel) {
return sortedApprovalsForSameLabel.stream()
+ .map(ApprovalCopier.Result.PatchSetApprovalData::patchSetApproval)
.map(psa -> LabelVote.create(psa.label(), psa.value()))
.distinct()
.map(LabelVote::format)
diff --git a/java/com/google/gerrit/server/change/LabelNormalizer.java b/java/com/google/gerrit/server/change/LabelNormalizer.java
index fb6b177..b1fcf48 100644
--- a/java/com/google/gerrit/server/change/LabelNormalizer.java
+++ b/java/com/google/gerrit/server/change/LabelNormalizer.java
@@ -21,6 +21,7 @@
import com.google.auto.value.AutoValue;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterables;
import com.google.common.collect.Streams;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.LabelType;
@@ -131,6 +132,20 @@
return Result.create(unchanged, updated, deleted);
}
+ /**
+ * Returns a copy of the given approval normalized to the defined ranges for the label type. If
+ * the approval is for an unknown label {@link Optional#empty()} is returned
+ *
+ * @param notes change notes containing the given approval
+ * @param approval approval that should be normalized
+ */
+ public Optional<PatchSetApproval> normalize(ChangeNotes notes, PatchSetApproval approval) {
+ Result result = normalize(notes, ImmutableSet.of(approval));
+ return Optional.ofNullable(
+ Iterables.getFirst(
+ result.unchanged(), Iterables.getFirst(result.updated(), /* defaultValue= */ null)));
+ }
+
private PatchSetApproval applyTypeFloor(LabelType lt, PatchSetApproval a) {
PatchSetApproval.Builder b = a.toBuilder();
LabelValue atMin = lt.getMin();
diff --git a/java/com/google/gerrit/server/change/PatchSetInserter.java b/java/com/google/gerrit/server/change/PatchSetInserter.java
index f7bec1c0..1fe67af 100644
--- a/java/com/google/gerrit/server/change/PatchSetInserter.java
+++ b/java/com/google/gerrit/server/change/PatchSetInserter.java
@@ -15,6 +15,7 @@
package com.google.gerrit.server.change;
import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.CC;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
import static com.google.gerrit.server.project.ProjectCache.illegalState;
@@ -368,7 +369,9 @@
ctx,
patchSet,
mailMessage,
- approvalCopierResult.outdatedApprovals(),
+ approvalCopierResult.outdatedApprovals().stream()
+ .map(ApprovalCopier.Result.PatchSetApprovalData::patchSetApproval)
+ .collect(toImmutableSet()),
oldReviewers.byState(REVIEWER),
oldReviewers.byState(CC),
changeKind,
diff --git a/java/com/google/gerrit/server/git/receive/ReplaceOp.java b/java/com/google/gerrit/server/git/receive/ReplaceOp.java
index 6e5cfff..0e17342 100644
--- a/java/com/google/gerrit/server/git/receive/ReplaceOp.java
+++ b/java/com/google/gerrit/server/git/receive/ReplaceOp.java
@@ -510,7 +510,9 @@
ctx,
newPatchSet,
mailMessage,
- approvalCopierResult.outdatedApprovals(),
+ approvalCopierResult.outdatedApprovals().stream()
+ .map(ApprovalCopier.Result.PatchSetApprovalData::patchSetApproval)
+ .collect(toImmutableSet()),
Streams.concat(
oldRecipients.getReviewers().stream(),
reviewerAdditions.flattenResults(ReviewerOp.Result::addedReviewers).stream()
@@ -595,6 +597,7 @@
return Optional.of(
"The following approvals got outdated and were removed:\n"
+ approvalCopierResult.outdatedApprovals().stream()
+ .map(ApprovalCopier.Result.PatchSetApprovalData::patchSetApproval)
.map(
outdatedApproval ->
String.format(
diff --git a/javatests/com/google/gerrit/acceptance/api/change/CopiedApprovalsInChangeMessageIT.java b/javatests/com/google/gerrit/acceptance/api/change/CopiedApprovalsInChangeMessageIT.java
index f8cf5fd..2b1bef0 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/CopiedApprovalsInChangeMessageIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/CopiedApprovalsInChangeMessageIT.java
@@ -45,12 +45,29 @@
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.server.approval.ApprovalCopier;
import com.google.gerrit.server.approval.ApprovalsUtil;
+import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.gerrit.server.project.testing.TestLabels;
import com.google.gerrit.server.util.AccountTemplateUtil;
import com.google.gerrit.server.util.time.TimeUtil;
import com.google.inject.Inject;
import org.junit.Test;
+/**
+ * Tests to verify that copied/outdated approvals are included into the change message that is
+ * posted on patch set creation. Includes verifying that the copied/outdated approvals in the change
+ * message are correctly formatted.
+ *
+ * <p>Some of the tests only verify the correct formatting of the copied/outdated approvals in the
+ * change message that is done by {@link
+ * ApprovalsUtil#formatApprovalCopierResult(com.google.gerrit.server.approval.ApprovalCopier.Result,
+ * LabelTypes)}. This method does the formatting based on the inputs that it gets, but it doesn't do
+ * any verification of these inputs. This means it's possible to provide inputs that are
+ * inconsistent with the approval copying logic in {@link ApprovalCopier}. E.g. it's possible to
+ * provide "is:MAX" as a passing atom for a "Code-Review-1" vote and have "is:MAX" highlighted as
+ * passing in the message although the "Code-Review-1" vote doesn't match with "is:MAX". For easier
+ * readability the formatting tests avoid using such inconsistent input data, but it's not
+ * impossible that in some cases we made a mistake and the input data is inconsistent.
+ */
public class CopiedApprovalsInChangeMessageIT extends AbstractDaemonTest {
@Inject private ApprovalsUtil approvalsUtil;
@Inject private ProjectOperations projectOperations;
@@ -98,7 +115,11 @@
PatchSetApproval patchSetApproval = createPatchSetApproval(admin, "Code-Review", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue("Copied Votes:\n* Code-Review+1 (label type is missing)\n");
@@ -111,7 +132,11 @@
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
/* copiedApprovals= */ ImmutableSet.of(),
- /* outdatedApprovals= */ ImmutableSet.of(patchSetApproval));
+ /* outdatedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())));
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue("Outdated Votes:\n* Code-Review+1 (label type is missing)\n");
}
@@ -125,7 +150,11 @@
PatchSetApproval patchSetApproval = createPatchSetApproval(admin, "Code-Review", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue("Copied Votes:\n* Code-Review+1\n");
@@ -141,7 +170,11 @@
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
/* copiedApprovals= */ ImmutableSet.of(),
- /* outdatedApprovals= */ ImmutableSet.of(patchSetApproval));
+ /* outdatedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())));
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue("Outdated Votes:\n* Code-Review+1\n");
}
@@ -153,13 +186,17 @@
ImmutableList.of(
createLabelType(
/* labelName= */ "Code-Review", /* copyCondition= */ "is:MIN OR is:MAX")));
- PatchSetApproval patchSetApproval = createPatchSetApproval(admin, "Code-Review", 1);
+ PatchSetApproval patchSetApproval = createPatchSetApproval(admin, "Code-Review", -2);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval,
+ /* passingAtoms= */ ImmutableSet.of("is:MIN"),
+ /* failingAtoms= */ ImmutableSet.of("is:MAX"))),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
- .hasValue("Copied Votes:\n* Code-Review+1 (copy condition: \"is:MIN OR is:MAX\")\n");
+ .hasValue("Copied Votes:\n* Code-Review-2 (copy condition: \"**is:MIN** OR is:MAX\")\n");
}
@Test
@@ -168,14 +205,21 @@
new LabelTypes(
ImmutableList.of(
createLabelType(
- /* labelName= */ "Code-Review", /* copyCondition= */ "is:MIN OR is:MAX")));
- PatchSetApproval patchSetApproval = createPatchSetApproval(admin, "Code-Review", 1);
+ /* labelName= */ "Code-Review",
+ /* copyCondition= */ "changekind:TRIVIAL_REBASE is:MAX")));
+ PatchSetApproval patchSetApproval = createPatchSetApproval(admin, "Code-Review", 2);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
/* copiedApprovals= */ ImmutableSet.of(),
- /* outdatedApprovals= */ ImmutableSet.of(patchSetApproval));
+ /* outdatedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval,
+ /* passingAtoms= */ ImmutableSet.of("is:MAX"),
+ /* failingAtoms= */ ImmutableSet.of("changekind:TRIVIAL_REBASE"))));
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
- .hasValue("Outdated Votes:\n* Code-Review+1 (copy condition: \"is:MIN OR is:MAX\")\n");
+ .hasValue(
+ "Outdated Votes:\n* Code-Review+2 (copy condition:"
+ + " \"changekind:TRIVIAL_REBASE **is:MAX**\")\n");
}
@Test
@@ -189,7 +233,11 @@
PatchSetApproval patchSetApproval = createPatchSetApproval(admin, "Code-Review", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue(
@@ -208,7 +256,11 @@
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
/* copiedApprovals= */ ImmutableSet.of(),
- /* outdatedApprovals= */ ImmutableSet.of(patchSetApproval));
+ /* outdatedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())));
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue(
"Outdated Votes:\n* Code-Review+1 (non-parseable copy condition: \"foo bar baz\")\n");
@@ -225,17 +277,22 @@
/* labelName= */ "Code-Review",
/* copyCondition= */ String.format(
"is:MIN OR (is:MAX approverin:%s)", groupUuid))));
- PatchSetApproval patchSetApproval = createPatchSetApproval(admin, "Code-Review", 1);
+ PatchSetApproval patchSetApproval = createPatchSetApproval(admin, "Code-Review", 2);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval,
+ /* passingAtoms= */ ImmutableSet.of(
+ "is:MAX", String.format("approverin:%s", groupUuid)),
+ /* failingAtoms= */ ImmutableSet.of("is:MIN"))),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue(
String.format(
"Copied Votes:\n"
- + "* Code-Review+1 by %s"
- + " (copy condition: \"is:MIN OR (is:MAX approverin:%s)\")\n",
+ + "* Code-Review+2 by %s"
+ + " (copy condition: \"is:MIN OR (**is:MAX** **approverin:%s**)\")\n",
AccountTemplateUtil.getAccountTemplate(admin.id()), groupUuid));
}
@@ -254,12 +311,17 @@
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
/* copiedApprovals= */ ImmutableSet.of(),
- /* outdatedApprovals= */ ImmutableSet.of(patchSetApproval));
+ /* outdatedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval,
+ /* passingAtoms= */ ImmutableSet.of(String.format("approverin:%s", groupUuid)),
+ /* failingAtoms= */ ImmutableSet.of("is:MIN", "is:MAX"))));
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue(
String.format(
"Outdated Votes:\n"
- + "* Code-Review+1 by %s (copy condition: \"is:MIN OR (is:MAX approverin:%s)\")\n",
+ + "* Code-Review+1 by %s (copy condition: \"is:MIN"
+ + " OR (is:MAX **approverin:%s**)\")\n",
AccountTemplateUtil.getAccountTemplate(admin.id()), groupUuid));
}
@@ -275,10 +337,15 @@
/* labelName= */ "Code-Review",
/* copyCondition= */ String.format(
"is:MIN OR (is:MAX approverin:%s)", groupUuid))));
- PatchSetApproval patchSetApproval = createPatchSetApproval(admin, "Code-Review", 1);
+ PatchSetApproval patchSetApproval = createPatchSetApproval(admin, "Code-Review", 2);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval,
+ /* passingAtoms= */ ImmutableSet.of(
+ "is:MAX", String.format("approverin:%s", groupUuid)),
+ /* failingAtoms= */ ImmutableSet.of("is:MIN"))),
/* outdatedApprovals= */ ImmutableSet.of());
// Set 'user' as the current user in the request scope.
@@ -291,8 +358,8 @@
.hasValue(
String.format(
"Copied Votes:\n"
- + "* Code-Review+1 by %s"
- + " (copy condition: \"is:MIN OR (is:MAX approverin:%s)\")\n",
+ + "* Code-Review+2 by %s"
+ + " (copy condition: \"is:MIN OR (**is:MAX** **approverin:%s**)\")\n",
AccountTemplateUtil.getAccountTemplate(admin.id()), groupUuid));
}
@@ -313,7 +380,11 @@
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
/* copiedApprovals= */ ImmutableSet.of(),
- /* outdatedApprovals= */ ImmutableSet.of(patchSetApproval));
+ /* outdatedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval,
+ /* passingAtoms= */ ImmutableSet.of(String.format("approverin:%s", groupUuid)),
+ /* failingAtoms= */ ImmutableSet.of("is:MIN", "is:MAX"))));
// Set 'user' as the current user in the request scope.
// 'user' cannot see the Administrators group that is used in the copy condition.
@@ -325,7 +396,8 @@
.hasValue(
String.format(
"Outdated Votes:\n"
- + "* Code-Review+1 by %s (copy condition: \"is:MIN OR (is:MAX approverin:%s)\")\n",
+ + "* Code-Review+1 by %s (copy condition: \"is:MIN"
+ + " OR (is:MAX **approverin:%s**)\")\n",
AccountTemplateUtil.getAccountTemplate(admin.id()), groupUuid));
}
@@ -344,7 +416,11 @@
PatchSetApproval patchSetApproval = createPatchSetApproval(admin, "Code-Review", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue(
@@ -371,7 +447,11 @@
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
/* copiedApprovals= */ ImmutableSet.of(),
- /* outdatedApprovals= */ ImmutableSet.of(patchSetApproval));
+ /* outdatedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())));
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue(
String.format(
@@ -388,7 +468,15 @@
PatchSetApproval patchSetApproval2 = createPatchSetApproval(user, "Code-Review", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval1, patchSetApproval2),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of()),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue("Copied Votes:\n* Code-Review+1 (label type is missing)\n");
@@ -401,7 +489,15 @@
PatchSetApproval patchSetApproval2 = createPatchSetApproval(user, "Verified", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval1, patchSetApproval2),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of()),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue(
@@ -417,7 +513,15 @@
PatchSetApproval patchSetApproval2 = createPatchSetApproval(user, "Code-Review", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval1, patchSetApproval2),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of()),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue("Copied Votes:\n* Code-Review+1, Code-Review+2 (label type is missing)\n");
@@ -433,7 +537,15 @@
PatchSetApproval patchSetApproval2 = createPatchSetApproval(user, "Code-Review", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval1, patchSetApproval2),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of()),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue("Copied Votes:\n* Code-Review+1\n");
@@ -450,7 +562,15 @@
PatchSetApproval patchSetApproval2 = createPatchSetApproval(user, "Verified", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval1, patchSetApproval2),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of()),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue("Copied Votes:\n* Code-Review+1\n* Verified+1\n");
@@ -466,7 +586,15 @@
PatchSetApproval patchSetApproval2 = createPatchSetApproval(user, "Code-Review", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval1, patchSetApproval2),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of()),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue("Copied Votes:\n* Code-Review+1, Code-Review+2\n");
@@ -480,14 +608,22 @@
ImmutableList.of(
createLabelType(
/* labelName= */ "Code-Review", /* copyCondition= */ "is:MIN OR is:MAX")));
- PatchSetApproval patchSetApproval1 = createPatchSetApproval(admin, "Code-Review", 1);
- PatchSetApproval patchSetApproval2 = createPatchSetApproval(user, "Code-Review", 1);
+ PatchSetApproval patchSetApproval1 = createPatchSetApproval(admin, "Code-Review", 2);
+ PatchSetApproval patchSetApproval2 = createPatchSetApproval(user, "Code-Review", 2);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval1, patchSetApproval2),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of("is:MAX"),
+ /* failingAtoms= */ ImmutableSet.of("is:MIN")),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of("is:MAX"),
+ /* failingAtoms= */ ImmutableSet.of("is:MIN"))),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
- .hasValue("Copied Votes:\n* Code-Review+1 (copy condition: \"is:MIN OR is:MAX\")\n");
+ .hasValue("Copied Votes:\n* Code-Review+2 (copy condition: \"is:MIN OR **is:MAX**\")\n");
}
@Test
@@ -498,39 +634,92 @@
ImmutableList.of(
createLabelType(
/* labelName= */ "Code-Review", /* copyCondition= */ "is:MIN OR is:MAX"),
- createLabelType(
- /* labelName= */ "Verified", /* copyCondition= */ "is:MIN OR is:MAX")));
- PatchSetApproval patchSetApproval1 = createPatchSetApproval(admin, "Code-Review", 1);
+ LabelType.builder(
+ "Verified",
+ ImmutableList.of(
+ LabelValue.create((short) -1, "Fails"),
+ LabelValue.create((short) 0, "No Vote"),
+ LabelValue.create((short) 1, "Succeeds")))
+ .setCopyCondition("is:MIN OR is:MAX")
+ .build()));
+ PatchSetApproval patchSetApproval1 = createPatchSetApproval(admin, "Code-Review", 2);
PatchSetApproval patchSetApproval2 = createPatchSetApproval(user, "Verified", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval1, patchSetApproval2),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of("is:MAX"),
+ /* failingAtoms= */ ImmutableSet.of("is:MIN")),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of("is:MAX"),
+ /* failingAtoms= */ ImmutableSet.of("is:MIN"))),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue(
"Copied Votes:\n"
- + "* Code-Review+1 (copy condition: \"is:MIN OR is:MAX\")\n"
- + "* Verified+1 (copy condition: \"is:MIN OR is:MAX\")\n");
+ + "* Code-Review+2 (copy condition: \"is:MIN OR **is:MAX**\")\n"
+ + "* Verified+1 (copy condition: \"is:MIN OR **is:MAX**\")\n");
}
@Test
- public void formatMultipleApprovals_differentValue_withCopyCondition_noUserInPredicate()
- throws Exception {
+ public void
+ formatMultipleApprovals_differentValue_withCopyCondition_noUserInPredicate_samePassingAtoms()
+ throws Exception {
LabelTypes labelTypes =
new LabelTypes(
ImmutableList.of(
createLabelType(
- /* labelName= */ "Code-Review", /* copyCondition= */ "is:MIN OR is:MAX")));
+ /* labelName= */ "Code-Review", /* copyCondition= */ "changekind:REWORK")));
PatchSetApproval patchSetApproval1 = createPatchSetApproval(admin, "Code-Review", 2);
PatchSetApproval patchSetApproval2 = createPatchSetApproval(user, "Code-Review", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval1, patchSetApproval2),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of("changekind:REWORK"),
+ /* failingAtoms= */ ImmutableSet.of()),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of("changekind:REWORK"),
+ /* failingAtoms= */ ImmutableSet.of())),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue(
"Copied Votes:\n"
- + "* Code-Review+1, Code-Review+2 (copy condition: \"is:MIN OR is:MAX\")\n");
+ + "* Code-Review+1, Code-Review+2 (copy condition: \"**changekind:REWORK**\")\n");
+ }
+
+ @Test
+ public void
+ formatMultipleApprovals_differentValue_withCopyCondition_noUserInPredicate_differentPassingAtoms()
+ throws Exception {
+ LabelTypes labelTypes =
+ new LabelTypes(
+ ImmutableList.of(
+ createLabelType(
+ /* labelName= */ "Code-Review", /* copyCondition= */ "is:1 OR is:2")));
+ PatchSetApproval patchSetApproval1 = createPatchSetApproval(admin, "Code-Review", 2);
+ PatchSetApproval patchSetApproval2 = createPatchSetApproval(user, "Code-Review", 1);
+ ApprovalCopier.Result approvalCopierResult =
+ ApprovalCopier.Result.create(
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of("is:2"),
+ /* failingAtoms= */ ImmutableSet.of("is:1")),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of("is:1"),
+ /* failingAtoms= */ ImmutableSet.of("is:2"))),
+ /* outdatedApprovals= */ ImmutableSet.of());
+ assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
+ .hasValue(
+ "Copied Votes:\n"
+ + "* Code-Review+1 (copy condition: \"**is:1** OR is:2\")\n"
+ + "* Code-Review+2 (copy condition: \"is:1 OR **is:2**\")\n");
}
@Test
@@ -545,7 +734,15 @@
PatchSetApproval patchSetApproval2 = createPatchSetApproval(user, "Code-Review", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval1, patchSetApproval2),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of()),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue(
@@ -565,7 +762,15 @@
PatchSetApproval patchSetApproval2 = createPatchSetApproval(user, "Verified", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval1, patchSetApproval2),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of()),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue(
@@ -587,7 +792,15 @@
PatchSetApproval patchSetApproval2 = createPatchSetApproval(user, "Code-Review", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval1, patchSetApproval2),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of()),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue(
@@ -597,8 +810,9 @@
}
@Test
- public void formatMultipleApprovals_sameVote_withCopyCondition_withUserInPredicate()
- throws Exception {
+ public void
+ formatMultipleApprovals_sameVote_withCopyCondition_withUserInPredicate_samePassingAtoms()
+ throws Exception {
String groupUuid =
groupCache.get(AccountGroup.nameKey("Administrators")).get().getGroupUUID().get();
LabelTypes labelTypes =
@@ -608,24 +822,86 @@
/* labelName= */ "Code-Review",
/* copyCondition= */ String.format(
"is:MIN OR (is:MAX approverin:%s)", groupUuid))));
- PatchSetApproval patchSetApproval1 = createPatchSetApproval(user, "Code-Review", 1);
- PatchSetApproval patchSetApproval2 = createPatchSetApproval(admin, "Code-Review", 1);
+ PatchSetApproval patchSetApproval1 = createPatchSetApproval(user, "Code-Review", 2);
+ PatchSetApproval patchSetApproval2 = createPatchSetApproval(admin, "Code-Review", 2);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval1, patchSetApproval2),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of(
+ "is:MAX", String.format("approverin:%s", groupUuid)),
+ /* failingAtoms= */ ImmutableSet.of("is:MIN")),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of(
+ "is:MAX", String.format("approverin:%s", groupUuid)),
+ /* failingAtoms= */ ImmutableSet.of("is:MIN"))),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue(
String.format(
"Copied Votes:\n"
- + "* Code-Review+1 by %s, %s"
- + " (copy condition: \"is:MIN OR (is:MAX approverin:%s)\")\n",
+ + "* Code-Review+2 by %s, %s"
+ + " (copy condition: \"is:MIN OR (**is:MAX** **approverin:%s**)\")\n",
AccountTemplateUtil.getAccountTemplate(admin.id()),
AccountTemplateUtil.getAccountTemplate(user.id()),
groupUuid));
}
@Test
+ public void
+ formatMultipleApprovals_sameVote_withCopyCondition_withUserInPredicate_differentPassingAtoms()
+ throws Exception {
+ String administratorsGroupUuid =
+ groupCache.get(AccountGroup.nameKey("Administrators")).get().getGroupUUID().get();
+ String registeredUsersGroupUuid = SystemGroupBackend.REGISTERED_USERS.get();
+ LabelTypes labelTypes =
+ new LabelTypes(
+ ImmutableList.of(
+ createLabelType(
+ /* labelName= */ "Code-Review",
+ /* copyCondition= */ String.format(
+ "is:MIN OR (is:MAX approverin:%s) OR (is:MAX approverin:%s)",
+ administratorsGroupUuid, registeredUsersGroupUuid))));
+ PatchSetApproval patchSetApproval1 = createPatchSetApproval(user, "Code-Review", 2);
+ PatchSetApproval patchSetApproval2 = createPatchSetApproval(admin, "Code-Review", 2);
+ ApprovalCopier.Result approvalCopierResult =
+ ApprovalCopier.Result.create(
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of(
+ "is:MAX", String.format("approverin:%s", registeredUsersGroupUuid)),
+ /* failingAtoms= */ ImmutableSet.of(
+ "is:MIN", String.format("approverin:%s", administratorsGroupUuid))),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of(
+ "is:MAX",
+ String.format("approverin:%s", administratorsGroupUuid),
+ String.format("approverin:%s", registeredUsersGroupUuid)),
+ /* failingAtoms= */ ImmutableSet.of("is:MIN"))),
+ /* outdatedApprovals= */ ImmutableSet.of());
+ assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
+ .hasValue(
+ String.format(
+ "Copied Votes:\n"
+ + "* Code-Review+2 by %s"
+ + " (copy condition: \"is:MIN OR (**is:MAX** **approverin:%s**)"
+ + " OR (**is:MAX** **approverin:%s**)\")\n"
+ + "* Code-Review+2 by %s"
+ + " (copy condition: \"is:MIN OR (**is:MAX** approverin:%s)"
+ + " OR (**is:MAX** **approverin:%s**)\")\n",
+ AccountTemplateUtil.getAccountTemplate(admin.id()),
+ administratorsGroupUuid,
+ registeredUsersGroupUuid,
+ AccountTemplateUtil.getAccountTemplate(user.id()),
+ administratorsGroupUuid,
+ registeredUsersGroupUuid));
+ }
+
+ @Test
public void formatMultipleApprovals_differentLabel_withCopyCondition_withUserInPredicate()
throws Exception {
String groupUuid =
@@ -641,18 +917,30 @@
/* labelName= */ "Verified",
/* copyCondition= */ String.format(
"is:MIN OR (is:MAX approverin:%s)", groupUuid))));
- PatchSetApproval patchSetApproval1 = createPatchSetApproval(user, "Code-Review", 1);
+ PatchSetApproval patchSetApproval1 = createPatchSetApproval(user, "Code-Review", -2);
PatchSetApproval patchSetApproval2 = createPatchSetApproval(admin, "Verified", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval1, patchSetApproval2),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of("is:MIN"),
+ /* failingAtoms= */ ImmutableSet.of(
+ "is:MAX", String.format("approverin:%s", groupUuid))),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of(
+ "is:MAX", String.format("approverin:%s", groupUuid)),
+ /* failingAtoms= */ ImmutableSet.of("is:MIN"))),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue(
String.format(
"Copied Votes:\n"
- + "* Code-Review+1 by %s (copy condition: \"is:MIN OR (is:MAX approverin:%s)\")\n"
- + "* Verified+1 by %s (copy condition: \"is:MIN OR (is:MAX approverin:%s)\")\n",
+ + "* Code-Review-2 by %s (copy condition: \"**is:MIN**"
+ + " OR (is:MAX approverin:%s)\")\n"
+ + "* Verified+1 by %s (copy condition: \"is:MIN"
+ + " OR (**is:MAX** **approverin:%s**)\")\n",
AccountTemplateUtil.getAccountTemplate(user.id()),
groupUuid,
AccountTemplateUtil.getAccountTemplate(admin.id()),
@@ -660,8 +948,9 @@
}
@Test
- public void formatMultipleApprovals_differentValue_withCopyCondition_withUserInPredicate()
- throws Exception {
+ public void
+ formatMultipleApprovals_differentValue_withCopyCondition_withUserInPredicate_samePassingAtoms()
+ throws Exception {
String groupUuid =
groupCache.get(AccountGroup.nameKey("Administrators")).get().getGroupUUID().get();
LabelTypes labelTypes =
@@ -670,51 +959,122 @@
createLabelType(
/* labelName= */ "Code-Review",
/* copyCondition= */ String.format(
- "is:MIN OR (is:MAX approverin:%s)", groupUuid))));
+ "is:MIN OR (is:ANY approverin:%s)", groupUuid))));
PatchSetApproval patchSetApproval1 = createPatchSetApproval(admin, "Code-Review", 2);
PatchSetApproval patchSetApproval2 = createPatchSetApproval(user, "Code-Review", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval1, patchSetApproval2),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of(
+ "is:ANY", String.format("approverin:%s", groupUuid)),
+ /* failingAtoms= */ ImmutableSet.of("is:MIN")),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of(
+ "is:ANY", String.format("approverin:%s", groupUuid)),
+ /* failingAtoms= */ ImmutableSet.of("is:MIN"))),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue(
String.format(
"Copied Votes:\n"
+ "* Code-Review+1 by %s, Code-Review+2 by %s"
- + " (copy condition: \"is:MIN OR (is:MAX approverin:%s)\")\n",
+ + " (copy condition: \"is:MIN OR (**is:ANY** **approverin:%s**)\")\n",
AccountTemplateUtil.getAccountTemplate(user.id()),
AccountTemplateUtil.getAccountTemplate(admin.id()),
groupUuid));
}
@Test
+ public void
+ formatMultipleApprovals_differentValue_withCopyCondition_withUserInPredicate_differentPassingAtoms()
+ throws Exception {
+ String groupUuid =
+ groupCache.get(AccountGroup.nameKey("Administrators")).get().getGroupUUID().get();
+ LabelTypes labelTypes =
+ new LabelTypes(
+ ImmutableList.of(
+ createLabelType(
+ /* labelName= */ "Code-Review",
+ /* copyCondition= */ String.format(
+ "is:MIN OR (is:1 approverin:%s) OR (is:2 approverin:%s)",
+ groupUuid, groupUuid))));
+ PatchSetApproval patchSetApproval1 = createPatchSetApproval(admin, "Code-Review", 2);
+ PatchSetApproval patchSetApproval2 = createPatchSetApproval(user, "Code-Review", 1);
+ ApprovalCopier.Result approvalCopierResult =
+ ApprovalCopier.Result.create(
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of(
+ "is:2", String.format("approverin:%s", groupUuid)),
+ /* failingAtoms= */ ImmutableSet.of("is:MIN", "is:1")),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of(
+ "is:1", String.format("approverin:%s", groupUuid)),
+ /* failingAtoms= */ ImmutableSet.of("is:MIN", "is:2"))),
+ /* outdatedApprovals= */ ImmutableSet.of());
+ assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
+ .hasValue(
+ String.format(
+ "Copied Votes:\n"
+ + "* Code-Review+1 by %s"
+ + " (copy condition: \"is:MIN OR (**is:1** **approverin:%s**)"
+ + " OR (is:2 **approverin:%s**)\")\n"
+ + "* Code-Review+2 by %s"
+ + " (copy condition: \"is:MIN OR (is:1 **approverin:%s**)"
+ + " OR (**is:2** **approverin:%s**)\")\n",
+ AccountTemplateUtil.getAccountTemplate(user.id()),
+ groupUuid,
+ groupUuid,
+ AccountTemplateUtil.getAccountTemplate(admin.id()),
+ groupUuid,
+ groupUuid));
+ }
+
+ @Test
public void formatMultipleApprovals_differentAndSameValue_withCopyCondition_withUserInPredicate()
throws Exception {
TestAccount user2 = accountCreator.user2();
- String groupUuid =
- groupCache.get(AccountGroup.nameKey("Administrators")).get().getGroupUUID().get();
+ String groupUuid = SystemGroupBackend.REGISTERED_USERS.get();
LabelTypes labelTypes =
new LabelTypes(
ImmutableList.of(
createLabelType(
/* labelName= */ "Code-Review",
/* copyCondition= */ String.format(
- "is:MIN OR (is:MAX approverin:%s)", groupUuid))));
+ "is:MIN OR (is:ANY approverin:%s)", groupUuid))));
PatchSetApproval patchSetApproval1 = createPatchSetApproval(admin, "Code-Review", 2);
PatchSetApproval patchSetApproval2 = createPatchSetApproval(user2, "Code-Review", 1);
PatchSetApproval patchSetApproval3 = createPatchSetApproval(user, "Code-Review", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
/* copiedApprovals= */ ImmutableSet.of(
- patchSetApproval1, patchSetApproval2, patchSetApproval3),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of(
+ "is:ANY", String.format("approverin:%s", groupUuid)),
+ /* failingAtoms= */ ImmutableSet.of("is:MIN")),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of(
+ "is:ANY", String.format("approverin:%s", groupUuid)),
+ /* failingAtoms= */ ImmutableSet.of("is:MIN")),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval3,
+ /* passingAtoms= */ ImmutableSet.of(
+ "is:ANY", String.format("approverin:%s", groupUuid)),
+ /* failingAtoms= */ ImmutableSet.of("is:MIN"))),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue(
String.format(
"Copied Votes:\n"
+ "* Code-Review+1 by %s, %s, Code-Review+2 by %s"
- + " (copy condition: \"is:MIN OR (is:MAX approverin:%s)\")\n",
+ + " (copy condition: \"is:MIN OR (**is:ANY** **approverin:%s**)\")\n",
AccountTemplateUtil.getAccountTemplate(user.id()),
AccountTemplateUtil.getAccountTemplate(user2.id()),
AccountTemplateUtil.getAccountTemplate(admin.id()),
@@ -737,7 +1097,15 @@
PatchSetApproval patchSetApproval2 = createPatchSetApproval(user, "Code-Review", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval1, patchSetApproval2),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of()),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue(
@@ -769,7 +1137,15 @@
PatchSetApproval patchSetApproval2 = createPatchSetApproval(user, "Verified", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval1, patchSetApproval2),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of()),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue(
@@ -799,7 +1175,15 @@
PatchSetApproval patchSetApproval2 = createPatchSetApproval(user, "Code-Review", 1);
ApprovalCopier.Result approvalCopierResult =
ApprovalCopier.Result.create(
- /* copiedApprovals= */ ImmutableSet.of(patchSetApproval1, patchSetApproval2),
+ /* copiedApprovals= */ ImmutableSet.of(
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval1,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of()),
+ ApprovalCopier.Result.PatchSetApprovalData.create(
+ patchSetApproval2,
+ /* passingAtoms= */ ImmutableSet.of(),
+ /* failingAtoms= */ ImmutableSet.of())),
/* outdatedApprovals= */ ImmutableSet.of());
assertThat(approvalsUtil.formatApprovalCopierResult(approvalCopierResult, labelTypes))
.hasValue(
@@ -849,7 +1233,7 @@
+ "\n"
+ "Copied Votes:\n"
+ "* Code-Review-2 (copy condition: \"changekind:NO_CHANGE"
- + " OR changekind:TRIVIAL_REBASE OR is:MIN\")\n"
+ + " OR changekind:TRIVIAL_REBASE OR **is:MIN**\")\n"
+ "\n"
+ "Outdated Votes:\n"
+ "* Verified+1\n");
@@ -900,7 +1284,7 @@
+ "\n"
+ "Copied Votes:\n"
+ "* Code-Review-2 (copy condition: \"changekind:NO_CHANGE"
- + " OR changekind:TRIVIAL_REBASE OR is:MIN\")\n"
+ + " OR changekind:TRIVIAL_REBASE OR **is:MIN**\")\n"
+ "\n"
+ "Outdated Votes:\n"
+ "* Verified+1\n");
@@ -946,7 +1330,7 @@
+ "\n"
+ "Copied Votes:\n"
+ "* Code-Review-2 (copy condition: \"changekind:NO_CHANGE"
- + " OR changekind:TRIVIAL_REBASE OR is:MIN\")\n"
+ + " OR changekind:TRIVIAL_REBASE OR **is:MIN**\")\n"
+ "\n"
+ "Outdated Votes:\n"
+ "* Verified+1\n");
diff --git a/javatests/com/google/gerrit/acceptance/server/change/ApprovalCopierIT.java b/javatests/com/google/gerrit/acceptance/server/change/ApprovalCopierIT.java
index 0d06946..379a712 100644
--- a/javatests/com/google/gerrit/acceptance/server/change/ApprovalCopierIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/change/ApprovalCopierIT.java
@@ -17,18 +17,23 @@
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.common.truth.Truth.assertAbout;
import static com.google.common.truth.Truth.assertThat;
-import static com.google.gerrit.acceptance.server.change.ApprovalCopierIT.PatchSetApprovalSubject.assertThatList;
-import static com.google.gerrit.acceptance.server.change.ApprovalCopierIT.PatchSetApprovalSubject.hasTestId;
+import static com.google.gerrit.acceptance.server.change.ApprovalCopierIT.ApprovalDataSubject.assertThat;
+import static com.google.gerrit.acceptance.server.change.ApprovalCopierIT.ApprovalDataSubject.assertThatList;
+import static com.google.gerrit.acceptance.server.change.ApprovalCopierIT.ApprovalDataSubject.hasTestId;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabel;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static com.google.gerrit.server.project.testing.TestLabels.labelBuilder;
import static com.google.gerrit.server.project.testing.TestLabels.value;
+import static com.google.gerrit.truth.ListSubject.elements;
import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableSet;
import com.google.common.truth.Correspondence;
import com.google.common.truth.FailureMetadata;
+import com.google.common.truth.StandardSubjectBuilder;
+import com.google.common.truth.StringSubject;
import com.google.common.truth.Subject;
+import com.google.common.truth.Truth8;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
@@ -43,6 +48,7 @@
import com.google.gerrit.entities.PatchSetApproval;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.client.ChangeKind;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.server.approval.ApprovalCopier;
import com.google.gerrit.server.query.change.ChangeData;
@@ -50,6 +56,7 @@
import com.google.gerrit.truth.NullAwareCorrespondence;
import com.google.inject.Inject;
import java.io.IOException;
+import java.util.Optional;
import java.util.Set;
import java.util.function.Predicate;
import org.eclipse.jgit.lib.Repository;
@@ -71,6 +78,24 @@
@Before
public void setup() throws Exception {
+ // Overwrite "Code-Review" label that is inherited from All-Projects.
+ try (ProjectConfigUpdate u = updateProject(project)) {
+ LabelType.Builder codeReview =
+ labelBuilder(
+ LabelId.CODE_REVIEW,
+ value(2, "Looks good to me, approved"),
+ value(1, "Looks good to me, but someone else must approve"),
+ value(0, "No score"),
+ value(-1, "I would prefer this is not submitted as is"),
+ value(-2, "This shall not be submitted"))
+ .setCopyCondition(
+ String.format(
+ "changekind:%s OR changekind:%s OR is:MIN",
+ ChangeKind.NO_CHANGE, ChangeKind.TRIVIAL_REBASE.name()));
+ u.getConfig().upsertLabelType(codeReview.build());
+ u.save();
+ }
+
// Add Verified label.
try (ProjectConfigUpdate u = updateProject(project)) {
LabelType.Builder verified =
@@ -153,6 +178,18 @@
.containsExactly(
PatchSetApprovalTestId.create(patchSet1Id, admin.id(), LabelId.CODE_REVIEW, 2),
PatchSetApprovalTestId.create(patchSet1Id, user.id(), LabelId.VERIFIED, 1));
+
+ ApprovalDataSubject codeReviewApprovalSubject =
+ assertThat(approvalCopierResult.outdatedApprovals(), LabelId.CODE_REVIEW, admin.id());
+ codeReviewApprovalSubject.hasPassingAtomsThat().isEmpty();
+ codeReviewApprovalSubject
+ .hasFailingAtomsThat()
+ .containsExactly("changekind:NO_CHANGE", "changekind:TRIVIAL_REBASE", "is:MIN");
+
+ ApprovalDataSubject verifiedApprovalSubject =
+ assertThat(approvalCopierResult.outdatedApprovals(), LabelId.VERIFIED, user.id());
+ verifiedApprovalSubject.hasPassingAtomsThat().isEmpty();
+ verifiedApprovalSubject.hasFailingAtomsThat().containsExactly("is:MIN");
}
@Test
@@ -176,6 +213,18 @@
PatchSetApprovalTestId.create(patchSet2Id, admin.id(), LabelId.CODE_REVIEW, -2),
PatchSetApprovalTestId.create(patchSet2Id, user.id(), LabelId.VERIFIED, -1));
assertThatList(approvalCopierResult.outdatedApprovals()).isEmpty();
+
+ ApprovalDataSubject codeReviewApprovalSubject =
+ assertThat(approvalCopierResult.copiedApprovals(), LabelId.CODE_REVIEW, admin.id());
+ codeReviewApprovalSubject.hasPassingAtomsThat().containsExactly("is:MIN");
+ codeReviewApprovalSubject
+ .hasFailingAtomsThat()
+ .containsExactly("changekind:NO_CHANGE", "changekind:TRIVIAL_REBASE");
+
+ ApprovalDataSubject verifiedApprovalSubject =
+ assertThat(approvalCopierResult.copiedApprovals(), LabelId.VERIFIED, user.id());
+ verifiedApprovalSubject.hasPassingAtomsThat().containsExactly("is:MIN");
+ verifiedApprovalSubject.hasFailingAtomsThat().isEmpty();
}
@Test
@@ -230,6 +279,30 @@
.containsExactly(
PatchSetApprovalTestId.create(patchSet1Id, user.id(), LabelId.CODE_REVIEW, 1),
PatchSetApprovalTestId.create(patchSet1Id, admin.id(), LabelId.VERIFIED, 1));
+
+ ApprovalDataSubject copiedCodeReviewApprovalSubject =
+ assertThat(approvalCopierResult.copiedApprovals(), LabelId.CODE_REVIEW, admin.id());
+ copiedCodeReviewApprovalSubject.hasPassingAtomsThat().containsExactly("is:MIN");
+ copiedCodeReviewApprovalSubject
+ .hasFailingAtomsThat()
+ .containsExactly("changekind:NO_CHANGE", "changekind:TRIVIAL_REBASE");
+
+ ApprovalDataSubject copiedVerifiedApprovalSubject =
+ assertThat(approvalCopierResult.copiedApprovals(), LabelId.VERIFIED, user.id());
+ copiedVerifiedApprovalSubject.hasPassingAtomsThat().containsExactly("is:MIN");
+ copiedVerifiedApprovalSubject.hasFailingAtomsThat().isEmpty();
+
+ ApprovalDataSubject outdatedCodeReviewApprovalSubject1 =
+ assertThat(approvalCopierResult.outdatedApprovals(), LabelId.CODE_REVIEW, user.id());
+ outdatedCodeReviewApprovalSubject1.hasPassingAtomsThat().isEmpty();
+ outdatedCodeReviewApprovalSubject1
+ .hasFailingAtomsThat()
+ .containsExactly("changekind:NO_CHANGE", "changekind:TRIVIAL_REBASE", "is:MIN");
+
+ ApprovalDataSubject outdatedVerifiedApprovalSubject1 =
+ assertThat(approvalCopierResult.outdatedApprovals(), LabelId.VERIFIED, admin.id());
+ outdatedVerifiedApprovalSubject1.hasPassingAtomsThat().isEmpty();
+ outdatedVerifiedApprovalSubject1.hasFailingAtomsThat().containsExactly("is:MIN");
}
@Test
@@ -275,6 +348,11 @@
.comparingElementsUsing(hasTestId())
.containsExactly(
PatchSetApprovalTestId.create(patchSet1Id, admin.id(), LabelId.CODE_REVIEW, -2));
+
+ ApprovalDataSubject codeReviewApprovalSubject1 =
+ assertThat(approvalCopierResult.outdatedApprovals(), LabelId.CODE_REVIEW, admin.id());
+ codeReviewApprovalSubject1.hasPassingAtomsThat().isEmpty();
+ codeReviewApprovalSubject1.hasFailingAtomsThat().isEmpty();
}
@Test
@@ -347,12 +425,14 @@
ApprovalCopier.Result approvalCopierResult =
invokeApprovalCopierForCurrentPatchSet(
r.getChange().getId(), /* expectedCurrentPatchSetNum= */ 2);
- ImmutableSet<PatchSetApproval> copiedApprovals = approvalCopierResult.copiedApprovals();
- assertThatList(filter(copiedApprovals, PatchSetApproval::copied))
+ ImmutableSet<ApprovalCopier.Result.PatchSetApprovalData> copiedApprovals =
+ approvalCopierResult.copiedApprovals();
+ assertThatList(filter(copiedApprovals, approval -> approval.patchSetApproval().copied()))
.comparingElementsUsing(hasTestId())
.containsExactly(
PatchSetApprovalTestId.create(patchSet2Id, user.id(), LabelId.VERIFIED, -1));
- assertThatList(filter(copiedApprovals, psa -> !psa.copied())).isEmpty();
+ assertThatList(filter(copiedApprovals, approval -> !approval.patchSetApproval().copied()))
+ .isEmpty();
}
private void vote(String changeId, TestAccount testAccount, String label, int value)
@@ -362,8 +442,9 @@
requestScopeOperations.setApiUser(admin.id());
}
- private ImmutableSet<PatchSetApproval> filter(
- Set<PatchSetApproval> approvals, Predicate<PatchSetApproval> filter) {
+ private ImmutableSet<ApprovalCopier.Result.PatchSetApprovalData> filter(
+ Set<ApprovalCopier.Result.PatchSetApprovalData> approvals,
+ Predicate<ApprovalCopier.Result.PatchSetApprovalData> filter) {
return approvals.stream().filter(filter).collect(toImmutableSet());
}
@@ -378,20 +459,75 @@
}
}
- public static class PatchSetApprovalSubject extends Subject {
- public static Correspondence<PatchSetApproval, PatchSetApprovalTestId> hasTestId() {
- return NullAwareCorrespondence.transforming(PatchSetApprovalTestId::create, "has test ID");
+ public static class ApprovalDataSubject extends Subject {
+ public static Correspondence<ApprovalCopier.Result.PatchSetApprovalData, PatchSetApprovalTestId>
+ hasTestId() {
+ return NullAwareCorrespondence.transforming(
+ approvalData -> PatchSetApprovalTestId.create(approvalData.patchSetApproval()),
+ "has test ID");
}
+ public static ApprovalDataSubject assertThat(
+ ApprovalCopier.Result.PatchSetApprovalData approvalData) {
+ return assertAbout(approvalDatas()).that(approvalData);
+ }
+
+ public static ApprovalDataSubject assertThat(
+ ImmutableSet<ApprovalCopier.Result.PatchSetApprovalData> approvalDatas,
+ String labelId,
+ Account.Id accountId) {
+ Optional<ApprovalCopier.Result.PatchSetApprovalData> approvalDataForLabelAndAccount =
+ approvalDatas.stream()
+ .filter(
+ approvalData ->
+ approvalData.patchSetApproval().label().equals(labelId)
+ && approvalData.patchSetApproval().accountId().equals(accountId))
+ .findAny();
+ Truth8.assertThat(approvalDataForLabelAndAccount).isPresent();
+ return assertAbout(approvalDatas()).that(approvalDataForLabelAndAccount.get());
+ }
+
+ public static ListSubject<ApprovalDataSubject, ApprovalCopier.Result.PatchSetApprovalData>
+ assertThatList(ImmutableSet<ApprovalCopier.Result.PatchSetApprovalData> approvalDatas) {
+ return ListSubject.assertThat(approvalDatas.asList(), approvalDatas());
+ }
+
+ private static Factory<ApprovalDataSubject, ApprovalCopier.Result.PatchSetApprovalData>
+ approvalDatas() {
+ return ApprovalDataSubject::new;
+ }
+
+ private final ApprovalCopier.Result.PatchSetApprovalData approvalData;
+
+ private ApprovalDataSubject(
+ FailureMetadata metadata, ApprovalCopier.Result.PatchSetApprovalData approvalData) {
+ super(metadata, approvalData);
+ this.approvalData = approvalData;
+ }
+
+ public ListSubject<StringSubject, String> hasPassingAtomsThat() {
+ return check("passingAtoms()")
+ .about(elements())
+ .that(approvalData().passingAtoms().asList(), StandardSubjectBuilder::that);
+ }
+
+ public ListSubject<StringSubject, String> hasFailingAtomsThat() {
+ return check("failingAtoms()")
+ .about(elements())
+ .that(approvalData().failingAtoms().asList(), StandardSubjectBuilder::that);
+ }
+
+ private ApprovalCopier.Result.PatchSetApprovalData approvalData() {
+ isNotNull();
+ return approvalData;
+ }
+ }
+
+ public static class PatchSetApprovalSubject extends Subject {
public static PatchSetApprovalSubject assertThat(PatchSetApproval patchSetApproval) {
return assertAbout(patchSetApprovals()).that(patchSetApproval);
}
- public static ListSubject<PatchSetApprovalSubject, PatchSetApproval> assertThatList(
- ImmutableSet<PatchSetApproval> patchSetApprovals) {
- return ListSubject.assertThat(patchSetApprovals.asList(), patchSetApprovals());
- }
-
private static Factory<PatchSetApprovalSubject, PatchSetApproval> patchSetApprovals() {
return PatchSetApprovalSubject::new;
}
