Add reviewers by email to REST API and Email sender
This change adds support for adding reviewers by email in PostReviewer
and ChangeInfo. It also adds support for deleting reviewers using the
ReviewerResource by adding Address to it.
It further adds support for sending emails to reviewers that were
added by email. This support includes emails for adding and removing a
reviewer as well as for general change notifications.
This change also adds tests and updates the documentation.
Bug: Issue 4134
Change-Id: I901d711956ee69c25a05b455e068ec109821d79d
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 7bea2b7..da5d546 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -2805,6 +2805,41 @@
}
----
+If link:config-project-config.html#reviewer.enableByEmail[reviewer.enableByEmail] is set
+for the project, reviewers and CCs are not required to have a Gerrit account. If you POST
+an email address of a reviewer or CC then, they will be added to the change even if they
+don't have a Gerrit account.
+
+If this option is disabled, the request would fail with `400 Bad Request` if the email
+address can't be resolved to an active Gerrit account.
+
+Note that the name is optional so both "un.registered@reviewer.com" and
+"John Doe <un.registered@reviewer.com>" are valid inputs.
+
+Reviewers without Gerrit accounts can only be added on changes visible to anonymous users.
+
+.Request
+----
+ POST /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/reviewers HTTP/1.0
+ Content-Type: application/json; charset=UTF-8
+
+ {
+ "reviewer": "John Doe <un.registered@reviewer.com>"
+ }
+----
+
+.Response
+----
+ HTTP/1.1 200 OK
+ Content-Disposition: attachment
+ Content-Type: application/json; charset=UTF-8
+
+ )]}'
+ {
+ "input": "John Doe <un.registered@reviewer.com>"
+ }
+----
+
[[delete-reviewer]]
=== Delete Reviewer
--
@@ -6183,6 +6218,10 @@
|`approvals` |
The approvals of the reviewer as a map that maps the label names to the
approval values ("`-2`", "`-1`", "`0`", "`+1`", "`+2`").
+|`_account_id` |
+This field is inherited from `AccountInfo` but is optional here if an
+unregistered reviewer was added by email. See
+link:rest-api-changes.html#add-reviewer[add-reviewer] for details.
|==========================
[[reviewer-input]]
diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
index 24b3724..abdde61 100644
--- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -88,6 +88,7 @@
import com.google.gerrit.server.index.change.ChangeIndex;
import com.google.gerrit.server.index.change.ChangeIndexCollection;
import com.google.gerrit.server.index.change.ChangeIndexer;
+import com.google.gerrit.server.mail.Address;
import com.google.gerrit.server.mail.send.EmailHeader;
import com.google.gerrit.server.notedb.ChangeNoteUtil;
import com.google.gerrit.server.notedb.ChangeNotes;
@@ -1200,21 +1201,29 @@
}
protected void assertNotifyTo(TestAccount expected) {
+ assertNotifyTo(expected.emailAddress);
+ }
+
+ protected void assertNotifyTo(Address expected) {
assertThat(sender.getMessages()).hasSize(1);
Message m = sender.getMessages().get(0);
- assertThat(m.rcpt()).containsExactly(expected.emailAddress);
+ assertThat(m.rcpt()).containsExactly(expected);
assertThat(((EmailHeader.AddressList) m.headers().get("To")).getAddressList())
- .containsExactly(expected.emailAddress);
+ .containsExactly(expected);
assertThat(m.headers().get("CC").isEmpty()).isTrue();
}
protected void assertNotifyCc(TestAccount expected) {
+ assertNotifyCc(expected.emailAddress);
+ }
+
+ protected void assertNotifyCc(Address expected) {
assertThat(sender.getMessages()).hasSize(1);
Message m = sender.getMessages().get(0);
- assertThat(m.rcpt()).containsExactly(expected.emailAddress);
+ assertThat(m.rcpt()).containsExactly(expected);
assertThat(m.headers().get("To").isEmpty()).isTrue();
assertThat(((EmailHeader.AddressList) m.headers().get("CC")).getAddressList())
- .containsExactly(expected.emailAddress);
+ .containsExactly(expected);
}
protected void assertNotifyBcc(TestAccount expected) {
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java
new file mode 100644
index 0000000..2911b62
--- /dev/null
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java
@@ -0,0 +1,245 @@
+// Copyright (C) 2017 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.acceptance.rest.change;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.TruthJUnit.assume;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.NoHttpd;
+import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.extensions.api.changes.AddReviewerInput;
+import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.client.ListChangesOption;
+import com.google.gerrit.extensions.client.ReviewerState;
+import com.google.gerrit.extensions.common.AccountInfo;
+import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.restapi.BadRequestException;
+import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
+import com.google.gerrit.server.git.ProjectConfig;
+import com.google.gerrit.server.mail.Address;
+import com.google.gerrit.testutil.FakeEmailSender.Message;
+import java.util.EnumSet;
+import java.util.List;
+import org.junit.Before;
+import org.junit.Test;
+
+@NoHttpd
+public class ChangeReviewersByEmailIT extends AbstractDaemonTest {
+
+ @Before
+ public void setUp() throws Exception {
+ ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
+ cfg.setEnableReviewerByEmail(true);
+ saveProjectConfig(project, cfg);
+ }
+
+ @Test
+ public void addByEmail() throws Exception {
+ assume().that(notesMigration.readChanges()).isTrue();
+ AccountInfo acc = new AccountInfo("Foo Bar", "foo.bar@gerritcodereview.com");
+
+ for (ReviewerState state : ImmutableList.of(ReviewerState.CC, ReviewerState.REVIEWER)) {
+ PushOneCommit.Result r = createChange();
+
+ AddReviewerInput input = new AddReviewerInput();
+ input.reviewer = toRfcAddressString(acc);
+ input.state = state;
+ gApi.changes().id(r.getChangeId()).addReviewer(input);
+
+ ChangeInfo info =
+ gApi.changes().id(r.getChangeId()).get(EnumSet.of(ListChangesOption.DETAILED_LABELS));
+ assertThat(info.reviewers).isEqualTo(ImmutableMap.of(state, ImmutableList.of(acc)));
+ // All reviewers added by email should be removable
+ assertThat(info.removableReviewers).isEqualTo(ImmutableList.of(acc));
+ }
+ }
+
+ @Test
+ public void removeByEmail() throws Exception {
+ assume().that(notesMigration.readChanges()).isTrue();
+ AccountInfo acc = new AccountInfo("Foo Bar", "foo.bar@gerritcodereview.com");
+
+ for (ReviewerState state : ImmutableList.of(ReviewerState.CC, ReviewerState.REVIEWER)) {
+ PushOneCommit.Result r = createChange();
+
+ AddReviewerInput addInput = new AddReviewerInput();
+ addInput.reviewer = toRfcAddressString(acc);
+ addInput.state = state;
+ gApi.changes().id(r.getChangeId()).addReviewer(addInput);
+
+ gApi.changes().id(r.getChangeId()).reviewer(acc.email).remove();
+
+ ChangeInfo info =
+ gApi.changes().id(r.getChangeId()).get(EnumSet.of(ListChangesOption.DETAILED_LABELS));
+ assertThat(info.reviewers).isEmpty();
+ }
+ }
+
+ @Test
+ public void convertFromCCToReviewer() throws Exception {
+ assume().that(notesMigration.readChanges()).isTrue();
+ AccountInfo acc = new AccountInfo("Foo Bar", "foo.bar@gerritcodereview.com");
+
+ PushOneCommit.Result r = createChange();
+
+ AddReviewerInput addInput = new AddReviewerInput();
+ addInput.reviewer = toRfcAddressString(acc);
+ addInput.state = ReviewerState.CC;
+ gApi.changes().id(r.getChangeId()).addReviewer(addInput);
+
+ AddReviewerInput modifyInput = new AddReviewerInput();
+ modifyInput.reviewer = addInput.reviewer;
+ modifyInput.state = ReviewerState.REVIEWER;
+ gApi.changes().id(r.getChangeId()).addReviewer(modifyInput);
+
+ ChangeInfo info =
+ gApi.changes().id(r.getChangeId()).get(EnumSet.of(ListChangesOption.DETAILED_LABELS));
+ assertThat(info.reviewers)
+ .isEqualTo(ImmutableMap.of(ReviewerState.REVIEWER, ImmutableList.of(acc)));
+ }
+
+ @Test
+ public void addedReviewersGetNotified() throws Exception {
+ assume().that(notesMigration.readChanges()).isTrue();
+ AccountInfo acc = new AccountInfo("Foo Bar", "foo.bar@gerritcodereview.com");
+
+ for (ReviewerState state : ImmutableList.of(ReviewerState.CC, ReviewerState.REVIEWER)) {
+ PushOneCommit.Result r = createChange();
+
+ AddReviewerInput input = new AddReviewerInput();
+ input.reviewer = toRfcAddressString(acc);
+ input.state = state;
+ gApi.changes().id(r.getChangeId()).addReviewer(input);
+
+ List<Message> messages = sender.getMessages();
+ assertThat(messages).hasSize(1);
+ assertThat(messages.get(0).rcpt()).containsExactly(Address.parse(input.reviewer));
+ sender.clear();
+ }
+ }
+
+ @Test
+ public void removingReviewerTriggersNotification() throws Exception {
+ assume().that(notesMigration.readChanges()).isTrue();
+ AccountInfo acc = new AccountInfo("Foo Bar", "foo.bar@gerritcodereview.com");
+
+ for (ReviewerState state : ImmutableList.of(ReviewerState.CC, ReviewerState.REVIEWER)) {
+ PushOneCommit.Result r = createChange();
+
+ AddReviewerInput addInput = new AddReviewerInput();
+ addInput.reviewer = toRfcAddressString(acc);
+ addInput.state = state;
+ gApi.changes().id(r.getChangeId()).addReviewer(addInput);
+
+ // Review change as user
+ ReviewInput reviewInput = new ReviewInput();
+ reviewInput.message = "I have a comment";
+ setApiUser(user);
+ revision(r).review(reviewInput);
+ setApiUser(admin);
+
+ sender.clear();
+
+ // Delete as admin
+ gApi.changes().id(r.getChangeId()).reviewer(addInput.reviewer).remove();
+
+ List<Message> messages = sender.getMessages();
+ assertThat(messages).hasSize(1);
+ assertThat(messages.get(0).rcpt())
+ .containsExactly(Address.parse(addInput.reviewer), user.emailAddress);
+ sender.clear();
+ }
+ }
+
+ @Test
+ public void reviewerAndCCReceiveRegularNotification() throws Exception {
+ assume().that(notesMigration.readChanges()).isTrue();
+ AccountInfo acc = new AccountInfo("Foo Bar", "foo.bar@gerritcodereview.com");
+
+ for (ReviewerState state : ImmutableList.of(ReviewerState.CC, ReviewerState.REVIEWER)) {
+ PushOneCommit.Result r = createChange();
+
+ AddReviewerInput input = new AddReviewerInput();
+ input.reviewer = toRfcAddressString(acc);
+ input.state = state;
+ gApi.changes().id(r.getChangeId()).addReviewer(input);
+ sender.clear();
+
+ gApi.changes()
+ .id(r.getChangeId())
+ .revision(r.getCommit().name())
+ .review(ReviewInput.approve());
+
+ if (state == ReviewerState.CC) {
+ assertNotifyCc(Address.parse(input.reviewer));
+ } else {
+ assertNotifyTo(Address.parse(input.reviewer));
+ }
+ }
+ }
+
+ @Test
+ public void rejectMissingEmail() throws Exception {
+ assume().that(notesMigration.readChanges()).isTrue();
+ PushOneCommit.Result r = createChange();
+
+ exception.expect(UnprocessableEntityException.class);
+ exception.expectMessage("email invalid");
+ gApi.changes().id(r.getChangeId()).addReviewer("");
+ }
+
+ @Test
+ public void rejectMalformedEmail() throws Exception {
+ assume().that(notesMigration.readChanges()).isTrue();
+ PushOneCommit.Result r = createChange();
+
+ exception.expect(UnprocessableEntityException.class);
+ exception.expectMessage("email invalid");
+ gApi.changes().id(r.getChangeId()).addReviewer("Foo Bar <foo.bar@");
+ }
+
+ @Test
+ public void rejectOnNonPublicChange() throws Exception {
+ assume().that(notesMigration.readChanges()).isTrue();
+ PushOneCommit.Result r = createDraftChange();
+
+ exception.expect(BadRequestException.class);
+ exception.expectMessage("change is not publicly visible");
+ gApi.changes().id(r.getChangeId()).addReviewer("Foo Bar <foo.bar@gerritcodereview.com>");
+ }
+
+ @Test
+ public void rejectWhenFeatureIsDisabled() throws Exception {
+ assume().that(notesMigration.readChanges()).isTrue();
+
+ ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
+ cfg.setEnableReviewerByEmail(false);
+ saveProjectConfig(project, cfg);
+
+ PushOneCommit.Result r = createChange();
+
+ exception.expect(UnprocessableEntityException.class);
+ exception.expectMessage(
+ "Foo Bar <foo.bar@gerritcodereview.com> does not identify a registered user or group");
+ gApi.changes().id(r.getChangeId()).addReviewer("Foo Bar <foo.bar@gerritcodereview.com>");
+ }
+
+ private static String toRfcAddressString(AccountInfo info) {
+ return (new Address(info.name, info.email)).toString();
+ }
+}
diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ReviewerInfo.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ReviewerInfo.java
index af61481..3a33de9 100644
--- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ReviewerInfo.java
+++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ReviewerInfo.java
@@ -25,6 +25,13 @@
*/
@Nullable public Map<String, String> approvals;
+ public static ReviewerInfo byEmail(@Nullable String name, String email) {
+ ReviewerInfo info = new ReviewerInfo();
+ info.name = name;
+ info.email = email;
+ return info;
+ }
+
public ReviewerInfo(Integer id) {
super(id);
}
@@ -33,4 +40,6 @@
public String toString() {
return username;
}
+
+ private ReviewerInfo() {}
}
diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/AccountInfo.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/AccountInfo.java
index 2fb32d7..f20509b 100644
--- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/AccountInfo.java
+++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/AccountInfo.java
@@ -15,6 +15,7 @@
package com.google.gerrit.extensions.common;
import java.util.List;
+import java.util.Objects;
public class AccountInfo {
public Integer _accountId;
@@ -29,4 +30,34 @@
public AccountInfo(Integer id) {
this._accountId = id;
}
+
+ /** To be used ONLY in connection with unregistered reviewers and CCs. */
+ public AccountInfo(String name, String email) {
+ this.name = name;
+ this.email = email;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (o instanceof AccountInfo) {
+ AccountInfo accountInfo = (AccountInfo) o;
+ return Objects.equals(_accountId, accountInfo._accountId)
+ && Objects.equals(name, accountInfo.name)
+ && Objects.equals(email, accountInfo.email)
+ && Objects.equals(secondaryEmails, accountInfo.secondaryEmails)
+ && Objects.equals(username, accountInfo.username)
+ && Objects.equals(avatars, accountInfo.avatars)
+ && Objects.equals(_moreAccounts, accountInfo._moreAccounts)
+ && Objects.equals(status, accountInfo.status);
+ }
+ return false;
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(
+ _accountId, name, email, secondaryEmails, username, avatars, _moreAccounts, status);
+ }
+
+ protected AccountInfo() {}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
index 64aadde..bfa2ee9 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
@@ -108,6 +108,7 @@
import com.google.gerrit.server.git.MergeUtil;
import com.google.gerrit.server.index.change.ChangeField;
import com.google.gerrit.server.index.change.ChangeIndexCollection;
+import com.google.gerrit.server.mail.Address;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ReviewerStateInternal;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
@@ -531,6 +532,12 @@
cd.reviewers().asTable().rowMap().entrySet()) {
out.reviewers.put(e.getKey().asReviewerState(), toAccountInfo(e.getValue().keySet()));
}
+ // TODO(hiesel) Load from ChangeData instead after the data was added there
+ for (Map.Entry<ReviewerStateInternal, Map<Address, Timestamp>> e :
+ cd.notes().getReviewersByEmail().asTable().rowMap().entrySet()) {
+ out.reviewers.put(
+ e.getKey().asReviewerState(), toAccountInfoByEmail(e.getValue().keySet()));
+ }
out.removableReviewers = removableReviewers(ctl, out);
}
@@ -1075,9 +1082,11 @@
Collection<AccountInfo> ccs = out.reviewers.get(ReviewerState.CC);
if (ccs != null) {
for (AccountInfo ai : ccs) {
- Account.Id id = new Account.Id(ai._accountId);
- if (ctl.canRemoveReviewer(id, 0)) {
- removable.add(id);
+ if (ai._accountId != null) {
+ Account.Id id = new Account.Id(ai._accountId);
+ if (ctl.canRemoveReviewer(id, 0)) {
+ removable.add(id);
+ }
}
}
}
@@ -1091,6 +1100,14 @@
for (Account.Id id : removable) {
result.add(accountLoader.get(id));
}
+ // Reviewers added by email are always removable
+ for (Collection<AccountInfo> infos : out.reviewers.values()) {
+ for (AccountInfo info : infos) {
+ if (info._accountId == null) {
+ result.add(info);
+ }
+ }
+ }
return result;
}
@@ -1102,6 +1119,14 @@
.collect(toList());
}
+ private Collection<AccountInfo> toAccountInfoByEmail(Collection<Address> addresses) {
+ return addresses
+ .stream()
+ .map(a -> new AccountInfo(a.getName(), a.getEmail()))
+ .sorted(AccountInfoComparator.ORDER_NULLS_FIRST)
+ .collect(toList());
+ }
+
@Nullable
private Repository openRepoIfNecessary(ChangeControl ctl) throws IOException {
if (has(ALL_COMMITS) || has(CURRENT_COMMIT) || has(COMMIT_FOOTERS)) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewer.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewer.java
index 1485d03..4822478 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewer.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewer.java
@@ -14,92 +14,38 @@
package com.google.gerrit.server.change;
-import com.google.common.collect.Iterables;
import com.google.gerrit.common.TimeUtil;
-import com.google.gerrit.common.data.LabelType;
-import com.google.gerrit.common.data.LabelTypes;
import com.google.gerrit.extensions.api.changes.DeleteReviewerInput;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
-import com.google.gerrit.extensions.restapi.AuthException;
-import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestModifyView;
-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;
-import com.google.gerrit.reviewdb.client.PatchSetApproval;
-import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.server.ReviewDb;
-import com.google.gerrit.reviewdb.server.ReviewDbUtil;
-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.extensions.events.ReviewerDeleted;
-import com.google.gerrit.server.mail.send.DeleteReviewerSender;
-import com.google.gerrit.server.notedb.ChangeUpdate;
-import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
-import com.google.gerrit.server.notedb.NotesMigration;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
-import com.google.gerrit.server.update.BatchUpdateReviewDb;
-import com.google.gerrit.server.update.ChangeContext;
-import com.google.gerrit.server.update.Context;
import com.google.gerrit.server.update.UpdateException;
-import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
-import java.util.ArrayList;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
@Singleton
public class DeleteReviewer implements RestModifyView<ReviewerResource, DeleteReviewerInput> {
- private static final Logger log = LoggerFactory.getLogger(DeleteReviewer.class);
private final Provider<ReviewDb> dbProvider;
- private final ApprovalsUtil approvalsUtil;
- private final PatchSetUtil psUtil;
- private final ChangeMessagesUtil cmUtil;
private final BatchUpdate.Factory batchUpdateFactory;
- private final IdentifiedUser.GenericFactory userFactory;
- private final ReviewerDeleted reviewerDeleted;
- private final Provider<IdentifiedUser> user;
- private final DeleteReviewerSender.Factory deleteReviewerSenderFactory;
- private final NotesMigration migration;
- private final NotifyUtil notifyUtil;
+ private final DeleteReviewerOp.Factory deleteReviewerOpFactory;
+ private final DeleteReviewerByEmailOp.Factory deleteReviewerByEmailOpFactory;
@Inject
DeleteReviewer(
Provider<ReviewDb> dbProvider,
- ApprovalsUtil approvalsUtil,
- PatchSetUtil psUtil,
- ChangeMessagesUtil cmUtil,
BatchUpdate.Factory batchUpdateFactory,
- IdentifiedUser.GenericFactory userFactory,
- ReviewerDeleted reviewerDeleted,
- Provider<IdentifiedUser> user,
- DeleteReviewerSender.Factory deleteReviewerSenderFactory,
- NotesMigration migration,
- NotifyUtil notifyUtil) {
+ DeleteReviewerOp.Factory deleteReviewerOpFactory,
+ DeleteReviewerByEmailOp.Factory deleteReviewerByEmailOpFactory) {
this.dbProvider = dbProvider;
- this.approvalsUtil = approvalsUtil;
- this.psUtil = psUtil;
- this.cmUtil = cmUtil;
this.batchUpdateFactory = batchUpdateFactory;
- this.userFactory = userFactory;
- this.reviewerDeleted = reviewerDeleted;
- this.user = user;
- this.deleteReviewerSenderFactory = deleteReviewerSenderFactory;
- this.migration = migration;
- this.notifyUtil = notifyUtil;
+ this.deleteReviewerOpFactory = deleteReviewerOpFactory;
+ this.deleteReviewerByEmailOpFactory = deleteReviewerByEmailOpFactory;
}
@Override
@@ -118,151 +64,15 @@
rsrc.getChangeResource().getProject(),
rsrc.getChangeResource().getUser(),
TimeUtil.nowTs())) {
- Op op = new Op(rsrc.getReviewerUser().getAccount(), input);
+ BatchUpdateOp op;
+ if (rsrc.isByEmail()) {
+ op = deleteReviewerByEmailOpFactory.create(rsrc.getReviewerByEmail(), input);
+ } else {
+ op = deleteReviewerOpFactory.create(rsrc.getReviewerUser().getAccount(), input);
+ }
bu.addOp(rsrc.getChange().getId(), op);
bu.execute();
}
-
return Response.none();
}
-
- private class Op implements BatchUpdateOp {
- private final Account reviewer;
- private final DeleteReviewerInput input;
- ChangeMessage changeMessage;
- Change currChange;
- PatchSet currPs;
- Map<String, Short> newApprovals = new HashMap<>();
- Map<String, Short> oldApprovals = new HashMap<>();
-
- Op(Account reviewerAccount, DeleteReviewerInput input) {
- this.reviewer = reviewerAccount;
- this.input = input;
- }
-
- @Override
- public boolean updateChange(ChangeContext ctx)
- throws AuthException, ResourceNotFoundException, OrmException {
- Account.Id reviewerId = reviewer.getId();
- if (!approvalsUtil.getReviewers(ctx.getDb(), ctx.getNotes()).all().contains(reviewerId)) {
- throw new ResourceNotFoundException();
- }
- currChange = ctx.getChange();
- currPs = psUtil.current(ctx.getDb(), ctx.getNotes());
-
- LabelTypes labelTypes = ctx.getControl().getLabelTypes();
- // removing a reviewer will remove all her votes
- for (LabelType lt : labelTypes.getLabelTypes()) {
- newApprovals.put(lt.getName(), (short) 0);
- }
-
- StringBuilder msg = new StringBuilder();
- msg.append("Removed reviewer " + reviewer.getFullName());
- StringBuilder removedVotesMsg = new StringBuilder();
- removedVotesMsg.append(" with the following votes:\n\n");
- List<PatchSetApproval> del = new ArrayList<>();
- boolean votesRemoved = false;
- for (PatchSetApproval a : approvals(ctx, reviewerId)) {
- if (ctx.getControl().canRemoveReviewer(a)) {
- del.add(a);
- if (a.getPatchSetId().equals(currPs.getId()) && a.getValue() != 0) {
- oldApprovals.put(a.getLabel(), a.getValue());
- removedVotesMsg
- .append("* ")
- .append(a.getLabel())
- .append(formatLabelValue(a.getValue()))
- .append(" by ")
- .append(userFactory.create(a.getAccountId()).getNameEmail())
- .append("\n");
- votesRemoved = true;
- }
- } else {
- throw new AuthException("delete reviewer not permitted");
- }
- }
-
- if (votesRemoved) {
- msg.append(removedVotesMsg);
- } else {
- msg.append(".");
- }
- ctx.getDb().patchSetApprovals().delete(del);
- ChangeUpdate update = ctx.getUpdate(currPs.getId());
- update.removeReviewer(reviewerId);
-
- changeMessage =
- ChangeMessagesUtil.newMessage(
- ctx, msg.toString(), ChangeMessagesUtil.TAG_DELETE_REVIEWER);
- cmUtil.addChangeMessage(ctx.getDb(), update, changeMessage);
-
- return true;
- }
-
- @Override
- public void postUpdate(Context ctx) {
- if (NotifyUtil.shouldNotify(input.notify, input.notifyDetails)) {
- emailReviewers(ctx.getProject(), currChange, changeMessage);
- }
- reviewerDeleted.fire(
- currChange,
- currPs,
- reviewer,
- ctx.getAccount(),
- changeMessage.getMessage(),
- newApprovals,
- oldApprovals,
- input.notify,
- ctx.getWhen());
- }
-
- private Iterable<PatchSetApproval> approvals(ChangeContext ctx, Account.Id accountId)
- throws OrmException {
- Change.Id changeId = ctx.getNotes().getChangeId();
- Iterable<PatchSetApproval> approvals;
- PrimaryStorage r = PrimaryStorage.of(ctx.getChange());
-
- if (migration.readChanges() && r == PrimaryStorage.REVIEW_DB) {
- // Because NoteDb and ReviewDb have different semantics for zero-value
- // approvals, we must fall back to ReviewDb as the source of truth here.
- ReviewDb db = ctx.getDb();
-
- if (db instanceof BatchUpdateReviewDb) {
- db = ((BatchUpdateReviewDb) db).unsafeGetDelegate();
- }
- db = ReviewDbUtil.unwrapDb(db);
- approvals = db.patchSetApprovals().byChange(changeId);
- } else {
- approvals = approvalsUtil.byChange(ctx.getDb(), ctx.getNotes()).values();
- }
-
- return Iterables.filter(approvals, psa -> accountId.equals(psa.getAccountId()));
- }
-
- private String formatLabelValue(short value) {
- if (value > 0) {
- return "+" + value;
- }
- return Short.toString(value);
- }
-
- private void emailReviewers(
- Project.NameKey projectName, Change change, ChangeMessage changeMessage) {
- Account.Id userId = user.get().getAccountId();
- if (userId.equals(reviewer.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.getId()));
- cm.setChangeMessage(changeMessage.getMessage(), changeMessage.getWrittenOn());
- cm.setNotify(input.notify);
- cm.setAccountsToNotify(notifyUtil.resolveAccounts(input.notifyDetails));
- cm.send();
- } catch (Exception err) {
- log.error("Cannot email update for change " + change.getId(), err);
- }
- }
- }
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewerByEmailOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewerByEmailOp.java
new file mode 100644
index 0000000..adfe3f5
--- /dev/null
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewerByEmailOp.java
@@ -0,0 +1,96 @@
+// Copyright (C) 2017 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.change;
+
+import com.google.gerrit.extensions.api.changes.DeleteReviewerInput;
+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.mail.Address;
+import com.google.gerrit.server.mail.send.DeleteReviewerSender;
+import com.google.gerrit.server.update.BatchUpdateOp;
+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.assistedinject.Assisted;
+import java.util.Collections;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class DeleteReviewerByEmailOp implements BatchUpdateOp {
+ private static final Logger log = LoggerFactory.getLogger(DeleteReviewer.class);
+
+ public interface Factory {
+ DeleteReviewerByEmailOp create(Address reviewer, DeleteReviewerInput input);
+ }
+
+ private final DeleteReviewerSender.Factory deleteReviewerSenderFactory;
+ private final NotifyUtil notifyUtil;
+ private final Address reviewer;
+ private final DeleteReviewerInput input;
+
+ private ChangeMessage changeMessage;
+ private Change.Id changeId;
+
+ @Inject
+ DeleteReviewerByEmailOp(
+ DeleteReviewerSender.Factory deleteReviewerSenderFactory,
+ NotifyUtil notifyUtil,
+ @Assisted Address reviewer,
+ @Assisted DeleteReviewerInput input) {
+ this.deleteReviewerSenderFactory = deleteReviewerSenderFactory;
+ this.notifyUtil = notifyUtil;
+ this.reviewer = reviewer;
+ this.input = input;
+ }
+
+ @Override
+ public boolean updateChange(ChangeContext ctx) throws OrmException {
+ changeId = ctx.getChange().getId();
+ PatchSet.Id psId = ctx.getChange().currentPatchSetId();
+ String msg = "Removed reviewer " + reviewer;
+ changeMessage =
+ new ChangeMessage(
+ new ChangeMessage.Key(changeId, ChangeUtil.messageUuid()),
+ ctx.getAccountId(),
+ ctx.getWhen(),
+ psId);
+ changeMessage.setMessage(msg);
+
+ ctx.getUpdate(psId).setChangeMessage(msg);
+ ctx.getUpdate(psId).removeReviewerByEmail(reviewer);
+ return true;
+ }
+
+ @Override
+ public void postUpdate(Context ctx) {
+ if (!NotifyUtil.shouldNotify(input.notify, input.notifyDetails)) {
+ return;
+ }
+ try {
+ DeleteReviewerSender cm = deleteReviewerSenderFactory.create(ctx.getProject(), changeId);
+ 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.send();
+ } catch (Exception err) {
+ log.error("Cannot email update for change " + changeId, err);
+ }
+ }
+}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewerOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewerOp.java
new file mode 100644
index 0000000..a255f79
--- /dev/null
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewerOp.java
@@ -0,0 +1,232 @@
+// Copyright (C) 2017 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.change;
+
+import com.google.common.collect.Iterables;
+import com.google.gerrit.common.data.LabelType;
+import com.google.gerrit.common.data.LabelTypes;
+import com.google.gerrit.extensions.api.changes.DeleteReviewerInput;
+import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
+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;
+import com.google.gerrit.reviewdb.client.PatchSetApproval;
+import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.reviewdb.server.ReviewDbUtil;
+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.extensions.events.ReviewerDeleted;
+import com.google.gerrit.server.mail.send.DeleteReviewerSender;
+import com.google.gerrit.server.notedb.ChangeUpdate;
+import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
+import com.google.gerrit.server.notedb.NotesMigration;
+import com.google.gerrit.server.update.BatchUpdateOp;
+import com.google.gerrit.server.update.BatchUpdateReviewDb;
+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 com.google.inject.assistedinject.Assisted;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class DeleteReviewerOp implements BatchUpdateOp {
+ private static final Logger log = LoggerFactory.getLogger(DeleteReviewer.class);
+
+ public interface Factory {
+ DeleteReviewerOp create(Account reviewerAccount, DeleteReviewerInput input);
+ }
+
+ private final ApprovalsUtil approvalsUtil;
+ private final PatchSetUtil psUtil;
+ private final ChangeMessagesUtil cmUtil;
+ private final IdentifiedUser.GenericFactory userFactory;
+ private final ReviewerDeleted reviewerDeleted;
+ private final Provider<IdentifiedUser> user;
+ private final DeleteReviewerSender.Factory deleteReviewerSenderFactory;
+ private final NotesMigration migration;
+ private final NotifyUtil notifyUtil;
+
+ private final Account reviewer;
+ private final DeleteReviewerInput input;
+
+ ChangeMessage changeMessage;
+ Change currChange;
+ PatchSet currPs;
+ Map<String, Short> newApprovals = new HashMap<>();
+ Map<String, Short> oldApprovals = new HashMap<>();
+
+ @Inject
+ DeleteReviewerOp(
+ ApprovalsUtil approvalsUtil,
+ PatchSetUtil psUtil,
+ ChangeMessagesUtil cmUtil,
+ IdentifiedUser.GenericFactory userFactory,
+ ReviewerDeleted reviewerDeleted,
+ Provider<IdentifiedUser> user,
+ DeleteReviewerSender.Factory deleteReviewerSenderFactory,
+ NotesMigration migration,
+ NotifyUtil notifyUtil,
+ @Assisted Account reviewerAccount,
+ @Assisted DeleteReviewerInput input) {
+ this.approvalsUtil = approvalsUtil;
+ this.psUtil = psUtil;
+ this.cmUtil = cmUtil;
+ this.userFactory = userFactory;
+ this.reviewerDeleted = reviewerDeleted;
+ this.user = user;
+ this.deleteReviewerSenderFactory = deleteReviewerSenderFactory;
+ this.migration = migration;
+ this.notifyUtil = notifyUtil;
+
+ this.reviewer = reviewerAccount;
+ this.input = input;
+ }
+
+ @Override
+ public boolean updateChange(ChangeContext ctx)
+ throws AuthException, ResourceNotFoundException, OrmException {
+ Account.Id reviewerId = reviewer.getId();
+ if (!approvalsUtil.getReviewers(ctx.getDb(), ctx.getNotes()).all().contains(reviewerId)) {
+ throw new ResourceNotFoundException();
+ }
+ currChange = ctx.getChange();
+ currPs = psUtil.current(ctx.getDb(), ctx.getNotes());
+
+ LabelTypes labelTypes = ctx.getControl().getLabelTypes();
+ // removing a reviewer will remove all her votes
+ for (LabelType lt : labelTypes.getLabelTypes()) {
+ newApprovals.put(lt.getName(), (short) 0);
+ }
+
+ StringBuilder msg = new StringBuilder();
+ msg.append("Removed reviewer " + reviewer.getFullName());
+ StringBuilder removedVotesMsg = new StringBuilder();
+ removedVotesMsg.append(" with the following votes:\n\n");
+ List<PatchSetApproval> del = new ArrayList<>();
+ boolean votesRemoved = false;
+ for (PatchSetApproval a : approvals(ctx, reviewerId)) {
+ if (ctx.getControl().canRemoveReviewer(a)) {
+ del.add(a);
+ if (a.getPatchSetId().equals(currPs.getId()) && a.getValue() != 0) {
+ oldApprovals.put(a.getLabel(), a.getValue());
+ removedVotesMsg
+ .append("* ")
+ .append(a.getLabel())
+ .append(formatLabelValue(a.getValue()))
+ .append(" by ")
+ .append(userFactory.create(a.getAccountId()).getNameEmail())
+ .append("\n");
+ votesRemoved = true;
+ }
+ } else {
+ throw new AuthException("delete reviewer not permitted");
+ }
+ }
+
+ if (votesRemoved) {
+ msg.append(removedVotesMsg);
+ } else {
+ msg.append(".");
+ }
+ ctx.getDb().patchSetApprovals().delete(del);
+ ChangeUpdate update = ctx.getUpdate(currPs.getId());
+ update.removeReviewer(reviewerId);
+
+ changeMessage =
+ ChangeMessagesUtil.newMessage(ctx, msg.toString(), ChangeMessagesUtil.TAG_DELETE_REVIEWER);
+ cmUtil.addChangeMessage(ctx.getDb(), update, changeMessage);
+
+ return true;
+ }
+
+ @Override
+ public void postUpdate(Context ctx) {
+ if (NotifyUtil.shouldNotify(input.notify, input.notifyDetails)) {
+ emailReviewers(ctx.getProject(), currChange, changeMessage);
+ }
+ reviewerDeleted.fire(
+ currChange,
+ currPs,
+ reviewer,
+ ctx.getAccount(),
+ changeMessage.getMessage(),
+ newApprovals,
+ oldApprovals,
+ input.notify,
+ ctx.getWhen());
+ }
+
+ private Iterable<PatchSetApproval> approvals(ChangeContext ctx, Account.Id accountId)
+ throws OrmException {
+ Change.Id changeId = ctx.getNotes().getChangeId();
+ Iterable<PatchSetApproval> approvals;
+ PrimaryStorage r = PrimaryStorage.of(ctx.getChange());
+
+ if (migration.readChanges() && r == PrimaryStorage.REVIEW_DB) {
+ // Because NoteDb and ReviewDb have different semantics for zero-value
+ // approvals, we must fall back to ReviewDb as the source of truth here.
+ ReviewDb db = ctx.getDb();
+
+ if (db instanceof BatchUpdateReviewDb) {
+ db = ((BatchUpdateReviewDb) db).unsafeGetDelegate();
+ }
+ db = ReviewDbUtil.unwrapDb(db);
+ approvals = db.patchSetApprovals().byChange(changeId);
+ } else {
+ approvals = approvalsUtil.byChange(ctx.getDb(), ctx.getNotes()).values();
+ }
+
+ return Iterables.filter(approvals, psa -> accountId.equals(psa.getAccountId()));
+ }
+
+ private String formatLabelValue(short value) {
+ if (value > 0) {
+ return "+" + value;
+ }
+ return Short.toString(value);
+ }
+
+ private void emailReviewers(
+ Project.NameKey projectName, Change change, ChangeMessage changeMessage) {
+ Account.Id userId = user.get().getAccountId();
+ if (userId.equals(reviewer.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.getId()));
+ cm.setChangeMessage(changeMessage.getMessage(), changeMessage.getWrittenOn());
+ cm.setNotify(input.notify);
+ cm.setAccountsToNotify(notifyUtil.resolveAccounts(input.notifyDetails));
+ cm.send();
+ } catch (Exception err) {
+ log.error("Cannot email update for change " + change.getId(), err);
+ }
+ }
+}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ListReviewers.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ListReviewers.java
index 27ec89d..1dba58c 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ListReviewers.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ListReviewers.java
@@ -19,6 +19,7 @@
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ApprovalsUtil;
+import com.google.gerrit.server.mail.Address;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -48,11 +49,16 @@
@Override
public List<ReviewerInfo> apply(ChangeResource rsrc) throws OrmException {
- Map<Account.Id, ReviewerResource> reviewers = new LinkedHashMap<>();
+ Map<String, ReviewerResource> reviewers = new LinkedHashMap<>();
ReviewDb db = dbProvider.get();
for (Account.Id accountId : approvalsUtil.getReviewers(db, rsrc.getNotes()).all()) {
- if (!reviewers.containsKey(accountId)) {
- reviewers.put(accountId, resourceFactory.create(rsrc, accountId));
+ if (!reviewers.containsKey(accountId.toString())) {
+ reviewers.put(accountId.toString(), resourceFactory.create(rsrc, accountId));
+ }
+ }
+ for (Address adr : rsrc.getNotes().getReviewersByEmail().all()) {
+ if (!reviewers.containsKey(adr.toString())) {
+ reviewers.put(adr.toString(), new ReviewerResource(rsrc, adr));
}
}
return json.format(reviewers.values());
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ListRevisionReviewers.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ListRevisionReviewers.java
index d0c8ca0..5aaee56 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ListRevisionReviewers.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ListRevisionReviewers.java
@@ -20,6 +20,7 @@
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ApprovalsUtil;
+import com.google.gerrit.server.mail.Address;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -54,11 +55,16 @@
throw new MethodNotAllowedException("Cannot list reviewers on non-current patch set");
}
- Map<Account.Id, ReviewerResource> reviewers = new LinkedHashMap<>();
+ Map<String, ReviewerResource> reviewers = new LinkedHashMap<>();
ReviewDb db = dbProvider.get();
for (Account.Id accountId : approvalsUtil.getReviewers(db, rsrc.getNotes()).all()) {
- if (!reviewers.containsKey(accountId)) {
- reviewers.put(accountId, resourceFactory.create(rsrc, accountId));
+ if (!reviewers.containsKey(accountId.toString())) {
+ reviewers.put(accountId.toString(), resourceFactory.create(rsrc, accountId));
+ }
+ }
+ for (Address address : rsrc.getNotes().getReviewersByEmail().all()) {
+ if (!reviewers.containsKey(address.toString())) {
+ reviewers.put(address.toString(), new ReviewerResource(rsrc, address));
}
}
return json.format(reviewers.values());
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java
index 875286a..f23ab0c 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java
@@ -161,5 +161,7 @@
factory(SetAssigneeOp.Factory.class);
factory(SetHashtagsOp.Factory.class);
factory(ChangeResource.Factory.class);
+ factory(DeleteReviewerOp.Factory.class);
+ factory(DeleteReviewerByEmailOp.Factory.class);
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
index 76cc7e8..bd7939d 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
@@ -86,6 +86,7 @@
import com.google.gerrit.server.ReviewerSet;
import com.google.gerrit.server.account.AccountsCollection;
import com.google.gerrit.server.extensions.events.CommentAdded;
+import com.google.gerrit.server.mail.Address;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.notedb.NotesMigration;
@@ -313,14 +314,18 @@
ListMultimap<RecipientType, Account.Id> accountsToNotify) {
List<Account.Id> to = new ArrayList<>();
List<Account.Id> cc = new ArrayList<>();
+ List<Address> toByEmail = new ArrayList<>();
+ List<Address> ccByEmail = new ArrayList<>();
for (PostReviewers.Addition addition : reviewerAdditions) {
if (addition.op.state == ReviewerState.REVIEWER) {
to.addAll(addition.op.reviewers.keySet());
+ toByEmail.addAll(addition.op.reviewersByEmail);
} else if (addition.op.state == ReviewerState.CC) {
cc.addAll(addition.op.reviewers.keySet());
+ ccByEmail.addAll(addition.op.reviewersByEmail);
}
}
- postReviewers.emailReviewers(change, to, cc, notify, accountsToNotify);
+ postReviewers.emailReviewers(change, to, cc, toByEmail, ccByEmail, notify, accountsToNotify);
}
private RevisionResource onBehalfOf(RevisionResource rev, ReviewInput in)
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java
index 7031d51..9df0bd7 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java
@@ -23,6 +23,7 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ListMultimap;
import com.google.common.collect.Lists;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.common.data.GroupDescription;
import com.google.gerrit.common.errors.NoSuchGroupException;
@@ -32,6 +33,7 @@
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;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestModifyView;
@@ -42,6 +44,7 @@
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;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
@@ -52,12 +55,17 @@
import com.google.gerrit.server.account.GroupMembers;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.extensions.events.ReviewerAdded;
+import com.google.gerrit.server.git.ProjectConfig;
import com.google.gerrit.server.group.GroupsCollection;
import com.google.gerrit.server.group.SystemGroupBackend;
+import com.google.gerrit.server.mail.Address;
import com.google.gerrit.server.mail.send.AddReviewerSender;
+import com.google.gerrit.server.mail.send.OutgoingEmailValidator;
import com.google.gerrit.server.notedb.NotesMigration;
+import com.google.gerrit.server.notedb.ReviewerStateInternal;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.NoSuchProjectException;
+import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
@@ -104,6 +112,8 @@
private final NotesMigration migration;
private final AccountCache accountCache;
private final NotifyUtil notifyUtil;
+ private final ProjectCache projectCache;
+ private final Provider<AnonymousUser> anonymousProvider;
@Inject
PostReviewers(
@@ -124,7 +134,9 @@
ReviewerAdded reviewerAdded,
NotesMigration migration,
AccountCache accountCache,
- NotifyUtil notifyUtil) {
+ NotifyUtil notifyUtil,
+ ProjectCache projectCache,
+ Provider<AnonymousUser> anonymousProvider) {
this.accounts = accounts;
this.reviewerFactory = reviewerFactory;
this.approvalsUtil = approvalsUtil;
@@ -143,6 +155,8 @@
this.migration = migration;
this.accountCache = accountCache;
this.notifyUtil = notifyUtil;
+ this.projectCache = projectCache;
+ this.anonymousProvider = anonymousProvider;
}
@Override
@@ -167,6 +181,7 @@
return addition.result;
}
+ // TODO(hiesel) Refactor this as it starts to become unreadable
public Addition prepareApplication(
ChangeResource rsrc, AddReviewerInput input, boolean allowGroup)
throws OrmException, RestApiException, IOException {
@@ -174,17 +189,28 @@
try {
accountId = accounts.parse(input.reviewer).getAccountId();
} catch (UnprocessableEntityException e) {
+ ProjectConfig projectConfig = projectCache.checkedGet(rsrc.getProject()).getConfig();
if (allowGroup) {
try {
return putGroup(rsrc, input);
} catch (UnprocessableEntityException e2) {
- throw new UnprocessableEntityException(
- MessageFormat.format(
- ChangeMessages.get().reviewerNotFoundUserOrGroup, input.reviewer));
+ if (!projectConfig.getEnableReviewerByEmail()) {
+ throw new UnprocessableEntityException(
+ MessageFormat.format(
+ ChangeMessages.get().reviewerNotFoundUserOrGroup, input.reviewer));
+ }
}
}
- throw new UnprocessableEntityException(
- MessageFormat.format(ChangeMessages.get().reviewerNotFoundUser, input.reviewer));
+ if (!projectConfig.getEnableReviewerByEmail()) {
+ throw new UnprocessableEntityException(
+ MessageFormat.format(ChangeMessages.get().reviewerNotFoundUser, input.reviewer));
+ }
+ return putAccountByEmail(
+ input.reviewer,
+ rsrc,
+ input.state(),
+ input.notify,
+ notifyUtil.resolveAccounts(input.notifyDetails));
}
return putAccount(
input.reviewer,
@@ -199,6 +225,7 @@
user.getUserName(),
revision.getChangeResource(),
ImmutableMap.of(user.getAccountId(), revision.getControl()),
+ null,
CC,
NotifyHandling.NONE,
ImmutableListMultimap.of());
@@ -218,6 +245,7 @@
reviewer,
rsrc.getChangeResource(),
ImmutableMap.of(member.getId(), control),
+ null,
state,
notify,
accountsToNotify);
@@ -228,6 +256,32 @@
throw new UnprocessableEntityException(String.format("Account of %s is inactive.", reviewer));
}
+ private Addition putAccountByEmail(
+ String reviewer,
+ ChangeResource rsrc,
+ ReviewerState state,
+ NotifyHandling notify,
+ ListMultimap<RecipientType, Account.Id> accountsToNotify)
+ throws UnprocessableEntityException, OrmException, BadRequestException {
+ if (!rsrc.getControl().forUser(anonymousProvider.get()).isVisible(dbProvider.get())) {
+ throw new BadRequestException("change is not publicly visible");
+ }
+ if (!migration.readChanges()) {
+ throw new BadRequestException("feature only supported in NoteDb");
+ }
+ Address adr;
+ try {
+ adr = Address.parse(reviewer);
+ } catch (IllegalArgumentException e) {
+ throw new UnprocessableEntityException(String.format("email invalid %s", reviewer));
+ }
+ if (!OutgoingEmailValidator.isValid(adr.getEmail())) {
+ throw new UnprocessableEntityException(String.format("email invalid %s", reviewer));
+ }
+ return new Addition(
+ reviewer, rsrc, null, ImmutableList.of(adr), state, notify, accountsToNotify);
+ }
+
private Addition putGroup(ChangeResource rsrc, AddReviewerInput input)
throws RestApiException, OrmException, IOException {
GroupDescription.Basic group = groupsCollection.parseInternal(input.reviewer);
@@ -283,6 +337,7 @@
input.reviewer,
rsrc,
reviewers,
+ null,
input.state(),
input.notify,
notifyUtil.resolveAccounts(input.notifyDetails));
@@ -314,26 +369,28 @@
final Op op;
private final Map<Account.Id, ChangeControl> reviewers;
+ private final Collection<Address> reviewersByEmail;
protected Addition(String reviewer) {
- this(reviewer, null, null, REVIEWER, null, ImmutableListMultimap.of());
+ this(reviewer, null, null, null, REVIEWER, null, ImmutableListMultimap.of());
}
protected Addition(
String reviewer,
ChangeResource rsrc,
- Map<Account.Id, ChangeControl> reviewers,
+ @Nullable Map<Account.Id, ChangeControl> reviewers,
+ @Nullable Collection<Address> reviewersByEmail,
ReviewerState state,
NotifyHandling notify,
ListMultimap<RecipientType, Account.Id> accountsToNotify) {
result = new AddReviewerResult(reviewer);
- if (reviewers == null) {
- this.reviewers = ImmutableMap.of();
+ this.reviewers = reviewers == null ? ImmutableMap.of() : reviewers;
+ this.reviewersByEmail = reviewersByEmail == null ? ImmutableList.of() : reviewersByEmail;
+ if (reviewers == null && reviewersByEmail == null) {
op = null;
return;
}
- this.reviewers = reviewers;
- op = new Op(rsrc, reviewers, state, notify, accountsToNotify);
+ op = new Op(rsrc, this.reviewers, this.reviewersByEmail, state, notify, accountsToNotify);
}
void gatherResults() throws OrmException {
@@ -345,6 +402,9 @@
result.ccs.add(json.format(new ReviewerInfo(accountId.get()), reviewers.get(accountId)));
}
accountLoaderFactory.create(true).fill(result.ccs);
+ for (Address a : reviewersByEmail) {
+ result.ccs.add(new AccountInfo(a.getName(), a.getEmail()));
+ }
} else {
result.reviewers = Lists.newArrayListWithCapacity(op.addedReviewers.size());
for (PatchSetApproval psa : op.addedReviewers) {
@@ -356,17 +416,22 @@
ImmutableList.of(psa)));
}
accountLoaderFactory.create(true).fill(result.reviewers);
+ for (Address a : reviewersByEmail) {
+ result.reviewers.add(ReviewerInfo.byEmail(a.getName(), a.getEmail()));
+ }
}
}
}
public class Op implements BatchUpdateOp {
final Map<Account.Id, ChangeControl> reviewers;
+ final Collection<Address> reviewersByEmail;
final ReviewerState state;
final NotifyHandling notify;
final ListMultimap<RecipientType, Account.Id> accountsToNotify;
- List<PatchSetApproval> addedReviewers;
- Collection<Account.Id> addedCCs;
+ List<PatchSetApproval> addedReviewers = new ArrayList<>();
+ Collection<Account.Id> addedCCs = new ArrayList<>();
+ Collection<Address> addedCCsByEmail = new ArrayList<>();
private final ChangeResource rsrc;
private PatchSet patchSet;
@@ -374,11 +439,13 @@
Op(
ChangeResource rsrc,
Map<Account.Id, ChangeControl> reviewers,
+ Collection<Address> reviewersByEmail,
ReviewerState state,
NotifyHandling notify,
ListMultimap<RecipientType, Account.Id> accountsToNotify) {
this.rsrc = rsrc;
this.reviewers = reviewers;
+ this.reviewersByEmail = reviewersByEmail;
this.state = state;
this.notify = notify;
this.accountsToNotify = checkNotNull(accountsToNotify);
@@ -387,27 +454,34 @@
@Override
public boolean updateChange(ChangeContext ctx)
throws RestApiException, OrmException, IOException {
- if (migration.readChanges() && state == CC) {
- addedCCs =
- approvalsUtil.addCcs(
- ctx.getNotes(),
- ctx.getUpdate(ctx.getChange().currentPatchSetId()),
- reviewers.keySet());
- if (addedCCs.isEmpty()) {
- return false;
+ if (!reviewers.isEmpty()) {
+ if (migration.readChanges() && state == CC) {
+ addedCCs =
+ approvalsUtil.addCcs(
+ ctx.getNotes(),
+ ctx.getUpdate(ctx.getChange().currentPatchSetId()),
+ reviewers.keySet());
+ if (addedCCs.isEmpty()) {
+ return false;
+ }
+ } else {
+ addedReviewers =
+ approvalsUtil.addReviewers(
+ ctx.getDb(),
+ ctx.getNotes(),
+ ctx.getUpdate(ctx.getChange().currentPatchSetId()),
+ rsrc.getControl().getLabelTypes(),
+ rsrc.getChange(),
+ reviewers.keySet());
+ if (addedReviewers.isEmpty()) {
+ return false;
+ }
}
- } else {
- addedReviewers =
- approvalsUtil.addReviewers(
- ctx.getDb(),
- ctx.getNotes(),
- ctx.getUpdate(ctx.getChange().currentPatchSetId()),
- rsrc.getControl().getLabelTypes(),
- rsrc.getChange(),
- reviewers.keySet());
- if (addedReviewers.isEmpty()) {
- return false;
- }
+ }
+
+ for (Address a : reviewersByEmail) {
+ ctx.getUpdate(ctx.getChange().currentPatchSetId())
+ .putReviewerByEmail(a, ReviewerStateInternal.fromReviewerState(state));
}
patchSet = psUtil.current(dbProvider.get(), rsrc.getNotes());
@@ -416,26 +490,19 @@
@Override
public void postUpdate(Context ctx) throws Exception {
- if (addedReviewers != null || addedCCs != null) {
- if (addedReviewers == null) {
- addedReviewers = new ArrayList<>();
- }
- if (addedCCs == null) {
- addedCCs = new ArrayList<>();
- }
- emailReviewers(
- rsrc.getChange(),
- Lists.transform(addedReviewers, r -> r.getAccountId()),
- addedCCs,
- notify,
- accountsToNotify);
- if (!addedReviewers.isEmpty()) {
- List<Account> reviewers =
- Lists.transform(
- addedReviewers, psa -> accountCache.get(psa.getAccountId()).getAccount());
- reviewerAdded.fire(
- rsrc.getChange(), patchSet, reviewers, ctx.getAccount(), ctx.getWhen());
- }
+ emailReviewers(
+ rsrc.getChange(),
+ Lists.transform(addedReviewers, r -> r.getAccountId()),
+ addedCCs == null ? ImmutableList.of() : addedCCs,
+ reviewersByEmail,
+ addedCCsByEmail,
+ notify,
+ accountsToNotify);
+ if (!addedReviewers.isEmpty()) {
+ List<Account> reviewers =
+ Lists.transform(
+ addedReviewers, psa -> accountCache.get(psa.getAccountId()).getAccount());
+ reviewerAdded.fire(rsrc.getChange(), patchSet, reviewers, ctx.getAccount(), ctx.getWhen());
}
}
}
@@ -444,9 +511,11 @@
Change change,
Collection<Account.Id> added,
Collection<Account.Id> copied,
+ Collection<Address> addedByEmail,
+ Collection<Address> copiedByEmail,
NotifyHandling notify,
ListMultimap<RecipientType, Account.Id> accountsToNotify) {
- if (added.isEmpty() && copied.isEmpty()) {
+ if (added.isEmpty() && copied.isEmpty() && addedByEmail.isEmpty() && copiedByEmail.isEmpty()) {
return;
}
@@ -466,7 +535,7 @@
toCopy.add(id);
}
}
- if (toMail.isEmpty() && toCopy.isEmpty()) {
+ if (toMail.isEmpty() && toCopy.isEmpty() && addedByEmail.isEmpty() && copiedByEmail.isEmpty()) {
return;
}
@@ -478,7 +547,9 @@
cm.setAccountsToNotify(accountsToNotify);
cm.setFrom(userId);
cm.addReviewers(toMail);
+ cm.addReviewersByEmail(addedByEmail);
cm.addExtraCC(toCopy);
+ cm.addExtraCCByEmail(copiedByEmail);
cm.send();
} catch (Exception err) {
log.error("Cannot send email to new reviewers of change " + change.getId(), err);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerResource.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerResource.java
index 6ff4a50..f6f7919 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerResource.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerResource.java
@@ -14,11 +14,15 @@
package com.google.gerrit.server.change;
+import static com.google.common.base.Preconditions.checkArgument;
+
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.restapi.RestResource;
import com.google.gerrit.extensions.restapi.RestView;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.mail.Address;
import com.google.gerrit.server.project.ChangeControl;
import com.google.inject.TypeLiteral;
import com.google.inject.assistedinject.Assisted;
@@ -36,7 +40,8 @@
private final ChangeResource change;
private final RevisionResource revision;
- private final IdentifiedUser user;
+ @Nullable private final IdentifiedUser user;
+ @Nullable private final Address address;
@AssistedInject
ReviewerResource(
@@ -44,8 +49,9 @@
@Assisted ChangeResource change,
@Assisted Account.Id id) {
this.change = change;
- this.revision = null;
this.user = userFactory.create(id);
+ this.revision = null;
+ this.address = null;
}
@AssistedInject
@@ -56,6 +62,21 @@
this.revision = revision;
this.change = revision.getChangeResource();
this.user = userFactory.create(id);
+ this.address = null;
+ }
+
+ ReviewerResource(ChangeResource change, Address address) {
+ this.change = change;
+ this.address = address;
+ this.revision = null;
+ this.user = null;
+ }
+
+ ReviewerResource(RevisionResource revision, Address address) {
+ this.revision = revision;
+ this.change = revision.getChangeResource();
+ this.address = address;
+ this.user = null;
}
public ChangeResource getChangeResource() {
@@ -75,10 +96,28 @@
}
public IdentifiedUser getReviewerUser() {
+ checkArgument(user != null, "no user provided");
return user;
}
+ public Address getReviewerByEmail() {
+ checkArgument(address != null, "no address provided");
+ return address;
+ }
+
/**
+ * Check if this resource was constructed by email or by {@code Account.Id}.
+ *
+ * @return true if the resource was constructed by providing an {@code Address}; false if the
+ * resource was constructed by providing an {@code Account.Id}.
+ */
+ public boolean isByEmail() {
+ return user == null;
+ }
+
+ /**
+ * Get the control for the caller's user.
+ *
* @return the control for the caller's user (as opposed to the reviewer's user as returned by
* {@link #getReviewerControl()}).
*/
@@ -87,10 +126,13 @@
}
/**
+ * Get the control for the reviewer's user.
+ *
* @return the control for the reviewer's user (as opposed to the caller's user as returned by
* {@link #getControl()}).
*/
public ChangeControl getReviewerControl() {
+ checkArgument(user != null, "no user provided");
return change.getControl().forUser(user);
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Reviewers.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Reviewers.java
index 14c74bc..0762f0e 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Reviewers.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Reviewers.java
@@ -25,6 +25,7 @@
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.account.AccountsCollection;
+import com.google.gerrit.server.mail.Address;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -69,12 +70,26 @@
@Override
public ReviewerResource parse(ChangeResource rsrc, IdString id)
throws OrmException, ResourceNotFoundException, AuthException {
- Account.Id accountId = accounts.parse(TopLevelResource.INSTANCE, id).getUser().getAccountId();
+ Address address = Address.tryParse(id.get());
+ Account.Id accountId = null;
+ try {
+ accountId = accounts.parse(TopLevelResource.INSTANCE, id).getUser().getAccountId();
+ } catch (ResourceNotFoundException e) {
+ if (address == null) {
+ throw e;
+ }
+ }
// See if the id exists as a reviewer for this change
- if (fetchAccountIds(rsrc).contains(accountId)) {
+ if (accountId != null && fetchAccountIds(rsrc).contains(accountId)) {
return resourceFactory.create(rsrc, accountId);
}
+
+ // See if the address exists as a reviewer on the change
+ if (address != null && rsrc.getNotes().getReviewersByEmail().all().contains(address)) {
+ return new ReviewerResource(rsrc, address);
+ }
+
throw new ResourceNotFoundException(id);
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/RevisionReviewers.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/RevisionReviewers.java
index d3623cf..2dc7ad8 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/RevisionReviewers.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/RevisionReviewers.java
@@ -26,6 +26,7 @@
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.account.AccountsCollection;
+import com.google.gerrit.server.mail.Address;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -73,14 +74,28 @@
if (!rsrc.isCurrent()) {
throw new MethodNotAllowedException("Cannot access on non-current patch set");
}
+ Address address = Address.tryParse(id.get());
- Account.Id accountId = accounts.parse(TopLevelResource.INSTANCE, id).getUser().getAccountId();
-
+ Account.Id accountId = null;
+ try {
+ accountId = accounts.parse(TopLevelResource.INSTANCE, id).getUser().getAccountId();
+ } catch (ResourceNotFoundException e) {
+ if (address == null) {
+ throw e;
+ }
+ }
Collection<Account.Id> reviewers =
approvalsUtil.getReviewers(dbProvider.get(), rsrc.getNotes()).all();
+ // See if the id exists as a reviewer for this change
if (reviewers.contains(accountId)) {
return resourceFactory.create(rsrc, accountId);
}
+
+ // See if the address exists as a reviewer on the change
+ if (address != null && rsrc.getNotes().getReviewersByEmail().all().contains(address)) {
+ return new ReviewerResource(rsrc, address);
+ }
+
throw new ResourceNotFoundException(id);
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/Address.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/Address.java
index f3b08fb..7f3ac01 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/Address.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/Address.java
@@ -42,6 +42,14 @@
throw new IllegalArgumentException("Invalid email address: " + in);
}
+ public static Address tryParse(String in) {
+ try {
+ return parse(in);
+ } catch (IllegalArgumentException e) {
+ return null;
+ }
+ }
+
final String name;
final String email;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ChangeEmail.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ChangeEmail.java
index bc09488..3c76319 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ChangeEmail.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ChangeEmail.java
@@ -180,6 +180,20 @@
setHeader("X-Gerrit-Change-Number", "" + change.getChangeId());
setChangeUrlHeader();
setCommitIdHeader();
+
+ if (notify.ordinal() >= NotifyHandling.OWNER_REVIEWERS.ordinal()) {
+ try {
+ // TODO(hiesel) Load from index instead
+ addByEmail(
+ RecipientType.CC,
+ changeData.notes().getReviewersByEmail().byState(ReviewerStateInternal.CC));
+ addByEmail(
+ RecipientType.TO,
+ changeData.notes().getReviewersByEmail().byState(ReviewerStateInternal.REVIEWER));
+ } catch (OrmException e) {
+ throw new EmailException("Failed to add unregistered CCs " + change.getChangeId(), e);
+ }
+ }
}
private void setChangeUrlHeader() {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/DeleteReviewerSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/DeleteReviewerSender.java
index a563846..0fea7ce 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/DeleteReviewerSender.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/DeleteReviewerSender.java
@@ -20,6 +20,7 @@
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.account.WatchConfig.NotifyType;
+import com.google.gerrit.server.mail.Address;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
@@ -32,6 +33,7 @@
/** Let users know that a reviewer and possibly her review have been removed. */
public class DeleteReviewerSender extends ReplyToChangeSender {
private final Set<Account.Id> reviewers = new HashSet<>();
+ private final Set<Address> reviewersByEmail = new HashSet<>();
public interface Factory extends ReplyToChangeSender.Factory<DeleteReviewerSender> {
@Override
@@ -49,6 +51,10 @@
reviewers.addAll(cc);
}
+ public void addReviewersByEmail(Collection<Address> cc) {
+ reviewersByEmail.addAll(cc);
+ }
+
@Override
protected void init() throws EmailException {
super.init();
@@ -58,6 +64,7 @@
ccExistingReviewers();
includeWatchers(NotifyType.ALL_COMMENTS);
add(RecipientType.TO, reviewers);
+ addByEmail(RecipientType.TO, reviewersByEmail);
removeUsersThatIgnoredTheChange();
}
@@ -70,13 +77,16 @@
}
public List<String> getReviewerNames() {
- if (reviewers.isEmpty()) {
+ if (reviewers.isEmpty() && reviewersByEmail.isEmpty()) {
return null;
}
List<String> names = new ArrayList<>();
for (Account.Id id : reviewers) {
names.add(getNameFor(id));
}
+ for (Address a : reviewersByEmail) {
+ names.add(a.toString());
+ }
return names;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/NewChangeSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/NewChangeSender.java
index f1a9ad8..3f6d991 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/NewChangeSender.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/NewChangeSender.java
@@ -17,6 +17,7 @@
import com.google.gerrit.common.errors.EmailException;
import com.google.gerrit.extensions.api.changes.RecipientType;
import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.server.mail.Address;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.OrmException;
import java.util.ArrayList;
@@ -28,7 +29,9 @@
/** Sends an email alerting a user to a new change for them to review. */
public abstract class NewChangeSender extends ChangeEmail {
private final Set<Account.Id> reviewers = new HashSet<>();
+ private final Set<Address> reviewersByEmail = new HashSet<>();
private final Set<Account.Id> extraCC = new HashSet<>();
+ private final Set<Address> extraCCByEmail = new HashSet<>();
protected NewChangeSender(EmailArguments ea, ChangeData cd) throws OrmException {
super(ea, "newchange", cd);
@@ -38,10 +41,18 @@
reviewers.addAll(cc);
}
+ public void addReviewersByEmail(final Collection<Address> cc) {
+ reviewersByEmail.addAll(cc);
+ }
+
public void addExtraCC(final Collection<Account.Id> cc) {
extraCC.addAll(cc);
}
+ public void addExtraCCByEmail(final Collection<Address> cc) {
+ extraCCByEmail.addAll(cc);
+ }
+
@Override
protected void init() throws EmailException {
super.init();
@@ -55,9 +66,11 @@
case ALL:
default:
add(RecipientType.CC, extraCC);
+ extraCCByEmail.stream().forEach(cc -> add(RecipientType.CC, cc));
//$FALL-THROUGH$
case OWNER_REVIEWERS:
add(RecipientType.TO, reviewers);
+ addByEmail(RecipientType.TO, reviewersByEmail);
break;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/OutgoingEmail.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/OutgoingEmail.java
index 730b710..6877879 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/OutgoingEmail.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/OutgoingEmail.java
@@ -441,6 +441,13 @@
}
}
+ /** Schedule this message for delivery to the listed address. */
+ protected void addByEmail(final RecipientType rt, final Collection<Address> list) {
+ for (final Address id : list) {
+ add(rt, id);
+ }
+ }
+
protected void add(final RecipientType rt, final UserIdentity who) {
if (who != null && who.getAccount() != null) {
add(rt, who.getAccount());
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ReviewerStateInternal.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ReviewerStateInternal.java
index 66c0c82..fad9832 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ReviewerStateInternal.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ReviewerStateInternal.java
@@ -29,13 +29,17 @@
/** The user was previously a reviewer on the change, but was removed. */
REMOVED(new FooterKey("Removed"), ReviewerState.REMOVED);
+ public static ReviewerStateInternal fromReviewerState(ReviewerState state) {
+ return ReviewerStateInternal.values()[state.ordinal()];
+ }
+
static {
boolean ok = true;
if (ReviewerStateInternal.values().length != ReviewerState.values().length) {
ok = false;
}
- for (ReviewerStateInternal s : ReviewerStateInternal.values()) {
- ok &= s.name().equals(s.state.name());
+ for (int i = 0; i < ReviewerStateInternal.values().length; i++) {
+ ok &= ReviewerState.values()[i].equals(ReviewerStateInternal.values()[i].state);
}
if (!ok) {
throw new IllegalStateException(
