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
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/ChangeListServiceImpl.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/ChangeListServiceImpl.java index 28a0d03..773d1f4 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/ChangeListServiceImpl.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/ChangeListServiceImpl.java
@@ -102,9 +102,9 @@ 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 @@ // if (!want.isEmpty()) { for (Change c : db.changes().get(want)) { - if (canRead(c)) { + if (canRead(c, db)) { r.add(c); } } @@ -236,20 +236,20 @@ } d = new AccountDashboardInfo(target); - d.setByOwner(filter(changes.byOwnerOpen(target), stars, ac)); - d.setClosed(filter(changes.byOwnerClosed(target), stars, ac)); + d.setByOwner(filter(changes.byOwnerOpen(target), stars, ac, db)); + 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 @@ } 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())); @@ -377,4 +378,4 @@ return atEnd; } } -} +} \ No newline at end of file
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeDetailFactory.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeDetailFactory.java index c94c0e3..956528c 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeDetailFactory.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeDetailFactory.java
@@ -116,7 +116,7 @@ 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());
diff --git a/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java b/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java index b146d2f..9de9483 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java +++ b/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java
@@ -24,6 +24,7 @@ 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.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 @@ * * @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 @@ * @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 @@ } } - fireEvent(change, event); + fireEvent(change, event, db); final List<String> args = new ArrayList<String>(); addArg(args, "--change", event.change.id); @@ -289,14 +296,16 @@ * @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 @@ * @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 @@ * @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 @@ } } - 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 @@ } } - 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) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java index 5a72ff6..c895881 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java
@@ -253,7 +253,7 @@ 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 @@ cm.setChangeMessage(cmsg); cm.send(); - hooks.doPatchsetCreatedHook(change, ps); + hooks.doPatchsetCreatedHook(change, ps, db); return change.getId(); } finally { @@ -400,7 +400,7 @@ 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(
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java index 3281306..7e080cf 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
@@ -1388,7 +1388,7 @@ 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); }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java index ffaca26..6550bb7 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
@@ -995,7 +995,7 @@ 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 @@ + 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 @@ log.error("Cannot send email for submitted patch set " + psi, e); } - hooks.doChangeMergedHook(result.change, currentUser.getAccount(), - result.patchSet); + try { + hooks.doChangeMergedHook(result.change, currentUser.getAccount(), + result.patchSet, db); + } catch (OrmException err) { + log.error("Cannot open change: " + result.change.getChangeId(), err); + } } }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java index c25392c..feadaf1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java
@@ -117,7 +117,7 @@ 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()); } }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java index 9d6c70b..0c81252 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java
@@ -401,11 +401,11 @@ } } - 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. */
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/OutgoingEmail.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/OutgoingEmail.java index ce788cf..d208c84 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/OutgoingEmail.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/OutgoingEmail.java
@@ -18,6 +18,7 @@ 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 @@ /** Schedule delivery of this message to the given account. */ protected void add(final RecipientType rt, final Account.Id to) { - if (!rcptTo.contains(to) && isVisibleTo(to)) { - rcptTo.add(to); - add(rt, toAddress(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; }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/AddReviewer.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/AddReviewer.java index 514ea5e..9fdc899 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/AddReviewer.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/AddReviewer.java
@@ -159,7 +159,9 @@ 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 @@ } 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)));
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java index be20b3d..a276c84 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java
@@ -310,14 +310,14 @@ } } - 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) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java index c7e2055..5621288 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
@@ -27,6 +27,7 @@ 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 ChangeControl validateFor(final Change.Id id) - throws NoSuchChangeException { - return validate(controlFor(id)); + throws NoSuchChangeException, OrmException { + return validate(controlFor(id), db.get()); } public ChangeControl validateFor(final Change change) - throws NoSuchChangeException { - return validate(controlFor(change)); + throws NoSuchChangeException, OrmException { + return validate(controlFor(change), db.get()); } - private static ChangeControl validate(final ChangeControl c) - throws NoSuchChangeException { - if (!c.isVisible()) { + private static ChangeControl validate(final ChangeControl c, final ReviewDb db) + throws NoSuchChangeException, OrmException{ + if (!c.isVisible(db)) { throw new NoSuchChangeException(c.getChange().getId()); } return c; @@ -159,7 +160,15 @@ } /** 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 @@ 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 @@ } } + private boolean isDraftVisible(ReviewDb db) throws OrmException { + return isOwner() || isReviewer(db); + } + private static boolean isUser(Term who) { return who.isStructure() && who.arity() == 1
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/IsVisibleToPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/IsVisibleToPredicate.java index 1e9d405..739d9bc 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/IsVisibleToPredicate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/IsVisibleToPredicate.java
@@ -55,7 +55,7 @@ } 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 {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/LabelPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/LabelPredicate.java index ece61ef..7342c4f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/LabelPredicate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/LabelPredicate.java
@@ -154,7 +154,7 @@ 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;
diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/SetReviewersCommand.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/SetReviewersCommand.java index 845ae95..985c2d5 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/SetReviewersCommand.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/SetReviewersCommand.java
@@ -260,11 +260,13 @@ 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); } }