Add an ApprovalsUtil method for getting reviewers

Various different places in the code were replicating the behavior
that a user with nonzero votes on some patch set of a change is
called a "reviewer" and a user with only zero votes is called a "cc".
Consolidate this in ApprovalsUtil, using an explicit enum of reviewer
states.

This also happens to eliminate most uses of
patchSetApprovals().byChange().

Change-Id: I1836de83e86145a20a3c100f19a31f98bb3ca64b

gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java

@@ -14,9 +14,15 @@
 
 package com.google.gerrit.server;
 
+import static com.google.common.base.Preconditions.checkArgument;
+
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
+import com.google.common.collect.LinkedHashMultimap;
 import com.google.common.collect.Lists;
+import com.google.common.collect.Multimaps;
+import com.google.common.collect.SetMultimap;
 import com.google.common.collect.Sets;
 import com.google.gerrit.common.data.LabelType;
 import com.google.gerrit.common.data.LabelTypes;
@@ -32,6 +38,7 @@ import com.google.gerrit.server.util.TimeUtil;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 
+import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
 import java.util.Set;
@@ -49,8 +56,60 @@ import java.util.Set;
  * The methods in this class do not begin/commit transactions.
  */
 public class ApprovalsUtil {
+  @VisibleForTesting
   @Inject
-  ApprovalsUtil() {
+  public ApprovalsUtil() {
+  }
+
+  public static enum ReviewerState {
+    REVIEWER, CC;
+  }
+
+  /**
+   * Get all reviewers for a change.
+   *
+   * @param db review database.
+   * @param changeId change ID.
+   * @return multimap of reviewers keyed by state, where each account appears
+   *     exactly once in {@link SetMultimap#values()}.
+   * @throws OrmException if reviewers for the change could not be read.
+   */
+  public SetMultimap<ReviewerState, Account.Id> getReviewers(ReviewDb db,
+      Change.Id changeId) throws OrmException {
+    return getReviewers(db.patchSetApprovals().byChange(changeId));
+  }
+
+  /**
+   * Get all reviewers for a change.
+   *
+   * @param allApprovals all approvals to consider; must all belong to the same
+   *     change.
+   * @return multimap of reviewers keyed by state, where each account appears
+   *     exactly once in {@link SetMultimap#values()}.
+   */
+  public static SetMultimap<ReviewerState, Account.Id> getReviewers(
+      Iterable<PatchSetApproval> allApprovals) {
+    PatchSetApproval first = null;
+    SetMultimap<ReviewerState, Account.Id> reviewers =
+        LinkedHashMultimap.create();
+    for (PatchSetApproval psa : allApprovals) {
+      if (first == null) {
+        first = psa;
+      } else {
+        checkArgument(
+            first.getKey().getParentKey().getParentKey().equals(
+              psa.getKey().getParentKey().getParentKey()),
+            "multiple change IDs: %s, %s", first.getKey(), psa.getKey());
+      }
+      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);
+      }
+    }
+    return Multimaps.unmodifiableSetMultimap(reviewers);
   }
 
   /**
@@ -98,8 +157,8 @@ public class ApprovalsUtil {
 
   public List<PatchSetApproval> addReviewers(ReviewDb db, LabelTypes labelTypes,
       Change change, PatchSet ps, PatchSetInfo info,
-      Iterable<Account.Id> wantReviewers, Set<Account.Id> existingReviewers)
-      throws OrmException {
+      Iterable<Account.Id> wantReviewers,
+      Collection<Account.Id> existingReviewers) throws OrmException {
     return addReviewers(db, labelTypes, change, ps.getId(), ps.isDraft(),
         info.getAuthor().getAccount(), info.getCommitter().getAccount(),
         wantReviewers, existingReviewers);
@@ -119,8 +178,8 @@ public class ApprovalsUtil {
   private List<PatchSetApproval> addReviewers(ReviewDb db,
       LabelTypes labelTypes, Change change, PatchSet.Id psId, boolean isDraft,
       Account.Id authorId, Account.Id committerId,
-      Iterable<Account.Id> wantReviewers, Set<Account.Id> existingReviewers)
-      throws OrmException {
+      Iterable<Account.Id> wantReviewers,
+      Collection<Account.Id> existingReviewers) throws OrmException {
     List<LabelType> allTypes = labelTypes.getLabelTypes();
     if (allTypes.isEmpty()) {
       return ImmutableList.of();

gerrit-server/src/main/java/com/google/gerrit/server/change/ListReviewers.java

@@ -18,8 +18,8 @@ import com.google.common.collect.Maps;
 import com.google.gerrit.extensions.restapi.RestReadView;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Change;
-import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.ApprovalsUtil;
 import com.google.gerrit.server.change.ReviewerJson.ReviewerInfo;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
@@ -30,14 +30,17 @@ import java.util.Map;
 
 class ListReviewers implements RestReadView<ChangeResource> {
   private final Provider<ReviewDb> dbProvider;
+  private final ApprovalsUtil approvalsUtil;
   private final ReviewerJson json;
   private final ReviewerResource.Factory resourceFactory;
 
   @Inject
   ListReviewers(Provider<ReviewDb> dbProvider,
+      ApprovalsUtil approvalsUtil,
       ReviewerResource.Factory resourceFactory,
       ReviewerJson json) {
     this.dbProvider = dbProvider;
+    this.approvalsUtil = approvalsUtil;
     this.resourceFactory = resourceFactory;
     this.json = json;
   }
@@ -47,9 +50,8 @@ class ListReviewers implements RestReadView<ChangeResource> {
     Map<Account.Id, ReviewerResource> reviewers = Maps.newLinkedHashMap();
     ReviewDb db = dbProvider.get();
     Change.Id changeId = rsrc.getChange().getId();
-    for (PatchSetApproval patchSetApproval
-         : db.patchSetApprovals().byChange(changeId)) {
-      Account.Id accountId = patchSetApproval.getAccountId();
+    for (Account.Id accountId
+        : approvalsUtil.getReviewers(db, changeId).values()) {
       if (!reviewers.containsKey(accountId)) {
         reviewers.put(accountId, resourceFactory.create(rsrc, accountId));
       }

gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java

@@ -17,18 +17,18 @@ package com.google.gerrit.server.change;
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
 
-import com.google.common.collect.Sets;
+import com.google.common.collect.SetMultimap;
 import com.google.common.util.concurrent.CheckedFuture;
 import com.google.gerrit.common.ChangeHooks;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.ChangeMessage;
 import com.google.gerrit.reviewdb.client.PatchSet;
-import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.client.PatchSetInfo;
 import com.google.gerrit.reviewdb.client.RevId;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.ApprovalsUtil;
+import com.google.gerrit.server.ApprovalsUtil.ReviewerState;
 import com.google.gerrit.server.ChangeUtil;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.events.CommitReceivedEvent;
@@ -59,8 +59,6 @@ import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
 import java.util.Collections;
-import java.util.List;
-import java.util.Set;
 
 public class PatchSetInserter {
   private static final Logger log =
@@ -230,17 +228,9 @@ public class PatchSetInserter {
       ChangeUtil.insertAncestors(db, patchSet.getId(), commit);
       db.patchSets().insert(Collections.singleton(patchSet));
 
-      final List<PatchSetApproval> oldPatchSetApprovals =
-          db.patchSetApprovals().byChange(change.getId()).toList();
-      final Set<Account.Id> oldReviewers = Sets.newHashSet();
-      final Set<Account.Id> oldCC = Sets.newHashSet();
-      for (PatchSetApproval a : oldPatchSetApprovals) {
-        if (a.getValue() != 0) {
-          oldReviewers.add(a.getAccountId());
-        } else {
-          oldCC.add(a.getAccountId());
-        }
-      }
+      SetMultimap<ReviewerState, Account.Id> oldReviewers = sendMail
+          ? approvalsUtil.getReviewers(db, change.getId())
+          : null;
 
       updatedChange =
           db.changes().atomicUpdate(change.getId(), new AtomicUpdate<Change>() {
@@ -293,8 +283,8 @@ public class PatchSetInserter {
           cm.setFrom(user.getAccountId());
           cm.setPatchSet(patchSet, info);
           cm.setChangeMessage(changeMessage);
-          cm.addReviewers(oldReviewers);
-          cm.addExtraCC(oldCC);
+          cm.addReviewers(oldReviewers.get(ReviewerState.REVIEWER));
+          cm.addExtraCC(oldReviewers.get(ReviewerState.CC));
           cm.send();
         } catch (Exception err) {
           log.error("Cannot send email for new patch set on change " + updatedChange.getId(),

gerrit-server/src/main/java/com/google/gerrit/server/change/Reviewers.java

@@ -14,7 +14,6 @@
 
 package com.google.gerrit.server.change;
 
-import com.google.common.collect.Sets;
 import com.google.gerrit.extensions.registration.DynamicMap;
 import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.ChildCollection;
@@ -23,30 +22,33 @@ import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.RestView;
 import com.google.gerrit.extensions.restapi.TopLevelResource;
 import com.google.gerrit.reviewdb.client.Account;
-import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.ApprovalsUtil;
 import com.google.gerrit.server.account.AccountsCollection;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 
-import java.util.Set;
+import java.util.Collection;
 
 public class Reviewers implements
     ChildCollection<ChangeResource, ReviewerResource> {
   private final DynamicMap<RestView<ReviewerResource>> views;
   private final Provider<ReviewDb> dbProvider;
+  private final ApprovalsUtil approvalsUtil;
   private final AccountsCollection accounts;
   private final ReviewerResource.Factory resourceFactory;
   private final Provider<ListReviewers> list;
 
   @Inject
   Reviewers(Provider<ReviewDb> dbProvider,
+      ApprovalsUtil approvalsUtil,
       AccountsCollection accounts,
       ReviewerResource.Factory resourceFactory,
       DynamicMap<RestView<ReviewerResource>> views,
       Provider<ListReviewers> list) {
     this.dbProvider = dbProvider;
+    this.approvalsUtil = approvalsUtil;
     this.accounts = accounts;
     this.resourceFactory = resourceFactory;
     this.views = views;
@@ -76,13 +78,9 @@ public class Reviewers implements
     throw new ResourceNotFoundException(id);
   }
 
-  private Set<Account.Id> fetchAccountIds(ChangeResource rsrc)
+  private Collection<Account.Id> fetchAccountIds(ChangeResource rsrc)
       throws OrmException {
-    Set<Account.Id> accountIds = Sets.newHashSet();
-    for (PatchSetApproval a
-         : dbProvider.get().patchSetApprovals().byChange(rsrc.getChange().getId())) {
-      accountIds.add(a.getAccountId());
-    }
-    return accountIds;
+    return approvalsUtil.getReviewers(
+        dbProvider.get(), rsrc.getChange().getId()).values();
   }
 }

gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java

@@ -16,7 +16,6 @@ package com.google.gerrit.server.events;
 
 import com.google.common.collect.Lists;
 import com.google.common.collect.Multimap;
-import com.google.common.collect.Sets;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.common.data.LabelType;
 import com.google.gerrit.common.data.LabelTypes;
@@ -33,6 +32,7 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.client.RevId;
 import com.google.gerrit.reviewdb.client.UserIdentity;
 import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.ApprovalsUtil;
 import com.google.gerrit.server.GerritPersonIdent;
 import com.google.gerrit.server.account.AccountCache;
 import com.google.gerrit.server.config.CanonicalWebUrl;
@@ -71,7 +71,6 @@ import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
 import java.util.Map;
-import java.util.Set;
 
 @Singleton
 public class EventFactory {
@@ -84,6 +83,7 @@ public class EventFactory {
   private final PersonIdent myIdent;
   private final Provider<ReviewDb> db;
   private final GitRepositoryManager repoManager;
+  private final ApprovalsUtil approvalsUtil;
 
   @Inject
   EventFactory(AccountCache accountCache,
@@ -91,7 +91,8 @@ public class EventFactory {
       PatchSetInfoFactory psif,
       PatchListCache patchListCache, SchemaFactory<ReviewDb> schema,
       @GerritPersonIdent PersonIdent myIdent,
-      Provider<ReviewDb> db, GitRepositoryManager repoManager) {
+      Provider<ReviewDb> db, GitRepositoryManager repoManager,
+      ApprovalsUtil approvalsUtil) {
     this.accountCache = accountCache;
     this.urlProvider = urlProvider;
     this.patchListCache = patchListCache;
@@ -100,6 +101,7 @@ public class EventFactory {
     this.myIdent = myIdent;
     this.db = db;
     this.repoManager = repoManager;
+    this.approvalsUtil = approvalsUtil;
   }
 
   /**
@@ -168,17 +170,12 @@ public class EventFactory {
    */
   public void addAllReviewers(ChangeAttribute a, Change change)
       throws OrmException {
-    List<PatchSetApproval> approvals =
-        db.get().patchSetApprovals().byChange(change.getId()).toList();
-    if (!approvals.isEmpty()) {
-      a.allReviewers = Lists.newArrayList();
-      Set<Account.Id> seen = Sets.newHashSet();
-      for (PatchSetApproval psa : approvals) {
-        Account.Id id = psa.getAccountId();
-        if (!seen.contains(id)) {
-          seen.add(id);
-          a.allReviewers.add(asAccountAttribute(id));
-        }
+    Collection<Account.Id> reviewers =
+        approvalsUtil.getReviewers(db.get(), change.getId()).values();
+    if (!reviewers.isEmpty()) {
+      a.allReviewers = Lists.newArrayListWithCapacity(reviewers.size());
+      for (Account.Id id : reviewers) {
+        a.allReviewers.add(asAccountAttribute(id));
       }
     }
   }

gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java

@@ -16,8 +16,9 @@ package com.google.gerrit.server.git;
 
 import static com.google.gerrit.reviewdb.client.RefNames.REFS_CHANGES;
 import static com.google.gerrit.server.git.MultiProgressMonitor.UNKNOWN;
-import static com.google.gerrit.server.mail.MailUtil.getRecipientsFromApprovals;
 import static com.google.gerrit.server.mail.MailUtil.getRecipientsFromFooters;
+import static com.google.gerrit.server.mail.MailUtil.getRecipientsFromReviewers;
+
 import static org.eclipse.jgit.lib.Constants.R_HEADS;
 import static org.eclipse.jgit.lib.RefDatabase.ALL;
 import static org.eclipse.jgit.transport.ReceiveCommand.Result.NOT_ATTEMPTED;
@@ -63,6 +64,7 @@ import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.reviewdb.client.RevId;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.ApprovalsUtil;
+import com.google.gerrit.server.ApprovalsUtil.ReviewerState;
 import com.google.gerrit.server.ChangeUtil;
 import com.google.gerrit.server.GerritPersonIdent;
 import com.google.gerrit.server.IdentifiedUser;
@@ -1859,8 +1861,9 @@ public class ReceiveCommits {
 
         List<PatchSetApproval> oldChangeApprovals =
             db.patchSetApprovals().byChange(change.getId()).toList();
-        final MailRecipients oldRecipients = getRecipientsFromApprovals(
-            oldChangeApprovals);
+        SetMultimap<ReviewerState, Account.Id> reviewers =
+            ApprovalsUtil.getReviewers(oldChangeApprovals);
+        MailRecipients oldRecipients = getRecipientsFromReviewers(reviewers);
         approvalsUtil.copyLabels(db, labelTypes, oldChangeApprovals,
             priorPatchSet, newPatchSet, changeKind);
         approvalsUtil.addReviewers(db, labelTypes, change, newPatchSet, info,

gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java

@@ -14,6 +14,7 @@
 
 package com.google.gerrit.server.mail;
 
+import com.google.common.collect.SetMultimap;
 import com.google.gerrit.common.errors.EmailException;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.AccountGroup;
@@ -22,9 +23,9 @@ import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.ChangeMessage;
 import com.google.gerrit.reviewdb.client.Patch;
 import com.google.gerrit.reviewdb.client.PatchSet;
-import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.client.PatchSetInfo;
 import com.google.gerrit.reviewdb.client.StarredChange;
+import com.google.gerrit.server.ApprovalsUtil.ReviewerState;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.mail.ProjectWatch.Watchers;
 import com.google.gerrit.server.patch.PatchList;
@@ -65,6 +66,8 @@ public abstract class ChangeEmail extends NotificationEmail {
   protected Set<Account.Id> authors;
   protected boolean emailOnlyAuthors;
 
+  private SetMultimap<ReviewerState, Account.Id> reviewers;
+
   protected ChangeEmail(EmailArguments ea, Change c, String mc) {
     super(ea, mc, c.getProject(), c.getDest());
     change = c;
@@ -93,19 +96,22 @@ public abstract class ChangeEmail extends NotificationEmail {
     changeMessage = cm;
   }
 
+  private SetMultimap<ReviewerState, Account.Id> getReviewers()
+      throws OrmException {
+    if (reviewers == null) {
+      reviewers =
+          args.approvalsUtil.getReviewers(args.db.get(), change.getId());
+    }
+    return reviewers;
+  }
+
   /** Format the message body by calling {@link #appendText(String)}. */
   protected void format() throws EmailException {
     formatChange();
     appendText(velocifyFile("ChangeFooter.vm"));
     try {
-      HashSet<Account.Id> reviewers = new HashSet<Account.Id>();
-      for (PatchSetApproval p : args.db.get().patchSetApprovals().byChange(
-          change.getId())) {
-        reviewers.add(p.getAccountId());
-      }
-
       TreeSet<String> names = new TreeSet<String>();
-      for (Account.Id who : reviewers) {
+      for (Account.Id who : getReviewers().values()) {
         names.add(getNameEmailFor(who));
       }
 
@@ -310,31 +316,23 @@ public abstract class ChangeEmail extends NotificationEmail {
 
   /** Any user who has published comments on this change. */
   protected void ccAllApprovals() {
-    ccApprovals(true);
+    try {
+      for (Account.Id id : getReviewers().values()) {
+        add(RecipientType.CC, id);
+      }
+    } catch (OrmException err) {
+      log.warn("Cannot CC users that reviewed updated change", err);
+    }
   }
 
   /** Users who have non-zero approval codes on the change. */
   protected void ccExistingReviewers() {
-    ccApprovals(false);
-  }
-
-  private void ccApprovals(final boolean includeZero) {
     try {
-      // CC anyone else who has posted an approval mark on this change
-      //
-      for (PatchSetApproval ap : args.db.get().patchSetApprovals().byChange(
-          change.getId())) {
-        if (!includeZero && ap.getValue() == 0) {
-          continue;
-        }
-        add(RecipientType.CC, ap.getAccountId());
+      for (Account.Id id : getReviewers().get(ReviewerState.REVIEWER)) {
+        add(RecipientType.CC, id);
       }
     } catch (OrmException err) {
-      if (includeZero) {
-        log.warn("Cannot CC users that commented on updated change", err);
-      } else {
-        log.warn("Cannot CC users that reviewed updated change", err);
-      }
+      log.warn("Cannot CC users that commented on updated change", err);
     }
   }
 

gerrit-server/src/main/java/com/google/gerrit/server/mail/EmailArguments.java

@@ -17,6 +17,7 @@ package com.google.gerrit.server.mail;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.AnonymousUser;
+import com.google.gerrit.server.ApprovalsUtil;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.IdentifiedUser.GenericFactory;
 import com.google.gerrit.server.account.AccountCache;
@@ -46,6 +47,7 @@ class EmailArguments {
   final GroupIncludeCache groupIncludes;
   final AccountCache accountCache;
   final PatchListCache patchListCache;
+  final ApprovalsUtil approvalsUtil;
   final FromAddressGenerator fromAddressGenerator;
   final EmailSender emailSender;
   final PatchSetInfoFactory patchSetInfoFactory;
@@ -66,7 +68,9 @@ class EmailArguments {
   EmailArguments(GitRepositoryManager server, ProjectCache projectCache,
       GroupBackend groupBackend, GroupIncludeCache groupIncludes,
       AccountCache accountCache,
-      PatchListCache patchListCache, FromAddressGenerator fromAddressGenerator,
+      PatchListCache patchListCache,
+      ApprovalsUtil approvalsUtil,
+      FromAddressGenerator fromAddressGenerator,
       EmailSender emailSender, PatchSetInfoFactory patchSetInfoFactory,
       GenericFactory identifiedUserFactory,
       CapabilityControl.Factory capabilityControlFactory,
@@ -85,6 +89,7 @@ class EmailArguments {
     this.groupIncludes = groupIncludes;
     this.accountCache = accountCache;
     this.patchListCache = patchListCache;
+    this.approvalsUtil = approvalsUtil;
     this.fromAddressGenerator = fromAddressGenerator;
     this.emailSender = emailSender;
     this.patchSetInfoFactory = patchSetInfoFactory;

gerrit-server/src/main/java/com/google/gerrit/server/mail/MailUtil.java

@@ -14,10 +14,11 @@
 
 package com.google.gerrit.server.mail;
 
+import com.google.common.collect.Multimap;
 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.reviewdb.client.PatchSetApproval;
+import com.google.gerrit.server.ApprovalsUtil.ReviewerState;
 import com.google.gerrit.server.account.AccountResolver;
 import com.google.gwtorm.server.OrmException;
 
@@ -55,16 +56,11 @@ public class MailUtil {
     return recipients;
   }
 
-  public static MailRecipients getRecipientsFromApprovals(
-      final List<PatchSetApproval> approvals) {
-    final MailRecipients recipients = new MailRecipients();
-    for (PatchSetApproval a : approvals) {
-      if (a.getValue() != 0) {
-        recipients.reviewers.add(a.getAccountId());
-      } else {
-        recipients.cc.add(a.getAccountId());
-      }
-    }
+  public static MailRecipients getRecipientsFromReviewers(
+      Multimap<ReviewerState, Account.Id> reviewers) {
+    MailRecipients recipients = new MailRecipients();
+    recipients.reviewers.addAll(reviewers.get(ReviewerState.REVIEWER));
+    recipients.cc.addAll(reviewers.get(ReviewerState.CC));
     return recipients;
   }
 

gerrit-server/src/main/java/com/google/gerrit/server/mail/PatchSetNotificationSender.java

@@ -14,7 +14,6 @@
 
 package com.google.gerrit.server.mail;
 
-import static com.google.gerrit.server.mail.MailUtil.getRecipientsFromApprovals;
 import static com.google.gerrit.server.mail.MailUtil.getRecipientsFromFooters;
 
 import com.google.gerrit.common.ChangeHooks;
@@ -23,7 +22,6 @@ import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.ChangeMessage;
 import com.google.gerrit.reviewdb.client.PatchSet;
-import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.client.PatchSetInfo;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.ApprovalsUtil;
@@ -117,14 +115,9 @@ public class PatchSetNotificationSender {
           log.error("Cannot send email for new change " + updatedChange.getId(), e);
         }
       } else {
-        final List<PatchSetApproval> patchSetApprovals =
-            db.patchSetApprovals().byChange(
-                updatedChange.getId()).toList();
-        final MailRecipients oldRecipients =
-            getRecipientsFromApprovals(patchSetApprovals);
         approvalsUtil.addReviewers(db, labelTypes, updatedChange,
             updatedPatchSet, info, recipients.getReviewers(),
-            oldRecipients.getAll());
+            approvalsUtil.getReviewers(db, updatedChange.getId()).values());
         final ChangeMessage msg =
             new ChangeMessage(new ChangeMessage.Key(updatedChange.getId(),
                 ChangeUtil.messageUUID(db)), me,

gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java

@@ -28,6 +28,7 @@ import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.ApprovalsUtil;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.query.change.ChangeData;
@@ -48,6 +49,7 @@ import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
 
@@ -179,16 +181,19 @@ public class ChangeControl {
     }
   }
 
+  private final ApprovalsUtil approvalsUtil;
   private final RefControl refControl;
   private final Change change;
 
-  ChangeControl(final RefControl r, final Change c) {
+  ChangeControl(ApprovalsUtil au, RefControl r, Change c) {
+    this.approvalsUtil = au;
     this.refControl = r;
     this.change = c;
   }
 
   public ChangeControl forUser(final CurrentUser who) {
-    return new ChangeControl(getRefControl().forUser(who), getChange());
+    return new ChangeControl(approvalsUtil, getRefControl().forUser(who),
+        getChange());
   }
 
   public RefControl getRefControl() {
@@ -323,18 +328,11 @@ public class ChangeControl {
   public boolean isReviewer(ReviewDb db, @Nullable ChangeData cd)
       throws OrmException {
     if (getCurrentUser().isIdentifiedUser()) {
-      final IdentifiedUser user = (IdentifiedUser) getCurrentUser();
-      Iterable<PatchSetApproval> results;
-      if (cd != null) {
-        results = cd.currentApprovals(Providers.of(db));
-      } else {
-        results = db.patchSetApprovals().byChange(change.getId());
-      }
-      for (PatchSetApproval approval : results) {
-        if (user.getAccountId().equals(approval.getAccountId())) {
-          return true;
-        }
-      }
+      Collection<Account.Id> results = cd != null
+          ? cd.reviewers(Providers.of(db)).values()
+          : approvalsUtil.getReviewers(db, change.getId()).values();
+      IdentifiedUser user = (IdentifiedUser) getCurrentUser();
+      return results.contains(user.getAccountId());
     }
     return false;
   }

gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java

@@ -30,6 +30,7 @@ import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.reviewdb.client.Branch;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.server.ApprovalsUtil;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.InternalUser;
@@ -145,6 +146,7 @@ public class ProjectControl {
   private final CurrentUser user;
   private final ProjectState state;
   private final GitRepositoryManager repoManager;
+  private final ApprovalsUtil approvalsUtil;
   private final PermissionCollection.Factory permissionFilter;
   private final Collection<ContributorAgreement> contributorAgreements;
 
@@ -157,11 +159,15 @@ public class ProjectControl {
   @Inject
   ProjectControl(@GitUploadPackGroups Set<AccountGroup.UUID> uploadGroups,
       @GitReceivePackGroups Set<AccountGroup.UUID> receiveGroups,
-      final ProjectCache pc, final PermissionCollection.Factory permissionFilter,
-      final GitRepositoryManager repoManager,
-      @CanonicalWebUrl @Nullable final String canonicalWebUrl,
-      @Assisted CurrentUser who, @Assisted ProjectState ps) {
+      ProjectCache pc,
+      PermissionCollection.Factory permissionFilter,
+      GitRepositoryManager repoManager,
+      ApprovalsUtil approvalsUtil,
+      @CanonicalWebUrl @Nullable String canonicalWebUrl,
+      @Assisted CurrentUser who,
+      @Assisted ProjectState ps) {
     this.repoManager = repoManager;
+    this.approvalsUtil = approvalsUtil;
     this.uploadGroups = uploadGroups;
     this.receiveGroups = receiveGroups;
     this.permissionFilter = permissionFilter;
@@ -179,7 +185,8 @@ public class ProjectControl {
   }
 
   public ChangeControl controlFor(final Change change) {
-    return new ChangeControl(controlForRef(change.getDest()), change);
+    return new ChangeControl(approvalsUtil, controlForRef(change.getDest()),
+        change);
   }
 
   public RefControl controlForRef(Branch.NameKey ref) {

gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java

@@ -22,8 +22,10 @@ import com.google.common.collect.ListMultimap;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Maps;
 import com.google.common.collect.Ordering;
+import com.google.common.collect.SetMultimap;
 import com.google.common.collect.Sets;
 import com.google.gerrit.common.data.SubmitRecord;
+import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.ChangeMessage;
 import com.google.gerrit.reviewdb.client.Patch;
@@ -31,6 +33,8 @@ import com.google.gerrit.reviewdb.client.PatchLineComment;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.ApprovalsUtil;
+import com.google.gerrit.server.ApprovalsUtil.ReviewerState;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.patch.PatchList;
@@ -473,6 +477,11 @@ public class ChangeData {
     return allApprovals;
   }
 
+  public SetMultimap<ReviewerState, Account.Id> reviewers(Provider<ReviewDb> db)
+      throws OrmException {
+    return ApprovalsUtil.getReviewers(allApprovals(db));
+  }
+
   public Collection<PatchLineComment> comments(Provider<ReviewDb> db)
       throws OrmException {
     if (comments == null) {