From 1531f6101a3ac160bf3a2ce0c902b635cff5d12e Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Tue, 9 Oct 2018 14:16:04 -0700 Subject: [PATCH] Support arbitrary reviewer strings in push options This change alters the semantics of adding reviewers/CCs during push to match the semantics of the Post Reviewers endpoint. The fact that they differed is mostly an accident of history: support for adding reviewers over push long predates reviewers-by-email or any of the other fanciness added to Post Reviewers in the past few years. The most significant semantic change is increasing the space of allowed reviewers via push option: * By email, if the project allows it. * Group names. For now at least, attempting to add a non-existing reviewer still fails, whether due to a non-existing email when reviewers by email are not supported, or a non-email input. The error message in this case has changed to align with the error message from the REST API. Due to an issue in the implementation of GroupResolver, adding groups via push currently only works for internal groups if the group is visible to anonymous users. This is still strictly more functionality than before, when groups weren't supported at all, so keep the behavior even with this known bug. Change-Id: I24c0c27664d04f431e5b0f0c854df4a6de2ac014 --- .../acceptance/AbstractNotificationTest.java | 3 +- .../gerrit/server/change/ChangeInserter.java | 23 +++-- .../gerrit/server/change/ReviewerAdder.java | 60 ++++++++----- .../server/git/receive/ReceiveCommits.java | 80 ++++++++++++----- .../gerrit/server/git/receive/ReplaceOp.java | 85 +++++++++++-------- .../server/restapi/change/PostReview.java | 3 +- .../server/restapi/change/PostReviewers.java | 3 +- .../server/restapi/change/PutAssignee.java | 2 +- .../acceptance/git/AbstractPushForReview.java | 71 ++++++++++++++-- .../server/mail/ChangeNotificationsIT.java | 22 ++++- 10 files changed, 257 insertions(+), 95 deletions(-) diff --git a/java/com/google/gerrit/acceptance/AbstractNotificationTest.java b/java/com/google/gerrit/acceptance/AbstractNotificationTest.java index a4945c7df4..7a30f0cd8f 100644 --- a/java/com/google/gerrit/acceptance/AbstractNotificationTest.java +++ b/java/com/google/gerrit/acceptance/AbstractNotificationTest.java @@ -319,7 +319,8 @@ public abstract class AbstractNotificationTest extends AbstractDaemonTest { public final String ccerByEmail = "ccByEmail@example.com"; private final Map watchers = new HashMap<>(); private final Map accountsByEmail = new HashMap<>(); - boolean supportReviewersByEmail; + + public boolean supportReviewersByEmail; private String usersCacheKey() { return description.getClassName(); diff --git a/java/com/google/gerrit/server/change/ChangeInserter.java b/java/com/google/gerrit/server/change/ChangeInserter.java index 0b4eabd2af..831d3884c4 100644 --- a/java/com/google/gerrit/server/change/ChangeInserter.java +++ b/java/com/google/gerrit/server/change/ChangeInserter.java @@ -25,19 +25,19 @@ import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER; import com.google.common.base.MoreObjects; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableListMultimap; +import com.google.common.collect.Iterables; import com.google.common.collect.ListMultimap; import com.google.common.collect.Streams; import com.google.common.flogger.FluentLogger; import com.google.gerrit.common.FooterConstants; import com.google.gerrit.common.data.LabelType; import com.google.gerrit.common.data.LabelTypes; -import com.google.gerrit.extensions.api.changes.AddReviewerResult; import com.google.gerrit.extensions.api.changes.NotifyHandling; import com.google.gerrit.extensions.api.changes.RecipientType; import com.google.gerrit.extensions.client.ReviewerState; -import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.RestApiException; +import com.google.gerrit.extensions.restapi.UnprocessableEntityException; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Branch; import com.google.gerrit.reviewdb.client.Change; @@ -269,6 +269,13 @@ public class ChangeInserter implements InsertChangeOp { public ChangeInserter setReviewersAndCcs( Iterable reviewers, Iterable ccs) { + return setReviewersAndCcsAsStrings( + Iterables.transform(reviewers, Account.Id::toString), + Iterables.transform(ccs, Account.Id::toString)); + } + + public ChangeInserter setReviewersAndCcsAsStrings( + Iterable reviewers, Iterable ccs) { reviewerInputs = Streams.concat( Streams.stream(reviewers) @@ -425,13 +432,11 @@ public class ChangeInserter implements InsertChangeOp { update.fixStatus(change.getStatus()); reviewerAdditions = - reviewerAdder.prepare(ctx.getNotes(), ctx.getUser(), getReviewerInputs(), true); + reviewerAdder.prepare( + ctx.getDb(), ctx.getNotes(), ctx.getUser(), getReviewerInputs(), true); Optional reviewerError = reviewerAdditions.getFailures().stream().findFirst(); if (reviewerError.isPresent()) { - // TODO(dborowitz): How best to report this? Erroring out would match existing - // ReceiveCommits behavior, but it may be worth rethinking. - AddReviewerResult result = reviewerError.get().result; - throw new BadRequestException("Failed to add reviewer " + result.input + ": " + result.error); + throw new UnprocessableEntityException(reviewerError.get().result.error); } reviewerAdditions.updateChange(ctx, patchSet); @@ -563,11 +568,11 @@ public class ChangeInserter implements InsertChangeOp { } private static InternalAddReviewerInput newAddReviewerInput( - Account.Id accountId, ReviewerState state) { + String reviewer, ReviewerState state) { // Disable individual emails when adding reviewers, as all reviewers will receive the single // bulk new change email. InternalAddReviewerInput input = - ReviewerAdder.newAddReviewerInput(accountId, state, NotifyHandling.NONE); + ReviewerAdder.newAddReviewerInput(reviewer, state, NotifyHandling.NONE); // Ignore failures for reasons like the reviewer being inactive or being unable to see the // change. This is required for the push path, where it automatically sets reviewers from diff --git a/java/com/google/gerrit/server/change/ReviewerAdder.java b/java/com/google/gerrit/server/change/ReviewerAdder.java index ed56cd19af..441a7d40a9 100644 --- a/java/com/google/gerrit/server/change/ReviewerAdder.java +++ b/java/com/google/gerrit/server/change/ReviewerAdder.java @@ -148,7 +148,6 @@ public class ReviewerAdder { private final GroupResolver groupResolver; private final GroupMembers groupMembers; private final AccountLoader.Factory accountLoaderFactory; - private final Provider dbProvider; private final Config cfg; private final ReviewerJson json; private final NotesMigration migration; @@ -165,7 +164,6 @@ public class ReviewerAdder { GroupResolver groupResolver, GroupMembers groupMembers, AccountLoader.Factory accountLoaderFactory, - Provider db, @GerritServerConfig Config cfg, ReviewerJson json, NotesMigration migration, @@ -179,7 +177,6 @@ public class ReviewerAdder { this.groupResolver = groupResolver; this.groupMembers = groupMembers; this.accountLoaderFactory = accountLoaderFactory; - this.dbProvider = db; this.cfg = cfg; this.json = json; this.migration = migration; @@ -193,6 +190,7 @@ public class ReviewerAdder { /** * Prepare application of a single {@link AddReviewerInput}. * + * @param db database. * @param notes change notes. * @param user user performing the reviewer addition. * @param input input describing user or group to add as a reviewer. @@ -206,9 +204,8 @@ public class ReviewerAdder { * @throws ConfigInvalidException */ public ReviewerAddition prepare( - ChangeNotes notes, CurrentUser user, AddReviewerInput input, boolean allowGroup) + ReviewDb db, ChangeNotes notes, CurrentUser user, AddReviewerInput input, boolean allowGroup) throws OrmException, IOException, PermissionBackendException, ConfigInvalidException { - Branch.NameKey dest = notes.getChange().getDest(); checkNotNull(input.reviewer); ListMultimap accountsToNotify; try { @@ -219,16 +216,17 @@ public class ReviewerAdder { boolean confirmed = input.confirmed(); boolean allowByEmail = projectCache - .checkedGet(dest.getParentKey()) + .checkedGet(notes.getProjectName()) .is(BooleanProjectConfig.ENABLE_REVIEWER_BY_EMAIL); ReviewerAddition byAccountId = - addByAccountId(input, dest, user, accountsToNotify, allowGroup, allowByEmail); + addByAccountId(db, input, notes, user, accountsToNotify, allowGroup, allowByEmail); ReviewerAddition wholeGroup = null; if (byAccountId == null || !byAccountId.exactMatchFound) { wholeGroup = - addWholeGroup(input, dest, user, accountsToNotify, confirmed, allowGroup, allowByEmail); + addWholeGroup( + db, input, notes, user, accountsToNotify, confirmed, allowGroup, allowByEmail); if (wholeGroup != null && wholeGroup.exactMatchFound) { return wholeGroup; } @@ -241,12 +239,13 @@ public class ReviewerAdder { return wholeGroup; } - return addByEmail(input, notes, user, accountsToNotify); + return addByEmail(db, input, notes, user, accountsToNotify); } public ReviewerAddition ccCurrentUser(CurrentUser user, RevisionResource revision) { return new ReviewerAddition( newAddReviewerInput(user.getUserName().orElse(null), CC, NotifyHandling.NONE), + revision.getNotes(), revision.getUser(), ImmutableSet.of(user.getAccountId()), null, @@ -256,8 +255,9 @@ public class ReviewerAdder { @Nullable private ReviewerAddition addByAccountId( + ReviewDb db, AddReviewerInput input, - Branch.NameKey dest, + ChangeNotes notes, CurrentUser user, ListMultimap accountsToNotify, boolean allowGroup, @@ -283,9 +283,10 @@ public class ReviewerAdder { return null; } - if (isValidReviewer(dest, reviewerUser.getAccount())) { + if (isValidReviewer(db, notes.getChange().getDest(), reviewerUser.getAccount())) { return new ReviewerAddition( input, + notes, user, ImmutableSet.of(reviewerUser.getAccountId()), null, @@ -309,8 +310,9 @@ public class ReviewerAdder { @Nullable private ReviewerAddition addWholeGroup( + ReviewDb db, AddReviewerInput input, - Branch.NameKey dest, + ChangeNotes notes, CurrentUser user, ListMultimap accountsToNotify, boolean confirmed, @@ -323,6 +325,8 @@ public class ReviewerAdder { GroupDescription.Basic group; try { + // TODO(dborowitz): This currently doesn't work in the push path because InternalGroupBackend + // depends on the Provider which returns anonymous in that path. group = groupResolver.parseInternal(input.reviewer); } catch (UnprocessableEntityException e) { if (!allowByEmail) { @@ -344,7 +348,7 @@ public class ReviewerAdder { Set reviewers = new HashSet<>(); Set members; try { - members = groupMembers.listAccounts(group.getGroupUUID(), dest.getParentKey()); + members = groupMembers.listAccounts(group.getGroupUUID(), notes.getProjectName()); } catch (NoSuchProjectException e) { return fail(input, FailureType.OTHER, e.getMessage()); } @@ -372,16 +376,17 @@ public class ReviewerAdder { } for (Account member : members) { - if (isValidReviewer(dest, member)) { + if (isValidReviewer(db, notes.getChange().getDest(), member)) { reviewers.add(member.getId()); } } - return new ReviewerAddition(input, user, reviewers, null, accountsToNotify, true); + return new ReviewerAddition(input, notes, user, reviewers, null, accountsToNotify, true); } @Nullable private ReviewerAddition addByEmail( + ReviewDb db, AddReviewerInput input, ChangeNotes notes, CurrentUser user, @@ -390,7 +395,7 @@ public class ReviewerAdder { try { permissionBackend .user(anonymousProvider.get()) - .database(dbProvider) + .database(db) .change(notes) .check(ChangePermission.READ); } catch (AuthException e) { @@ -414,10 +419,11 @@ public class ReviewerAdder { FailureType.NOT_FOUND, MessageFormat.format(ChangeMessages.get().reviewerInvalid, input.reviewer)); } - return new ReviewerAddition(input, user, null, ImmutableList.of(adr), accountsToNotify, true); + return new ReviewerAddition( + input, notes, user, null, ImmutableList.of(adr), accountsToNotify, true); } - private boolean isValidReviewer(Branch.NameKey branch, Account member) + private boolean isValidReviewer(ReviewDb db, Branch.NameKey branch, Account member) throws PermissionBackendException { if (!member.isActive()) { return false; @@ -429,7 +435,7 @@ public class ReviewerAdder { // see private changes. permissionBackend .absentUser(member.getId()) - .database(dbProvider) + .database(db) .ref(branch) .check(RefPermission.READ); return true; @@ -473,6 +479,7 @@ public class ReviewerAdder { private ReviewerAddition( AddReviewerInput input, + ChangeNotes notes, CurrentUser caller, @Nullable Iterable reviewers, @Nullable Iterable
reviewersByEmail, @@ -485,7 +492,9 @@ public class ReviewerAdder { this.input = input; this.failureType = null; result = new AddReviewerResult(input.reviewer); - this.reviewers = reviewers == null ? ImmutableSet.of() : ImmutableSet.copyOf(reviewers); + // Always silently ignore adding the owner as any type of reviewer on their own change. They + // may still be implicitly added as a reviewer if they vote, but not via the reviewer API. + this.reviewers = omitOwner(notes, reviewers); this.reviewersByEmail = reviewersByEmail == null ? ImmutableSet.of() : ImmutableSet.copyOf(reviewersByEmail); this.caller = caller.asIdentifiedUser(); @@ -495,6 +504,14 @@ public class ReviewerAdder { this.exactMatchFound = exactMatchFound; } + private ImmutableSet omitOwner(ChangeNotes notes, Iterable reviewers) { + return reviewers != null + ? Streams.stream(reviewers) + .filter(id -> !id.equals(notes.getChange().getOwner())) + .collect(toImmutableSet()) + : ImmutableSet.of(); + } + public void gatherResults(ChangeData cd) throws OrmException, PermissionBackendException { checkState(op != null, "addition did not result in an update op"); checkState(op.getResult() != null, "op did not return a result"); @@ -552,6 +569,7 @@ public class ReviewerAdder { } public ReviewerAdditionList prepare( + ReviewDb db, ChangeNotes notes, CurrentUser user, Iterable inputs, @@ -572,7 +590,7 @@ public class ReviewerAdder { .collect(toImmutableList()); List additions = new ArrayList<>(); for (AddReviewerInput input : sorted) { - additions.add(prepare(notes, user, input, allowGroup)); + additions.add(prepare(db, notes, user, input, allowGroup)); } return new ReviewerAdditionList(additions); } diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java index 41ed4f500c..6e800d0ce0 100644 --- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java +++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java @@ -18,6 +18,7 @@ import static com.google.common.base.MoreObjects.firstNonNull; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; +import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.gerrit.common.FooterConstants.CHANGE_ID; import static com.google.gerrit.reviewdb.client.RefNames.REFS_CHANGES; import static com.google.gerrit.reviewdb.client.RefNames.isConfigRef; @@ -47,6 +48,7 @@ import com.google.common.base.Throwables; import com.google.common.collect.BiMap; import com.google.common.collect.HashBiMap; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.common.collect.LinkedListMultimap; import com.google.common.collect.ListMultimap; @@ -55,6 +57,7 @@ import com.google.common.collect.Maps; import com.google.common.collect.MultimapBuilder; import com.google.common.collect.Sets; import com.google.common.collect.SortedSetMultimap; +import com.google.common.collect.Streams; import com.google.common.flogger.FluentLogger; import com.google.gerrit.common.FooterConstants; import com.google.gerrit.common.Nullable; @@ -73,6 +76,7 @@ import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.RestApiException; +import com.google.gerrit.extensions.restapi.UnprocessableEntityException; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.BooleanProjectConfig; import com.google.gerrit.reviewdb.client.Branch; @@ -841,10 +845,12 @@ class ReceiveCommits { } catch (ResourceConflictException e) { addError(e.getMessage()); reject(magicBranchCmd, "conflict"); - } catch (RestApiException | IOException err) { - logger.atSevere().withCause(err).log( - "Can't insert change/patch set for %s", project.getName()); - reject(magicBranchCmd, "internal server error: " + err.getMessage()); + } catch (BadRequestException | UnprocessableEntityException e) { + logger.atFine().withCause(e).log("Rejecting due to client error"); + reject(magicBranchCmd, e.getMessage()); + } catch (RestApiException | IOException e) { + logger.atSevere().withCause(e).log("Can't insert change/patch set for %s", project.getName()); + reject(magicBranchCmd, "internal server error: " + e.getMessage()); } if (magicBranch != null && magicBranch.submit) { @@ -1327,8 +1333,8 @@ class ReceiveCommits { private final boolean defaultPublishComments; Branch.NameKey dest; PermissionBackend.ForRef perm; - Set reviewer = Sets.newLinkedHashSet(); - Set cc = Sets.newLinkedHashSet(); + Set reviewer = Sets.newLinkedHashSet(); + Set cc = Sets.newLinkedHashSet(); Map labels = new HashMap<>(); String message; List baseCommit; @@ -1418,15 +1424,15 @@ class ReceiveCommits { @Option( name = "--reviewer", aliases = {"-r"}, - metaVar = "USER", + metaVar = "REVIEWER", usage = "add reviewer to changes") - void reviewer(Account.Id id) { - reviewer.add(id); + void reviewer(String str) { + reviewer.add(str); } - @Option(name = "--cc", metaVar = "USER", usage = "add user as CC to changes") - void cc(Account.Id id) { - cc.add(id); + @Option(name = "--cc", metaVar = "CC", usage = "add CC to changes") + void cc(String str) { + cc.add(str); } @Option( @@ -1500,8 +1506,38 @@ class ReceiveCommits { : false; } - MailRecipients getMailRecipients() { - return new MailRecipients(reviewer, cc); + /** + * Get reviewer strings from magic branch options, combined with additional recipients computed + * from some other place. + * + *

The set of reviewers on a change includes strings passed explicitly via options as well as + * account IDs computed from the commit message itself. + * + * @param additionalRecipients recipients parsed from the commit. + * @return set of reviewer strings to pass to {@code ReviewerAdder}. + */ + ImmutableSet getCombinedReviewers(MailRecipients additionalRecipients) { + return getCombinedReviewers(reviewer, additionalRecipients.getReviewers()); + } + + /** + * Get CC strings from magic branch options, combined with additional recipients computed from + * some other place. + * + *

The set of CCs on a change includes strings passed explicitly via options as well as + * account IDs computed from the commit message itself. + * + * @param additionalRecipients recipients parsed from the commit. + * @return set of CC strings to pass to {@code ReviewerAdder}. + */ + ImmutableSet getCombinedCcs(MailRecipients additionalRecipients) { + return getCombinedReviewers(cc, additionalRecipients.getCcOnly()); + } + + private static ImmutableSet getCombinedReviewers( + Set strings, Set ids) { + return Streams.concat(strings.stream(), ids.stream().map(Account.Id::toString)) + .collect(toImmutableSet()); } ListMultimap getAccountsToNotify() { @@ -2392,12 +2428,16 @@ class ReceiveCommits { final PatchSet.Id psId = ins.setGroups(groups).getPatchSetId(); Account.Id me = user.getAccountId(); List footerLines = commit.getFooterLines(); - MailRecipients recipients = new MailRecipients(); checkNotNull(magicBranch); - recipients.add(magicBranch.getMailRecipients()); + + // TODO(dborowitz): Support reviewers by email from footers? Maybe not: kernel developers + // with AOSP accounts already complain about these notifications, and that would make it + // worse. Might be better to get rid of the feature entirely: + // https://groups.google.com/d/topic/repo-discuss/tIFxY7L4DXk/discussion + MailRecipients fromFooters = getRecipientsFromFooters(accountResolver, footerLines); + fromFooters.remove(me); + Map approvals = magicBranch.labels; - recipients.add(getRecipientsFromFooters(accountResolver, footerLines)); - recipients.remove(me); StringBuilder msg = new StringBuilder( ApprovalsUtil.renderMessageWithApprovals( @@ -2408,7 +2448,9 @@ class ReceiveCommits { } bu.insertChange( - ins.setReviewersAndCcs(recipients.getReviewers(), recipients.getCcOnly()) + ins.setReviewersAndCcsAsStrings( + magicBranch.getCombinedReviewers(fromFooters), + magicBranch.getCombinedCcs(fromFooters)) .setApprovals(approvals) .setMessage(msg.toString()) .setNotify(magicBranch.getNotify()) diff --git a/java/com/google/gerrit/server/git/receive/ReplaceOp.java b/java/com/google/gerrit/server/git/receive/ReplaceOp.java index d5cb4b368c..a580cf6170 100644 --- a/java/com/google/gerrit/server/git/receive/ReplaceOp.java +++ b/java/com/google/gerrit/server/git/receive/ReplaceOp.java @@ -15,6 +15,7 @@ package com.google.gerrit.server.git.receive; import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.gerrit.common.FooterConstants.CHANGE_ID; import static com.google.gerrit.server.change.ReviewerAdder.newAddReviewerInputFromCommitIdentity; import static com.google.gerrit.server.mail.MailUtil.getRecipientsFromFooters; @@ -29,13 +30,12 @@ import com.google.common.collect.Streams; import com.google.common.flogger.FluentLogger; import com.google.gerrit.common.Nullable; import com.google.gerrit.common.data.LabelType; -import com.google.gerrit.extensions.api.changes.AddReviewerResult; +import com.google.gerrit.extensions.api.changes.AddReviewerInput; import com.google.gerrit.extensions.api.changes.NotifyHandling; import com.google.gerrit.extensions.client.ChangeKind; import com.google.gerrit.extensions.client.ReviewerState; -import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.RestApiException; -import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.extensions.restapi.UnprocessableEntityException; import com.google.gerrit.reviewdb.client.Branch; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.ChangeMessage; @@ -50,6 +50,7 @@ import com.google.gerrit.server.CommentsUtil; import com.google.gerrit.server.PatchSetUtil; import com.google.gerrit.server.PublishCommentUtil; import com.google.gerrit.server.account.AccountResolver; +import com.google.gerrit.server.change.AddReviewersOp; import com.google.gerrit.server.change.ChangeKindCache; import com.google.gerrit.server.change.EmailReviewComments; import com.google.gerrit.server.change.ReviewerAdder; @@ -86,6 +87,7 @@ import java.util.Optional; import java.util.Set; import java.util.concurrent.ExecutorService; import java.util.concurrent.Future; +import java.util.stream.Stream; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.revwalk.RevCommit; @@ -144,7 +146,6 @@ public class ReplaceOp implements BatchUpdateOp { private List groups; private final Map approvals = new HashMap<>(); - private final MailRecipients recipients = new MailRecipients(); private RevCommit commit; private ReceiveCommand cmd; private ChangeNotes notes; @@ -156,6 +157,7 @@ public class ReplaceOp implements BatchUpdateOp { private MergedByPushOp mergedByPushOp; private RequestScopePropagator requestScopePropagator; private ReviewerAdditionList reviewerAdditions; + private MailRecipients oldRecipients; @Inject ReplaceOp( @@ -257,13 +259,15 @@ public class ReplaceOp implements BatchUpdateOp { groups = prevPs != null ? prevPs.getGroups() : ImmutableList.of(); } + ChangeData cd = changeDataFactory.create(ctx.getDb(), ctx.getNotes()); + oldRecipients = getRecipientsFromReviewers(cd.reviewers()); + ChangeUpdate update = ctx.getUpdate(patchSetId); update.setSubjectForCommit("Create patch set " + patchSetId.get()); String reviewMessage = null; String psDescription = null; if (magicBranch != null) { - recipients.add(magicBranch.getMailRecipients()); reviewMessage = magicBranch.message; psDescription = magicBranch.message; approvals.putAll(magicBranch.labels); @@ -312,10 +316,6 @@ public class ReplaceOp implements BatchUpdateOp { update.setPsDescription(psDescription); MailRecipients fromFooters = getRecipientsFromFooters(accountResolver, commit.getFooterLines()); - recipients.add(fromFooters); - recipients.remove(ctx.getAccountId()); - ChangeData cd = changeDataFactory.create(ctx.getDb(), ctx.getNotes()); - MailRecipients oldRecipients = getRecipientsFromReviewers(cd.reviewers()); Iterable newApprovals = approvalsUtil.addApprovalsForNewPatchSet( ctx.getDb(), @@ -334,16 +334,14 @@ public class ReplaceOp implements BatchUpdateOp { reviewerAdditions = reviewerAdder.prepare( + ctx.getDb(), ctx.getNotes(), ctx.getUser(), - getReviewerInputs(recipients, ctx.getChange(), info), + getReviewerInputs(magicBranch, fromFooters, ctx.getChange(), info), true); Optional reviewerError = reviewerAdditions.getFailures().stream().findFirst(); if (reviewerError.isPresent()) { - // TODO(dborowitz): How best to report this? Erroring out would match existing - // ReceiveCommits behavior, but it may be worth rethinking. - AddReviewerResult result = reviewerError.get().result; - throw new BadRequestException("Failed to add reviewer " + result.input + ": " + result.error); + throw new UnprocessableEntityException(reviewerError.get().result.error); } reviewerAdditions.updateChange(ctx, newPatchSet); @@ -354,8 +352,6 @@ public class ReplaceOp implements BatchUpdateOp { update.putReviewer(ctx.getAccountId(), REVIEWER); } - recipients.add(oldRecipients); - msg = createChangeMessage(ctx, reviewMessage); cmUtil.addChangeMessage(ctx.getDb(), update, msg); @@ -368,36 +364,43 @@ public class ReplaceOp implements BatchUpdateOp { return true; } - private static ImmutableList getReviewerInputs( - MailRecipients recipients, Change change, PatchSetInfo psInfo) { + private static ImmutableList getReviewerInputs( + @Nullable MagicBranchInput magicBranch, + MailRecipients fromFooters, + Change change, + PatchSetInfo psInfo) { // Disable individual emails when adding reviewers, as all reviewers will receive the single // bulk new change email. - return Streams.concat( - recipients - .getReviewers() - .stream() - .distinct() - .map(id -> newAddReviewerInput(id, ReviewerState.REVIEWER)), - recipients - .getCcOnly() - .stream() - .distinct() - .map(id -> newAddReviewerInput(id, ReviewerState.CC)), + Stream inputs = + Streams.concat( Streams.stream( newAddReviewerInputFromCommitIdentity( change, psInfo.getAuthor().getAccount(), NotifyHandling.NONE)), Streams.stream( newAddReviewerInputFromCommitIdentity( - change, psInfo.getCommitter().getAccount(), NotifyHandling.NONE))) - .collect(toImmutableList()); + change, psInfo.getCommitter().getAccount(), NotifyHandling.NONE))); + if (magicBranch != null) { + inputs = + Streams.concat( + inputs, + magicBranch + .getCombinedReviewers(fromFooters) + .stream() + .map(r -> newAddReviewerInput(r, ReviewerState.REVIEWER)), + magicBranch + .getCombinedCcs(fromFooters) + .stream() + .map(r -> newAddReviewerInput(r, ReviewerState.CC))); + } + return inputs.collect(toImmutableList()); } private static InternalAddReviewerInput newAddReviewerInput( - Account.Id accountId, ReviewerState state) { + String reviewer, ReviewerState state) { // Disable individual emails when adding reviewers, as all reviewers will receive the single // bulk new patch set email. InternalAddReviewerInput input = - ReviewerAdder.newAddReviewerInput(accountId, state, NotifyHandling.NONE); + ReviewerAdder.newAddReviewerInput(reviewer, state, NotifyHandling.NONE); // Ignore failures for reasons like the reviewer being inactive or being unable to see the // change. See discussion in ChangeInserter. @@ -576,8 +579,20 @@ public class ReplaceOp implements BatchUpdateOp { cm.setNotify(magicBranch.getNotify(notes)); cm.setAccountsToNotify(magicBranch.getAccountsToNotify()); } - cm.addReviewers(recipients.getReviewers()); - cm.addExtraCC(recipients.getCcOnly()); + cm.addReviewers( + Streams.concat( + oldRecipients.getReviewers().stream(), + reviewerAdditions + .flattenResults(AddReviewersOp.Result::addedReviewers) + .stream() + .map(PatchSetApproval::getAccountId)) + .collect(toImmutableSet())); + cm.addExtraCC( + Streams.concat( + oldRecipients.getCcOnly().stream(), + reviewerAdditions.flattenResults(AddReviewersOp.Result::addedCCs).stream()) + .collect(toImmutableSet())); + // TODO(dborowitz): Support byEmail cm.send(); } catch (Exception e) { logger.atSevere().withCause(e).log( diff --git a/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java index 6583d16c88..8b5ed68e7b 100644 --- a/java/com/google/gerrit/server/restapi/change/PostReview.java +++ b/java/com/google/gerrit/server/restapi/change/PostReview.java @@ -288,7 +288,8 @@ public class PostReview reviewerInput.notify = NotifyHandling.NONE; ReviewerAddition result = - reviewerAdder.prepare(revision.getNotes(), revision.getUser(), reviewerInput, true); + reviewerAdder.prepare( + db.get(), revision.getNotes(), revision.getUser(), reviewerInput, true); reviewerJsonResults.put(reviewerInput.reviewer, result.result); if (result.result.error != null) { hasError = true; diff --git a/java/com/google/gerrit/server/restapi/change/PostReviewers.java b/java/com/google/gerrit/server/restapi/change/PostReviewers.java index 9147d44e41..3e6618931e 100644 --- a/java/com/google/gerrit/server/restapi/change/PostReviewers.java +++ b/java/com/google/gerrit/server/restapi/change/PostReviewers.java @@ -68,7 +68,8 @@ public class PostReviewers throw new BadRequestException("missing reviewer field"); } - ReviewerAddition addition = reviewerAdder.prepare(rsrc.getNotes(), rsrc.getUser(), input, true); + ReviewerAddition addition = + reviewerAdder.prepare(dbProvider.get(), rsrc.getNotes(), rsrc.getUser(), input, true); if (addition.op == null) { return addition.result; } diff --git a/java/com/google/gerrit/server/restapi/change/PutAssignee.java b/java/com/google/gerrit/server/restapi/change/PutAssignee.java index 7ca674f6f0..7878ce5fe9 100644 --- a/java/com/google/gerrit/server/restapi/change/PutAssignee.java +++ b/java/com/google/gerrit/server/restapi/change/PutAssignee.java @@ -124,7 +124,7 @@ public class PutAssignee extends RetryingRestModifyView ccs = + firstNonNull(ci.reviewers.get(ReviewerState.CC), ImmutableList.of()) + .stream() + .sorted(comparing((AccountInfo a) -> a.email)) + .collect(toImmutableList()); + assertThat(ccs).hasSize(2); + assertThat(ccs.get(0).email).isEqualTo("non.existing.1@example.com"); + assertThat(ccs.get(0)._accountId).isNull(); + assertThat(ccs.get(1).email).isEqualTo("non.existing.2@example.com"); + assertThat(ccs.get(1)._accountId).isNull(); + } else { + r.assertErrorStatus("non.existing.1@example.com does not identify a registered user"); + } } @Test @@ -581,11 +611,12 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { GroupInput gin = new GroupInput(); gin.name = group; gin.members = ImmutableList.of(user.username, user2.username); + gin.visibleToAll = true; // TODO(dborowitz): Shouldn't be necessary; see ReviewerAdder. gApi.groups().create(gin); PushOneCommit.Result r = pushTo("refs/for/master%cc=" + group); - // TODO(dborowitz): Support adding groups. - r.assertErrorStatus("user \"" + group + "\" not found"); + r.assertOkStatus(); + r.assertChange(Change.Status.NEW, null, ImmutableList.of(), ImmutableList.of(user, user2)); } @Test @@ -625,7 +656,34 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { + nonExistingEmail + ",r=" + user.email); - r.assertErrorStatus("user \"" + nonExistingEmail + "\" not found"); + r.assertErrorStatus(nonExistingEmail + " does not identify a registered user or group"); + } + + @Test + public void pushForMasterWithReviewerByEmail() throws Exception { + ConfigInput conf = new ConfigInput(); + conf.enableReviewerByEmail = InheritableBoolean.TRUE; + gApi.projects().name(project.get()).config(conf); + + PushOneCommit.Result r = + pushTo("refs/for/master%r=non.existing.1@example.com,r=non.existing.2@example.com"); + if (notesMigration.readChanges()) { + r.assertOkStatus(); + + ChangeInfo ci = get(r.getChangeId(), DETAILED_LABELS); + ImmutableList reviewers = + firstNonNull(ci.reviewers.get(ReviewerState.REVIEWER), ImmutableList.of()) + .stream() + .sorted(comparing((AccountInfo a) -> a.email)) + .collect(toImmutableList()); + assertThat(reviewers).hasSize(2); + assertThat(reviewers.get(0).email).isEqualTo("non.existing.1@example.com"); + assertThat(reviewers.get(0)._accountId).isNull(); + assertThat(reviewers.get(1).email).isEqualTo("non.existing.2@example.com"); + assertThat(reviewers.get(1)._accountId).isNull(); + } else { + r.assertErrorStatus("non.existing.1@example.com does not identify a registered user"); + } } @Test @@ -635,11 +693,12 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { GroupInput gin = new GroupInput(); gin.name = group; gin.members = ImmutableList.of(user.username, user2.username); + gin.visibleToAll = true; // TODO(dborowitz): Shouldn't be necessary; see ReviewerAdder. gApi.groups().create(gin); PushOneCommit.Result r = pushTo("refs/for/master%r=" + group); - // TODO(dborowitz): Support adding groups. - r.assertErrorStatus("user \"" + group + "\" not found"); + r.assertOkStatus(); + r.assertChange(Change.Status.NEW, null, ImmutableList.of(user, user2), ImmutableList.of()); } @Test diff --git a/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java b/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java index 672b8b5b2f..209d0a2bc5 100644 --- a/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java +++ b/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java @@ -1088,7 +1088,6 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { @Test public void createReviewableChangeWithReviewersAndCcs() throws Exception { - // TODO(logan): Support reviewers/CCs-by-email via push option. StagedPreChange spc = stagePreChange( "refs/for/master", @@ -1104,6 +1103,23 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { subject.bcc(NEW_CHANGES, NEW_PATCHSETS).noOneElse(); } + @Test + public void createReviewableChangeWithReviewersAndCcsByEmailInNoteDb() throws Exception { + assume().that(notesMigration.readChanges()).isTrue(); + StagedPreChange spc = + stagePreChange( + "refs/for/master", + users -> ImmutableList.of("r=nobody1@example.com,cc=nobody2@example.com")); + spc.supportReviewersByEmail = true; + assertThat(sender) + .sent("newchange", spc) + .to("nobody1@example.com") + .to(spc.watchingProjectOwner) + .cc("nobody2@example.com") + .bcc(NEW_CHANGES, NEW_PATCHSETS) + .noOneElse(); + } + /* * DeleteReviewerSender tests. */ @@ -1730,6 +1746,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("newpatchset", sc) .notTo(sc.owner) // TODO(logan): This shouldn't be sent *from* the owner. .to(sc.reviewer) + .to(other) .cc(sc.ccer) .cc(sc.reviewerByEmail, sc.ccerByEmail) .noOneElse(); @@ -1745,6 +1762,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("newpatchset", sc) .notTo(sc.owner) // TODO(logan): This shouldn't be sent *from* the owner. .to(sc.reviewer, sc.ccer) + .to(other) .noOneElse(); } @@ -1758,6 +1776,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("newpatchset", sc) .notTo(sc.owner) // TODO(logan): This shouldn't be sent *from* the owner. .to(sc.reviewer) + .to(other) .cc(sc.ccer) .cc(sc.reviewerByEmail, sc.ccerByEmail) .noOneElse(); @@ -1772,6 +1791,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { assertThat(sender) .sent("newpatchset", sc) .to(sc.reviewer, sc.ccer) + .to(other) .notTo(sc.owner) // TODO(logan): This shouldn't be sent *from* the owner. .noOneElse(); }