ChangeControl visibility takes draft into account

isVisible() in ChangeControl now considers whether or not the change
is a draft. As ReviewDb is needed to see whether or not a user has
visibility for a certain change, this change also refactors most
uses of isVisible() to also pass in ReviewDb. As a bonus, much of the
notification routines seem to work as wanted, due to the new visibility
checks.

Change-Id: I4c3b068413e4db30edb23f498f61048142cb6713

gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/ChangeListServiceImpl.java

@@ -102,9 +102,9 @@ public class ChangeListServiceImpl extends BaseServiceImplementation implements
     this.queryRewriter = queryRewriter;
   }
 
-  private boolean canRead(final Change c) {
+  private boolean canRead(final Change c, final ReviewDb db) throws OrmException {
     try {
-      return changeControlFactory.controlFor(c).isVisible();
+      return changeControlFactory.controlFor(c).isVisible(db);
     } catch (NoSuchChangeException e) {
       return false;
     }
@@ -187,7 +187,7 @@ public class ChangeListServiceImpl extends BaseServiceImplementation implements
         //
         if (!want.isEmpty()) {
           for (Change c : db.changes().get(want)) {
-            if (canRead(c)) {
+            if (canRead(c, db)) {
               r.add(c);
             }
           }
@@ -236,20 +236,20 @@ public class ChangeListServiceImpl extends BaseServiceImplementation implements
         }
 
         d = new AccountDashboardInfo(target);
-        d.setByOwner(filter(changes.byOwnerOpen(target), stars, ac));
+        d.setByOwner(filter(changes.byOwnerOpen(target), stars, ac, db));
-        d.setClosed(filter(changes.byOwnerClosed(target), stars, ac));
+        d.setClosed(filter(changes.byOwnerClosed(target), stars, ac, db));
 
         for (final ChangeInfo c : d.getByOwner()) {
           openReviews.remove(c.getId());
         }
-        d.setForReview(filter(changes.get(openReviews), stars, ac));
+        d.setForReview(filter(changes.get(openReviews), stars, ac, db));
         Collections.sort(d.getForReview(), ID_COMP);
 
         for (final ChangeInfo c : d.getClosed()) {
           closedReviews.remove(c.getId());
         }
         if (!closedReviews.isEmpty()) {
-          d.getClosed().addAll(filter(changes.get(closedReviews), stars, ac));
+          d.getClosed().addAll(filter(changes.get(closedReviews), stars, ac, db));
           Collections.sort(d.getClosed(), SORT_KEY_COMP);
         }
 
@@ -304,10 +304,11 @@ public class ChangeListServiceImpl extends BaseServiceImplementation implements
   }
 
   private List<ChangeInfo> filter(final ResultSet<Change> rs,
-      final Set<Change.Id> starred, final AccountInfoCacheFactory accts) {
+      final Set<Change.Id> starred, final AccountInfoCacheFactory accts,
+      final ReviewDb db) throws OrmException {
     final ArrayList<ChangeInfo> r = new ArrayList<ChangeInfo>();
     for (final Change c : rs) {
-      if (canRead(c)) {
+      if (canRead(c, db)) {
         final ChangeInfo ci = new ChangeInfo(c);
         accts.want(ci.getOwner());
         ci.setStarred(starred.contains(ci.getId()));

gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeDetailFactory.java

@@ -116,7 +116,7 @@ public class ChangeDetailFactory extends Handler<ChangeDetail> {
 
     detail = new ChangeDetail();
     detail.setChange(change);
-    detail.setAllowsAnonymous(control.forUser(anonymousUser).isVisible());
+    detail.setAllowsAnonymous(control.forUser(anonymousUser).isVisible(db));
 
     detail.setCanAbandon(change.getStatus().isOpen() && control.canAbandon());
     detail.setCanRestore(change.getStatus() == Change.Status.ABANDONED && control.canRestore());

gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java

@@ -24,6 +24,7 @@ import com.google.gerrit.reviewdb.Change;
 import com.google.gerrit.reviewdb.ContributorAgreement;
 import com.google.gerrit.reviewdb.PatchSet;
 import com.google.gerrit.reviewdb.Project;
+import com.google.gerrit.reviewdb.ReviewDb;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.account.AccountCache;
 import com.google.gerrit.server.account.AccountState;
@@ -44,6 +45,7 @@ import com.google.gerrit.server.git.WorkQueue;
 import com.google.gerrit.server.project.ProjectCache;
 import com.google.gerrit.server.project.ProjectControl;
 import com.google.gerrit.server.project.ProjectState;
+import com.google.gwtorm.client.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
 
@@ -219,15 +221,17 @@ public class ChangeHookRunner {
      *
      * @param change The change itself.
      * @param patchSet The Patchset that was created.
+     * @throws OrmException
      */
-    public void doPatchsetCreatedHook(final Change change, final PatchSet patchSet) {
+    public void doPatchsetCreatedHook(final Change change, final PatchSet patchSet,
+          final ReviewDb db) throws OrmException {
         final PatchSetCreatedEvent event = new PatchSetCreatedEvent();
         final AccountState uploader = accountCache.get(patchSet.getUploader());
 
         event.change = eventFactory.asChangeAttribute(change);
         event.patchSet = eventFactory.asPatchSetAttribute(patchSet);
         event.uploader = eventFactory.asAccountAttribute(uploader.getAccount());
-        fireEvent(change, event);
+        fireEvent(change, event, db);
 
         final List<String> args = new ArrayList<String>();
         addArg(args, "--change", event.change.id);
@@ -249,8 +253,11 @@ public class ChangeHookRunner {
      * @param account The gerrit user who commited the change.
      * @param comment The comment given.
      * @param approvals Map of Approval Categories and Scores
+     * @throws OrmException
      */
-    public void doCommentAddedHook(final Change change, final Account account, final PatchSet patchSet, final String comment, final Map<ApprovalCategory.Id, ApprovalCategoryValue.Id> approvals) {
+    public void doCommentAddedHook(final Change change, final Account account,
+          final PatchSet patchSet, final String comment, final Map<ApprovalCategory.Id,
+          ApprovalCategoryValue.Id> approvals, final ReviewDb db) throws OrmException {
         final CommentAddedEvent event = new CommentAddedEvent();
 
         event.change = eventFactory.asChangeAttribute(change);
@@ -266,7 +273,7 @@ public class ChangeHookRunner {
             }
         }
 
-        fireEvent(change, event);
+        fireEvent(change, event, db);
 
         final List<String> args = new ArrayList<String>();
         addArg(args, "--change", event.change.id);
@@ -289,14 +296,16 @@ public class ChangeHookRunner {
      * @param change The change itself.
      * @param account The gerrit user who commited the change.
      * @param patchSet The patchset that was merged.
+     * @throws OrmException
      */
-    public void doChangeMergedHook(final Change change, final Account account, final PatchSet patchSet) {
+    public void doChangeMergedHook(final Change change, final Account account,
+          final PatchSet patchSet, final ReviewDb db) throws OrmException {
         final ChangeMergedEvent event = new ChangeMergedEvent();
 
         event.change = eventFactory.asChangeAttribute(change);
         event.submitter = eventFactory.asAccountAttribute(account);
         event.patchSet = eventFactory.asPatchSetAttribute(patchSet);
-        fireEvent(change, event);
+        fireEvent(change, event, db);
 
         final List<String> args = new ArrayList<String>();
         addArg(args, "--change", event.change.id);
@@ -315,14 +324,16 @@ public class ChangeHookRunner {
      * @param change The change itself.
      * @param account The gerrit user who abandoned the change.
      * @param reason Reason for abandoning the change.
+     * @throws OrmException
      */
-    public void doChangeAbandonedHook(final Change change, final Account account, final String reason) {
+    public void doChangeAbandonedHook(final Change change, final Account account,
+          final String reason, final ReviewDb db) throws OrmException {
         final ChangeAbandonedEvent event = new ChangeAbandonedEvent();
 
         event.change = eventFactory.asChangeAttribute(change);
         event.abandoner = eventFactory.asAccountAttribute(account);
         event.reason = reason;
-        fireEvent(change, event);
+        fireEvent(change, event, db);
 
         final List<String> args = new ArrayList<String>();
         addArg(args, "--change", event.change.id);
@@ -341,14 +352,16 @@ public class ChangeHookRunner {
      * @param change The change itself.
      * @param account The gerrit user who restored the change.
      * @param reason Reason for restoring the change.
+     * @throws OrmException
      */
-    public void doChangeRestoreHook(final Change change, final Account account, final String reason) {
+    public void doChangeRestoreHook(final Change change, final Account account,
+          final String reason, final ReviewDb db) throws OrmException {
         final ChangeRestoreEvent event = new ChangeRestoreEvent();
 
         event.change = eventFactory.asChangeAttribute(change);
         event.restorer = eventFactory.asAccountAttribute(account);
         event.reason = reason;
-        fireEvent(change, event);
+        fireEvent(change, event, db);
 
         final List<String> args = new ArrayList<String>();
         addArg(args, "--change", event.change.id);
@@ -410,9 +423,9 @@ public class ChangeHookRunner {
       }
     }
 
-    private void fireEvent(final Change change, final ChangeEvent event) {
+    private void fireEvent(final Change change, final ChangeEvent event, final ReviewDb db) throws OrmException {
       for (ChangeListenerHolder holder : listeners.values()) {
-          if (isVisibleTo(change, holder.user)) {
+          if (isVisibleTo(change, holder.user, db)) {
               holder.listener.onChangeEvent(event);
           }
       }
@@ -426,13 +439,13 @@ public class ChangeHookRunner {
       }
     }
 
-    private boolean isVisibleTo(Change change, IdentifiedUser user) {
+    private boolean isVisibleTo(Change change, IdentifiedUser user, ReviewDb db) throws OrmException {
         final ProjectState pe = projectCache.get(change.getProject());
         if (pe == null) {
           return false;
         }
         final ProjectControl pc = pe.controlFor(user);
-        return pc.controlFor(change).isVisible();
+        return pc.controlFor(change).isVisible(db);
     }
 
     private boolean isVisibleTo(Branch.NameKey branchName, IdentifiedUser user) {

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

@@ -253,7 +253,7 @@ public class ChangeUtil {
     updatedChange(db, user, updatedChange, cmsg, senderFactory,
         "Change is no longer open or patchset is not latest");
 
-    hooks.doChangeAbandonedHook(updatedChange, user.getAccount(), message);
+    hooks.doChangeAbandonedHook(updatedChange, user.getAccount(), message, db);
   }
 
   public static Change.Id revert(final PatchSet.Id patchSetId,
@@ -351,7 +351,7 @@ public class ChangeUtil {
       cm.setChangeMessage(cmsg);
       cm.send();
 
-      hooks.doPatchsetCreatedHook(change, ps);
+      hooks.doPatchsetCreatedHook(change, ps, db);
 
       return change.getId();
     } finally {
@@ -400,7 +400,7 @@ public class ChangeUtil {
     updatedChange(db, user, updatedChange, cmsg, senderFactory,
        "Change is not abandoned or patchset is not latest");
 
-    hooks.doChangeRestoreHook(updatedChange, user.getAccount(), message);
+    hooks.doChangeRestoreHook(updatedChange, user.getAccount(), message, db);
   }
 
   private static <T extends ReplyToChangeSender> void updatedChange(

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

@@ -1388,7 +1388,7 @@ public class MergeOp {
     try {
       hooks.doChangeMergedHook(c, //
           accountCache.get(submitter.getAccountId()).getAccount(), //
-          schema.patchSets().get(c.currentPatchSetId()));
+          schema.patchSets().get(c.currentPatchSetId()), schema);
     } catch (OrmException ex) {
       log.error("Cannot run hook for submitted patch set " + c.getId(), ex);
     }

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

@@ -995,7 +995,7 @@ public class ReceiveCommits implements PreReceiveHook, PostReceiveHook {
       log.error("Cannot send email for new change " + change.getId(), e);
     }
 
-    hooks.doPatchsetCreatedHook(change, ps);
+    hooks.doPatchsetCreatedHook(change, ps, db);
   }
 
   private static boolean isReviewer(final FooterLine candidateFooterLine) {
@@ -1309,7 +1309,7 @@ public class ReceiveCommits implements PreReceiveHook, PostReceiveHook {
           + repo.getDirectory() + ": " + ru.getResult());
     }
     replication.scheduleUpdate(project.getNameKey(), ru.getName());
-    hooks.doPatchsetCreatedHook(result.change, ps);
+    hooks.doPatchsetCreatedHook(result.change, ps, db);
     request.cmd.setResult(ReceiveCommand.Result.OK);
 
     try {
@@ -1846,8 +1846,12 @@ public class ReceiveCommits implements PreReceiveHook, PostReceiveHook {
         log.error("Cannot send email for submitted patch set " + psi, e);
       }
 
+      try {
         hooks.doChangeMergedHook(result.change, currentUser.getAccount(),
-          result.patchSet);
+            result.patchSet, db);
+      } catch (OrmException err) {
+        log.error("Cannot open change: " + result.change.getChangeId(), err);
+      }
     }
   }
 

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

@@ -117,7 +117,7 @@ public class VisibleRefFilter implements RefFilter {
     try {
       final Set<Change.Id> visibleChanges = new HashSet<Change.Id>();
       for (Change change : reviewDb.changes().byProject(project.getNameKey())) {
-        if (projectCtl.controlFor(change).isVisible()) {
+        if (projectCtl.controlFor(change).isVisible(reviewDb)) {
           visibleChanges.add(change.getId());
         }
       }

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

@@ -401,11 +401,11 @@ public abstract class ChangeEmail extends OutgoingEmail {
     }
   }
 
-  protected boolean isVisibleTo(final Account.Id to) {
+  protected boolean isVisibleTo(final Account.Id to) throws OrmException {
     return projectState == null
         || change == null
         || projectState.controlFor(args.identifiedUserFactory.create(to))
-            .controlFor(change).isVisible();
+            .controlFor(change).isVisible(args.db.get());
   }
 
   /** Find all users who are authors of any part of this change. */

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

@@ -18,6 +18,7 @@ import com.google.gerrit.reviewdb.Account;
 import com.google.gerrit.reviewdb.UserIdentity;
 import com.google.gerrit.server.account.AccountState;
 import com.google.gerrit.server.mail.EmailHeader.AddressList;
+import com.google.gwtorm.client.OrmException;
 
 import org.apache.commons.lang.StringUtils;
 import org.apache.velocity.Template;
@@ -305,13 +306,17 @@ public abstract class OutgoingEmail {
 
   /** Schedule delivery of this message to the given account. */
   protected void add(final RecipientType rt, final Account.Id to) {
+    try {
       if (!rcptTo.contains(to) && isVisibleTo(to)) {
         rcptTo.add(to);
         add(rt, toAddress(to));
       }
+    } catch (OrmException e) {
+      log.error("Error reading database for account: " + to, e);
+    }
   }
 
-  protected boolean isVisibleTo(final Account.Id to) {
+  protected boolean isVisibleTo(final Account.Id to) throws OrmException {
     return true;
   }
 

gerrit-server/src/main/java/com/google/gerrit/server/patch/AddReviewer.java

@@ -159,7 +159,9 @@ public class AddReviewer implements Callable<ReviewerResult> {
           if (member.isActive()) {
             final IdentifiedUser user =
                 identifiedUserFactory.create(member.getId());
-            if (control.forUser(user).isVisible()) {
+            // Does not account for draft status as a user might want to let a
+            // reviewer see a draft.
+            if (control.forUser(user).isRefVisible()) {
               reviewerIds.add(member.getId());
             }
           }
@@ -175,7 +177,9 @@ public class AddReviewer implements Callable<ReviewerResult> {
       }
 
       final IdentifiedUser user = identifiedUserFactory.create(account.getId());
-      if (!control.forUser(user).isVisible()) {
+      // Does not account for draft status as a user might want to let a
+      // reviewer see a draft.
+      if (!control.forUser(user).isRefVisible()) {
         result.addError(new ReviewerResult.Error(
             ReviewerResult.Error.Type.CHANGE_NOT_VISIBLE,
             formatUser(account, reviewer)));

gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java

@@ -310,14 +310,14 @@ public class PublishComments implements Callable<VoidResult> {
     }
   }
 
-  private void fireHook() {
+  private void fireHook() throws OrmException {
     final Map<ApprovalCategory.Id, ApprovalCategoryValue.Id> changed =
         new HashMap<ApprovalCategory.Id, ApprovalCategoryValue.Id>();
     for (ApprovalCategoryValue.Id v : approvals) {
       changed.put(v.getParentKey(), v);
     }
 
-    hooks.doCommentAddedHook(change, user.getAccount(), patchSet, messageText, changed);
+    hooks.doCommentAddedHook(change, user.getAccount(), patchSet, messageText, changed, db);
   }
 
   private void summarizeInlineComments(StringBuilder in) {

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

@@ -27,6 +27,7 @@ import com.google.gerrit.rules.StoredValues;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gwtorm.client.OrmException;
+import com.google.gwtorm.client.ResultSet;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 
@@ -108,18 +109,18 @@ public class ChangeControl {
     }
 
     public ChangeControl validateFor(final Change.Id id)
-        throws NoSuchChangeException {
+        throws NoSuchChangeException, OrmException {
-      return validate(controlFor(id));
+      return validate(controlFor(id), db.get());
     }
 
     public ChangeControl validateFor(final Change change)
-        throws NoSuchChangeException {
+        throws NoSuchChangeException, OrmException {
-      return validate(controlFor(change));
+      return validate(controlFor(change), db.get());
     }
 
-    private static ChangeControl validate(final ChangeControl c)
+    private static ChangeControl validate(final ChangeControl c, final ReviewDb db)
-        throws NoSuchChangeException {
+        throws NoSuchChangeException, OrmException{
-      if (!c.isVisible()) {
+      if (!c.isVisible(db)) {
         throw new NoSuchChangeException(c.getChange().getId());
       }
       return c;
@@ -159,7 +160,15 @@ public class ChangeControl {
   }
 
   /** Can this user see this change? */
-  public boolean isVisible() {
+  public boolean isVisible(ReviewDb db) throws OrmException {
+    if (change.getStatus() == Change.Status.DRAFT && !isDraftVisible(db)) {
+      return false;
+    }
+    return isRefVisible();
+  }
+
+  /** Can the user see this change? Does not account for draft status */
+  public boolean isRefVisible() {
     return getRefControl().isVisible();
   }
 
@@ -201,6 +210,21 @@ public class ChangeControl {
     return false;
   }
 
+  /** Is this user a reviewer for the change? */
+  public boolean isReviewer(ReviewDb db) throws OrmException {
+    if (getCurrentUser() instanceof IdentifiedUser) {
+      final IdentifiedUser user = (IdentifiedUser) getCurrentUser();
+      ResultSet<PatchSetApproval> results =
+        db.patchSetApprovals().byChange(change.getId());
+      for (PatchSetApproval approval : results) {
+        if (user.getAccountId().equals(approval.getAccountId())) {
+          return true;
+        }
+      }
+    }
+    return false;
+  }
+
   /** @return true if the user is allowed to remove this reviewer. */
   public boolean canRemoveReviewer(PatchSetApproval approval) {
     if (getChange().getStatus().isOpen()) {
@@ -449,6 +473,10 @@ public class ChangeControl {
     }
   }
 
+  private boolean isDraftVisible(ReviewDb db) throws OrmException {
+    return isOwner() || isReviewer(db);
+  }
+
   private static boolean isUser(Term who) {
     return who.isStructure()
         && who.arity() == 1

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

@@ -55,7 +55,7 @@ class IsVisibleToPredicate extends OperatorPredicate<ChangeData> {
     }
     try {
       Change c = cd.change(db);
-      if (c != null && changeControl.controlFor(c, user).isVisible()) {
+      if (c != null && changeControl.controlFor(c, user).isVisible(db.get())) {
         cd.cacheVisibleTo(user);
         return true;
       } else {

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

@@ -154,7 +154,7 @@ class LabelPredicate extends OperatorPredicate<ChangeData> {
           try {
             ChangeControl cc = ccFactory.controlFor(object.change(dbProvider), //
                 userFactory.create(dbProvider, p.getAccountId()));
-            if (!cc.isVisible()) {
+            if (!cc.isVisible(dbProvider.get())) {
               // The user can't see the change anymore.
               //
               continue;

gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/SetReviewersCommand.java

@@ -260,11 +260,13 @@ public class SetReviewersCommand extends BaseCommand {
     try {
       if (change != null
           && inProject(change)
-          && changeControlFactory.controlFor(change).isVisible()) {
+          && changeControlFactory.controlFor(change).isVisible(db)) {
         matched.add(change.getId());
       }
     } catch (NoSuchChangeException e) {
       // Ignore this change.
+    } catch (OrmException e) {
+      log.warn("Error reading change " + change.getId(), e);
     }
   }