Return list of reviewers as part of ChangeInfo

ChangeInfo now includes the list of reviewers when DETAILED_LABELS are
requested.

So far the list of reviewers could only indirectly be computed from
the returned approvals, but this relied on having a dummy 0 approval
for users that were added as reviewer, but that haven't voted yet.

When notedb is enabled we no longer store dummy 0 approvals for
reviewers, but we track the list of reviewers explicitly in notedb.

When notedb was enabled the ChangeIT#addReviewerToClosedChange() test
was failing because it relied on the dummy 0 approvals.

With this change, reviewers are now included in ChangeInfo, adapt the
tests to look at this field instead of computing the list of reviewers
from the approvals.

When notedb is enabled the PostReviewers REST endpoint records users
as 'REVIEWER' in notedb. If notedb is disabled the PostReviewers REST
endpoint behaves differently. In this case it adds dummy 0 approvals
for the users, but these translate into 'CC's when the ChangeInfo is
constructed. Hence the tests that check on reviewers must do different
assertions depending on whether notedb is enabled or not.

Change-Id: I5e7b99d85d97abc43ac17da44b7179f4ceb3e268
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2015-11-10 17:38:40 -08:00
parent 2df725626c
commit 66af3d8517
18 changed files with 268 additions and 88 deletions

View File

