From 3949eea76e8cf0fd57c1b00014494fdfbc91a248 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Wed, 30 Jan 2019 12:13:22 -0800 Subject: [PATCH] Refactor NotifyUtil analogously to AccountResolver NotifyUtil#resolveAccounts was basically a wrapper around AccountResolver. Give the class a new name, NotifyResolver, and have it resolve inputs to a Result type. This Result type encapsulates both the NotifyHandling enum and the ListMultimap that got passed around everywhere. This has several significant advantages. First, this pair of objects is used in many places, so cutting down two arguments (including the unwieldy ListMultimap type) to one is a win on its own. This change fixes some such methods, and the rest will be fixed in future cleanups. Second and more subtly, this change results in more explicit and less error-prone behavior when a null notify field is passed in a REST API object. Callers have to explicitly specify a non-null NotifyHandling input to NotifyResolver#resolve, which typically means they have to do a firstNonNull(input.notify, ). It turns out that in the REST API docs, there are several different defaults for the different *Input types, including some that have different behavior for WIP/ready changes. The documentation is structured per Input type, which means it's always clear from the documentation what the default should be if you have in front of you a FooInput. In other words, this pattern makes it easier to inspect the code for correctness. Change-Id: I609674d22b6a16b8fac32aeea1a57d293ee601d5 --- .../gerrit/server/change/AbandonOp.java | 22 ++-- .../server/change/AddReviewersEmail.java | 15 +-- .../gerrit/server/change/AddReviewersOp.java | 19 +--- .../gerrit/server/change/BatchAbandon.java | 21 ++-- .../{NotifyUtil.java => NotifyResolver.java} | 100 ++++++++---------- .../gerrit/server/change/ReviewerAdder.java | 57 +++++----- .../gerrit/server/edit/ChangeEditUtil.java | 15 +-- .../gerrit/server/restapi/change/Abandon.java | 26 ++--- .../restapi/change/CherryPickChange.java | 27 +++-- .../server/restapi/change/CreateChange.java | 24 +++-- .../change/DeleteReviewerByEmailOp.java | 23 ++-- .../restapi/change/DeleteReviewerOp.java | 41 +++---- .../server/restapi/change/DeleteVote.java | 24 +++-- .../server/restapi/change/PostReview.java | 37 ++----- .../restapi/change/PublishChangeEdit.java | 14 +-- .../server/restapi/change/PutMessage.java | 28 +++-- .../gerrit/server/restapi/change/Revert.java | 20 ++-- .../google/gerrit/server/submit/MergeOp.java | 20 ++-- .../gerrit/server/submit/SubmitStrategy.java | 12 +-- .../server/submit/SubmitStrategyFactory.java | 8 +- .../server/submit/SubmitStrategyOp.java | 4 +- .../server/mail/ChangeNotificationsIT.java | 4 +- 22 files changed, 262 insertions(+), 299 deletions(-) rename java/com/google/gerrit/server/change/{NotifyUtil.java => NotifyResolver.java} (52%) diff --git a/java/com/google/gerrit/server/change/AbandonOp.java b/java/com/google/gerrit/server/change/AbandonOp.java index 3999955de7..7cabe25263 100644 --- a/java/com/google/gerrit/server/change/AbandonOp.java +++ b/java/com/google/gerrit/server/change/AbandonOp.java @@ -15,13 +15,9 @@ package com.google.gerrit.server.change; import com.google.common.base.Strings; -import com.google.common.collect.ListMultimap; import com.google.common.flogger.FluentLogger; import com.google.gerrit.common.Nullable; -import com.google.gerrit.extensions.api.changes.NotifyHandling; -import com.google.gerrit.extensions.api.changes.RecipientType; import com.google.gerrit.extensions.restapi.ResourceConflictException; -import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.ChangeMessage; import com.google.gerrit.reviewdb.client.PatchSet; @@ -49,8 +45,7 @@ public class AbandonOp implements BatchUpdateOp { private final ChangeAbandoned changeAbandoned; private final String msgTxt; - private final NotifyHandling notifyHandling; - private final ListMultimap accountsToNotify; + private final NotifyResolver.Result notify; private final AccountState accountState; private Change change; @@ -61,8 +56,7 @@ public class AbandonOp implements BatchUpdateOp { AbandonOp create( @Assisted @Nullable AccountState accountState, @Assisted @Nullable String msgTxt, - @Assisted NotifyHandling notifyHandling, - @Assisted ListMultimap accountsToNotify); + @Assisted NotifyResolver.Result notify); } @Inject @@ -73,8 +67,7 @@ public class AbandonOp implements BatchUpdateOp { ChangeAbandoned changeAbandoned, @Assisted @Nullable AccountState accountState, @Assisted @Nullable String msgTxt, - @Assisted NotifyHandling notifyHandling, - @Assisted ListMultimap accountsToNotify) { + @Assisted NotifyResolver.Result notify) { this.abandonedSenderFactory = abandonedSenderFactory; this.cmUtil = cmUtil; this.psUtil = psUtil; @@ -82,8 +75,7 @@ public class AbandonOp implements BatchUpdateOp { this.accountState = accountState; this.msgTxt = Strings.nullToEmpty(msgTxt); - this.notifyHandling = notifyHandling; - this.accountsToNotify = accountsToNotify; + this.notify = notify; } @Nullable @@ -128,12 +120,12 @@ public class AbandonOp implements BatchUpdateOp { cm.setFrom(accountState.getAccount().getId()); } cm.setChangeMessage(message.getMessage(), ctx.getWhen()); - cm.setNotify(notifyHandling); - cm.setAccountsToNotify(accountsToNotify); + cm.setNotify(notify.handling()); + cm.setAccountsToNotify(notify.accounts()); cm.send(); } catch (Exception e) { logger.atSevere().withCause(e).log("Cannot email update for change %s", change.getId()); } - changeAbandoned.fire(change, patchSet, accountState, msgTxt, ctx.getWhen(), notifyHandling); + changeAbandoned.fire(change, patchSet, accountState, msgTxt, ctx.getWhen(), notify.handling()); } } diff --git a/java/com/google/gerrit/server/change/AddReviewersEmail.java b/java/com/google/gerrit/server/change/AddReviewersEmail.java index 4173950a58..da02646663 100644 --- a/java/com/google/gerrit/server/change/AddReviewersEmail.java +++ b/java/com/google/gerrit/server/change/AddReviewersEmail.java @@ -16,12 +16,8 @@ package com.google.gerrit.server.change; import static com.google.common.collect.ImmutableList.toImmutableList; -import com.google.common.base.MoreObjects; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ListMultimap; import com.google.common.flogger.FluentLogger; -import com.google.gerrit.extensions.api.changes.NotifyHandling; -import com.google.gerrit.extensions.api.changes.RecipientType; import com.google.gerrit.mail.Address; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; @@ -49,9 +45,7 @@ public class AddReviewersEmail { Collection copied, Collection
addedByEmail, Collection
copiedByEmail, - NotifyHandling notify, - ListMultimap accountsToNotify, - boolean readyForReview) { + NotifyResolver.Result notify) { // The user knows they added themselves, don't bother emailing them. Account.Id userId = user.getAccountId(); ImmutableList toMail = @@ -64,11 +58,8 @@ public class AddReviewersEmail { try { AddReviewerSender cm = addReviewerSenderFactory.create(change.getProject(), change.getId()); - // Default to silent operation on WIP changes. - NotifyHandling defaultNotifyHandling = - readyForReview ? NotifyHandling.ALL : NotifyHandling.NONE; - cm.setNotify(MoreObjects.firstNonNull(notify, defaultNotifyHandling)); - cm.setAccountsToNotify(accountsToNotify); + cm.setNotify(notify.handling()); + cm.setAccountsToNotify(notify.accounts()); cm.setFrom(userId); cm.addReviewers(toMail); cm.addReviewersByEmail(addedByEmail); diff --git a/java/com/google/gerrit/server/change/AddReviewersOp.java b/java/com/google/gerrit/server/change/AddReviewersOp.java index ea42652e96..6cb5896c24 100644 --- a/java/com/google/gerrit/server/change/AddReviewersOp.java +++ b/java/com/google/gerrit/server/change/AddReviewersOp.java @@ -25,12 +25,8 @@ import static java.util.stream.Collectors.toList; import com.google.auto.value.AutoValue; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.ListMultimap; import com.google.common.collect.Lists; import com.google.common.collect.Streams; -import com.google.gerrit.common.Nullable; -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.RestApiException; import com.google.gerrit.mail.Address; @@ -70,15 +66,13 @@ public class AddReviewersOp implements BatchUpdateOp { * @param addresses email addresses to add. * @param state resulting reviewer state. * @param notify notification handling. - * @param accountsToNotify additional accounts to notify. * @return batch update operation. */ AddReviewersOp create( Set accountIds, Collection
addresses, ReviewerState state, - @Nullable NotifyHandling notify, - ListMultimap accountsToNotify); + NotifyResolver.Result notify); } @AutoValue @@ -118,8 +112,7 @@ public class AddReviewersOp implements BatchUpdateOp { private final Set accountIds; private final Collection
addresses; private final ReviewerState state; - private final NotifyHandling notify; - private final ListMultimap accountsToNotify; + private final NotifyResolver.Result notify; // Unlike addedCCs, addedReviewers is a PatchSetApproval because the AddReviewerResult returned // via the REST API is supposed to include vote information. @@ -143,8 +136,7 @@ public class AddReviewersOp implements BatchUpdateOp { @Assisted Set accountIds, @Assisted Collection
addresses, @Assisted ReviewerState state, - @Assisted @Nullable NotifyHandling notify, - @Assisted ListMultimap accountsToNotify) { + @Assisted NotifyResolver.Result notify) { checkArgument(state == REVIEWER || state == CC, "must be %s or %s: %s", REVIEWER, CC, state); this.approvalsUtil = approvalsUtil; this.psUtil = psUtil; @@ -157,7 +149,6 @@ public class AddReviewersOp implements BatchUpdateOp { this.addresses = addresses; this.state = state; this.notify = notify; - this.accountsToNotify = accountsToNotify; } void setPatchSet(PatchSet patchSet) { @@ -253,9 +244,7 @@ public class AddReviewersOp implements BatchUpdateOp { addedCCs, addedReviewersByEmail, addedCCsByEmail, - notify, - accountsToNotify, - !change.isWorkInProgress()); + notify); if (!addedReviewers.isEmpty()) { List reviewers = addedReviewers diff --git a/java/com/google/gerrit/server/change/BatchAbandon.java b/java/com/google/gerrit/server/change/BatchAbandon.java index b15db602f3..6b7c43b7ba 100644 --- a/java/com/google/gerrit/server/change/BatchAbandon.java +++ b/java/com/google/gerrit/server/change/BatchAbandon.java @@ -14,13 +14,9 @@ package com.google.gerrit.server.change; -import com.google.common.collect.ImmutableListMultimap; -import com.google.common.collect.ListMultimap; import com.google.gerrit.extensions.api.changes.NotifyHandling; -import com.google.gerrit.extensions.api.changes.RecipientType; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.RestApiException; -import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.account.AccountState; @@ -54,8 +50,7 @@ public class BatchAbandon { CurrentUser user, Collection changes, String msgTxt, - NotifyHandling notifyHandling, - ListMultimap accountsToNotify) + NotifyResolver.Result notify) throws RestApiException, UpdateException { if (changes.isEmpty()) { return; @@ -69,9 +64,7 @@ public class BatchAbandon { "Project name \"%s\" doesn't match \"%s\"", change.project().get(), project.get())); } - u.addOp( - change.getId(), - abandonOpFactory.create(accountState, msgTxt, notifyHandling, accountsToNotify)); + u.addOp(change.getId(), abandonOpFactory.create(accountState, msgTxt, notify)); } u.execute(); } @@ -90,8 +83,7 @@ public class BatchAbandon { user, changes, msgTxt, - NotifyHandling.ALL, - ImmutableListMultimap.of()); + NotifyResolver.Result.create(NotifyHandling.ALL)); } public void batchAbandon( @@ -101,6 +93,11 @@ public class BatchAbandon { Collection changes) throws RestApiException, UpdateException { batchAbandon( - updateFactory, project, user, changes, "", NotifyHandling.ALL, ImmutableListMultimap.of()); + updateFactory, + project, + user, + changes, + "", + NotifyResolver.Result.create(NotifyHandling.ALL)); } } diff --git a/java/com/google/gerrit/server/change/NotifyUtil.java b/java/com/google/gerrit/server/change/NotifyResolver.java similarity index 52% rename from java/com/google/gerrit/server/change/NotifyUtil.java rename to java/com/google/gerrit/server/change/NotifyResolver.java index fbe83562bd..5ad9b470e3 100644 --- a/java/com/google/gerrit/server/change/NotifyUtil.java +++ b/java/com/google/gerrit/server/change/NotifyResolver.java @@ -14,11 +14,12 @@ package com.google.gerrit.server.change; +import static java.util.Objects.requireNonNull; import static java.util.stream.Collectors.joining; +import com.google.auto.value.AutoValue; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableListMultimap; -import com.google.common.collect.ListMultimap; -import com.google.common.collect.MultimapBuilder; import com.google.gerrit.common.Nullable; import com.google.gerrit.extensions.api.changes.NotifyHandling; import com.google.gerrit.extensions.api.changes.NotifyInfo; @@ -37,67 +38,60 @@ import java.util.Map; import org.eclipse.jgit.errors.ConfigInvalidException; @Singleton -public class NotifyUtil { +public class NotifyResolver { + @AutoValue + public abstract static class Result { + static Result none() { + return create(NotifyHandling.NONE); + } + + public static Result create(NotifyHandling notifyHandling) { + return create(notifyHandling, ImmutableListMultimap.of()); + } + + private static Result create( + NotifyHandling handling, ImmutableListMultimap recipients) { + return new AutoValue_NotifyResolver_Result(handling, recipients); + } + + public abstract NotifyHandling handling(); + + // TODO(dborowitz): Should be ImmutableSetMultimap. + public abstract ImmutableListMultimap accounts(); + + public boolean shouldNotify() { + return !accounts().isEmpty() || handling().compareTo(NotifyHandling.NONE) > 0; + } + } + private final AccountResolver accountResolver; @Inject - NotifyUtil(AccountResolver accountResolver) { + NotifyResolver(AccountResolver accountResolver) { this.accountResolver = accountResolver; } - public static boolean shouldNotify( - NotifyHandling notify, @Nullable Map notifyDetails) { - if (!isNullOrEmpty(notifyDetails)) { - return true; - } - - return notify.compareTo(NotifyHandling.NONE) > 0; - } - - private static boolean isNullOrEmpty(@Nullable Map notifyDetails) { - if (notifyDetails == null || notifyDetails.isEmpty()) { - return true; - } - - for (NotifyInfo notifyInfo : notifyDetails.values()) { - if (!isEmpty(notifyInfo)) { - return false; + public Result resolve( + NotifyHandling handling, @Nullable Map notifyDetails) + throws BadRequestException, OrmException, IOException, ConfigInvalidException { + requireNonNull(handling); + ImmutableListMultimap.Builder b = ImmutableListMultimap.builder(); + if (notifyDetails != null) { + for (Map.Entry e : notifyDetails.entrySet()) { + b.putAll(e.getKey(), find(e.getValue().accounts)); } } - - return true; + return Result.create(handling, b.build()); } - private static boolean isEmpty(NotifyInfo notifyInfo) { - return notifyInfo.accounts == null || notifyInfo.accounts.isEmpty(); - } - - public ListMultimap resolveAccounts( - @Nullable Map notifyDetails) + private ImmutableList find(@Nullable List inputs) throws OrmException, BadRequestException, IOException, ConfigInvalidException { - if (isNullOrEmpty(notifyDetails)) { - return ImmutableListMultimap.of(); + if (inputs == null || inputs.isEmpty()) { + return ImmutableList.of(); } - - ListMultimap m = null; - for (Map.Entry e : notifyDetails.entrySet()) { - List accounts = e.getValue().accounts; - if (accounts != null) { - if (m == null) { - m = MultimapBuilder.hashKeys().arrayListValues().build(); - } - m.putAll(e.getKey(), find(accounts)); - } - } - - return m != null ? m : ImmutableListMultimap.of(); - } - - private List find(List nameOrEmails) - throws OrmException, BadRequestException, IOException, ConfigInvalidException { - List problems = new ArrayList<>(nameOrEmails.size()); - List r = new ArrayList<>(nameOrEmails.size()); - for (String nameOrEmail : nameOrEmails) { + ImmutableList.Builder r = ImmutableList.builder(); + List problems = new ArrayList<>(inputs.size()); + for (String nameOrEmail : inputs) { try { r.add(accountResolver.resolve(nameOrEmail).asUnique().getAccount().getId()); } catch (UnprocessableEntityException e) { @@ -107,10 +101,10 @@ public class NotifyUtil { if (!problems.isEmpty()) { throw new BadRequestException( - "The following accounts that should be notified could not be resolved:\n" + "Some accounts that should be notified could not be resolved: " + problems.stream().collect(joining("\n"))); } - return r; + return r.build(); } } diff --git a/java/com/google/gerrit/server/change/ReviewerAdder.java b/java/com/google/gerrit/server/change/ReviewerAdder.java index e4b1f2897a..6bfffa73d6 100644 --- a/java/com/google/gerrit/server/change/ReviewerAdder.java +++ b/java/com/google/gerrit/server/change/ReviewerAdder.java @@ -24,9 +24,7 @@ import static java.util.Comparator.comparing; import static java.util.Objects.requireNonNull; 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; @@ -36,7 +34,6 @@ import com.google.gerrit.common.data.GroupDescription; import com.google.gerrit.extensions.api.changes.AddReviewerInput; 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.api.changes.ReviewerInfo; import com.google.gerrit.extensions.client.ReviewerState; import com.google.gerrit.extensions.common.AccountInfo; @@ -151,7 +148,7 @@ public class ReviewerAdder { private final AccountLoader.Factory accountLoaderFactory; private final Config cfg; private final ReviewerJson json; - private final NotifyUtil notifyUtil; + private final NotifyResolver notifyResolver; private final ProjectCache projectCache; private final Provider anonymousProvider; private final AddReviewersOp.Factory addReviewersOpFactory; @@ -166,7 +163,7 @@ public class ReviewerAdder { AccountLoader.Factory accountLoaderFactory, @GerritServerConfig Config cfg, ReviewerJson json, - NotifyUtil notifyUtil, + NotifyResolver notifyResolver, ProjectCache projectCache, Provider anonymousProvider, AddReviewersOp.Factory addReviewersOpFactory, @@ -178,7 +175,7 @@ public class ReviewerAdder { this.accountLoaderFactory = accountLoaderFactory; this.cfg = cfg; this.json = json; - this.notifyUtil = notifyUtil; + this.notifyResolver = notifyResolver; this.projectCache = projectCache; this.anonymousProvider = anonymousProvider; this.addReviewersOpFactory = addReviewersOpFactory; @@ -204,9 +201,9 @@ public class ReviewerAdder { ChangeNotes notes, CurrentUser user, AddReviewerInput input, boolean allowGroup) throws OrmException, IOException, PermissionBackendException, ConfigInvalidException { requireNonNull(input.reviewer); - ListMultimap accountsToNotify; + NotifyResolver.Result notify; try { - accountsToNotify = notifyUtil.resolveAccounts(input.notifyDetails); + notify = resolveNotify(notes, input); } catch (BadRequestException e) { return fail(input, FailureType.OTHER, e.getMessage()); } @@ -216,12 +213,11 @@ public class ReviewerAdder { .checkedGet(notes.getProjectName()) .is(BooleanProjectConfig.ENABLE_REVIEWER_BY_EMAIL); - ReviewerAddition byAccountId = addByAccountId(input, notes, user, accountsToNotify); + ReviewerAddition byAccountId = addByAccountId(input, notes, user, notify); ReviewerAddition wholeGroup = null; if (!byAccountId.exactMatchFound) { - wholeGroup = - addWholeGroup(input, notes, user, accountsToNotify, confirmed, allowGroup, allowByEmail); + wholeGroup = addWholeGroup(input, notes, user, notify, confirmed, allowGroup, allowByEmail); if (wholeGroup != null && wholeGroup.exactMatchFound) { return wholeGroup; } @@ -243,7 +239,17 @@ public class ReviewerAdder { return wholeGroup; } - return addByEmail(input, notes, user, accountsToNotify); + return addByEmail(input, notes, user, notify); + } + + private NotifyResolver.Result resolveNotify(ChangeNotes notes, AddReviewerInput input) + throws BadRequestException, OrmException, ConfigInvalidException, IOException { + NotifyHandling notifyHandling = input.notify; + if (notifyHandling == null) { + notifyHandling = + notes.getChange().isWorkInProgress() ? NotifyHandling.NONE : NotifyHandling.ALL; + } + return notifyResolver.resolve(notifyHandling, input.notifyDetails); } public ReviewerAddition ccCurrentUser(CurrentUser user, RevisionResource revision) { @@ -253,16 +259,13 @@ public class ReviewerAdder { revision.getUser(), ImmutableSet.of(user.getAccountId()), null, - ImmutableListMultimap.of(), + NotifyResolver.Result.none(), true); } @Nullable private ReviewerAddition addByAccountId( - AddReviewerInput input, - ChangeNotes notes, - CurrentUser user, - ListMultimap accountsToNotify) + AddReviewerInput input, ChangeNotes notes, CurrentUser user, NotifyResolver.Result notify) throws OrmException, PermissionBackendException, IOException, ConfigInvalidException { IdentifiedUser reviewerUser; boolean exactMatchFound = false; @@ -285,7 +288,7 @@ public class ReviewerAdder { user, ImmutableSet.of(reviewerUser.getAccountId()), null, - accountsToNotify, + notify, exactMatchFound); } return fail( @@ -299,7 +302,7 @@ public class ReviewerAdder { AddReviewerInput input, ChangeNotes notes, CurrentUser user, - ListMultimap accountsToNotify, + NotifyResolver.Result notify, boolean confirmed, boolean allowGroup, boolean allowByEmail) @@ -371,15 +374,12 @@ public class ReviewerAdder { } } - return new ReviewerAddition(input, notes, user, reviewers, null, accountsToNotify, true); + return new ReviewerAddition(input, notes, user, reviewers, null, notify, true); } @Nullable private ReviewerAddition addByEmail( - AddReviewerInput input, - ChangeNotes notes, - CurrentUser user, - ListMultimap accountsToNotify) + AddReviewerInput input, ChangeNotes notes, CurrentUser user, NotifyResolver.Result notify) throws PermissionBackendException { try { permissionBackend.user(anonymousProvider.get()).change(notes).check(ChangePermission.READ); @@ -397,8 +397,7 @@ public class ReviewerAdder { FailureType.NOT_FOUND, MessageFormat.format(ChangeMessages.get().reviewerInvalid, input.reviewer)); } - return new ReviewerAddition( - input, notes, user, null, ImmutableList.of(adr), accountsToNotify, true); + return new ReviewerAddition(input, notes, user, null, ImmutableList.of(adr), notify, true); } private boolean isValidReviewer(Branch.NameKey branch, Account member) @@ -453,7 +452,7 @@ public class ReviewerAdder { CurrentUser caller, @Nullable Iterable reviewers, @Nullable Iterable
reviewersByEmail, - ListMultimap accountsToNotify, + NotifyResolver.Result notify, boolean exactMatchFound) { checkArgument( reviewers != null || reviewersByEmail != null, @@ -468,9 +467,7 @@ public class ReviewerAdder { this.reviewersByEmail = reviewersByEmail == null ? ImmutableSet.of() : ImmutableSet.copyOf(reviewersByEmail); this.caller = caller.asIdentifiedUser(); - op = - addReviewersOpFactory.create( - this.reviewers, this.reviewersByEmail, state(), input.notify, accountsToNotify); + op = addReviewersOpFactory.create(this.reviewers, this.reviewersByEmail, state(), notify); this.exactMatchFound = exactMatchFound; } diff --git a/java/com/google/gerrit/server/edit/ChangeEditUtil.java b/java/com/google/gerrit/server/edit/ChangeEditUtil.java index d6fdf563a5..3002565400 100644 --- a/java/com/google/gerrit/server/edit/ChangeEditUtil.java +++ b/java/com/google/gerrit/server/edit/ChangeEditUtil.java @@ -16,14 +16,10 @@ package com.google.gerrit.server.edit; import static com.google.common.base.Preconditions.checkArgument; -import com.google.common.collect.ListMultimap; -import com.google.gerrit.extensions.api.changes.NotifyHandling; -import com.google.gerrit.extensions.api.changes.RecipientType; import com.google.gerrit.extensions.client.ChangeKind; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.RestApiException; -import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.RefNames; @@ -32,6 +28,7 @@ import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.PatchSetUtil; import com.google.gerrit.server.change.ChangeKindCache; +import com.google.gerrit.server.change.NotifyResolver; import com.google.gerrit.server.change.PatchSetInserter; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.index.change.ChangeIndexer; @@ -148,7 +145,6 @@ public class ChangeEditUtil { * @param edit change edit to publish * @param notify Notify handling that defines to whom email notifications should be sent after the * change edit is published. - * @param accountsToNotify Accounts that should be notified after the change edit is published. * @throws IOException * @throws OrmException * @throws UpdateException @@ -158,9 +154,8 @@ public class ChangeEditUtil { BatchUpdate.Factory updateFactory, ChangeNotes notes, CurrentUser user, - final ChangeEdit edit, - NotifyHandling notify, - ListMultimap accountsToNotify) + ChangeEdit edit, + NotifyResolver.Result notify) throws IOException, OrmException, RestApiException, UpdateException { Change change = edit.getChange(); try (Repository repo = gitManager.openRepository(change.getProject()); @@ -177,8 +172,8 @@ public class ChangeEditUtil { PatchSetInserter inserter = patchSetInserterFactory .create(notes, psId, squashed) - .setNotify(notify) - .setAccountsToNotify(accountsToNotify); + .setNotify(notify.handling()) + .setAccountsToNotify(notify.accounts()); StringBuilder message = new StringBuilder("Patch Set ").append(inserter.getPatchSetId().get()).append(": "); diff --git a/java/com/google/gerrit/server/restapi/change/Abandon.java b/java/com/google/gerrit/server/restapi/change/Abandon.java index 851752d05f..7793b589eb 100644 --- a/java/com/google/gerrit/server/restapi/change/Abandon.java +++ b/java/com/google/gerrit/server/restapi/change/Abandon.java @@ -14,16 +14,12 @@ package com.google.gerrit.server.restapi.change; -import com.google.common.collect.ImmutableListMultimap; -import com.google.common.collect.ListMultimap; import com.google.common.flogger.FluentLogger; import com.google.gerrit.extensions.api.changes.AbandonInput; import com.google.gerrit.extensions.api.changes.NotifyHandling; -import com.google.gerrit.extensions.api.changes.RecipientType; import com.google.gerrit.extensions.common.ChangeInfo; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.webui.UiAction; -import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.PatchSetUtil; @@ -31,7 +27,7 @@ import com.google.gerrit.server.account.AccountState; import com.google.gerrit.server.change.AbandonOp; import com.google.gerrit.server.change.ChangeJson; import com.google.gerrit.server.change.ChangeResource; -import com.google.gerrit.server.change.NotifyUtil; +import com.google.gerrit.server.change.NotifyResolver; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.permissions.ChangePermission; import com.google.gerrit.server.permissions.PermissionBackendException; @@ -53,7 +49,7 @@ public class Abandon extends RetryingRestModifyView accountsToNotify) + NotifyResolver.Result notify) throws RestApiException, UpdateException { AccountState accountState = user.isIdentifiedUser() ? user.asIdentifiedUser().state() : null; - AbandonOp op = abandonOpFactory.create(accountState, msgTxt, notifyHandling, accountsToNotify); + AbandonOp op = abandonOpFactory.create(accountState, msgTxt, notify); try (BatchUpdate u = updateFactory.create(notes.getProjectName(), user, TimeUtil.nowTs())) { u.addOp(notes.getChangeId(), op).execute(); } diff --git a/java/com/google/gerrit/server/restapi/change/CherryPickChange.java b/java/com/google/gerrit/server/restapi/change/CherryPickChange.java index 5317efc31a..a33bcc11db 100644 --- a/java/com/google/gerrit/server/restapi/change/CherryPickChange.java +++ b/java/com/google/gerrit/server/restapi/change/CherryPickChange.java @@ -14,12 +14,15 @@ package com.google.gerrit.server.restapi.change; +import static com.google.common.base.MoreObjects.firstNonNull; + import com.google.auto.value.AutoValue; import com.google.common.base.Strings; import com.google.common.collect.ImmutableSet; import com.google.gerrit.common.FooterConstants; import com.google.gerrit.common.Nullable; import com.google.gerrit.extensions.api.changes.CherryPickInput; +import com.google.gerrit.extensions.api.changes.NotifyHandling; import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.MergeConflictException; import com.google.gerrit.extensions.restapi.ResourceConflictException; @@ -37,7 +40,7 @@ import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.ReviewerSet; import com.google.gerrit.server.change.ChangeInserter; -import com.google.gerrit.server.change.NotifyUtil; +import com.google.gerrit.server.change.NotifyResolver; import com.google.gerrit.server.change.PatchSetInserter; import com.google.gerrit.server.git.CodeReviewCommit; import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk; @@ -103,7 +106,7 @@ public class CherryPickChange { private final ChangeNotes.Factory changeNotesFactory; private final ProjectCache projectCache; private final ApprovalsUtil approvalsUtil; - private final NotifyUtil notifyUtil; + private final NotifyResolver notifyResolver; @Inject CherryPickChange( @@ -118,7 +121,7 @@ public class CherryPickChange { ChangeNotes.Factory changeNotesFactory, ProjectCache projectCache, ApprovalsUtil approvalsUtil, - NotifyUtil notifyUtil) { + NotifyResolver notifyResolver) { this.seq = seq; this.queryProvider = queryProvider; this.gitManager = gitManager; @@ -130,7 +133,7 @@ public class CherryPickChange { this.changeNotesFactory = changeNotesFactory; this.projectCache = projectCache; this.approvalsUtil = approvalsUtil; - this.notifyUtil = notifyUtil; + this.notifyResolver = notifyResolver; } public Result cherryPick( @@ -324,10 +327,11 @@ public class CherryPickChange { Change destChange = destNotes.getChange(); PatchSet.Id psId = ChangeUtil.nextPatchSetId(git, destChange.currentPatchSetId()); PatchSetInserter inserter = patchSetInserterFactory.create(destNotes, psId, cherryPickCommit); + NotifyResolver.Result notify = resolveNotify(input); inserter .setMessage("Uploaded patch set " + inserter.getPatchSetId().get() + ".") - .setNotify(input.notify) - .setAccountsToNotify(notifyUtil.resolveAccounts(input.notifyDetails)); + .setNotify(notify.handling()) + .setAccountsToNotify(notify.accounts()); bu.addOp(destChange.getId(), inserter); return destChange.getId(); } @@ -344,6 +348,7 @@ public class CherryPickChange { Change.Id changeId = new Change.Id(seq.nextChangeId()); ChangeInserter ins = changeInserterFactory.create(changeId, cherryPickCommit, refName); Branch.NameKey sourceBranch = sourceChange == null ? null : sourceChange.getDest(); + NotifyResolver.Result notify = resolveNotify(input); ins.setMessage( messageForDestinationChange( ins.getPatchSetId(), sourceBranch, sourceCommit, cherryPickCommit)) @@ -351,8 +356,8 @@ public class CherryPickChange { .setWorkInProgress( (sourceChange != null && sourceChange.isWorkInProgress()) || !cherryPickCommit.getFilesWithGitConflicts().isEmpty()) - .setNotify(input.notify) - .setAccountsToNotify(notifyUtil.resolveAccounts(input.notifyDetails)); + .setNotify(notify.handling()) + .setAccountsToNotify(notify.accounts()); if (input.keepReviewers && sourceChange != null) { ReviewerSet reviewerSet = approvalsUtil.getReviewers(changeNotesFactory.createChecked(sourceChange)); @@ -368,6 +373,12 @@ public class CherryPickChange { return changeId; } + private NotifyResolver.Result resolveNotify(CherryPickInput input) + throws BadRequestException, OrmException, ConfigInvalidException, IOException { + return notifyResolver.resolve( + firstNonNull(input.notify, NotifyHandling.ALL), input.notifyDetails); + } + private String messageForDestinationChange( PatchSet.Id patchSetId, Branch.NameKey sourceBranch, diff --git a/java/com/google/gerrit/server/restapi/change/CreateChange.java b/java/com/google/gerrit/server/restapi/change/CreateChange.java index 2295b369b4..c7d2e8bc63 100644 --- a/java/com/google/gerrit/server/restapi/change/CreateChange.java +++ b/java/com/google/gerrit/server/restapi/change/CreateChange.java @@ -14,13 +14,14 @@ package com.google.gerrit.server.restapi.change; +import static com.google.common.base.MoreObjects.firstNonNull; import static org.eclipse.jgit.lib.Constants.SIGNED_OFF_BY_TAG; import com.google.common.base.Joiner; -import com.google.common.base.MoreObjects; import com.google.common.base.Strings; import com.google.common.collect.Iterables; import com.google.gerrit.common.Nullable; +import com.google.gerrit.extensions.api.changes.NotifyHandling; import com.google.gerrit.extensions.client.ChangeStatus; import com.google.gerrit.extensions.client.SubmitType; import com.google.gerrit.extensions.common.ChangeInfo; @@ -48,7 +49,7 @@ import com.google.gerrit.server.change.ChangeFinder; import com.google.gerrit.server.change.ChangeInserter; import com.google.gerrit.server.change.ChangeJson; import com.google.gerrit.server.change.ChangeResource; -import com.google.gerrit.server.change.NotifyUtil; +import com.google.gerrit.server.change.NotifyResolver; import com.google.gerrit.server.config.AnonymousCowardName; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.git.GitRepositoryManager; @@ -112,7 +113,7 @@ public class CreateChange private final PatchSetUtil psUtil; private final MergeUtil.Factory mergeUtilFactory; private final SubmitType submitType; - private final NotifyUtil notifyUtil; + private final NotifyResolver notifyResolver; private final ContributorAgreementsChecker contributorAgreements; private final boolean disablePrivateChanges; @@ -133,7 +134,7 @@ public class CreateChange PatchSetUtil psUtil, @GerritServerConfig Config config, MergeUtil.Factory mergeUtilFactory, - NotifyUtil notifyUtil, + NotifyResolver notifyResolver, ContributorAgreementsChecker contributorAgreements) { super(retryHelper); this.anonymousCowardName = anonymousCowardName; @@ -151,7 +152,7 @@ public class CreateChange this.submitType = config.getEnum("project", null, "submitType", SubmitType.MERGE_IF_NECESSARY); this.disablePrivateChanges = config.getBoolean("change", null, "disablePrivateChanges", false); this.mergeUtilFactory = mergeUtilFactory; - this.notifyUtil = notifyUtil; + this.notifyResolver = notifyResolver; this.contributorAgreements = contributorAgreements; } @@ -234,8 +235,7 @@ public class CreateChange input.workInProgress = true; } else { input.workInProgress = - MoreObjects.firstNonNull( - me.state().getGeneralPreferences().workInProgressByDefault, false); + firstNonNull(me.state().getGeneralPreferences().workInProgressByDefault, false); } } @@ -305,8 +305,11 @@ public class CreateChange ins.setPrivate(input.isPrivate); ins.setWorkInProgress(input.workInProgress); ins.setGroups(groups); - ins.setNotify(input.notify); - ins.setAccountsToNotify(notifyUtil.resolveAccounts(input.notifyDetails)); + NotifyResolver.Result notify = + notifyResolver.resolve( + firstNonNull(input.notify, NotifyHandling.ALL), input.notifyDetails); + ins.setNotify(notify.handling()); + ins.setAccountsToNotify(notify.accounts()); try (BatchUpdate bu = updateFactory.create(projectState.getNameKey(), me, now)) { bu.setRepository(git, rw, oi); bu.insertChange(ins); @@ -454,8 +457,7 @@ public class CreateChange MergeUtil mergeUtil = mergeUtilFactory.create(projectState); // default merge strategy from project settings String mergeStrategy = - MoreObjects.firstNonNull( - Strings.emptyToNull(merge.strategy), mergeUtil.mergeStrategyName()); + firstNonNull(Strings.emptyToNull(merge.strategy), mergeUtil.mergeStrategyName()); return MergeUtil.createMergeCommit( oi, diff --git a/java/com/google/gerrit/server/restapi/change/DeleteReviewerByEmailOp.java b/java/com/google/gerrit/server/restapi/change/DeleteReviewerByEmailOp.java index 3231d165f2..7a41f84aaf 100644 --- a/java/com/google/gerrit/server/restapi/change/DeleteReviewerByEmailOp.java +++ b/java/com/google/gerrit/server/restapi/change/DeleteReviewerByEmailOp.java @@ -14,6 +14,8 @@ package com.google.gerrit.server.restapi.change; +import static com.google.common.base.MoreObjects.firstNonNull; + import com.google.common.flogger.FluentLogger; import com.google.gerrit.extensions.api.changes.DeleteReviewerInput; import com.google.gerrit.extensions.api.changes.NotifyHandling; @@ -22,7 +24,7 @@ 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.server.ChangeUtil; -import com.google.gerrit.server.change.NotifyUtil; +import com.google.gerrit.server.change.NotifyResolver; import com.google.gerrit.server.mail.send.DeleteReviewerSender; import com.google.gerrit.server.update.BatchUpdateOp; import com.google.gerrit.server.update.ChangeContext; @@ -40,7 +42,7 @@ public class DeleteReviewerByEmailOp implements BatchUpdateOp { } private final DeleteReviewerSender.Factory deleteReviewerSenderFactory; - private final NotifyUtil notifyUtil; + private final NotifyResolver notifyResolver; private final Address reviewer; private final DeleteReviewerInput input; @@ -50,11 +52,11 @@ public class DeleteReviewerByEmailOp implements BatchUpdateOp { @Inject DeleteReviewerByEmailOp( DeleteReviewerSender.Factory deleteReviewerSenderFactory, - NotifyUtil notifyUtil, + NotifyResolver notifyResolver, @Assisted Address reviewer, @Assisted DeleteReviewerInput input) { this.deleteReviewerSenderFactory = deleteReviewerSenderFactory; - this.notifyUtil = notifyUtil; + this.notifyResolver = notifyResolver; this.reviewer = reviewer; this.input = input; } @@ -86,17 +88,20 @@ public class DeleteReviewerByEmailOp implements BatchUpdateOp { input.notify = NotifyHandling.ALL; } } - if (!NotifyUtil.shouldNotify(input.notify, input.notifyDetails)) { - return; - } try { + NotifyResolver.Result notify = + notifyResolver.resolve( + firstNonNull(input.notify, NotifyHandling.ALL), input.notifyDetails); + if (!notify.shouldNotify()) { + return; + } DeleteReviewerSender cm = deleteReviewerSenderFactory.create(ctx.getProject(), change.getId()); cm.setFrom(ctx.getAccountId()); cm.addReviewersByEmail(Collections.singleton(reviewer)); cm.setChangeMessage(changeMessage.getMessage(), changeMessage.getWrittenOn()); - cm.setNotify(input.notify); - cm.setAccountsToNotify(notifyUtil.resolveAccounts(input.notifyDetails)); + cm.setNotify(notify.handling()); + cm.setAccountsToNotify(notify.accounts()); cm.send(); } catch (Exception err) { logger.atSevere().withCause(err).log("Cannot email update for change %s", change.getId()); diff --git a/java/com/google/gerrit/server/restapi/change/DeleteReviewerOp.java b/java/com/google/gerrit/server/restapi/change/DeleteReviewerOp.java index 0cb4816f72..fb4c96ec10 100644 --- a/java/com/google/gerrit/server/restapi/change/DeleteReviewerOp.java +++ b/java/com/google/gerrit/server/restapi/change/DeleteReviewerOp.java @@ -18,6 +18,7 @@ import com.google.common.collect.Iterables; import com.google.common.flogger.FluentLogger; import com.google.gerrit.common.data.LabelType; import com.google.gerrit.common.data.LabelTypes; +import com.google.gerrit.common.errors.EmailException; import com.google.gerrit.extensions.api.changes.DeleteReviewerInput; import com.google.gerrit.extensions.api.changes.NotifyHandling; import com.google.gerrit.extensions.restapi.AuthException; @@ -27,13 +28,13 @@ 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.Project; +import com.google.gerrit.reviewdb.client.Project.NameKey; import com.google.gerrit.server.ApprovalsUtil; import com.google.gerrit.server.ChangeMessagesUtil; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.PatchSetUtil; import com.google.gerrit.server.account.AccountState; -import com.google.gerrit.server.change.NotifyUtil; +import com.google.gerrit.server.change.NotifyResolver; import com.google.gerrit.server.extensions.events.ReviewerDeleted; import com.google.gerrit.server.mail.send.DeleteReviewerSender; import com.google.gerrit.server.notedb.ChangeUpdate; @@ -68,7 +69,7 @@ public class DeleteReviewerOp implements BatchUpdateOp { private final ReviewerDeleted reviewerDeleted; private final Provider user; private final DeleteReviewerSender.Factory deleteReviewerSenderFactory; - private final NotifyUtil notifyUtil; + private final NotifyResolver notifyResolver; private final RemoveReviewerControl removeReviewerControl; private final ProjectCache projectCache; @@ -90,7 +91,7 @@ public class DeleteReviewerOp implements BatchUpdateOp { ReviewerDeleted reviewerDeleted, Provider user, DeleteReviewerSender.Factory deleteReviewerSenderFactory, - NotifyUtil notifyUtil, + NotifyResolver notifyResolver, RemoveReviewerControl removeReviewerControl, ProjectCache projectCache, @Assisted AccountState reviewerAccount, @@ -102,7 +103,7 @@ public class DeleteReviewerOp implements BatchUpdateOp { this.reviewerDeleted = reviewerDeleted; this.user = user; this.deleteReviewerSenderFactory = deleteReviewerSenderFactory; - this.notifyUtil = notifyUtil; + this.notifyResolver = notifyResolver; this.removeReviewerControl = removeReviewerControl; this.projectCache = projectCache; this.reviewer = reviewerAccount; @@ -176,8 +177,13 @@ public class DeleteReviewerOp implements BatchUpdateOp { input.notify = NotifyHandling.ALL; } } - if (NotifyUtil.shouldNotify(input.notify, input.notifyDetails)) { - emailReviewers(ctx.getProject(), currChange, changeMessage); + try { + NotifyResolver.Result notify = notifyResolver.resolve(input.notify, input.notifyDetails); + if (notify.shouldNotify()) { + emailReviewers(ctx.getProject(), currChange, changeMessage, notify); + } + } catch (Exception err) { + logger.atSevere().withCause(err).log("Cannot email update for change %s", currChange.getId()); } reviewerDeleted.fire( currChange, @@ -206,22 +212,19 @@ public class DeleteReviewerOp implements BatchUpdateOp { } private void emailReviewers( - Project.NameKey projectName, Change change, ChangeMessage changeMessage) { + NameKey projectName, Change change, ChangeMessage changeMessage, NotifyResolver.Result notify) + throws EmailException { Account.Id userId = user.get().getAccountId(); if (userId.equals(reviewer.getAccount().getId())) { // The user knows they removed themselves, don't bother emailing them. return; } - try { - DeleteReviewerSender cm = deleteReviewerSenderFactory.create(projectName, change.getId()); - cm.setFrom(userId); - cm.addReviewers(Collections.singleton(reviewer.getAccount().getId())); - cm.setChangeMessage(changeMessage.getMessage(), changeMessage.getWrittenOn()); - cm.setNotify(input.notify); - cm.setAccountsToNotify(notifyUtil.resolveAccounts(input.notifyDetails)); - cm.send(); - } catch (Exception err) { - logger.atSevere().withCause(err).log("Cannot email update for change %s", change.getId()); - } + DeleteReviewerSender cm = deleteReviewerSenderFactory.create(projectName, change.getId()); + cm.setFrom(userId); + cm.addReviewers(Collections.singleton(reviewer.getAccount().getId())); + cm.setChangeMessage(changeMessage.getMessage(), changeMessage.getWrittenOn()); + cm.setNotify(notify.handling()); + cm.setAccountsToNotify(notify.accounts()); + cm.send(); } } diff --git a/java/com/google/gerrit/server/restapi/change/DeleteVote.java b/java/com/google/gerrit/server/restapi/change/DeleteVote.java index 60d11633e5..2de2f99ea9 100644 --- a/java/com/google/gerrit/server/restapi/change/DeleteVote.java +++ b/java/com/google/gerrit/server/restapi/change/DeleteVote.java @@ -14,6 +14,7 @@ package com.google.gerrit.server.restapi.change; +import static com.google.common.base.MoreObjects.firstNonNull; import static java.util.Objects.requireNonNull; import com.google.common.flogger.FluentLogger; @@ -36,7 +37,7 @@ import com.google.gerrit.server.ChangeMessagesUtil; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.PatchSetUtil; import com.google.gerrit.server.account.AccountState; -import com.google.gerrit.server.change.NotifyUtil; +import com.google.gerrit.server.change.NotifyResolver; import com.google.gerrit.server.change.ReviewerResource; import com.google.gerrit.server.change.VoteResource; import com.google.gerrit.server.extensions.events.VoteDeleted; @@ -72,7 +73,7 @@ public class DeleteVote extends RetryingRestModifyView accountsToNotify = - notifyUtil.resolveAccounts(input.notifyDetails); + NotifyResolver.Result notify = notifyResolver.resolve(input.notify, input.notifyDetails); Map reviewerJsonResults = null; List reviewerResults = Lists.newArrayList(); @@ -368,7 +367,7 @@ public class PostReview // Add the review op. bu.addOp( revision.getChange().getId(), - new Op(projectState, revision.getPatchSet().getId(), input, accountsToNotify)); + new Op(projectState, revision.getPatchSet().getId(), input, notify.accounts())); bu.execute(); @@ -378,16 +377,8 @@ public class PostReview reviewerResult.gatherResults(cd); } - boolean readyForReview = - (output.ready != null && output.ready) || !revision.getChange().isWorkInProgress(); // Sending from AddReviewersOp was suppressed so we can send a single batch email here. - batchEmailReviewers( - revision.getUser(), - revision.getChange(), - reviewerResults, - input.notify, - accountsToNotify, - readyForReview); + batchEmailReviewers(revision.getUser(), revision.getChange(), reviewerResults, notify); } return Response.ok(output); @@ -421,9 +412,7 @@ public class PostReview CurrentUser user, Change change, List reviewerAdditions, - @Nullable NotifyHandling notify, - ListMultimap accountsToNotify, - boolean readyForReview) { + NotifyResolver.Result notify) { List to = new ArrayList<>(); List cc = new ArrayList<>(); List
toByEmail = new ArrayList<>(); @@ -438,15 +427,7 @@ public class PostReview } } addReviewersEmail.emailReviewers( - user.asIdentifiedUser(), - change, - to, - cc, - toByEmail, - ccByEmail, - notify, - accountsToNotify, - readyForReview); + user.asIdentifiedUser(), change, to, cc, toByEmail, ccByEmail, notify); } private RevisionResource onBehalfOf(RevisionResource rev, LabelTypes labelTypes, ReviewInput in) diff --git a/java/com/google/gerrit/server/restapi/change/PublishChangeEdit.java b/java/com/google/gerrit/server/restapi/change/PublishChangeEdit.java index 3d401c498a..b0cad84d08 100644 --- a/java/com/google/gerrit/server/restapi/change/PublishChangeEdit.java +++ b/java/com/google/gerrit/server/restapi/change/PublishChangeEdit.java @@ -14,12 +14,15 @@ package com.google.gerrit.server.restapi.change; +import static com.google.common.base.MoreObjects.firstNonNull; + +import com.google.gerrit.extensions.api.changes.NotifyHandling; import com.google.gerrit.extensions.api.changes.PublishChangeEditInput; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.Response; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.server.change.ChangeResource; -import com.google.gerrit.server.change.NotifyUtil; +import com.google.gerrit.server.change.NotifyResolver; import com.google.gerrit.server.edit.ChangeEdit; import com.google.gerrit.server.edit.ChangeEditUtil; import com.google.gerrit.server.project.ContributorAgreementsChecker; @@ -39,18 +42,18 @@ import org.eclipse.jgit.errors.ConfigInvalidException; public class PublishChangeEdit extends RetryingRestModifyView> { private final ChangeEditUtil editUtil; - private final NotifyUtil notifyUtil; + private final NotifyResolver notifyResolver; private final ContributorAgreementsChecker contributorAgreementsChecker; @Inject PublishChangeEdit( RetryHelper retryHelper, ChangeEditUtil editUtil, - NotifyUtil notifyUtil, + NotifyResolver notifyResolver, ContributorAgreementsChecker contributorAgreementsChecker) { super(retryHelper); this.editUtil = editUtil; - this.notifyUtil = notifyUtil; + this.notifyResolver = notifyResolver; this.contributorAgreementsChecker = contributorAgreementsChecker; } @@ -73,8 +76,7 @@ public class PublishChangeEdit rsrc.getNotes(), rsrc.getUser(), edit.get(), - in.notify, - notifyUtil.resolveAccounts(in.notifyDetails)); + notifyResolver.resolve(firstNonNull(in.notify, NotifyHandling.ALL), in.notifyDetails)); return Response.none(); } } diff --git a/java/com/google/gerrit/server/restapi/change/PutMessage.java b/java/com/google/gerrit/server/restapi/change/PutMessage.java index 80d0aba2f0..10192a96fa 100644 --- a/java/com/google/gerrit/server/restapi/change/PutMessage.java +++ b/java/com/google/gerrit/server/restapi/change/PutMessage.java @@ -29,7 +29,7 @@ import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.PatchSetUtil; import com.google.gerrit.server.change.ChangeResource; -import com.google.gerrit.server.change.NotifyUtil; +import com.google.gerrit.server.change.NotifyResolver; import com.google.gerrit.server.change.PatchSetInserter; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.notedb.ChangeNotes; @@ -71,7 +71,7 @@ public class PutMessage private final PatchSetInserter.Factory psInserterFactory; private final PermissionBackend permissionBackend; private final PatchSetUtil psUtil; - private final NotifyUtil notifyUtil; + private final NotifyResolver notifyResolver; private final ProjectCache projectCache; @Inject @@ -83,7 +83,7 @@ public class PutMessage PermissionBackend permissionBackend, @GerritPersonIdent PersonIdent gerritIdent, PatchSetUtil psUtil, - NotifyUtil notifyUtil, + NotifyResolver notifyResolver, ProjectCache projectCache) { super(retryHelper); this.repositoryManager = repositoryManager; @@ -92,7 +92,7 @@ public class PutMessage this.tz = gerritIdent.getTimeZone(); this.permissionBackend = permissionBackend; this.psUtil = psUtil; - this.notifyUtil = notifyUtil; + this.notifyResolver = notifyResolver; this.projectCache = projectCache; } @@ -117,11 +117,6 @@ public class PutMessage resource.getChange().getKey().get(), sanitizedCommitMessage); - NotifyHandling notify = input.notify; - if (notify == null) { - notify = resource.getChange().isWorkInProgress() ? NotifyHandling.OWNER : NotifyHandling.ALL; - } - try (Repository repository = repositoryManager.openRepository(resource.getProject()); RevWalk revWalk = new RevWalk(repository); ObjectInserter objectInserter = repository.newObjectInserter()) { @@ -145,8 +140,9 @@ public class PutMessage inserter.setMessage( String.format("Patch Set %s: Commit message was updated.", psId.getId())); inserter.setDescription("Edit commit message"); - inserter.setNotify(notify); - inserter.setAccountsToNotify(notifyUtil.resolveAccounts(input.notifyDetails)); + NotifyResolver.Result notify = resolveNotify(input, resource); + inserter.setNotify(notify.handling()); + inserter.setAccountsToNotify(notify.accounts()); bu.addOp(resource.getChange().getId(), inserter); bu.execute(); } @@ -154,6 +150,16 @@ public class PutMessage return Response.ok("ok"); } + private NotifyResolver.Result resolveNotify(CommitMessageInput input, ChangeResource resource) + throws BadRequestException, OrmException, ConfigInvalidException, IOException { + NotifyHandling notifyHandling = input.notify; + if (notifyHandling == null) { + notifyHandling = + resource.getChange().isWorkInProgress() ? NotifyHandling.OWNER : NotifyHandling.ALL; + } + return notifyResolver.resolve(notifyHandling, input.notifyDetails); + } + private ObjectId createCommit( ObjectInserter objectInserter, RevCommit basePatchSetCommit, diff --git a/java/com/google/gerrit/server/restapi/change/Revert.java b/java/com/google/gerrit/server/restapi/change/Revert.java index b070b1ce3f..492e7410bd 100644 --- a/java/com/google/gerrit/server/restapi/change/Revert.java +++ b/java/com/google/gerrit/server/restapi/change/Revert.java @@ -14,6 +14,7 @@ package com.google.gerrit.server.restapi.change; +import static com.google.common.base.MoreObjects.firstNonNull; import static com.google.gerrit.extensions.conditions.BooleanCondition.and; import static com.google.gerrit.server.permissions.RefPermission.CREATE_CHANGE; @@ -44,7 +45,7 @@ import com.google.gerrit.server.change.ChangeInserter; import com.google.gerrit.server.change.ChangeJson; import com.google.gerrit.server.change.ChangeMessages; import com.google.gerrit.server.change.ChangeResource; -import com.google.gerrit.server.change.NotifyUtil; +import com.google.gerrit.server.change.NotifyResolver; import com.google.gerrit.server.extensions.events.ChangeReverted; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.mail.send.RevertedSender; @@ -104,7 +105,7 @@ public class Revert extends RetryingRestModifyView accountsToNotify = - notifyUtil.resolveAccounts(input.notifyDetails); + NotifyResolver.Result notify = + notifyResolver.resolve( + firstNonNull(input.notify, NotifyHandling.ALL), input.notifyDetails); ChangeInserter ins = changeInserterFactory .create(changeId, revertCommit, notes.getChange().getDest().get()) .setTopic(changeToRevert.getTopic()); ins.setMessage("Uploaded patch set 1."); - ins.setNotify(input.notify); - ins.setAccountsToNotify(accountsToNotify); + ins.setNotify(notify.handling()); + ins.setAccountsToNotify(notify.accounts()); ReviewerSet reviewerSet = approvalsUtil.getReviewers(notes); @@ -241,7 +243,7 @@ public class Revert extends RetryingRestModifyView ormProvider; - private final NotifyUtil notifyUtil; + private final NotifyResolver notifyResolver; private final RetryHelper retryHelper; private final ChangeData.Factory changeDataFactory; @@ -239,7 +239,7 @@ public class MergeOp implements AutoCloseable { private MergeOpRepoManager orm; private CommitStatus commitStatus; private SubmitInput submitInput; - private ListMultimap accountsToNotify; + private NotifyResolver.Result notify; private Set allProjects; private boolean dryrun; private TopicMetrics topicMetrics; @@ -255,7 +255,7 @@ public class MergeOp implements AutoCloseable { SubmitStrategyFactory submitStrategyFactory, SubmoduleOp.Factory subOpFactory, Provider ormProvider, - NotifyUtil notifyUtil, + NotifyResolver notifyResolver, TopicMetrics topicMetrics, RetryHelper retryHelper, ChangeData.Factory changeDataFactory) { @@ -268,7 +268,7 @@ public class MergeOp implements AutoCloseable { this.submitStrategyFactory = submitStrategyFactory; this.subOpFactory = subOpFactory; this.ormProvider = ormProvider; - this.notifyUtil = notifyUtil; + this.notifyResolver = notifyResolver; this.retryHelper = retryHelper; this.topicMetrics = topicMetrics; this.changeDataFactory = changeDataFactory; @@ -443,7 +443,9 @@ public class MergeOp implements AutoCloseable { throws OrmException, RestApiException, UpdateException, IOException, ConfigInvalidException, PermissionBackendException { this.submitInput = submitInput; - this.accountsToNotify = notifyUtil.resolveAccounts(submitInput.notifyDetails); + this.notify = + notifyResolver.resolve( + firstNonNull(submitInput.notify, NotifyHandling.ALL), submitInput.notifyDetails); this.dryrun = dryrun; this.caller = caller; this.ts = TimeUtil.nowTs(); @@ -674,7 +676,7 @@ public class MergeOp implements AutoCloseable { commitStatus, submissionId, submitInput, - accountsToNotify, + notify, submoduleOp, dryrun); strategies.add(strategy); diff --git a/java/com/google/gerrit/server/submit/SubmitStrategy.java b/java/com/google/gerrit/server/submit/SubmitStrategy.java index c9baf99e30..974f5b89db 100644 --- a/java/com/google/gerrit/server/submit/SubmitStrategy.java +++ b/java/com/google/gerrit/server/submit/SubmitStrategy.java @@ -16,13 +16,10 @@ package com.google.gerrit.server.submit; import static java.util.Objects.requireNonNull; -import com.google.common.collect.ListMultimap; import com.google.common.collect.Sets; -import com.google.gerrit.extensions.api.changes.RecipientType; import com.google.gerrit.extensions.api.changes.SubmitInput; import com.google.gerrit.extensions.client.SubmitType; import com.google.gerrit.extensions.config.FactoryModule; -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.server.ApprovalsUtil; @@ -32,6 +29,7 @@ import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.PatchSetUtil; import com.google.gerrit.server.account.AccountCache; import com.google.gerrit.server.change.LabelNormalizer; +import com.google.gerrit.server.change.NotifyResolver; import com.google.gerrit.server.change.RebaseChangeOp; import com.google.gerrit.server.change.TestSubmitInput; import com.google.gerrit.server.extensions.events.ChangeMerged; @@ -94,7 +92,7 @@ public abstract class SubmitStrategy { Set incoming, RequestId submissionId, SubmitInput submitInput, - ListMultimap accountsToNotify, + NotifyResolver.Result notify, SubmoduleOp submoduleOp, boolean dryrun); } @@ -126,7 +124,7 @@ public abstract class SubmitStrategy { final RequestId submissionId; final SubmitType submitType; final SubmitInput submitInput; - final ListMultimap accountsToNotify; + final NotifyResolver.Result notify; final SubmoduleOp submoduleOp; final ProjectState project; @@ -165,7 +163,7 @@ public abstract class SubmitStrategy { @Assisted RequestId submissionId, @Assisted SubmitType submitType, @Assisted SubmitInput submitInput, - @Assisted ListMultimap accountsToNotify, + @Assisted NotifyResolver.Result notify, @Assisted SubmoduleOp submoduleOp, @Assisted boolean dryrun) { this.accountCache = accountCache; @@ -194,7 +192,7 @@ public abstract class SubmitStrategy { this.submissionId = submissionId; this.submitType = submitType; this.submitInput = submitInput; - this.accountsToNotify = accountsToNotify; + this.notify = notify; this.submoduleOp = submoduleOp; this.dryrun = dryrun; diff --git a/java/com/google/gerrit/server/submit/SubmitStrategyFactory.java b/java/com/google/gerrit/server/submit/SubmitStrategyFactory.java index 0a92d61a80..00ea7a315b 100644 --- a/java/com/google/gerrit/server/submit/SubmitStrategyFactory.java +++ b/java/com/google/gerrit/server/submit/SubmitStrategyFactory.java @@ -14,14 +14,12 @@ package com.google.gerrit.server.submit; -import com.google.common.collect.ListMultimap; import com.google.common.flogger.FluentLogger; -import com.google.gerrit.extensions.api.changes.RecipientType; import com.google.gerrit.extensions.api.changes.SubmitInput; import com.google.gerrit.extensions.client.SubmitType; -import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Branch; import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.change.NotifyResolver; import com.google.gerrit.server.git.CodeReviewCommit; import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk; import com.google.gerrit.server.git.MergeTip; @@ -57,7 +55,7 @@ public class SubmitStrategyFactory { CommitStatus commitStatus, RequestId submissionId, SubmitInput submitInput, - ListMultimap accountsToNotify, + NotifyResolver.Result notify, SubmoduleOp submoduleOp, boolean dryrun) throws IntegrationException { @@ -74,7 +72,7 @@ public class SubmitStrategyFactory { incoming, submissionId, submitInput, - accountsToNotify, + notify, submoduleOp, dryrun); switch (submitType) { diff --git a/java/com/google/gerrit/server/submit/SubmitStrategyOp.java b/java/com/google/gerrit/server/submit/SubmitStrategyOp.java index 8a6f9145ad..bc9109b0dd 100644 --- a/java/com/google/gerrit/server/submit/SubmitStrategyOp.java +++ b/java/com/google/gerrit/server/submit/SubmitStrategyOp.java @@ -505,8 +505,8 @@ abstract class SubmitStrategyOp implements BatchUpdateOp { ctx.getProject(), getId(), submitter.getAccountId(), - args.submitInput.notify, - args.accountsToNotify) + args.notify.handling(), + args.notify.accounts()) .sendAsync(); } catch (Exception e) { logger.atSevere().withCause(e).log("Cannot email merged notification for %s", getId()); diff --git a/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java b/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java index 15d9fa2a62..fc37f4cb0c 100644 --- a/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java +++ b/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java @@ -1276,7 +1276,9 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { } private StagedChange stageWipChangeWithExtraReviewer() throws Exception { - return stageChangeWithExtraReviewer(this::stageWipChange); + StagedChange sc = stageChangeWithExtraReviewer(this::stageWipChange); + assertThat(sender).didNotSend(); + return sc; } private void removeReviewer(StagedChange sc, TestAccount account) throws Exception {