diff --git a/Documentation/config-project-config.txt b/Documentation/config-project-config.txt index 34f39c8ca7..0cafc13ea7 100644 --- a/Documentation/config-project-config.txt +++ b/Documentation/config-project-config.txt @@ -291,6 +291,18 @@ Branches not listed in this section will not be included in the mergeability check. If the `branchOrder` section is not defined then the mergeability of a change into other branches will not be done. +[[reviewer-section]] +=== reviewer section + +Defines config options to adjust a project's reviewer workflow such as enabling +reviewers and CCs by email. + +[[reviewer.enableByEmail]]reviewer.enableByEmail:: ++ +A boolean indicating if reviewers and CCs that do not currently have a Gerrit +account can be added to a change by providing their email address. + +Defaults to `false`. [[file-groups]] == The file +groups+ diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index 7bea2b7e40..da5d546dcd 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt @@ -2805,6 +2805,41 @@ To confirm the addition of the reviewers, resend the request with the } ---- +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 " 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 " + } +---- + +.Response +---- + HTTP/1.1 200 OK + Content-Disposition: attachment + Content-Type: application/json; charset=UTF-8 + + )]}' + { + "input": "John Doe " + } +---- + [[delete-reviewer]] === Delete Reviewer -- @@ -6183,6 +6218,10 @@ In addition `ReviewerInfo` has the following fields: |`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 24b3724f8b..abdde612bb 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.group.SystemGroupBackend; 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 @@ public abstract class AbstractDaemonTest { } 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 0000000000..2911b62a5c --- /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 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 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 "); + } + + @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 does not identify a registered user or group"); + gApi.changes().id(r.getChangeId()).addReviewer("Foo Bar "); + } + + 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 af61481646..3a33de9a37 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 @@ public class ReviewerInfo extends AccountInfo { */ @Nullable public Map 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 class ReviewerInfo extends AccountInfo { public String toString() { return username; } + + private ReviewerInfo() {} } diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/projects/ConfigInfo.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/projects/ConfigInfo.java index cc91a4afe5..f6f9811370 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/projects/ConfigInfo.java +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/projects/ConfigInfo.java @@ -31,6 +31,7 @@ public class ConfigInfo { public InheritedBooleanInfo enableSignedPush; public InheritedBooleanInfo requireSignedPush; public InheritedBooleanInfo rejectImplicitMerges; + public InheritedBooleanInfo enableReviewerByEmail; public MaxObjectSizeLimitInfo maxObjectSizeLimit; public SubmitType submitType; public ProjectState state; 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 2fb32d7417..f20509ba6a 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 class AccountInfo { 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-gwtui/src/main/java/com/google/gerrit/client/admin/ProjectInfoScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/ProjectInfoScreen.java index 3645fb9249..783069660a 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/ProjectInfoScreen.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/ProjectInfoScreen.java @@ -86,6 +86,7 @@ public class ProjectInfoScreen extends ProjectScreen { private ListBox enableSignedPush; private ListBox requireSignedPush; private ListBox rejectImplicitMerges; + private ListBox enableReviewerByEmail; private NpTextBox maxObjectSizeLimit; private Label effectiveMaxObjectSizeLimit; private Map> pluginConfigWidgets; @@ -191,6 +192,7 @@ public class ProjectInfoScreen extends ProjectScreen { requireChangeID.setEnabled(isOwner); rejectImplicitMerges.setEnabled(isOwner); maxObjectSizeLimit.setEnabled(isOwner); + enableReviewerByEmail.setEnabled(isOwner); if (pluginConfigWidgets != null) { for (Map widgetMap : pluginConfigWidgets.values()) { @@ -264,6 +266,10 @@ public class ProjectInfoScreen extends ProjectScreen { saveEnabler.listenTo(rejectImplicitMerges); grid.addHtml(AdminConstants.I.rejectImplicitMerges(), rejectImplicitMerges); + enableReviewerByEmail = newInheritedBooleanBox(); + saveEnabler.listenTo(enableReviewerByEmail); + grid.addHtml(AdminConstants.I.rejectImplicitMerges(), enableReviewerByEmail); + maxObjectSizeLimit = new NpTextBox(); saveEnabler.listenTo(maxObjectSizeLimit); effectiveMaxObjectSizeLimit = new Label(); @@ -395,6 +401,7 @@ public class ProjectInfoScreen extends ProjectScreen { setBool(requireSignedPush, result.requireSignedPush()); } setBool(rejectImplicitMerges, result.rejectImplicitMerges()); + setBool(enableReviewerByEmail, result.enableReviewerByEmail()); setSubmitType(result.submitType()); setState(result.state()); maxObjectSizeLimit.setText(result.maxObjectSizeLimit().configuredValue()); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/projects/ConfigInfo.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/projects/ConfigInfo.java index 738319d410..4eda46be30 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/projects/ConfigInfo.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/projects/ConfigInfo.java @@ -57,6 +57,9 @@ public class ConfigInfo extends JavaScriptObject { public final native InheritedBooleanInfo rejectImplicitMerges() /*-{ return this.reject_implicit_merges; }-*/ ; + public final native InheritedBooleanInfo enableReviewerByEmail() + /*-{ return this.enable_reviewer_by_email; }-*/ ; + public final SubmitType submitType() { return SubmitType.valueOf(submitTypeRaw()); } diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Project.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Project.java index ba83c5848b..0706032777 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Project.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Project.java @@ -99,6 +99,8 @@ public final class Project { protected InheritableBoolean rejectImplicitMerges; + protected InheritableBoolean enableReviewerByEmail; + protected Project() {} public Project(Project.NameKey nameKey) { @@ -112,6 +114,7 @@ public final class Project { createNewChangeForAllNotInTarget = InheritableBoolean.INHERIT; enableSignedPush = InheritableBoolean.INHERIT; requireSignedPush = InheritableBoolean.INHERIT; + enableReviewerByEmail = InheritableBoolean.INHERIT; } public Project.NameKey getNameKey() { @@ -154,6 +157,10 @@ public final class Project { return rejectImplicitMerges; } + public InheritableBoolean getEnableReviewerByEmail() { + return enableReviewerByEmail; + } + public void setUseContributorAgreements(final InheritableBoolean u) { useContributorAgreements = u; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ReviewerByEmailSet.java b/gerrit-server/src/main/java/com/google/gerrit/server/ReviewerByEmailSet.java new file mode 100644 index 0000000000..c16c9c8f60 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/ReviewerByEmailSet.java @@ -0,0 +1,79 @@ +// 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; + +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.ImmutableTable; +import com.google.common.collect.Table; +import com.google.gerrit.server.mail.Address; +import com.google.gerrit.server.notedb.ReviewerStateInternal; +import java.sql.Timestamp; + +/** + * Set of reviewers on a change that do not have a Gerrit account and were added by email instead. + * + *

A given account may appear in multiple states and at different timestamps. No reviewers with + * state {@link ReviewerStateInternal#REMOVED} are ever exposed by this interface. + */ +public class ReviewerByEmailSet { + private static final ReviewerByEmailSet EMPTY = new ReviewerByEmailSet(ImmutableTable.of()); + + public static ReviewerByEmailSet fromTable( + Table table) { + return new ReviewerByEmailSet(table); + } + + public static ReviewerByEmailSet empty() { + return EMPTY; + } + + private final ImmutableTable table; + private ImmutableSet

users; + + private ReviewerByEmailSet(Table table) { + this.table = ImmutableTable.copyOf(table); + } + + public ImmutableSet
all() { + if (users == null) { + // Idempotent and immutable, don't bother locking. + users = ImmutableSet.copyOf(table.columnKeySet()); + } + return users; + } + + public ImmutableSet
byState(ReviewerStateInternal state) { + return table.row(state).keySet(); + } + + public ImmutableTable asTable() { + return table; + } + + @Override + public boolean equals(Object o) { + return (o instanceof ReviewerByEmailSet) && table.equals(((ReviewerByEmailSet) o).table); + } + + @Override + public int hashCode() { + return table.hashCode(); + } + + @Override + public String toString() { + return getClass().getSimpleName() + table; + } +} 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 64aadde663..bfa2ee93f6 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.LabelNormalizer; 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 @@ public class ChangeJson { 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> 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 @@ public class ChangeJson { Collection 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 @@ public class ChangeJson { for (Account.Id id : removable) { result.add(accountLoader.get(id)); } + // Reviewers added by email are always removable + for (Collection infos : out.reviewers.values()) { + for (AccountInfo info : infos) { + if (info._accountId == null) { + result.add(info); + } + } + } return result; } @@ -1102,6 +1119,14 @@ public class ChangeJson { .collect(toList()); } + private Collection toAccountInfoByEmail(Collection
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 1485d03952..482247847a 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 { - private static final Logger log = LoggerFactory.getLogger(DeleteReviewer.class); private final Provider 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 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 dbProvider, - ApprovalsUtil approvalsUtil, - PatchSetUtil psUtil, - ChangeMessagesUtil cmUtil, BatchUpdate.Factory batchUpdateFactory, - IdentifiedUser.GenericFactory userFactory, - ReviewerDeleted reviewerDeleted, - Provider 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 @@ public class DeleteReviewer implements RestModifyView newApprovals = new HashMap<>(); - Map 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 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 approvals(ChangeContext ctx, Account.Id accountId) - throws OrmException { - Change.Id changeId = ctx.getNotes().getChangeId(); - Iterable 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 0000000000..adfe3f5a27 --- /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 0000000000..a255f794e2 --- /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 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 newApprovals = new HashMap<>(); + Map oldApprovals = new HashMap<>(); + + @Inject + DeleteReviewerOp( + ApprovalsUtil approvalsUtil, + PatchSetUtil psUtil, + ChangeMessagesUtil cmUtil, + IdentifiedUser.GenericFactory userFactory, + ReviewerDeleted reviewerDeleted, + Provider 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 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 approvals(ChangeContext ctx, Account.Id accountId) + throws OrmException { + Change.Id changeId = ctx.getNotes().getChangeId(); + Iterable 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 27ec89dc73..1dba58c4d1 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.extensions.restapi.RestReadView; 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 @@ class ListReviewers implements RestReadView { @Override public List apply(ChangeResource rsrc) throws OrmException { - Map reviewers = new LinkedHashMap<>(); + Map 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 d0c8ca0f25..5aaee56354 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.extensions.restapi.RestReadView; 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 @@ class ListRevisionReviewers implements RestReadView { throw new MethodNotAllowedException("Cannot list reviewers on non-current patch set"); } - Map reviewers = new LinkedHashMap<>(); + Map 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 875286a836..f23ab0c04a 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 @@ public class Module extends RestApiModule { 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 76cc7e8855..bd7939d57b 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.PatchSetUtil; 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 @@ public class PostReview implements RestModifyView ListMultimap accountsToNotify) { List to = new ArrayList<>(); List cc = new ArrayList<>(); + List
toByEmail = new ArrayList<>(); + List
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 7031d51dfb..9df0bd72e3 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.ImmutableListMultimap; 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.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; 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.Change; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.AnonymousUser; 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.AccountsCollection; 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 @@ public class PostReviewers implements RestModifyView anonymousProvider; @Inject PostReviewers( @@ -124,7 +134,9 @@ public class PostReviewers implements RestModifyView anonymousProvider) { this.accounts = accounts; this.reviewerFactory = reviewerFactory; this.approvalsUtil = approvalsUtil; @@ -143,6 +155,8 @@ public class PostReviewers implements RestModifyView 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 @@ public class PostReviewers implements RestModifyView reviewers; + private final Collection
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 reviewers, + @Nullable Map reviewers, + @Nullable Collection
reviewersByEmail, ReviewerState state, NotifyHandling notify, ListMultimap 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 @@ public class PostReviewers implements RestModifyView reviewers; + final Collection
reviewersByEmail; final ReviewerState state; final NotifyHandling notify; final ListMultimap accountsToNotify; - List addedReviewers; - Collection addedCCs; + List addedReviewers = new ArrayList<>(); + Collection addedCCs = new ArrayList<>(); + Collection
addedCCsByEmail = new ArrayList<>(); private final ChangeResource rsrc; private PatchSet patchSet; @@ -374,11 +439,13 @@ public class PostReviewers implements RestModifyView reviewers, + Collection
reviewersByEmail, ReviewerState state, NotifyHandling notify, ListMultimap accountsToNotify) { this.rsrc = rsrc; this.reviewers = reviewers; + this.reviewersByEmail = reviewersByEmail; this.state = state; this.notify = notify; this.accountsToNotify = checkNotNull(accountsToNotify); @@ -387,55 +454,55 @@ public class PostReviewers implements RestModifyView(); - } - if (addedCCs == null) { - addedCCs = new ArrayList<>(); - } - emailReviewers( - rsrc.getChange(), - Lists.transform(addedReviewers, r -> r.getAccountId()), - addedCCs, - notify, - accountsToNotify); - if (!addedReviewers.isEmpty()) { - List 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 reviewers = + Lists.transform( + addedReviewers, psa -> accountCache.get(psa.getAccountId()).getAccount()); + reviewerAdded.fire(rsrc.getChange(), patchSet, reviewers, ctx.getAccount(), ctx.getWhen()); } } } @@ -444,9 +511,11 @@ public class PostReviewers implements RestModifyView added, Collection copied, + Collection
addedByEmail, + Collection
copiedByEmail, NotifyHandling notify, ListMultimap accountsToNotify) { - if (added.isEmpty() && copied.isEmpty()) { + if (added.isEmpty() && copied.isEmpty() && addedByEmail.isEmpty() && copiedByEmail.isEmpty()) { return; } @@ -466,7 +535,7 @@ public class PostReviewers implements RestModifyView 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/git/ProjectConfig.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java index 61d8cfecf8..e3d45ac734 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java @@ -155,6 +155,9 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. ImmutableSet.of( "MaxWithBlock", "AnyWithBlock", "MaxNoBlock", "NoBlock", "NoOp", "PatchSetLock"); + private static final String REVIEWER = "reviewer"; + private static final String KEY_ENABLE_REVIEWER_BY_EMAIL = "enableByEmail"; + private static final String LEGACY_PERMISSION_PUSH_TAG = "pushTag"; private static final String LEGACY_PERMISSION_PUSH_SIGNED_TAG = "pushSignedTag"; @@ -182,6 +185,7 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. private boolean checkReceivedObjects; private Set sectionsWithUnknownPermissions; private boolean hasLegacyPermissions; + private boolean enableReviewerByEmail; public static ProjectConfig read(MetaDataUpdate update) throws IOException, ConfigInvalidException { @@ -435,6 +439,16 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. return checkReceivedObjects; } + /** @return the enableReviewerByEmail for this project, default is false. */ + public boolean getEnableReviewerByEmail() { + return enableReviewerByEmail; + } + + /** Set enableReviewerByEmail for this project, default is false. */ + public void setEnableReviewerByEmail(boolean val) { + enableReviewerByEmail = val; + } + /** * Check all GroupReferences use current group name, repairing stale ones. * @@ -526,6 +540,7 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. mimeTypes = new ConfiguredMimeTypes(projectName.get(), rc); loadPluginSections(rc); loadReceiveSection(rc); + loadReviewerSection(rc); } private void loadAccountsSection(Config rc, Map groupsByName) { @@ -933,6 +948,10 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. maxObjectSizeLimit = rc.getLong(RECEIVE, null, KEY_MAX_OBJECT_SIZE_LIMIT, 0); } + private void loadReviewerSection(Config rc) { + enableReviewerByEmail = rc.getBoolean(REVIEWER, null, KEY_ENABLE_REVIEWER_BY_EMAIL, false); + } + private void loadPluginSections(Config rc) { pluginConfigs = new HashMap<>(); for (String plugin : rc.getSubsections(PLUGIN)) { @@ -1067,6 +1086,7 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. saveAccessSections(rc, keepGroups); saveNotifySections(rc, keepGroups); savePluginSections(rc, keepGroups); + saveReviewerSection(rc); groupList.retainUUIDs(keepGroups); saveLabelSections(rc); saveSubscribeSections(rc); @@ -1288,40 +1308,55 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. setBooleanConfigKey( rc, + LABEL, name, KEY_ALLOW_POST_SUBMIT, label.allowPostSubmit(), LabelType.DEF_ALLOW_POST_SUBMIT); setBooleanConfigKey( - rc, name, KEY_COPY_MIN_SCORE, label.isCopyMinScore(), LabelType.DEF_COPY_MIN_SCORE); - setBooleanConfigKey( - rc, name, KEY_COPY_MAX_SCORE, label.isCopyMaxScore(), LabelType.DEF_COPY_MAX_SCORE); + rc, + LABEL, + name, + KEY_COPY_MIN_SCORE, + label.isCopyMinScore(), + LabelType.DEF_COPY_MIN_SCORE); setBooleanConfigKey( rc, + LABEL, + name, + KEY_COPY_MAX_SCORE, + label.isCopyMaxScore(), + LabelType.DEF_COPY_MAX_SCORE); + setBooleanConfigKey( + rc, + LABEL, name, KEY_COPY_ALL_SCORES_ON_TRIVIAL_REBASE, label.isCopyAllScoresOnTrivialRebase(), LabelType.DEF_COPY_ALL_SCORES_ON_TRIVIAL_REBASE); setBooleanConfigKey( rc, + LABEL, name, KEY_COPY_ALL_SCORES_IF_NO_CODE_CHANGE, label.isCopyAllScoresIfNoCodeChange(), LabelType.DEF_COPY_ALL_SCORES_IF_NO_CODE_CHANGE); setBooleanConfigKey( rc, + LABEL, name, KEY_COPY_ALL_SCORES_IF_NO_CHANGE, label.isCopyAllScoresIfNoChange(), LabelType.DEF_COPY_ALL_SCORES_IF_NO_CHANGE); setBooleanConfigKey( rc, + LABEL, name, KEY_COPY_ALL_SCORES_ON_MERGE_FIRST_PARENT_UPDATE, label.isCopyAllScoresOnMergeFirstParentUpdate(), LabelType.DEF_COPY_ALL_SCORES_ON_MERGE_FIRST_PARENT_UPDATE); setBooleanConfigKey( - rc, name, KEY_CAN_OVERRIDE, label.canOverride(), LabelType.DEF_CAN_OVERRIDE); + rc, LABEL, name, KEY_CAN_OVERRIDE, label.canOverride(), LabelType.DEF_CAN_OVERRIDE); List values = Lists.newArrayListWithCapacity(label.getValues().size()); for (LabelValue value : label.getValues()) { values.add(value.format()); @@ -1335,11 +1370,11 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. } private static void setBooleanConfigKey( - Config rc, String name, String key, boolean value, boolean defaultValue) { + Config rc, String section, String name, String key, boolean value, boolean defaultValue) { if (value == defaultValue) { - rc.unset(LABEL, name, key); + rc.unset(section, name, key); } else { - rc.setBoolean(LABEL, name, key, value); + rc.setBoolean(section, name, key, value); } } @@ -1367,6 +1402,11 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. } } + private void saveReviewerSection(Config rc) { + setBooleanConfigKey( + rc, REVIEWER, null, KEY_ENABLE_REVIEWER_BY_EMAIL, enableReviewerByEmail, false); + } + private void saveGroupList() throws IOException { saveUTF8(GroupList.FILE_NAME, groupList.asText()); } 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 f3b08fbd06..7f3ac01de1 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 @@ public class Address { 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 bc09488e1d..3c76319ef4 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 @@ public abstract class ChangeEmail extends NotificationEmail { 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 a563846e94..0fea7cec14 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.Account; 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 @@ import java.util.Set; /** Let users know that a reviewer and possibly her review have been removed. */ public class DeleteReviewerSender extends ReplyToChangeSender { private final Set reviewers = new HashSet<>(); + private final Set
reviewersByEmail = new HashSet<>(); public interface Factory extends ReplyToChangeSender.Factory { @Override @@ -49,6 +51,10 @@ public class DeleteReviewerSender extends ReplyToChangeSender { reviewers.addAll(cc); } + public void addReviewersByEmail(Collection
cc) { + reviewersByEmail.addAll(cc); + } + @Override protected void init() throws EmailException { super.init(); @@ -58,6 +64,7 @@ public class DeleteReviewerSender extends ReplyToChangeSender { ccExistingReviewers(); includeWatchers(NotifyType.ALL_COMMENTS); add(RecipientType.TO, reviewers); + addByEmail(RecipientType.TO, reviewersByEmail); removeUsersThatIgnoredTheChange(); } @@ -70,13 +77,16 @@ public class DeleteReviewerSender extends ReplyToChangeSender { } public List getReviewerNames() { - if (reviewers.isEmpty()) { + if (reviewers.isEmpty() && reviewersByEmail.isEmpty()) { return null; } List 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 f1a9ad89d7..3f6d991d38 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 @@ package com.google.gerrit.server.mail.send; 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 @@ import java.util.Set; /** Sends an email alerting a user to a new change for them to review. */ public abstract class NewChangeSender extends ChangeEmail { private final Set reviewers = new HashSet<>(); + private final Set
reviewersByEmail = new HashSet<>(); private final Set extraCC = new HashSet<>(); + private final Set
extraCCByEmail = new HashSet<>(); protected NewChangeSender(EmailArguments ea, ChangeData cd) throws OrmException { super(ea, "newchange", cd); @@ -38,10 +41,18 @@ public abstract class NewChangeSender extends ChangeEmail { reviewers.addAll(cc); } + public void addReviewersByEmail(final Collection
cc) { + reviewersByEmail.addAll(cc); + } + public void addExtraCC(final Collection cc) { extraCC.addAll(cc); } + public void addExtraCCByEmail(final Collection
cc) { + extraCCByEmail.addAll(cc); + } + @Override protected void init() throws EmailException { super.init(); @@ -55,9 +66,11 @@ public abstract class NewChangeSender extends ChangeEmail { 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 730b71000d..6877879654 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 @@ public abstract class OutgoingEmail { } } + /** Schedule this message for delivery to the listed address. */ + protected void addByEmail(final RecipientType rt, final Collection
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/ChangeNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java index a839b91be4..d2889aefb1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java @@ -48,6 +48,7 @@ import com.google.gerrit.reviewdb.client.RevId; import com.google.gerrit.reviewdb.client.RobotComment; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.reviewdb.server.ReviewDbUtil; +import com.google.gerrit.server.ReviewerByEmailSet; import com.google.gerrit.server.ReviewerSet; import com.google.gerrit.server.ReviewerStatusUpdate; import com.google.gerrit.server.git.RefCache; @@ -428,6 +429,11 @@ public class ChangeNotes extends AbstractChangeNotes { return state.reviewers(); } + /** @return reviewers that do not currently have a Gerrit account and were added by email. */ + public ReviewerByEmailSet getReviewersByEmail() { + return state.reviewersByEmail(); + } + public ImmutableList getReviewerUpdates() { return state.reviewerUpdates(); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java index a2125f89af..d6cccd7d47 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java @@ -63,8 +63,10 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.reviewdb.client.RevId; import com.google.gerrit.reviewdb.server.ReviewDbUtil; +import com.google.gerrit.server.ReviewerByEmailSet; import com.google.gerrit.server.ReviewerSet; import com.google.gerrit.server.ReviewerStatusUpdate; +import com.google.gerrit.server.mail.Address; import com.google.gerrit.server.notedb.ChangeNotesCommit.ChangeNotesRevWalk; import com.google.gerrit.server.util.LabelVote; import java.io.IOException; @@ -128,6 +130,7 @@ class ChangeNotesParser { // Private final but mutable members initialized in the constructor and filled // in during the parsing process. private final Table reviewers; + private final Table reviewersByEmail; private final List allPastReviewers; private final List reviewerUpdates; private final List submitRecords; @@ -174,6 +177,7 @@ class ChangeNotesParser { approvals = new LinkedHashMap<>(); bufferedApprovals = new ArrayList<>(); reviewers = HashBasedTable.create(); + reviewersByEmail = HashBasedTable.create(); allPastReviewers = new ArrayList<>(); reviewerUpdates = new ArrayList<>(); submitRecords = Lists.newArrayListWithExpectedSize(1); @@ -201,6 +205,7 @@ class ChangeNotesParser { parseNotes(); allPastReviewers.addAll(reviewers.rowKeySet()); pruneReviewers(); + pruneReviewersByEmail(); updatePatchSetStates(); checkMandatoryFooters(); @@ -234,6 +239,7 @@ class ChangeNotesParser { patchSets, buildApprovals(), ReviewerSet.fromTable(Tables.transpose(reviewers)), + ReviewerByEmailSet.fromTable(Tables.transpose(reviewersByEmail)), allPastReviewers, buildReviewerUpdates(), submitRecords, @@ -374,6 +380,9 @@ class ChangeNotesParser { for (String line : commit.getFooterLineValues(state.getFooterKey())) { parseReviewer(ts, state, line); } + for (String line : commit.getFooterLineValues(state.getByEmailFooterKey())) { + parseReviewerByEmail(ts, state, line); + } // Don't update timestamp when a reviewer was added, matching RevewDb // behavior. } @@ -917,6 +926,19 @@ class ChangeNotesParser { } } + private void parseReviewerByEmail(Timestamp ts, ReviewerStateInternal state, String line) + throws ConfigInvalidException { + Address adr; + try { + adr = Address.parse(line); + } catch (IllegalArgumentException e) { + throw invalidFooter(state.getByEmailFooterKey(), line); + } + if (!reviewersByEmail.containsRow(adr)) { + reviewersByEmail.put(adr, state, ts); + } + } + private void parseReadOnlyUntil(ChangeNotesCommit commit) throws ConfigInvalidException { String raw = parseOneFooter(commit, FOOTER_READ_ONLY_UNTIL); if (raw == null) { @@ -956,6 +978,17 @@ class ChangeNotesParser { } } + private void pruneReviewersByEmail() { + Iterator> rit = + reviewersByEmail.cellSet().iterator(); + while (rit.hasNext()) { + Table.Cell e = rit.next(); + if (e.getColumnKey() == ReviewerStateInternal.REMOVED) { + rit.remove(); + } + } + } + private void updatePatchSetStates() { Set missing = new TreeSet<>(ReviewDbUtil.intKeyOrdering()); for (Iterator it = patchSets.values().iterator(); it.hasNext(); ) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesState.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesState.java index 809b492233..eee1a3414b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesState.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesState.java @@ -34,6 +34,7 @@ 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.RevId; +import com.google.gerrit.server.ReviewerByEmailSet; import com.google.gerrit.server.ReviewerSet; import com.google.gerrit.server.ReviewerStatusUpdate; import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage; @@ -65,6 +66,7 @@ public abstract class ChangeNotesState { ImmutableList.of(), ImmutableList.of(), ReviewerSet.empty(), + ReviewerByEmailSet.empty(), ImmutableList.of(), ImmutableList.of(), ImmutableList.of(), @@ -95,6 +97,7 @@ public abstract class ChangeNotesState { Map patchSets, ListMultimap approvals, ReviewerSet reviewers, + ReviewerByEmailSet reviewersByEmail, List allPastReviewers, List reviewerUpdates, List submitRecords, @@ -128,6 +131,7 @@ public abstract class ChangeNotesState { ImmutableList.copyOf(patchSets.entrySet()), ImmutableList.copyOf(approvals.entries()), reviewers, + reviewersByEmail, ImmutableList.copyOf(allPastReviewers), ImmutableList.copyOf(reviewerUpdates), ImmutableList.copyOf(submitRecords), @@ -204,6 +208,8 @@ public abstract class ChangeNotesState { abstract ReviewerSet reviewers(); + abstract ReviewerByEmailSet reviewersByEmail(); + abstract ImmutableList allPastReviewers(); abstract ImmutableList reviewerUpdates(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java index 863b571593..f699d761a9 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java @@ -61,6 +61,7 @@ import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.account.AccountCache; import com.google.gerrit.server.config.AnonymousCowardName; import com.google.gerrit.server.config.GerritServerConfig; +import com.google.gerrit.server.mail.Address; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.util.LabelVote; @@ -128,6 +129,7 @@ public class ChangeUpdate extends AbstractChangeUpdate { private final Table> approvals; private final Map reviewers = new LinkedHashMap<>(); + private final Map reviewersByEmail = new LinkedHashMap<>(); private final List comments = new ArrayList<>(); private String commitSubject; @@ -471,6 +473,15 @@ public class ChangeUpdate extends AbstractChangeUpdate { reviewers.put(reviewer, ReviewerStateInternal.REMOVED); } + public void putReviewerByEmail(Address reviewer, ReviewerStateInternal type) { + checkArgument(type != ReviewerStateInternal.REMOVED, "invalid ReviewerType"); + reviewersByEmail.put(reviewer, type); + } + + public void removeReviewerByEmail(Address reviewer) { + reviewersByEmail.put(reviewer, ReviewerStateInternal.REMOVED); + } + public void setPatchSetState(PatchSetState psState) { this.psState = psState; } @@ -660,6 +671,10 @@ public class ChangeUpdate extends AbstractChangeUpdate { addIdent(msg, e.getKey()).append('\n'); } + for (Map.Entry e : reviewersByEmail.entrySet()) { + addFooter(msg, e.getValue().getByEmailFooterKey(), e.getKey().toString()); + } + for (Table.Cell> c : approvals.cellSet()) { addFooter(msg, FOOTER_LABEL); // Label names/values are safe to append without sanitizing. @@ -749,6 +764,7 @@ public class ChangeUpdate extends AbstractChangeUpdate { && changeMessage == null && comments.isEmpty() && reviewers.isEmpty() + && reviewersByEmail.isEmpty() && changeId == null && branch == null && status == null 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 f25064691e..fad9832d90 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 @@ public enum ReviewerStateInternal { /** 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( @@ -58,6 +62,10 @@ public enum ReviewerStateInternal { return footerKey; } + FooterKey getByEmailFooterKey() { + return new FooterKey(footerKey.getName() + "-email"); + } + public ReviewerState asReviewerState() { return state; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ConfigInfoImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ConfigInfoImpl.java index 2f02728fbc..ce2141350d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ConfigInfoImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ConfigInfoImpl.java @@ -58,6 +58,7 @@ public class ConfigInfoImpl extends ConfigInfo { InheritedBooleanInfo enableSignedPush = new InheritedBooleanInfo(); InheritedBooleanInfo requireSignedPush = new InheritedBooleanInfo(); InheritedBooleanInfo rejectImplicitMerges = new InheritedBooleanInfo(); + InheritedBooleanInfo enableReviewerByEmail = new InheritedBooleanInfo(); useContributorAgreements.value = projectState.isUseContributorAgreements(); useSignedOffBy.value = projectState.isUseSignedOffBy(); @@ -73,6 +74,7 @@ public class ConfigInfoImpl extends ConfigInfo { enableSignedPush.configuredValue = p.getEnableSignedPush(); requireSignedPush.configuredValue = p.getRequireSignedPush(); rejectImplicitMerges.configuredValue = p.getRejectImplicitMerges(); + enableReviewerByEmail.configuredValue = p.getEnableReviewerByEmail(); ProjectState parentState = Iterables.getFirst(projectState.parents(), null); if (parentState != null) { @@ -85,6 +87,7 @@ public class ConfigInfoImpl extends ConfigInfo { enableSignedPush.inheritedValue = projectState.isEnableSignedPush(); requireSignedPush.inheritedValue = projectState.isRequireSignedPush(); rejectImplicitMerges.inheritedValue = projectState.isRejectImplicitMerges(); + enableReviewerByEmail.inheritedValue = projectState.isEnableReviewerByEmail(); } this.useContributorAgreements = useContributorAgreements; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java index 8b8745e19f..32dc41f4eb 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java @@ -394,6 +394,10 @@ public class ProjectState { return getInheritableBoolean(Project::getRejectImplicitMerges); } + public boolean isEnableReviewerByEmail() { + return getInheritableBoolean(Project::getEnableReviewerByEmail); + } + public LabelTypes getLabelTypes() { Map types = new LinkedHashMap<>(); for (ProjectState s : treeInOrder()) { diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/git/ProjectConfigTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/git/ProjectConfigTest.java index ab68c10a92..2b7b78d70d 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/git/ProjectConfigTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/git/ProjectConfigTest.java @@ -105,7 +105,9 @@ public class ProjectConfigTest extends LocalDiskRepositoryTestCase { + " accepted = group Developers\n" // + " accepted = group Staff\n" // + " autoVerify = group Developers\n" // - + " agreementUrl = http://www.example.com/agree\n")) // + + " agreementUrl = http://www.example.com/agree\n" // + + "[reviewer]\n" // + + " enableByEmail = true\n")) // )); ProjectConfig cfg = read(rev); @@ -132,6 +134,8 @@ public class ProjectConfigTest extends LocalDiskRepositoryTestCase { assertThat(submit.getExclusiveGroup()).isTrue(); assertThat(read.getExclusiveGroup()).isTrue(); assertThat(push.getExclusiveGroup()).isFalse(); + + assertThat(cfg.getEnableReviewerByEmail()).isTrue(); } @Test diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java index 1c65a37f89..5a1d10ca72 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java @@ -49,6 +49,7 @@ import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.ReviewerSet; import com.google.gerrit.server.config.GerritServerId; +import com.google.gerrit.server.mail.Address; import com.google.gerrit.server.notedb.ChangeNotesCommit.ChangeNotesRevWalk; import com.google.gerrit.server.util.RequestId; import com.google.gerrit.testutil.TestChanges; @@ -3298,6 +3299,105 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { assertThat(notes.isPrivate()).isFalse(); } + @Test + public void defaultReviewersByEmailIsEmpty() throws Exception { + Change c = newChange(); + ChangeNotes notes = newNotes(c); + assertThat(notes.getReviewersByEmail().all()).isEmpty(); + } + + @Test + public void putReviewerByEmail() throws Exception { + Address adr = new Address("Foo Bar", "foo.bar@gerritcodereview.com"); + + Change c = newChange(); + ChangeUpdate update = newUpdate(c, changeOwner); + update.putReviewerByEmail(adr, ReviewerStateInternal.REVIEWER); + update.commit(); + + ChangeNotes notes = newNotes(c); + assertThat(notes.getReviewersByEmail().all()).containsExactly(adr); + } + + @Test + public void putAndRemoveReviewerByEmail() throws Exception { + Address adr = new Address("Foo Bar", "foo.bar@gerritcodereview.com"); + + Change c = newChange(); + ChangeUpdate update = newUpdate(c, changeOwner); + update.putReviewerByEmail(adr, ReviewerStateInternal.REVIEWER); + update.commit(); + + update = newUpdate(c, changeOwner); + update.removeReviewerByEmail(adr); + update.commit(); + + ChangeNotes notes = newNotes(c); + assertThat(notes.getReviewersByEmail().all()).isEmpty(); + } + + @Test + public void putRemoveAndAddBackReviewerByEmail() throws Exception { + Address adr = new Address("Foo Bar", "foo.bar@gerritcodereview.com"); + + Change c = newChange(); + ChangeUpdate update = newUpdate(c, changeOwner); + update.putReviewerByEmail(adr, ReviewerStateInternal.REVIEWER); + update.commit(); + + update = newUpdate(c, changeOwner); + update.removeReviewerByEmail(adr); + update.commit(); + + update = newUpdate(c, changeOwner); + update.putReviewerByEmail(adr, ReviewerStateInternal.CC); + update.commit(); + + ChangeNotes notes = newNotes(c); + assertThat(notes.getReviewersByEmail().all()).containsExactly(adr); + } + + @Test + public void putReviewerByEmailAndCcByEmail() throws Exception { + Address adrReviewer = new Address("Foo Bar", "foo.bar@gerritcodereview.com"); + Address adrCc = new Address("Foo Bor", "foo.bar.2@gerritcodereview.com"); + + Change c = newChange(); + ChangeUpdate update = newUpdate(c, changeOwner); + update.putReviewerByEmail(adrReviewer, ReviewerStateInternal.REVIEWER); + update.commit(); + + update = newUpdate(c, changeOwner); + update.putReviewerByEmail(adrCc, ReviewerStateInternal.CC); + update.commit(); + + ChangeNotes notes = newNotes(c); + assertThat(notes.getReviewersByEmail().byState(ReviewerStateInternal.REVIEWER)) + .containsExactly(adrReviewer); + assertThat(notes.getReviewersByEmail().byState(ReviewerStateInternal.CC)) + .containsExactly(adrCc); + assertThat(notes.getReviewersByEmail().all()).containsExactly(adrReviewer, adrCc); + } + + @Test + public void putReviewerByEmailAndChangeToCc() throws Exception { + Address adr = new Address("Foo Bar", "foo.bar@gerritcodereview.com"); + + Change c = newChange(); + ChangeUpdate update = newUpdate(c, changeOwner); + update.putReviewerByEmail(adr, ReviewerStateInternal.REVIEWER); + update.commit(); + + update = newUpdate(c, changeOwner); + update.putReviewerByEmail(adr, ReviewerStateInternal.CC); + update.commit(); + + ChangeNotes notes = newNotes(c); + assertThat(notes.getReviewersByEmail().byState(ReviewerStateInternal.REVIEWER)).isEmpty(); + assertThat(notes.getReviewersByEmail().byState(ReviewerStateInternal.CC)).containsExactly(adr); + assertThat(notes.getReviewersByEmail().all()).containsExactly(adr); + } + private boolean testJson() { return noteUtil.getWriteJson(); } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/CommitMessageOutputTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/CommitMessageOutputTest.java index 25b516829b..83dcf61e72 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/CommitMessageOutputTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/CommitMessageOutputTest.java @@ -23,6 +23,7 @@ import com.google.gerrit.common.TimeUtil; 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.mail.Address; import com.google.gerrit.server.util.RequestId; import com.google.gerrit.testutil.ConfigSuite; import com.google.gerrit.testutil.TestChanges; @@ -382,6 +383,32 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest { assertBodyEquals("Update patch set 1\n\nPatch-set: 1\nCurrent: true\n", update.getResult()); } + @Test + public void reviewerByEmail() throws Exception { + Change c = newChange(); + ChangeUpdate update = newUpdate(c, changeOwner); + update.putReviewerByEmail( + new Address("John Doe", "j.doe@gerritcodereview.com"), ReviewerStateInternal.REVIEWER); + update.commit(); + + assertBodyEquals( + "Update patch set 1\n\nPatch-set: 1\n" + + "Reviewer-email: John Doe \n", + update.getResult()); + } + + @Test + public void ccByEmail() throws Exception { + Change c = newChange(); + ChangeUpdate update = newUpdate(c, changeOwner); + update.putReviewerByEmail(new Address("j.doe@gerritcodereview.com"), ReviewerStateInternal.CC); + update.commit(); + + assertBodyEquals( + "Update patch set 1\n\nPatch-set: 1\nCC-email: j.doe@gerritcodereview.com\n", + update.getResult()); + } + private RevCommit parseCommit(ObjectId id) throws Exception { if (id instanceof RevCommit) { return (RevCommit) id;