@@ -15,6 +15,8 @@
package com.google.gerrit.server;
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.CC;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Function;
@@ -44,7 +46,7 @@ import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.notedb.NotesMigration;
import com.google.gerrit.server.notedb.ReviewerState;
import com.google.gerrit.server.notedb.ReviewerStateInternal;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
@@ -114,10 +116,10 @@ public class ApprovalsUtil {
* @param notes change notes.
* @return multimap of reviewers keyed by state, where each account appears
* exactly once in {@link SetMultimap#values()}, and
* {@link ReviewerState#REMOVED} is not present.
* {@link ReviewerStateInternal#REMOVED} is not present.
* @throws OrmException if reviewers for the change could not be read.
*/
public ImmutableSetMultimap<ReviewerState, Account.Id> getReviewers(
public ImmutableSetMultimap<ReviewerStateInternal, Account.Id> getReviewers(
ReviewDb db, ChangeNotes notes) throws OrmException {
if (!migration.readChanges()) {
return getReviewers(db.patchSetApprovals().byChange(notes.getChangeId()));
@@ -132,9 +134,9 @@ public class ApprovalsUtil {
* change.
* @return multimap of reviewers keyed by state, where each account appears
* exactly once in {@link SetMultimap#values()}, and
* {@link ReviewerState#REMOVED} is not present.
* {@link ReviewerStateInternal#REMOVED} is not present.
*/
public ImmutableSetMultimap<ReviewerState, Account.Id> getReviewers(
public ImmutableSetMultimap<ReviewerStateInternal, Account.Id> getReviewers(
ChangeNotes notes, Iterable<PatchSetApproval> allApprovals)
throws OrmException {
if (!migration.readChanges()) {
@@ -143,10 +145,10 @@ public class ApprovalsUtil {
return notes.load().getReviewers();
}
private static ImmutableSetMultimap<ReviewerState, Account.Id> getReviewers(
private static ImmutableSetMultimap<ReviewerStateInternal, Account.Id> getReviewers(
Iterable<PatchSetApproval> allApprovals) {
PatchSetApproval first = null;
SetMultimap<ReviewerState, Account.Id> reviewers =
SetMultimap<ReviewerStateInternal, Account.Id> reviewers =
LinkedHashMultimap.create();
for (PatchSetApproval psa : allApprovals) {
if (first == null) {
@@ -159,10 +161,10 @@ public class ApprovalsUtil {
}
Account.Id id = psa.getAccountId();
if (psa.getValue() != 0) {
reviewers.put(ReviewerState.REVIEWER, id);
reviewers.remove(ReviewerState.CC, id);
} else if (!reviewers.containsEntry(ReviewerState.REVIEWER, id)) {
reviewers.put(ReviewerState.CC, id);
reviewers.put(REVIEWER, id);
reviewers.remove(CC, id);
} else if (!reviewers.containsEntry(REVIEWER, id)) {
reviewers.put(CC, id);
}
}
return ImmutableSetMultimap.copyOf(reviewers);
@@ -215,7 +217,7 @@ public class ApprovalsUtil {
cells.add(new PatchSetApproval(
new PatchSetApproval.Key(psId, account, labelId),
(short) 0, TimeUtil.nowTs()));
update.putReviewer(account, ReviewerState.REVIEWER);
update.putReviewer(account, REVIEWER);
}
db.patchSetApprovals().insert(cells);
return Collections.unmodifiableList(cells);

View File

@@ -40,6 +40,7 @@ import com.google.common.base.Joiner;
import com.google.common.base.MoreObjects;
import com.google.common.base.Optional;
import com.google.common.base.Throwables;
import com.google.common.collect.ComparisonChain;
import com.google.common.collect.FluentIterable;
import com.google.common.collect.HashBasedTable;
import com.google.common.collect.HashMultimap;
@@ -48,6 +49,7 @@ import com.google.common.collect.LinkedHashMultimap;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.collect.Multimap;
import com.google.common.collect.Ordering;
import com.google.common.collect.SetMultimap;
import com.google.common.collect.Sets;
import com.google.common.collect.Table;
@@ -94,6 +96,7 @@ import com.google.gerrit.server.api.accounts.GpgApiAdapter;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.LabelNormalizer;
import com.google.gerrit.server.git.MergeUtil;
import com.google.gerrit.server.notedb.ReviewerStateInternal;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.ProjectCache;
@@ -118,7 +121,9 @@ import java.sql.Timestamp;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.Comparator;
import java.util.EnumSet;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
@@ -437,6 +442,13 @@ public class ChangeJson {
out.permittedLabels = permittedLabels(ctl, cd);
}
out.removableReviewers = removableReviewers(ctl, out.labels.values());
out.reviewers = new HashMap<>();
for (Map.Entry<ReviewerStateInternal, Collection<Account.Id>> e
: cd.reviewers().asMap().entrySet()) {
out.reviewers.put(e.getKey().asReviewerState(),
toAccountInfo(e.getValue()));
}
}
boolean needMessages = has(MESSAGES);
@@ -828,6 +840,27 @@ public class ChangeJson {
return result;
}
private Collection<AccountInfo> toAccountInfo(
Collection<Account.Id> accounts) {
return FluentIterable.from(accounts)
.transform(new Function<Account.Id, AccountInfo>() {
@Override
public AccountInfo apply(Account.Id id) {
return accountLoader.get(id);
}
})
.toSortedList(new Comparator<AccountInfo>() {
@Override
public int compare(AccountInfo a, AccountInfo b) {
return ComparisonChain.start()
.compare(a.name, b.name, Ordering.natural().nullsFirst())
.compare(a.email, b.email, Ordering.natural().nullsFirst())
.compare(a._accountId, b._accountId,
Ordering.natural().nullsFirst()).result();
}
});
}
private Map<String, RevisionInfo> revisions(ChangeControl ctl,
Map<PatchSet.Id, PatchSet> map) throws PatchListNotAvailableException,
GpgException, OrmException, IOException {

View File

@@ -17,6 +17,8 @@ package com.google.gerrit.server.change;
import static com.google.common.base.MoreObjects.firstNonNull;
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkState;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.CC;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
import com.google.common.collect.SetMultimap;
import com.google.gerrit.common.ChangeHooks;
@@ -42,7 +44,7 @@ import com.google.gerrit.server.git.GroupCollector;
import com.google.gerrit.server.git.validators.CommitValidationException;
import com.google.gerrit.server.git.validators.CommitValidators;
import com.google.gerrit.server.mail.ReplacePatchSetSender;
import com.google.gerrit.server.notedb.ReviewerState;
import com.google.gerrit.server.notedb.ReviewerStateInternal;
import com.google.gerrit.server.patch.PatchSetInfoFactory;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.ChangeModifiedException;
@@ -106,7 +108,7 @@ public class PatchSetInserter extends BatchUpdate.Op {
private PatchSet patchSet;
private PatchSetInfo patchSetInfo;
private ChangeMessage changeMessage;
private SetMultimap<ReviewerState, Account.Id> oldReviewers;
private SetMultimap<ReviewerStateInternal, Account.Id> oldReviewers;
@AssistedInject
public PatchSetInserter(ChangeHooks hooks,
@@ -280,8 +282,8 @@ public class PatchSetInserter extends BatchUpdate.Op {
cm.setFrom(ctx.getUser().getAccountId());
cm.setPatchSet(patchSet, patchSetInfo);
cm.setChangeMessage(changeMessage);
cm.addReviewers(oldReviewers.get(ReviewerState.REVIEWER));
cm.addExtraCC(oldReviewers.get(ReviewerState.CC));
cm.addReviewers(oldReviewers.get(REVIEWER));
cm.addExtraCC(oldReviewers.get(CC));
cm.send();
} catch (Exception err) {
log.error("Cannot send email for new patch set on change "

View File

@@ -17,6 +17,7 @@ package com.google.gerrit.server.change;
import static com.google.common.base.MoreObjects.firstNonNull;
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.gerrit.server.PatchLineCommentsUtil.setCommentRevId;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
import static java.nio.charset.StandardCharsets.UTF_8;
import com.google.auto.value.AutoValue;
@@ -64,7 +65,6 @@ import com.google.gerrit.server.git.BatchUpdate.ChangeContext;
import com.google.gerrit.server.git.BatchUpdate.Context;
import com.google.gerrit.server.git.UpdateException;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.notedb.ReviewerState;
import com.google.gerrit.server.patch.PatchListCache;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.query.change.ChangeData;
@@ -549,7 +549,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
ups.add(c);
addLabelDelta(normName, c.getValue());
categories.put(normName, c.getValue());
update.putReviewer(user.getAccountId(), ReviewerState.REVIEWER);
update.putReviewer(user.getAccountId(), REVIEWER);
update.putApproval(ent.getKey(), ent.getValue());
}
}

View File

@@ -15,6 +15,7 @@
package com.google.gerrit.server.git;
import static com.google.common.base.Preconditions.checkState;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
import static org.eclipse.jgit.lib.RefDatabase.ALL;
import com.google.common.base.Optional;
@@ -61,7 +62,6 @@ import com.google.gerrit.server.git.validators.MergeValidators;
import com.google.gerrit.server.index.ChangeIndexer;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.notedb.ReviewerState;
import com.google.gerrit.server.patch.PatchSetInfoFactory;
import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
import com.google.gerrit.server.project.ChangeControl;
@@ -1053,7 +1053,7 @@ public class MergeOp {
logDebug("Add approval for " + cd + " from user " + user);
ChangeUpdate update = updateFactory.create(control, timestamp);
update.putReviewer(user.getAccountId(), ReviewerState.REVIEWER);
update.putReviewer(user.getAccountId(), REVIEWER);
List<SubmitRecord> record = records.get(cd.change().getId());
if (record != null) {
update.merge(record);

View File

@@ -14,6 +14,8 @@
package com.google.gerrit.server.mail;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
import com.google.gerrit.common.errors.EmailException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountGroup;
@@ -26,7 +28,6 @@ import com.google.gerrit.reviewdb.client.PatchSetInfo;
import com.google.gerrit.reviewdb.client.StarredChange;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.mail.ProjectWatch.Watchers;
import com.google.gerrit.server.notedb.ReviewerState;
import com.google.gerrit.server.patch.PatchList;
import com.google.gerrit.server.patch.PatchListEntry;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
@@ -329,7 +330,7 @@ public abstract class ChangeEmail extends NotificationEmail {
/** Users who have non-zero approval codes on the change. */
protected void ccExistingReviewers() {
try {
for (Account.Id id : changeData.reviewers().get(ReviewerState.REVIEWER)) {
for (Account.Id id : changeData.reviewers().get(REVIEWER)) {
add(RecipientType.CC, id);
}
} catch (OrmException err) {

View File

@@ -14,13 +14,16 @@
package com.google.gerrit.server.mail;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.CC;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
import com.google.common.collect.Multimap;
import com.google.gerrit.common.FooterConstants;
import com.google.gerrit.common.errors.NoSuchAccountException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.server.account.AccountResolver;
import com.google.gerrit.server.notedb.ReviewerState;
import com.google.gerrit.server.notedb.ReviewerStateInternal;
import com.google.gwtorm.server.OrmException;
import org.eclipse.jgit.revwalk.FooterKey;
@@ -56,10 +59,10 @@ public class MailUtil {
}
public static MailRecipients getRecipientsFromReviewers(
Multimap<ReviewerState, Account.Id> reviewers) {
Multimap<ReviewerStateInternal, Account.Id> reviewers) {
MailRecipients recipients = new MailRecipients();
recipients.reviewers.addAll(reviewers.get(ReviewerState.REVIEWER));
recipients.cc.addAll(reviewers.get(ReviewerState.CC));
recipients.reviewers.addAll(reviewers.get(REVIEWER));
recipients.cc.addAll(reviewers.get(CC));
return recipients;
}

View File

@@ -116,7 +116,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
private final Change change;
private ImmutableListMultimap<PatchSet.Id, PatchSetApproval> approvals;
private ImmutableSetMultimap<ReviewerState, Account.Id> reviewers;
private ImmutableSetMultimap<ReviewerStateInternal, Account.Id> reviewers;
private ImmutableList<Account.Id> allPastReviewers;
private ImmutableList<SubmitRecord> submitRecords;
private ImmutableList<ChangeMessage> allChangeMessages;
@@ -144,7 +144,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
return approvals;
}
public ImmutableSetMultimap<ReviewerState, Account.Id> getReviewers() {
public ImmutableSetMultimap<ReviewerStateInternal, Account.Id> getReviewers() {
return reviewers;
}
@@ -270,9 +270,9 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
} else {
hashtags = ImmutableSet.of();
}
ImmutableSetMultimap.Builder<ReviewerState, Account.Id> reviewers =
ImmutableSetMultimap.Builder<ReviewerStateInternal, Account.Id> reviewers =
ImmutableSetMultimap.builder();
for (Map.Entry<Account.Id, ReviewerState> e
for (Map.Entry<Account.Id, ReviewerStateInternal> e
: parser.reviewers.entrySet()) {
reviewers.put(e.getValue(), e.getKey());
}

View File

@@ -71,7 +71,7 @@ import java.util.Map;
import java.util.Set;
class ChangeNotesParser implements AutoCloseable {
final Map<Account.Id, ReviewerState> reviewers;
final Map<Account.Id, ReviewerStateInternal> reviewers;
final List<Account.Id> allPastReviewers;
final List<SubmitRecord> submitRecords;
final Multimap<RevId, PatchLineComment> comments;
@@ -168,7 +168,7 @@ class ChangeNotesParser implements AutoCloseable {
parseApproval(psId, accountId, commit, line);
}
for (ReviewerState state : ReviewerState.values()) {
for (ReviewerStateInternal state : ReviewerStateInternal.values()) {
for (String line : commit.getFooterLines(state.getFooterKey())) {
parseReviewer(state, line);
}
@@ -394,7 +394,7 @@ class ChangeNotesParser implements AutoCloseable {
GERRIT_PLACEHOLDER_HOST, email);
}
private void parseReviewer(ReviewerState state, String line)
private void parseReviewer(ReviewerStateInternal state, String line)
throws ConfigInvalidException {
PersonIdent ident = RawParseUtils.parsePersonIdent(line);
if (ident == null) {
@@ -407,11 +407,11 @@ class ChangeNotesParser implements AutoCloseable {
}
private void pruneReviewers() {
Iterator<Map.Entry<Account.Id, ReviewerState>> rit =
Iterator<Map.Entry<Account.Id, ReviewerStateInternal>> rit =
reviewers.entrySet().iterator();
while (rit.hasNext()) {
Map.Entry<Account.Id, ReviewerState> e = rit.next();
if (e.getValue() == ReviewerState.REMOVED) {
Map.Entry<Account.Id, ReviewerStateInternal> e = rit.next();
if (e.getValue() == ReviewerStateInternal.REMOVED) {
rit.remove();
for (Table<Account.Id, ?, ?> curr : approvals.values()) {
curr.rowKeySet().remove(e.getKey());

View File

@@ -86,7 +86,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
private final AccountCache accountCache;
private final Map<String, Optional<Short>> approvals;
private final Map<Account.Id, ReviewerState> reviewers;
private final Map<Account.Id, ReviewerStateInternal> reviewers;
private Change.Status status;
private String subject;
private List<SubmitRecord> submitRecords;
@@ -320,13 +320,13 @@ public class ChangeUpdate extends AbstractChangeUpdate {
this.hashtags = hashtags;
}
public void putReviewer(Account.Id reviewer, ReviewerState type) {
checkArgument(type != ReviewerState.REMOVED, "invalid ReviewerType");
public void putReviewer(Account.Id reviewer, ReviewerStateInternal type) {
checkArgument(type != ReviewerStateInternal.REMOVED, "invalid ReviewerType");
reviewers.put(reviewer, type);
}
public void removeReviewer(Account.Id reviewer) {
reviewers.put(reviewer, ReviewerState.REMOVED);
reviewers.put(reviewer, ReviewerStateInternal.REMOVED);
}
/** @return the tree id for the updated tree */
@@ -422,7 +422,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
addFooter(msg, FOOTER_HASHTAGS, Joiner.on(",").join(hashtags));
}
for (Map.Entry<Account.Id, ReviewerState> e : reviewers.entrySet()) {
for (Map.Entry<Account.Id, ReviewerStateInternal> e : reviewers.entrySet()) {
Account account = accountCache.get(e.getKey()).getAccount();
PersonIdent ident = newIdent(account, when);
addFooter(msg, e.getValue().getFooterKey())

View File

@@ -14,26 +14,51 @@
package com.google.gerrit.server.notedb;
import com.google.gerrit.extensions.client.ReviewerState;
import org.eclipse.jgit.revwalk.FooterKey;
import java.util.Arrays;
/** State of a reviewer on a change. */
public enum ReviewerState {
public enum ReviewerStateInternal {
/** The user has contributed at least one nonzero vote on the change. */
REVIEWER(new FooterKey("Reviewer")),
REVIEWER(new FooterKey("Reviewer"), ReviewerState.REVIEWER),
/** The reviewer was added to the change, but has not voted. */
CC(new FooterKey("CC")),
CC(new FooterKey("CC"), ReviewerState.CC),
/** The user was previously a reviewer on the change, but was removed. */
REMOVED(new FooterKey("Removed"));
REMOVED(new FooterKey("Removed"), ReviewerState.REMOVED);
static {
boolean ok = true;
if (ReviewerStateInternal.values().length != ReviewerState.values().length) {
ok = false;
}
for (ReviewerStateInternal s : ReviewerStateInternal.values()) {
ok &= s.name().equals(s.state.name());
}
if (!ok) {
throw new IllegalStateException("Mismatched reviewer state mapping: "
+ Arrays.asList(ReviewerStateInternal.values()) + " != "
+ Arrays.asList(ReviewerState.values()));
}
}
private final FooterKey footerKey;
private final ReviewerState state;
private ReviewerState(FooterKey footerKey) {
private ReviewerStateInternal(FooterKey footerKey, ReviewerState state) {
this.footerKey = footerKey;
this.state = state;
}
FooterKey getFooterKey() {
return footerKey;
}
public ReviewerState asReviewerState() {
return state;
}
}

View File

@@ -45,7 +45,7 @@ import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.MergeUtil;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.NotesMigration;
import com.google.gerrit.server.notedb.ReviewerState;
import com.google.gerrit.server.notedb.ReviewerStateInternal;
import com.google.gerrit.server.patch.PatchList;
import com.google.gerrit.server.patch.PatchListCache;
import com.google.gerrit.server.patch.PatchListEntry;
@@ -707,7 +707,7 @@ public class ChangeData {
return allApprovals;
}
public SetMultimap<ReviewerState, Account.Id> reviewers()
public SetMultimap<ReviewerStateInternal, Account.Id> reviewers()
throws OrmException {
return approvalsUtil.getReviewers(notes(), approvals().values());
}

View File

@@ -15,8 +15,8 @@
package com.google.gerrit.server.notedb;
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.server.notedb.ReviewerState.CC;
import static com.google.gerrit.server.notedb.ReviewerState.REVIEWER;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.CC;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
import static com.google.gerrit.testutil.TestChanges.incrementPatchSet;
import static java.nio.charset.StandardCharsets.UTF_8;

View File

@@ -15,8 +15,8 @@
package com.google.gerrit.server.notedb;
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.server.notedb.ReviewerState.CC;
import static com.google.gerrit.server.notedb.ReviewerState.REVIEWER;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.CC;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.common.TimeUtil;