Give CL author a default owner +1 vote.
* A changed file does not need other owner approval,
if the author is one of the owners.
* Find Owners button always shows file groups with owner approvals.
Change-Id: I6a716fbfcb65ab2a01b6ba595ae00ab4b2c3261e
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/Checker.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/Checker.java
index ff75081..6b9897d 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Checker.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Checker.java
@@ -49,7 +49,7 @@
}
/** Returns a map from reviewer email to vote value. */
- static Map<String, Integer> getVotes(AccountCache accountCache, ChangeData changeData)
+ Map<String, Integer> getVotes(AccountCache accountCache, ChangeData changeData)
throws OrmException {
Map<String, Integer> map = new HashMap<>();
for (PatchSetApproval p : changeData.currentApprovals()) {
@@ -59,6 +59,12 @@
Integer.valueOf(p.getValue()));
}
}
+ // Give CL author a default minVoteLevel vote.
+ String author =
+ accountCache.get(changeData.change().getOwner()).getAccount().getPreferredEmail();
+ if (!map.containsKey(author) || map.get(author) == 0) {
+ map.put(author, minVoteLevel);
+ }
return map;
}
@@ -69,8 +75,6 @@
for (String owner : owners) {
if (votes.containsKey(owner)) {
int v = votes.get(owner);
- // TODO: Maybe add a configurable feature in the next version
- // to exclude the committer's vote from the "foundApproval".
foundApproval |= (v >= minVoteLevel);
foundVeto |= (v < 0); // an owner's -1 vote is a veto
} else if (owner.equals("*")) {
diff --git a/src/main/resources/static/find-owners.js b/src/main/resources/static/find-owners.js
index 8122474..79bcc72 100644
--- a/src/main/resources/static/find-owners.js
+++ b/src/main/resources/static/find-owners.js
@@ -14,9 +14,9 @@
Gerrit.install(function(self) {
function onFindOwners(c) {
- const HTML_ALL_HAVE_OWNER_APPROVAL =
- '<b>All files have owner approval.</b><br>';
const HTML_BULLET = '<small>★</small>'; // a Black Star
+ const HTML_HAS_APPROVAL_HEADER =
+ '<hr><b>Files with +1 or +2 Code-Review vote from owners:</b><br>';
const HTML_IS_EXEMPTED =
'<b>This commit is exempted from owner approval.</b><br>';
const HTML_NEED_REVIEWER_HEADER =
@@ -39,15 +39,18 @@
const CHECKBOX_ID = 'FindOwners:CheckBox';
const HEADER_DIV_ID = 'FindOwners:Header';
const OWNERS_DIV_ID = 'FindOwners:Owners';
+ const HAS_APPROVAL_DIV_ID = 'FindOwners:HasApproval';
const NEED_APPROVAL_DIV_ID = 'FindOwners:NeedApproval';
const NEED_REVIEWER_DIV_ID = 'FindOwners:NeedReviewer';
// Aliases to values in the context.
const branch = c.change.branch;
const changeId = c.change._number;
+ const changeOwner = c.change.owner;
const message = c.revision.commit.message;
const project = c.change.project;
+ var minVoteLevel = 1; // could be changed by server returned results.
var reviewerId = {}; // map from a reviewer's email to account id.
var reviewerVote = {}; // map from a reviewer's email to Code-Review vote.
@@ -76,6 +79,14 @@
}
}
});
+ // Give CL author a default minVoteLevel vote.
+ if (changeOwner != null &&
+ 'email' in changeOwner && '_account_id' in changeOwner &&
+ (!(changeOwner.email in reviewerId) ||
+ reviewerVote[changeOwner.email] == 0)) {
+ reviewerId[changeOwner.email] = changeOwner._account_id;
+ reviewerVote[changeOwner.email] = minVoteLevel;
+ }
}
function checkAddRemoveLists() {
// Gerrit.post and delete are asynchronous.
@@ -121,7 +132,7 @@
return (owner in reviewers || owner == '*');
});
}
- function hasOwnerApproval(votes, minVoteLevel, owners) {
+ function hasOwnerApproval(votes, owners) {
var foundApproval = false;
for (var j = 0; j < owners.length; j++) {
if (owners[j] in votes) {
@@ -129,7 +140,6 @@
if (v < 0) {
return false; // cannot have any negative vote
}
- // TODO: do not count if owners[j] is the patch committer.
foundApproval |= v >= minVoteLevel;
}
}
@@ -164,6 +174,7 @@
addKeyValue('changeId', changeId);
addKeyValue('project', project);
addKeyValue('branch', branch);
+ addKeyValue('changeOwner.email', changeOwner.email);
addKeyValue('Gerrit.url', Gerrit.url());
addKeyValue('self.url', self.url());
showJsonLines(args, 'changeOwner', c.change.owner);
@@ -184,11 +195,14 @@
var header = emptyDiv(HEADER_DIV_ID);
var needReviewerDiv = emptyDiv(NEED_REVIEWER_DIV_ID);
var needApprovalDiv = emptyDiv(NEED_APPROVAL_DIV_ID);
+ var hasApprovalDiv = emptyDiv(HAS_APPROVAL_DIV_ID);
addApplyButton();
var ownersDiv = emptyDiv(OWNERS_DIV_ID);
var numCheckBoxes = 0;
var owner2boxes = {}; // owner name ==> array of checkbox id
var owner2email = {}; // owner name ==> email address
+ minVoteLevel =
+ ('minOwnerVoteLevel' in result ? result.minOwnerVoteLevel : 1);
function addApplyButton() {
var apply = c.button('Apply', {onclick: doApplyButton});
@@ -285,6 +299,7 @@
function updateDivContent() {
var groupNeedReviewer = [];
var groupNeedApproval = [];
+ var groupHasApproval = [];
numCheckBoxes = 0;
owner2boxes = {};
Object.keys(groups).sort().forEach(function(key) {
@@ -293,24 +308,21 @@
groupNeedReviewer.push(key);
} else if (g.needApproval) {
groupNeedApproval.push(key);
+ } else {
+ groupHasApproval.push(key);
}
});
- if (0 == groupNeedReviewer.length && 0 == groupNeedApproval.length) {
- showDiv(header, HTML_ALL_HAVE_OWNER_APPROVAL);
- } else {
- showDiv(header, HTML_SELECT_REVIEWERS);
- addGroupsToDiv(needReviewerDiv, groupNeedReviewer,
- HTML_NEED_REVIEWER_HEADER);
- addGroupsToDiv(needApprovalDiv, groupNeedApproval,
- HTML_NEED_APPROVAL_HEADER);
- addOwnersDiv(ownersDiv, HTML_OWNERS_HEADER);
- }
+ showDiv(header, HTML_SELECT_REVIEWERS);
+ addGroupsToDiv(needReviewerDiv, groupNeedReviewer,
+ HTML_NEED_REVIEWER_HEADER);
+ addGroupsToDiv(needApprovalDiv, groupNeedApproval,
+ HTML_NEED_APPROVAL_HEADER);
+ addGroupsToDiv(hasApprovalDiv, groupHasApproval,
+ HTML_HAS_APPROVAL_HEADER);
+ addOwnersDiv(ownersDiv, HTML_OWNERS_HEADER);
}
function createGroups() {
var owners2group = {}; // owner list to group name
- var minVoteLevel =
- ('minOwnerVoteLevel' in result ?
- result['minOwnerVoteLevel'] : 1);
Object.keys(result.file2owners).sort().forEach(function(name) {
var splitOwners = result.file2owners[name];
var owners = splitOwners.join(' ');
@@ -321,7 +333,7 @@
groupSize[name] = 1;
var needReviewer = !hasOwnerReviewer(reviewerId, splitOwners);
var needApproval = !needReviewer &&
- !hasOwnerApproval(reviewerVote, minVoteLevel, splitOwners);
+ !hasOwnerApproval(reviewerVote, splitOwners);
groups[name] = {
'needReviewer': needReviewer,
'needApproval': needApproval,
diff --git a/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwnersIT.java b/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwnersIT.java
index 9ab3e48..b72c860 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwnersIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwnersIT.java
@@ -37,6 +37,7 @@
import com.google.inject.Inject;
import java.util.Collection;
import org.eclipse.jgit.lib.ObjectLoader;
+import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevObject;
import org.eclipse.jgit.revwalk.RevTree;
import org.eclipse.jgit.revwalk.RevWalk;
@@ -301,6 +302,20 @@
}
@Test
+ public void authorDefaultVoteTest() throws Exception {
+ // CL author has default +1 owner vote.
+ addFile("add d1/OWNERS", "d1/OWNERS", user.email + "\n"); // d1 owned by user
+ addFile("add d2/OWNERS", "d2/OWNERS", admin.email + "\n"); // d2 owned by admin
+ // admin is the author of CLs created by createChange.
+ PushOneCommit.Result r1 = createChange("add d1/t.c", "d1/t.c", "Hello1");
+ PushOneCommit.Result r2 = createChange("add d2/t.c", "d2/t.c", "Hello2");
+ PushOneCommit.Result r3 = createChange("add d3/t.c", "d3/t.c", "Hello3");
+ assertThat(checkApproval(r1)).isEqualTo(-1); // owner is not change author
+ assertThat(checkApproval(r2)).isEqualTo(1); // owner is change author, default +1
+ assertThat(checkApproval(r3)).isEqualTo(0); // no owner is found in d3
+ }
+
+ @Test
public void actionApplyTest() throws Exception {
Cache cache = Cache.getInstance().init(0, 10);
assertThat(cache.size()).isEqualTo(0);
@@ -425,6 +440,14 @@
approveSubmit(commit);
}
+ private int checkApproval(PushOneCommit.Result r) throws Exception {
+ Repository repo = repoManager.openRepository(r.getChange().project());
+ Cache cache = Cache.getInstance().init(0, 0);
+ OwnersDb db = cache.get(accountCache, accounts, repo, r.getChange(), 1);
+ Checker c = new Checker(repo, r.getChange(), 1);
+ return c.findApproval(accountCache, db);
+ }
+
// Remove '"' and space; replace '\n' with ' '; ignore unpredictable "owner_revision".
private static String filteredJson(String json) {
return json.replaceAll("[\" ]*", "").replace('\n', ' ').replaceAll("owner_revision:[^ ]* ", "");
