opendev/gerrit
Code Issues Proposed changes

Support arbitrary reviewer strings in push options

Browse Source 
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
This commit is contained in:
Dave Borowitz 2018-10-09 14:16:04 -07:00
parent 2c60ec21bd
commit 1531f6101a
10 changed files with 257 additions and 95 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
java/com/google/gerrit/acceptance/AbstractNotificationTest.java
View File

@ -319,7 +319,8 @@ public abstract class AbstractNotificationTest extends AbstractDaemonTest {
public final String ccerByEmail = "ccByEmail@example.com";
private final Map<NotifyType, TestAccount> watchers = new HashMap<>();
private final Map<String, TestAccount> accountsByEmail = new HashMap<>();
boolean supportReviewersByEmail;
public boolean supportReviewersByEmail;
private String usersCacheKey() {
return description.getClassName();

23
java/com/google/gerrit/server/change/ChangeInserter.java
View File

@ -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<Account.Id> reviewers, Iterable<Account.Id> ccs) {
return setReviewersAndCcsAsStrings(
Iterables.transform(reviewers, Account.Id::toString),
Iterables.transform(ccs, Account.Id::toString));
}
public ChangeInserter setReviewersAndCcsAsStrings(
Iterable<String> reviewers, Iterable<String> 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<ReviewerAddition> 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

60
java/com/google/gerrit/server/change/ReviewerAdder.java
View File

@ -148,7 +148,6 @@ public class ReviewerAdder {
private final GroupResolver groupResolver;
private final GroupMembers groupMembers;
private final AccountLoader.Factory accountLoaderFactory;
private final Provider<ReviewDb> 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<ReviewDb> 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<RecipientType, Account.Id> 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<RecipientType, Account.Id> 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<RecipientType, Account.Id> 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<CurrentUser> which returns anonymous in that path.
group = groupResolver.parseInternal(input.reviewer);
} catch (UnprocessableEntityException e) {
if (!allowByEmail) {
@ -344,7 +348,7 @@ public class ReviewerAdder {
Set<Account.Id> reviewers = new HashSet<>();
Set<Account> 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<Account.Id> reviewers,
@Nullable Iterable<Address> 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<Account.Id> omitOwner(ChangeNotes notes, Iterable<Account.Id> 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<? extends AddReviewerInput> inputs,
@ -572,7 +590,7 @@ public class ReviewerAdder {
.collect(toImmutableList());
List<ReviewerAddition> 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);
}

80
java/com/google/gerrit/server/git/receive/ReceiveCommits.java
View File

@ -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<Account.Id> reviewer = Sets.newLinkedHashSet();
Set<Account.Id> cc = Sets.newLinkedHashSet();
Set<String> reviewer = Sets.newLinkedHashSet();
Set<String> cc = Sets.newLinkedHashSet();
Map<String, Short> labels = new HashMap<>();
String message;
List<RevCommit> 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.
*
* <p>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<String> getCombinedReviewers(MailRecipients additionalRecipients) {
return getCombinedReviewers(reviewer, additionalRecipients.getReviewers());
}
/**
* Get CC strings from magic branch options, combined with additional recipients computed from
* some other place.
*
* <p>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<String> getCombinedCcs(MailRecipients additionalRecipients) {
return getCombinedReviewers(cc, additionalRecipients.getCcOnly());
}
private static ImmutableSet<String> getCombinedReviewers(
Set<String> strings, Set<Account.Id> ids) {
return Streams.concat(strings.stream(), ids.stream().map(Account.Id::toString))
.collect(toImmutableSet());
}
ListMultimap<RecipientType, Account.Id> getAccountsToNotify() {
@ -2392,12 +2428,16 @@ class ReceiveCommits {
final PatchSet.Id psId = ins.setGroups(groups).getPatchSetId();
Account.Id me = user.getAccountId();
List<FooterLine> 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<String, Short> 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())

85
java/com/google/gerrit/server/git/receive/ReplaceOp.java
View File

@ -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<String> groups;
private final Map<String, Short> 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.<String>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<PatchSetApproval> 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<ReviewerAddition> 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<InternalAddReviewerInput> getReviewerInputs(
MailRecipients recipients, Change change, PatchSetInfo psInfo) {
private static ImmutableList<AddReviewerInput> 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<AddReviewerInput> 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(

3
java/com/google/gerrit/server/restapi/change/PostReview.java
View File

@ -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;

3
java/com/google/gerrit/server/restapi/change/PostReviewers.java
View File

@ -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;
}

2
java/com/google/gerrit/server/restapi/change/PutAssignee.java
View File

@ -124,7 +124,7 @@ public class PutAssignee extends RetryingRestModifyView<ChangeResource, Assignee
reviewerInput.state = ReviewerState.CC;
reviewerInput.confirmed = true;
reviewerInput.notify = NotifyHandling.NONE;
return reviewerAdder.prepare(rsrc.getNotes(), rsrc.getUser(), reviewerInput, false);
return reviewerAdder.prepare(db.get(), rsrc.getNotes(), rsrc.getUser(), reviewerInput, false);
}
@Override

71
javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
View File

@ -14,6 +14,7 @@
package com.google.gerrit.acceptance.git;
import static com.google.common.base.MoreObjects.firstNonNull;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth8.assertThat;
@ -35,6 +36,7 @@ import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static com.google.gerrit.server.project.testing.Util.category;
import static com.google.gerrit.server.project.testing.Util.value;
import static java.util.Comparator.comparing;
import static java.util.concurrent.TimeUnit.SECONDS;
import static java.util.stream.Collectors.joining;
import static java.util.stream.Collectors.toList;
@ -59,6 +61,7 @@ import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.groups.GroupInput;
import com.google.gerrit.extensions.api.projects.BranchInput;
import com.google.gerrit.extensions.api.projects.ConfigInput;
import com.google.gerrit.extensions.client.ChangeStatus;
import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
import com.google.gerrit.extensions.client.InheritableBoolean;
@ -571,7 +574,34 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
+ nonExistingEmail
+ ",cc="
+ user.email);
r.assertErrorStatus("user \"" + nonExistingEmail + "\" not found");
r.assertErrorStatus(nonExistingEmail + " does not identify a registered user or group");
}
@Test
public void pushForMasterWithCcByEmail() throws Exception {
ConfigInput conf = new ConfigInput();
conf.enableReviewerByEmail = InheritableBoolean.TRUE;
gApi.projects().name(project.get()).config(conf);
PushOneCommit.Result r =
pushTo("refs/for/master%cc=non.existing.1@example.com,cc=non.existing.2@example.com");
if (notesMigration.readChanges()) {
r.assertOkStatus();
ChangeInfo ci = get(r.getChangeId(), DETAILED_LABELS);
ImmutableList<AccountInfo> ccs =
firstNonNull(ci.reviewers.get(ReviewerState.CC), ImmutableList.<AccountInfo>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<AccountInfo> reviewers =
firstNonNull(ci.reviewers.get(ReviewerState.REVIEWER), ImmutableList.<AccountInfo>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

22
javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java
View File

@ -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();
}