opendev/gerrit
Code Issues Proposed changes

Carry sticky approvals forward multiple PatchSets

Browse Source 
NoteDb's logic for sticky approvals is incorrectly strict. This
change removes the requirement that an approval must be on the
immediately preceding PatchSet before copying it forward (when
copyMinScore or copyMaxScore applies).

Bug: Issue 4373
Change-Id: I089fac9e29eb540c843834ab4ef085166111ec3e
This commit is contained in:
Logan Hanks
2016-08-11 11:42:06 -07:00
parent a51f78b1b2
commit dfb92a7d17
2 changed files with 82 additions and 20 deletions
Download Patch File Download Diff File Expand all files Collapse all files

64
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/StickyApprovalsIT.java
View File

@@ -247,6 +247,70 @@ public class StickyApprovalsIT extends AbstractDaemonTest {
}
}
@Test
public void stickyAcrossMultiplePatchSets() throws Exception {
ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
cfg.getLabelSections().get("Code-Review")
.setCopyMaxScore(true);
cfg.getLabelSections().get("Verified")
.setCopyAllScoresIfNoCodeChange(true);
saveProjectConfig(project, cfg);
String changeId = createChange(REWORK);
vote(admin, changeId, 2, 1);
for (int i = 0; i < 5; i++) {
updateChange(changeId, NO_CODE_CHANGE);
ChangeInfo c = detailedChange(changeId);
assertVotes(c, admin, 2, 1, NO_CODE_CHANGE);
}
updateChange(changeId, REWORK);
ChangeInfo c = detailedChange(changeId);
assertVotes(c, admin, 2, 0, REWORK);
}
@Test
public void copyMinMaxAcrossMultiplePatchSets() throws Exception {
ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
cfg.getLabelSections().get("Code-Review")
.setCopyMaxScore(true);
cfg.getLabelSections().get("Code-Review")
.setCopyMinScore(true);
saveProjectConfig(project, cfg);
// Vote max score on PS1
String changeId = createChange(REWORK);
vote(admin, changeId, 2, 1);
// Have someone else vote min score on PS2
updateChange(changeId, REWORK);
vote(user, changeId, -2, 0);
ChangeInfo c = detailedChange(changeId);
assertVotes(c, admin, 2, 0, REWORK);
assertVotes(c, user, -2, 0, REWORK);
// No vote changes on PS3
updateChange(changeId, REWORK);
c = detailedChange(changeId);
assertVotes(c, admin, 2, 0, REWORK);
assertVotes(c, user, -2, 0, REWORK);
// Both users revote on PS4
updateChange(changeId, REWORK);
vote(admin, changeId, 1, 1);
vote(user, changeId, 1, 1);
c = detailedChange(changeId);
assertVotes(c, admin, 1, 1, REWORK);
assertVotes(c, user, 1, 1, REWORK);
// New approvals shouldn't carry through to PS5
updateChange(changeId, REWORK);
c = detailedChange(changeId);
assertVotes(c, admin, 0, 0, REWORK);
assertVotes(c, user, 0, 0, REWORK);
}
private ChangeInfo detailedChange(String changeId) throws Exception {
return gApi.changes().id(changeId)
.get(EnumSet.of(ListChangesOption.DETAILED_LABELS,

38
gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java
View File

@@ -44,9 +44,6 @@ import java.io.IOException;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.NavigableSet;
import java.util.Objects;
import java.util.SortedSet;
import java.util.TreeMap;
/**
@@ -104,6 +101,8 @@ public class ApprovalCopier {
ListMultimap<PatchSet.Id, PatchSetApproval> all = cd.approvals();
checkNotNull(all, "all should not be null");
Table<String, Account.Id, PatchSetApproval> wontCopy =
HashBasedTable.create();
Table<String, Account.Id, PatchSetApproval> byUser =
HashBasedTable.create();
for (PatchSetApproval psa : all.get(ps.getId())) {
@@ -111,7 +110,6 @@ public class ApprovalCopier {
}
TreeMap<Integer, PatchSet> patchSets = getPatchSets(cd);
NavigableSet<Integer> allPsIds = patchSets.navigableKeySet();
try (Repository repo =
repoManager.openRepository(project.getProject().getNameKey())) {
@@ -130,11 +128,18 @@ public class ApprovalCopier {
ObjectId.fromString(ps.getRevision().get()));
for (PatchSetApproval psa : priorApprovals) {
if (!byUser.contains(psa.getLabel(), psa.getAccountId())
&& canCopy(project, psa, ps.getId(), allPsIds, kind)) {
byUser.put(psa.getLabel(), psa.getAccountId(),
copy(psa, ps.getId()));
if (wontCopy.contains(psa.getLabel(), psa.getAccountId())) {
continue;
}
if (byUser.contains(psa.getLabel(), psa.getAccountId())) {
continue;
}
if (!canCopy(project, psa, ps.getId(), kind)) {
wontCopy.put(psa.getLabel(), psa.getAccountId(), psa);
continue;
}
byUser.put(psa.getLabel(), psa.getAccountId(),
copy(psa, ps.getId()));
}
}
return labelNormalizer.normalize(ctl, byUser.values()).getNormalized();
@@ -155,17 +160,15 @@ public class ApprovalCopier {
}
private static boolean canCopy(ProjectState project, PatchSetApproval psa,
PatchSet.Id psId, NavigableSet<Integer> allPsIds, ChangeKind kind) {
PatchSet.Id psId, ChangeKind kind) {
int n = psa.getKey().getParentKey().get();
checkArgument(n != psId.get());
LabelType type = project.getLabelTypes().byLabel(psa.getLabelId());
if (type == null) {
return false;
} else if (Objects.equals(n, previous(allPsIds, psId.get())) && (
type.isCopyMinScore() && type.isMaxNegative(psa)
|| type.isCopyMaxScore() && type.isMaxPositive(psa))) {
// Copy min/max score only from the immediately preceding patch set (which
// may not be psId.get() - 1).
} else if (
(type.isCopyMinScore() && type.isMaxNegative(psa))
|| (type.isCopyMaxScore() && type.isMaxPositive(psa))) {
return true;
}
switch (kind) {
@@ -192,9 +195,4 @@ public class ApprovalCopier {
}
return new PatchSetApproval(psId, src);
}
private static <T> T previous(NavigableSet<T> s, T v) {
SortedSet<T> head = s.headSet(v);
return !head.isEmpty() ? head.last() : null;
}
}
}