Merge "Merge branch 'stable-2.15'"
diff --git a/java/com/google/gerrit/acceptance/AbstractNotificationTest.java b/java/com/google/gerrit/acceptance/AbstractNotificationTest.java
index a4945c7..7a30f0c 100644
--- a/java/com/google/gerrit/acceptance/AbstractNotificationTest.java
+++ b/java/com/google/gerrit/acceptance/AbstractNotificationTest.java
@@ -319,7 +319,8 @@
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();
diff --git a/java/com/google/gerrit/server/change/AddReviewersOp.java b/java/com/google/gerrit/server/change/AddReviewersOp.java
index e558d00..2ed0f20 100644
--- a/java/com/google/gerrit/server/change/AddReviewersOp.java
+++ b/java/com/google/gerrit/server/change/AddReviewersOp.java
@@ -15,6 +15,7 @@
package com.google.gerrit.server.change;
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.ImmutableList.toImmutableList;
import static com.google.gerrit.extensions.client.ReviewerState.CC;
@@ -163,6 +164,10 @@
this.accountsToNotify = accountsToNotify;
}
+ void setPatchSet(PatchSet patchSet) {
+ this.patchSet = checkNotNull(patchSet);
+ }
+
@Override
public boolean updateChange(ChangeContext ctx)
throws RestApiException, OrmException, IOException {
@@ -207,7 +212,9 @@
checkAdded();
- patchSet = psUtil.current(ctx.getDb(), ctx.getNotes());
+ if (patchSet == null) {
+ patchSet = checkNotNull(psUtil.current(ctx.getDb(), ctx.getNotes()));
+ }
return true;
}
diff --git a/java/com/google/gerrit/server/change/ChangeInserter.java b/java/com/google/gerrit/server/change/ChangeInserter.java
index 38c97f7..831d388 100644
--- a/java/com/google/gerrit/server/change/ChangeInserter.java
+++ b/java/com/google/gerrit/server/change/ChangeInserter.java
@@ -16,32 +16,42 @@
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.collect.ImmutableList.toImmutableList;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.gerrit.reviewdb.client.Change.INITIAL_PATCH_SET_ID;
+import static com.google.gerrit.server.change.ReviewerAdder.newAddReviewerInputFromCommitIdentity;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
-import static java.util.stream.Collectors.toSet;
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.NotifyHandling;
import com.google.gerrit.extensions.api.changes.RecipientType;
-import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.client.ReviewerState;
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;
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.PatchSetInfo;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.ChangeMessagesUtil;
import com.google.gerrit.server.PatchSetUtil;
+import com.google.gerrit.server.change.ReviewerAdder.InternalAddReviewerInput;
+import com.google.gerrit.server.change.ReviewerAdder.ReviewerAddition;
+import com.google.gerrit.server.change.ReviewerAdder.ReviewerAdditionList;
import com.google.gerrit.server.config.SendEmailExecutor;
import com.google.gerrit.server.events.CommitReceivedEvent;
import com.google.gerrit.server.extensions.events.CommentAdded;
@@ -50,11 +60,8 @@
import com.google.gerrit.server.git.validators.CommitValidationException;
import com.google.gerrit.server.git.validators.CommitValidators;
import com.google.gerrit.server.mail.send.CreateChangeSender;
-import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
-import com.google.gerrit.server.notedb.NotesMigration;
import com.google.gerrit.server.patch.PatchSetInfoFactory;
-import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ProjectCache;
@@ -71,12 +78,12 @@
import java.io.IOException;
import java.util.Collections;
import java.util.HashMap;
-import java.util.HashSet;
import java.util.List;
import java.util.Map;
-import java.util.Set;
+import java.util.Optional;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Future;
+import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
@@ -101,7 +108,7 @@
private final CommitValidators.Factory commitValidatorsFactory;
private final RevisionCreated revisionCreated;
private final CommentAdded commentAdded;
- private final NotesMigration migration;
+ private final ReviewerAdder reviewerAdder;
private final Change.Id changeId;
private final PatchSet.Id psId;
@@ -119,14 +126,13 @@
private boolean validate = true;
private NotifyHandling notify = NotifyHandling.ALL;
private ListMultimap<RecipientType, Account.Id> accountsToNotify = ImmutableListMultimap.of();
- private Set<Account.Id> reviewers;
- private Set<Account.Id> extraCC;
private Map<String, Short> approvals;
private RequestScopePropagator requestScopePropagator;
private boolean fireRevisionCreated;
private boolean sendMail;
private boolean updateRef;
private Change.Id revertOf;
+ private ImmutableList<InternalAddReviewerInput> reviewerInputs;
// Fields set during the insertion process.
private ReceiveCommand cmd;
@@ -136,6 +142,7 @@
private PatchSet patchSet;
private String pushCert;
private ProjectState projectState;
+ private ReviewerAdditionList reviewerAdditions;
@Inject
ChangeInserter(
@@ -150,7 +157,7 @@
CommitValidators.Factory commitValidatorsFactory,
CommentAdded commentAdded,
RevisionCreated revisionCreated,
- NotesMigration migration,
+ ReviewerAdder reviewerAdder,
@Assisted Change.Id changeId,
@Assisted ObjectId commitId,
@Assisted String refName) {
@@ -165,14 +172,13 @@
this.commitValidatorsFactory = commitValidatorsFactory;
this.revisionCreated = revisionCreated;
this.commentAdded = commentAdded;
- this.migration = migration;
+ this.reviewerAdder = reviewerAdder;
this.changeId = changeId;
this.psId = new PatchSet.Id(changeId, INITIAL_PATCH_SET_ID);
this.commitId = commitId.copy();
this.refName = refName;
- this.reviewers = Collections.emptySet();
- this.extraCC = Collections.emptySet();
+ this.reviewerInputs = ImmutableList.of();
this.approvals = Collections.emptyMap();
this.fireRevisionCreated = true;
this.sendMail = true;
@@ -261,13 +267,22 @@
return this;
}
- public ChangeInserter setReviewers(Set<Account.Id> reviewers) {
- this.reviewers = reviewers;
- return this;
+ 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 setExtraCC(Set<Account.Id> extraCC) {
- this.extraCC = extraCC;
+ public ChangeInserter setReviewersAndCcsAsStrings(
+ Iterable<String> reviewers, Iterable<String> ccs) {
+ reviewerInputs =
+ Streams.concat(
+ Streams.stream(reviewers)
+ .distinct()
+ .map(id -> newAddReviewerInput(id, ReviewerState.REVIEWER)),
+ Streams.stream(ccs).distinct().map(id -> newAddReviewerInput(id, ReviewerState.CC)))
+ .collect(toImmutableList());
return this;
}
@@ -371,7 +386,8 @@
@Override
public boolean updateChange(ChangeContext ctx)
- throws RestApiException, OrmException, IOException, PermissionBackendException {
+ throws RestApiException, OrmException, IOException, PermissionBackendException,
+ ConfigInvalidException {
change = ctx.getChange(); // Use defensive copy created by ChangeControl.
ReviewDb db = ctx.getDb();
patchSetInfo =
@@ -415,29 +431,23 @@
*/
update.fixStatus(change.getStatus());
- Set<Account.Id> reviewersToAdd = new HashSet<>(reviewers);
- if (migration.readChanges()) {
- approvalsUtil.addCcs(
- ctx.getNotes(), update, filterOnChangeVisibility(db, ctx.getNotes(), extraCC));
- } else {
- reviewersToAdd.addAll(extraCC);
+ reviewerAdditions =
+ reviewerAdder.prepare(
+ ctx.getDb(), ctx.getNotes(), ctx.getUser(), getReviewerInputs(), true);
+ Optional<ReviewerAddition> reviewerError = reviewerAdditions.getFailures().stream().findFirst();
+ if (reviewerError.isPresent()) {
+ throw new UnprocessableEntityException(reviewerError.get().result.error);
}
+ reviewerAdditions.updateChange(ctx, patchSet);
LabelTypes labelTypes = projectState.getLabelTypes();
- approvalsUtil.addReviewers(
- db,
- update,
- labelTypes,
- change,
- patchSet,
- patchSetInfo,
- filterOnChangeVisibility(db, ctx.getNotes(), reviewersToAdd),
- Collections.<Account.Id>emptySet());
approvalsUtil.addApprovalsForNewPatchSet(
db, update, labelTypes, patchSet, ctx.getUser(), approvals);
+
// Check if approvals are changing in with this update. If so, add current user to reviewers.
// Note that this is done separately as addReviewers is filtering out the change owner as
// reviewer which is needed in several other code paths.
+ // TODO(dborowitz): Still necessary?
if (!approvals.isEmpty()) {
update.putReviewer(ctx.getAccountId(), REVIEWER);
}
@@ -454,37 +464,9 @@
return true;
}
- private Set<Account.Id> filterOnChangeVisibility(
- final ReviewDb db, ChangeNotes notes, Set<Account.Id> accounts) {
- return accounts
- .stream()
- .filter(
- accountId -> {
- if (!projectState.statePermitsRead()) {
- return false;
- }
-
- try {
- permissionBackend
- .absentUser(accountId)
- .change(notes)
- .database(db)
- .check(ChangePermission.READ);
- return true;
- } catch (PermissionBackendException e) {
- logger.atWarning().withCause(e).log(
- "Failed to check if account %d can see change %d",
- accountId.get(), notes.getChangeId().get());
- return false;
- } catch (AuthException e) {
- return false;
- }
- })
- .collect(toSet());
- }
-
@Override
- public void postUpdate(Context ctx) throws OrmException, IOException {
+ public void postUpdate(Context ctx) throws Exception {
+ reviewerAdditions.postUpdate(ctx);
if (sendMail && (notify != NotifyHandling.NONE || !accountsToNotify.isEmpty())) {
Runnable sender =
new Runnable() {
@@ -497,8 +479,17 @@
cm.setPatchSet(patchSet, patchSetInfo);
cm.setNotify(notify);
cm.setAccountsToNotify(accountsToNotify);
- cm.addReviewers(reviewers);
- cm.addExtraCC(extraCC);
+ cm.addReviewers(
+ reviewerAdditions
+ .flattenResults(AddReviewersOp.Result::addedReviewers)
+ .stream()
+ .map(PatchSetApproval::getAccountId)
+ .collect(toImmutableSet()));
+ cm.addReviewersByEmail(
+ reviewerAdditions.flattenResults(AddReviewersOp.Result::addedReviewersByEmail));
+ cm.addExtraCC(reviewerAdditions.flattenResults(AddReviewersOp.Result::addedCCs));
+ cm.addExtraCCByEmail(
+ reviewerAdditions.flattenResults(AddReviewersOp.Result::addedCCsByEmail));
cm.send();
} catch (Exception e) {
logger.atSevere().withCause(e).log(
@@ -575,4 +566,33 @@
throw new ResourceConflictException(e.getFullMessage());
}
}
+
+ private static InternalAddReviewerInput newAddReviewerInput(
+ 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(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
+ // certain commit footers: putting a nonexistent user in a footer should not cause an error. In
+ // theory we could provide finer control to do this for some reviewers and not others, but it's
+ // not worth complicating the ChangeInserter interface further at this time.
+ input.otherFailureBehavior = ReviewerAdder.FailureBehavior.IGNORE;
+
+ return input;
+ }
+
+ private ImmutableList<InternalAddReviewerInput> getReviewerInputs() {
+ return Streams.concat(
+ reviewerInputs.stream(),
+ Streams.stream(
+ newAddReviewerInputFromCommitIdentity(
+ change, patchSetInfo.getAuthor().getAccount(), NotifyHandling.NONE)),
+ Streams.stream(
+ newAddReviewerInputFromCommitIdentity(
+ change, patchSetInfo.getCommitter().getAccount(), NotifyHandling.NONE)))
+ .collect(toImmutableList());
+ }
}
diff --git a/java/com/google/gerrit/server/change/ReviewerAdder.java b/java/com/google/gerrit/server/change/ReviewerAdder.java
index f2be90d..441a7d4 100644
--- a/java/com/google/gerrit/server/change/ReviewerAdder.java
+++ b/java/com/google/gerrit/server/change/ReviewerAdder.java
@@ -17,14 +17,19 @@
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.ImmutableList.toImmutableList;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.gerrit.extensions.client.ReviewerState.CC;
import static com.google.gerrit.extensions.client.ReviewerState.REVIEWER;
+import static java.util.Comparator.comparing;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ListMultimap;
import com.google.common.collect.Lists;
+import com.google.common.collect.Ordering;
+import com.google.common.collect.Streams;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.data.GroupDescription;
import com.google.gerrit.extensions.api.changes.AddReviewerInput;
@@ -36,12 +41,15 @@
import com.google.gerrit.extensions.common.AccountInfo;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
+import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.mail.Address;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.BooleanProjectConfig;
import com.google.gerrit.reviewdb.client.Branch;
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.AnonymousUser;
@@ -63,13 +71,20 @@
import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.update.ChangeContext;
+import com.google.gerrit.server.update.Context;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import java.io.IOException;
import java.text.MessageFormat;
+import java.util.ArrayList;
+import java.util.Collection;
import java.util.HashSet;
+import java.util.List;
+import java.util.Optional;
import java.util.Set;
+import java.util.function.Function;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.Config;
@@ -77,12 +92,62 @@
public static final int DEFAULT_MAX_REVIEWERS_WITHOUT_CHECK = 10;
public static final int DEFAULT_MAX_REVIEWERS = 20;
+ public enum FailureBehavior {
+ FAIL,
+ IGNORE;
+ }
+
+ private enum FailureType {
+ NOT_FOUND,
+ OTHER;
+ }
+
+ // TODO(dborowitz): Subclassing is not the right way to do this. We should instead use an internal
+ // type in the public interfaces of ReviewerAdder, rather than passing around the REST API type
+ // internally.
+ public static class InternalAddReviewerInput extends AddReviewerInput {
+ /**
+ * Behavior when identifying reviewers fails for any reason <em>besides</em> the input not
+ * resolving to an account/group/email.
+ */
+ public FailureBehavior otherFailureBehavior = FailureBehavior.FAIL;
+ }
+
+ public static InternalAddReviewerInput newAddReviewerInput(
+ Account.Id reviewer, ReviewerState state, NotifyHandling notify) {
+ // AccountResolver always resolves by ID if the input string is numeric.
+ return newAddReviewerInput(reviewer.toString(), state, notify);
+ }
+
+ public static InternalAddReviewerInput newAddReviewerInput(
+ String reviewer, ReviewerState state, NotifyHandling notify) {
+ InternalAddReviewerInput in = new InternalAddReviewerInput();
+ in.reviewer = reviewer;
+ in.state = state;
+ in.notify = notify;
+ return in;
+ }
+
+ public static Optional<InternalAddReviewerInput> newAddReviewerInputFromCommitIdentity(
+ Change change, @Nullable Account.Id accountId, NotifyHandling notify) {
+ if (accountId == null || accountId.equals(change.getOwner())) {
+ // If git ident couldn't be resolved to a user, or if it's not forged, do nothing.
+ return Optional.empty();
+ }
+
+ InternalAddReviewerInput in = new InternalAddReviewerInput();
+ in.reviewer = accountId.toString();
+ in.state = REVIEWER;
+ in.notify = notify;
+ in.otherFailureBehavior = FailureBehavior.IGNORE;
+ return Optional.of(in);
+ }
+
private final AccountResolver accountResolver;
private final PermissionBackend permissionBackend;
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;
@@ -99,7 +164,6 @@
GroupResolver groupResolver,
GroupMembers groupMembers,
AccountLoader.Factory accountLoaderFactory,
- Provider<ReviewDb> db,
@GerritServerConfig Config cfg,
ReviewerJson json,
NotesMigration migration,
@@ -113,7 +177,6 @@
this.groupResolver = groupResolver;
this.groupMembers = groupMembers;
this.accountLoaderFactory = accountLoaderFactory;
- this.dbProvider = db;
this.cfg = cfg;
this.json = json;
this.migration = migration;
@@ -127,6 +190,7 @@
/**
* 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.
@@ -140,41 +204,29 @@
* @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();
- String reviewer = checkNotNull(input.reviewer);
- ReviewerState state = input.state();
- NotifyHandling notify = input.notify;
+ checkNotNull(input.reviewer);
ListMultimap<RecipientType, Account.Id> accountsToNotify;
try {
accountsToNotify = notifyUtil.resolveAccounts(input.notifyDetails);
} catch (BadRequestException e) {
- return fail(reviewer, e.getMessage());
+ return fail(input, FailureType.OTHER, e.getMessage());
}
boolean confirmed = input.confirmed();
boolean allowByEmail =
projectCache
- .checkedGet(dest.getParentKey())
+ .checkedGet(notes.getProjectName())
.is(BooleanProjectConfig.ENABLE_REVIEWER_BY_EMAIL);
ReviewerAddition byAccountId =
- addByAccountId(
- reviewer, dest, user, state, notify, accountsToNotify, allowGroup, allowByEmail);
+ addByAccountId(db, input, notes, user, accountsToNotify, allowGroup, allowByEmail);
ReviewerAddition wholeGroup = null;
if (byAccountId == null || !byAccountId.exactMatchFound) {
wholeGroup =
addWholeGroup(
- reviewer,
- dest,
- user,
- state,
- notify,
- accountsToNotify,
- confirmed,
- allowGroup,
- allowByEmail);
+ db, input, notes, user, accountsToNotify, confirmed, allowGroup, allowByEmail);
if (wholeGroup != null && wholeGroup.exactMatchFound) {
return wholeGroup;
}
@@ -187,28 +239,26 @@
return wholeGroup;
}
- return addByEmail(reviewer, notes, user, state, notify, accountsToNotify);
+ return addByEmail(db, input, notes, user, accountsToNotify);
}
public ReviewerAddition ccCurrentUser(CurrentUser user, RevisionResource revision) {
return new ReviewerAddition(
- user.getUserName().orElse(null),
+ newAddReviewerInput(user.getUserName().orElse(null), CC, NotifyHandling.NONE),
+ revision.getNotes(),
revision.getUser(),
ImmutableSet.of(user.getAccountId()),
null,
- CC,
- NotifyHandling.NONE,
ImmutableListMultimap.of(),
true);
}
@Nullable
private ReviewerAddition addByAccountId(
- String reviewer,
- Branch.NameKey dest,
+ ReviewDb db,
+ AddReviewerInput input,
+ ChangeNotes notes,
CurrentUser user,
- ReviewerState state,
- NotifyHandling notify,
ListMultimap<RecipientType, Account.Id> accountsToNotify,
boolean allowGroup,
boolean allowByEmail)
@@ -216,9 +266,9 @@
IdentifiedUser reviewerUser;
boolean exactMatchFound = false;
try {
- reviewerUser = accountResolver.parse(reviewer);
- if (reviewer.equalsIgnoreCase(reviewerUser.getName())
- || reviewer.equals(String.valueOf(reviewerUser.getAccountId()))) {
+ reviewerUser = accountResolver.parse(input.reviewer);
+ if (input.reviewer.equalsIgnoreCase(reviewerUser.getName())
+ || input.reviewer.equals(String.valueOf(reviewerUser.getAccountId()))) {
exactMatchFound = true;
}
} catch (UnprocessableEntityException | AuthException e) {
@@ -226,39 +276,44 @@
if (!allowGroup && !allowByEmail) {
// Only return failure if we aren't going to try other interpretations.
return fail(
- reviewer, MessageFormat.format(ChangeMessages.get().reviewerNotFoundUser, reviewer));
+ input,
+ FailureType.NOT_FOUND,
+ MessageFormat.format(ChangeMessages.get().reviewerNotFoundUser, input.reviewer));
}
return null;
}
- if (isValidReviewer(dest, reviewerUser.getAccount())) {
+ if (isValidReviewer(db, notes.getChange().getDest(), reviewerUser.getAccount())) {
return new ReviewerAddition(
- reviewer,
+ input,
+ notes,
user,
ImmutableSet.of(reviewerUser.getAccountId()),
null,
- state,
- notify,
accountsToNotify,
exactMatchFound);
}
if (!reviewerUser.getAccount().isActive()) {
- if (allowByEmail && state == CC) {
+ if (allowByEmail && input.state() == CC) {
return null;
}
- return fail(reviewer, MessageFormat.format(ChangeMessages.get().reviewerInactive, reviewer));
+ return fail(
+ input,
+ FailureType.OTHER,
+ MessageFormat.format(ChangeMessages.get().reviewerInactive, input.reviewer));
}
return fail(
- reviewer, MessageFormat.format(ChangeMessages.get().reviewerCantSeeChange, reviewer));
+ input,
+ FailureType.OTHER,
+ MessageFormat.format(ChangeMessages.get().reviewerCantSeeChange, input.reviewer));
}
@Nullable
private ReviewerAddition addWholeGroup(
- String reviewer,
- Branch.NameKey dest,
+ ReviewDb db,
+ AddReviewerInput input,
+ ChangeNotes notes,
CurrentUser user,
- ReviewerState state,
- NotifyHandling notify,
ListMultimap<RecipientType, Account.Id> accountsToNotify,
boolean confirmed,
boolean allowGroup,
@@ -270,27 +325,32 @@
GroupDescription.Basic group;
try {
- group = groupResolver.parseInternal(reviewer);
+ // 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) {
return fail(
- reviewer,
- MessageFormat.format(ChangeMessages.get().reviewerNotFoundUserOrGroup, reviewer));
+ input,
+ FailureType.NOT_FOUND,
+ MessageFormat.format(ChangeMessages.get().reviewerNotFoundUserOrGroup, input.reviewer));
}
return null;
}
if (!isLegalReviewerGroup(group.getGroupUUID())) {
return fail(
- reviewer, MessageFormat.format(ChangeMessages.get().groupIsNotAllowed, group.getName()));
+ input,
+ FailureType.OTHER,
+ MessageFormat.format(ChangeMessages.get().groupIsNotAllowed, group.getName()));
}
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(reviewer, e.getMessage());
+ return fail(input, FailureType.OTHER, e.getMessage());
}
// if maxAllowed is set to 0, it is allowed to add any number of
@@ -298,7 +358,8 @@
int maxAllowed = cfg.getInt("addreviewer", "maxAllowed", DEFAULT_MAX_REVIEWERS);
if (maxAllowed > 0 && members.size() > maxAllowed) {
return fail(
- reviewer,
+ input,
+ FailureType.OTHER,
MessageFormat.format(ChangeMessages.get().groupHasTooManyMembers, group.getName()));
}
@@ -307,56 +368,62 @@
cfg.getInt("addreviewer", "maxWithoutConfirmation", DEFAULT_MAX_REVIEWERS_WITHOUT_CHECK);
if (!confirmed && maxWithoutConfirmation > 0 && members.size() > maxWithoutConfirmation) {
return fail(
- reviewer,
+ input,
+ FailureType.OTHER,
true,
MessageFormat.format(
ChangeMessages.get().groupManyMembersConfirmation, group.getName(), members.size()));
}
for (Account member : members) {
- if (isValidReviewer(dest, member)) {
+ if (isValidReviewer(db, notes.getChange().getDest(), member)) {
reviewers.add(member.getId());
}
}
- return new ReviewerAddition(
- reviewer, user, reviewers, null, state, notify, accountsToNotify, true);
+ return new ReviewerAddition(input, notes, user, reviewers, null, accountsToNotify, true);
}
@Nullable
private ReviewerAddition addByEmail(
- String reviewer,
+ ReviewDb db,
+ AddReviewerInput input,
ChangeNotes notes,
CurrentUser user,
- ReviewerState state,
- NotifyHandling notify,
ListMultimap<RecipientType, Account.Id> accountsToNotify)
throws PermissionBackendException {
try {
permissionBackend
.user(anonymousProvider.get())
- .database(dbProvider)
+ .database(db)
.change(notes)
.check(ChangePermission.READ);
} catch (AuthException e) {
return fail(
- reviewer, MessageFormat.format(ChangeMessages.get().reviewerCantSeeChange, reviewer));
+ input,
+ FailureType.OTHER,
+ MessageFormat.format(ChangeMessages.get().reviewerCantSeeChange, input.reviewer));
}
if (!migration.readChanges()) {
// addByEmail depends on NoteDb.
return fail(
- reviewer, MessageFormat.format(ChangeMessages.get().reviewerNotFoundUser, reviewer));
+ input,
+ FailureType.NOT_FOUND,
+ MessageFormat.format(ChangeMessages.get().reviewerNotFoundUser, input.reviewer));
}
- Address adr = Address.tryParse(reviewer);
+ Address adr = Address.tryParse(input.reviewer);
if (adr == null || !validator.isValid(adr.getEmail())) {
- return fail(reviewer, MessageFormat.format(ChangeMessages.get().reviewerInvalid, reviewer));
+ return fail(
+ input,
+ FailureType.NOT_FOUND,
+ MessageFormat.format(ChangeMessages.get().reviewerInvalid, input.reviewer));
}
return new ReviewerAddition(
- reviewer, user, null, ImmutableList.of(adr), state, notify, accountsToNotify, true);
+ 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;
@@ -368,7 +435,7 @@
// see private changes.
permissionBackend
.absentUser(member.getId())
- .database(dbProvider)
+ .database(db)
.ref(branch)
.check(RefPermission.READ);
return true;
@@ -377,12 +444,13 @@
}
}
- private ReviewerAddition fail(String reviewer, String error) {
- return fail(reviewer, false, error);
+ private ReviewerAddition fail(AddReviewerInput input, FailureType failureType, String error) {
+ return fail(input, failureType, false, error);
}
- private ReviewerAddition fail(String reviewer, boolean confirm, String error) {
- ReviewerAddition addition = new ReviewerAddition(reviewer);
+ private ReviewerAddition fail(
+ AddReviewerInput input, FailureType failureType, boolean confirm, String error) {
+ ReviewerAddition addition = new ReviewerAddition(input, failureType);
addition.result.confirm = confirm ? true : null;
addition.result.error = error;
return addition;
@@ -393,45 +461,57 @@
@Nullable public final AddReviewersOp op;
public final ImmutableSet<Account.Id> reviewers;
public final ImmutableSet<Address> reviewersByEmail;
- public final ReviewerState state;
@Nullable final IdentifiedUser caller;
final boolean exactMatchFound;
+ private final AddReviewerInput input;
+ @Nullable private final FailureType failureType;
- private ReviewerAddition(String reviewer) {
- result = new AddReviewerResult(reviewer);
+ private ReviewerAddition(AddReviewerInput input, FailureType failureType) {
+ this.input = input;
+ this.failureType = checkNotNull(failureType);
+ result = new AddReviewerResult(input.reviewer);
op = null;
reviewers = ImmutableSet.of();
reviewersByEmail = ImmutableSet.of();
- state = REVIEWER;
caller = null;
exactMatchFound = false;
}
private ReviewerAddition(
- String reviewer,
+ AddReviewerInput input,
+ ChangeNotes notes,
CurrentUser caller,
@Nullable Iterable<Account.Id> reviewers,
@Nullable Iterable<Address> reviewersByEmail,
- ReviewerState state,
- @Nullable NotifyHandling notify,
ListMultimap<RecipientType, Account.Id> accountsToNotify,
boolean exactMatchFound) {
checkArgument(
reviewers != null || reviewersByEmail != null,
"must have either reviewers or reviewersByEmail");
- result = new AddReviewerResult(reviewer);
- this.reviewers = reviewers == null ? ImmutableSet.of() : ImmutableSet.copyOf(reviewers);
+ this.input = input;
+ this.failureType = null;
+ result = new AddReviewerResult(input.reviewer);
+ // 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.state = state;
this.caller = caller.asIdentifiedUser();
op =
addReviewersOpFactory.create(
- this.reviewers, this.reviewersByEmail, state, notify, accountsToNotify);
+ this.reviewers, this.reviewersByEmail, state(), input.notify, accountsToNotify);
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");
@@ -439,7 +519,7 @@
// Generate result details and fill AccountLoader. This occurs outside
// the Op because the accounts are in a different table.
AddReviewersOp.Result opResult = op.getResult();
- if (migration.readChanges() && state == CC) {
+ if (migration.readChanges() && state() == CC) {
result.ccs = Lists.newArrayListWithCapacity(opResult.addedCCs().size());
for (Account.Id accountId : opResult.addedCCs()) {
result.ccs.add(json.format(new ReviewerInfo(accountId.get()), accountId, cd));
@@ -465,9 +545,123 @@
}
}
}
+
+ public ReviewerState state() {
+ return input.state();
+ }
+
+ public boolean isFailure() {
+ return failureType != null;
+ }
+
+ public boolean isIgnorableFailure() {
+ checkState(failureType != null);
+ FailureBehavior behavior =
+ (input instanceof InternalAddReviewerInput)
+ ? ((InternalAddReviewerInput) input).otherFailureBehavior
+ : FailureBehavior.FAIL;
+ return failureType == FailureType.OTHER && behavior == FailureBehavior.IGNORE;
+ }
}
public static boolean isLegalReviewerGroup(AccountGroup.UUID groupUUID) {
return !SystemGroupBackend.isSystemGroup(groupUUID);
}
+
+ public ReviewerAdditionList prepare(
+ ReviewDb db,
+ ChangeNotes notes,
+ CurrentUser user,
+ Iterable<? extends AddReviewerInput> inputs,
+ boolean allowGroup)
+ throws OrmException, IOException, PermissionBackendException, ConfigInvalidException {
+ // Process CC ops before reviewer ops, so a user that appears in both lists ends up as a
+ // reviewer; the last call to ChangeUpdate#putReviewer wins. This can happen if the caller
+ // specifies the same string twice, or less obviously if they specify multiple groups with
+ // overlapping members.
+ // TODO(dborowitz): Consider changing interface to allow excluding reviewers that were
+ // previously processed, to proactively prevent overlap so we don't have to rely on this subtle
+ // behavior.
+ ImmutableList<AddReviewerInput> sorted =
+ Streams.stream(inputs)
+ .sorted(
+ comparing(
+ i -> i.state(), Ordering.explicit(ReviewerState.CC, ReviewerState.REVIEWER)))
+ .collect(toImmutableList());
+ List<ReviewerAddition> additions = new ArrayList<>();
+ for (AddReviewerInput input : sorted) {
+ additions.add(prepare(db, notes, user, input, allowGroup));
+ }
+ return new ReviewerAdditionList(additions);
+ }
+
+ // TODO(dborowitz): This class works, but ultimately feels wrong. It seems like an op but isn't
+ // really an op, it's a collection of ops, and it's only called from the body of other ops. We
+ // could make this class an op, but we would still have AddReviewersOp. Better would probably be
+ // to design a single op that supports combining multiple AddReviewerInputs together. That would
+ // probably also subsume the Addition class itself, which would be a good thing.
+ public static class ReviewerAdditionList {
+ private final ImmutableList<ReviewerAddition> additions;
+
+ private ReviewerAdditionList(List<ReviewerAddition> additions) {
+ this.additions = ImmutableList.copyOf(additions);
+ }
+
+ public ImmutableList<ReviewerAddition> getFailures() {
+ return additions
+ .stream()
+ .filter(a -> a.isFailure() && !a.isIgnorableFailure())
+ .collect(toImmutableList());
+ }
+
+ // We never call updateRepo on the addition ops, which is only ok because it's a no-op.
+
+ public void updateChange(ChangeContext ctx, PatchSet patchSet)
+ throws OrmException, RestApiException, IOException {
+ for (ReviewerAddition addition : additions()) {
+ addition.op.setPatchSet(patchSet);
+ addition.op.updateChange(ctx);
+ }
+ }
+
+ public void postUpdate(Context ctx) throws Exception {
+ for (ReviewerAddition addition : additions()) {
+ if (addition.op != null) {
+ addition.op.postUpdate(ctx);
+ }
+ }
+ }
+
+ public <T> ImmutableSet<T> flattenResults(
+ Function<AddReviewersOp.Result, ? extends Collection<T>> func) {
+ additions()
+ .forEach(
+ a ->
+ checkArgument(
+ a.op != null && a.op.getResult() != null, "missing result on %s", a));
+ return additions()
+ .stream()
+ .map(a -> a.op.getResult())
+ .map(func)
+ .flatMap(Collection::stream)
+ .collect(toImmutableSet());
+ }
+
+ private ImmutableList<ReviewerAddition> additions() {
+ return additions
+ .stream()
+ .filter(
+ a -> {
+ if (a.isFailure()) {
+ if (a.isIgnorableFailure()) {
+ return false;
+ }
+ // Shouldn't happen, caller should have checked that there were no errors.
+ throw new IllegalStateException("error in addition: " + a.result.error);
+ }
+ return true;
+ })
+ .collect(toImmutableList());
+ }
+ }
}
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index 2e51c17..6e800d0 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.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.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.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.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 @@
} 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 @@
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 @@
@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 @@
: 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 @@
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,8 +2448,9 @@
}
bu.insertChange(
- ins.setReviewers(recipients.getReviewers())
- .setExtraCC(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 2a46f3b..a580cf6 100644
--- a/java/com/google/gerrit/server/git/receive/ReplaceOp.java
+++ b/java/com/google/gerrit/server/git/receive/ReplaceOp.java
@@ -14,7 +14,10 @@
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;
import static com.google.gerrit.server.mail.MailUtil.getRecipientsFromReviewers;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
@@ -23,12 +26,16 @@
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
+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.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.RestApiException;
+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;
@@ -43,8 +50,13 @@
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;
+import com.google.gerrit.server.change.ReviewerAdder.InternalAddReviewerInput;
+import com.google.gerrit.server.change.ReviewerAdder.ReviewerAddition;
+import com.google.gerrit.server.change.ReviewerAdder.ReviewerAdditionList;
import com.google.gerrit.server.config.SendEmailExecutor;
import com.google.gerrit.server.extensions.events.CommentAdded;
import com.google.gerrit.server.extensions.events.RevisionCreated;
@@ -75,6 +87,8 @@
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;
import org.eclipse.jgit.revwalk.RevWalk;
@@ -117,6 +131,7 @@
private final PatchSetUtil psUtil;
private final ReplacePatchSetSender.Factory replacePatchSetFactory;
private final ProjectCache projectCache;
+ private final ReviewerAdder reviewerAdder;
private final ProjectState projectState;
private final Branch.NameKey dest;
@@ -131,7 +146,6 @@
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;
@@ -142,6 +156,8 @@
private String rejectMessage;
private MergedByPushOp mergedByPushOp;
private RequestScopePropagator requestScopePropagator;
+ private ReviewerAdditionList reviewerAdditions;
+ private MailRecipients oldRecipients;
@Inject
ReplaceOp(
@@ -161,6 +177,7 @@
ReplacePatchSetSender.Factory replacePatchSetFactory,
ProjectCache projectCache,
@SendEmailExecutor ExecutorService sendEmailExecutor,
+ ReviewerAdder reviewerAdder,
@Assisted ProjectState projectState,
@Assisted Branch.NameKey dest,
@Assisted boolean checkMergedInto,
@@ -188,6 +205,7 @@
this.replacePatchSetFactory = replacePatchSetFactory;
this.projectCache = projectCache;
this.sendEmailExecutor = sendEmailExecutor;
+ this.reviewerAdder = reviewerAdder;
this.projectState = projectState;
this.dest = dest;
@@ -228,7 +246,8 @@
@Override
public boolean updateChange(ChangeContext ctx)
- throws RestApiException, OrmException, IOException, PermissionBackendException {
+ throws RestApiException, OrmException, IOException, PermissionBackendException,
+ ConfigInvalidException {
notes = ctx.getNotes();
Change change = notes.getChange();
if (change == null || change.getStatus().isClosed()) {
@@ -240,13 +259,15 @@
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);
@@ -294,10 +315,7 @@
psDescription);
update.setPsDescription(psDescription);
- recipients.add(getRecipientsFromFooters(accountResolver, commit.getFooterLines()));
- recipients.remove(ctx.getAccountId());
- ChangeData cd = changeDataFactory.create(ctx.getDb(), ctx.getNotes());
- MailRecipients oldRecipients = getRecipientsFromReviewers(cd.reviewers());
+ MailRecipients fromFooters = getRecipientsFromFooters(accountResolver, commit.getFooterLines());
Iterable<PatchSetApproval> newApprovals =
approvalsUtil.addApprovalsForNewPatchSet(
ctx.getDb(),
@@ -313,15 +331,19 @@
ctx.getRevWalk(),
ctx.getRepoView().getConfig(),
newApprovals);
- approvalsUtil.addReviewers(
- ctx.getDb(),
- update,
- projectState.getLabelTypes(),
- change,
- newPatchSet,
- info,
- recipients.getReviewers(),
- oldRecipients.getAll());
+
+ reviewerAdditions =
+ reviewerAdder.prepare(
+ ctx.getDb(),
+ ctx.getNotes(),
+ ctx.getUser(),
+ getReviewerInputs(magicBranch, fromFooters, ctx.getChange(), info),
+ true);
+ Optional<ReviewerAddition> reviewerError = reviewerAdditions.getFailures().stream().findFirst();
+ if (reviewerError.isPresent()) {
+ throw new UnprocessableEntityException(reviewerError.get().result.error);
+ }
+ reviewerAdditions.updateChange(ctx, newPatchSet);
// Check if approvals are changing in with this update. If so, add current user to reviewers.
// Note that this is done separately as addReviewers is filtering out the change owner as
@@ -330,8 +352,6 @@
update.putReviewer(ctx.getAccountId(), REVIEWER);
}
- recipients.add(oldRecipients);
-
msg = createChangeMessage(ctx, reviewMessage);
cmUtil.addChangeMessage(ctx.getDb(), update, msg);
@@ -344,6 +364,51 @@
return true;
}
+ 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.
+ Stream<AddReviewerInput> inputs =
+ Streams.concat(
+ Streams.stream(
+ newAddReviewerInputFromCommitIdentity(
+ change, psInfo.getAuthor().getAccount(), NotifyHandling.NONE)),
+ Streams.stream(
+ newAddReviewerInputFromCommitIdentity(
+ 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(
+ 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(reviewer, state, NotifyHandling.NONE);
+
+ // Ignore failures for reasons like the reviewer being inactive or being unable to see the
+ // change. See discussion in ChangeInserter.
+ input.otherFailureBehavior = ReviewerAdder.FailureBehavior.IGNORE;
+
+ return input;
+ }
+
private ChangeMessage createChangeMessage(ChangeContext ctx, String reviewMessage)
throws OrmException, IOException {
String approvalMessage =
@@ -455,6 +520,7 @@
@Override
public void postUpdate(Context ctx) throws Exception {
+ reviewerAdditions.postUpdate(ctx);
if (changeKind != ChangeKind.TRIVIAL_REBASE) {
// TODO(dborowitz): Merge email templates so we only have to send one.
Runnable e = new ReplaceEmailTask(ctx);
@@ -513,8 +579,20 @@
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/git/receive/ResultChangeIds.java b/java/com/google/gerrit/server/git/receive/ResultChangeIds.java
index 4099e14..bbf8d95 100644
--- a/java/com/google/gerrit/server/git/receive/ResultChangeIds.java
+++ b/java/com/google/gerrit/server/git/receive/ResultChangeIds.java
@@ -27,7 +27,7 @@
* <p>This class is thread-safe.
*/
public class ResultChangeIds {
- enum Key {
+ public enum Key {
CREATED,
REPLACED,
AUTOCLOSED,
diff --git a/java/com/google/gerrit/server/restapi/change/CherryPickChange.java b/java/com/google/gerrit/server/restapi/change/CherryPickChange.java
index 65c652d..5061926 100644
--- a/java/com/google/gerrit/server/restapi/change/CherryPickChange.java
+++ b/java/com/google/gerrit/server/restapi/change/CherryPickChange.java
@@ -381,7 +381,7 @@
reviewers.remove(user.get().getAccountId());
Set<Account.Id> ccs = new HashSet<>(reviewerSet.byState(ReviewerStateInternal.CC));
ccs.remove(user.get().getAccountId());
- ins.setReviewers(reviewers).setExtraCC(ccs);
+ ins.setReviewersAndCcs(reviewers, ccs);
}
bu.insertChange(ins);
return changeId;
diff --git a/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java
index e47c582..8b5ed68 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 @@
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;
@@ -443,10 +444,10 @@
List<Address> toByEmail = new ArrayList<>();
List<Address> ccByEmail = new ArrayList<>();
for (ReviewerAddition addition : reviewerAdditions) {
- if (addition.state == ReviewerState.REVIEWER) {
+ if (addition.state() == ReviewerState.REVIEWER) {
to.addAll(addition.reviewers);
toByEmail.addAll(addition.reviewersByEmail);
- } else if (addition.state == ReviewerState.CC) {
+ } else if (addition.state() == ReviewerState.CC) {
cc.addAll(addition.reviewers);
ccByEmail.addAll(addition.reviewersByEmail);
}
diff --git a/java/com/google/gerrit/server/restapi/change/PostReviewers.java b/java/com/google/gerrit/server/restapi/change/PostReviewers.java
index 9147d44..3e66189 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 @@
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 7ca674f..7878ce5 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 @@
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
diff --git a/java/com/google/gerrit/server/restapi/change/Revert.java b/java/com/google/gerrit/server/restapi/change/Revert.java
index 6383b29..7309fde 100644
--- a/java/com/google/gerrit/server/restapi/change/Revert.java
+++ b/java/com/google/gerrit/server/restapi/change/Revert.java
@@ -237,11 +237,9 @@
reviewers.add(changeToRevert.getOwner());
reviewers.addAll(reviewerSet.byState(ReviewerStateInternal.REVIEWER));
reviewers.remove(user.getAccountId());
- ins.setReviewers(reviewers);
-
Set<Account.Id> ccs = new HashSet<>(reviewerSet.byState(ReviewerStateInternal.CC));
ccs.remove(user.getAccountId());
- ins.setExtraCC(ccs);
+ ins.setReviewersAndCcs(reviewers, ccs);
ins.setRevertOf(changeIdToRevert);
try (BatchUpdate bu = updateFactory.create(db.get(), project, user, now)) {
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
index 57f1be1..7063b27 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -1436,8 +1436,9 @@
List<Message> messages = sender.getMessages();
assertThat(messages).hasSize(1);
Message m = messages.get(0);
+ assertThat(m.from().getName()).isEqualTo("Administrator (Code Review)");
assertThat(m.rcpt()).containsExactly(user.emailAddress);
- assertThat(m.body()).contains(admin.fullName + " has uploaded this change for review");
+ assertThat(m.body()).contains("I'd like you to do a code review");
assertThat(m.body()).contains("Change subject: " + PushOneCommit.SUBJECT + "\n");
assertMailReplyTo(m, admin.email);
}
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
index 505c165..e5906a2 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
@@ -764,7 +764,7 @@
input.notify = NotifyHandling.ALL;
sender.clear();
gApi.changes().id(changeId).current().cherryPick(input);
- assertNotifyCc(admin);
+ assertNotifyTo(admin);
// Disable the notification. 'admin' as a reviewer should not be notified any more.
input.destination = "branch-2";
diff --git a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
index 488eb9f..78a3d15 100644
--- a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
+++ b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
@@ -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.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;
@@ -57,7 +59,9 @@
import com.google.gerrit.extensions.api.changes.DraftInput;
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;
@@ -485,7 +489,12 @@
r.assertOkStatus();
assertThat(sender.getMessages()).hasSize(1);
Message m = sender.getMessages().get(0);
- assertThat(m.rcpt()).containsExactly(user.emailAddress);
+ if (notesMigration.readChanges()) {
+ assertThat(m.rcpt()).containsExactly(user.emailAddress);
+ } else {
+ // CCs are considered reviewers in the storage layer and so get notified.
+ assertThat(m.rcpt()).containsExactly(user.emailAddress, user2.emailAddress);
+ }
sender.clear();
r = pushTo(pushSpec + ",notify=" + NotifyHandling.ALL);
@@ -565,7 +574,49 @@
+ 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
+ public void pushForMasterWithCcGroup() throws Exception {
+ TestAccount user2 = accountCreator.user2();
+ String group = name("group");
+ 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);
+ r.assertOkStatus();
+ r.assertChange(Change.Status.NEW, null, ImmutableList.of(), ImmutableList.of(user, user2));
}
@Test
@@ -605,7 +656,49 @@
+ 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
+ public void pushForMasterWithReviewerGroup() throws Exception {
+ TestAccount user2 = accountCreator.user2();
+ String group = name("group");
+ 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);
+ r.assertOkStatus();
+ r.assertChange(Change.Status.NEW, null, ImmutableList.of(user, user2), ImmutableList.of());
}
@Test
@@ -2254,6 +2347,7 @@
String name = "reviewers for " + (i + 1);
if (expectedReviewer != null) {
assertThat(cd.reviewers().all()).named(name).containsExactly(expectedReviewer.getId());
+ // Remove reviewer from PS1 so we can test adding this same reviewer on PS2 below.
gApi.changes().id(cd.getId().get()).reviewer(expectedReviewer.getId().toString()).remove();
}
assertThat(byCommit(c).reviewers().all()).named(name).isEmpty();
diff --git a/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java b/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java
index 2daccc0..209d0a2 100644
--- a/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java
@@ -1088,15 +1088,34 @@
@Test
public void createReviewableChangeWithReviewersAndCcs() throws Exception {
- // TODO(logan): Support reviewers/CCs-by-email via push option.
StagedPreChange spc =
stagePreChange(
"refs/for/master",
users -> ImmutableList.of("r=" + users.reviewer.username, "cc=" + users.ccer.username));
+ FakeEmailSenderSubject subject =
+ assertThat(sender).sent("newchange", spc).to(spc.reviewer, spc.watchingProjectOwner);
+ if (notesMigration.readChanges()) {
+ subject.cc(spc.ccer);
+ } else {
+ // CCs are considered reviewers in the storage layer.
+ subject.to(spc.ccer);
+ }
+ 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(spc.reviewer, spc.watchingProjectOwner)
- .cc(spc.ccer)
+ .to("nobody1@example.com")
+ .to(spc.watchingProjectOwner)
+ .cc("nobody2@example.com")
.bcc(NEW_CHANGES, NEW_PATCHSETS)
.noOneElse();
}
@@ -1727,6 +1746,7 @@
.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();
@@ -1742,6 +1762,7 @@
.sent("newpatchset", sc)
.notTo(sc.owner) // TODO(logan): This shouldn't be sent *from* the owner.
.to(sc.reviewer, sc.ccer)
+ .to(other)
.noOneElse();
}
@@ -1755,6 +1776,7 @@
.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();
@@ -1769,6 +1791,7 @@
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();
}
diff --git a/lib/jackson/BUILD b/lib/jackson/BUILD
index 5c15193..e09f57e 100644
--- a/lib/jackson/BUILD
+++ b/lib/jackson/BUILD
@@ -5,5 +5,9 @@
java_library(
name = "jackson-core",
data = ["//lib:LICENSE-Apache2.0"],
+ visibility = [
+ "//java/com/google/gerrit/elasticsearch:__pkg__",
+ "//plugins:__pkg__",
+ ],
exports = ["@jackson-core//jar"],
)
diff --git a/polygerrit-ui/app/elements/settings/gr-account-info/gr-account-info.html b/polygerrit-ui/app/elements/settings/gr-account-info/gr-account-info.html
index 29d554e..f534771 100644
--- a/polygerrit-ui/app/elements/settings/gr-account-info/gr-account-info.html
+++ b/polygerrit-ui/app/elements/settings/gr-account-info/gr-account-info.html
@@ -29,8 +29,8 @@
<template>
<style include="shared-styles">
gr-avatar {
- height: 10em;
- width: 10em;
+ height: 120px;
+ width: 120px;
margin-right: .15em;
vertical-align: -.25em;
}
@@ -44,7 +44,7 @@
<span class="title"></span>
<span class="value">
<gr-avatar account="[[_account]]"
- image-size="32"></gr-avatar>
+ image-size="120"></gr-avatar>
</span>
</section>
<section class$="[[_hideAvatarChangeUrl(_avatarChangeUrl)]]">
