From 1d31699e037218fd0d931bac9377054ddba84768 Mon Sep 17 00:00:00 2001 From: Patrick Hiesel Date: Wed, 15 Mar 2017 08:46:18 +0100 Subject: [PATCH 1/3] Add reviewers by email to NoteDb This change adds code for NoteDb to store reviewers and CCs that do not currently have a Gerrit account. Once completed, this feature will send out emails to these reviewers and provide an additional way of notifying users who do not have a Gerrit account. The feature will be supported exclusively on NoteDb. In a follow-up change, we will add migration logic for reviewers by email to regular reviewers should the user decide to create a Gerrit account. This migration will happen on a per-change basis when the user interacts with the change. Bug: Issue 4134 Change-Id: I8b9f53f9ba0928f721e608c402257542775289f9 --- .../gerrit/server/ReviewerByEmailSet.java | 79 ++++++++++++++ .../gerrit/server/notedb/ChangeNotes.java | 6 ++ .../server/notedb/ChangeNotesParser.java | 33 ++++++ .../server/notedb/ChangeNotesState.java | 6 ++ .../gerrit/server/notedb/ChangeUpdate.java | 16 +++ .../server/notedb/ReviewerStateInternal.java | 4 + .../gerrit/server/notedb/ChangeNotesTest.java | 100 ++++++++++++++++++ .../notedb/CommitMessageOutputTest.java | 27 +++++ 8 files changed, 271 insertions(+) create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/ReviewerByEmailSet.java 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/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..66c0c82e27 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 @@ -58,6 +58,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/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; From 6db5afdf59f5bd725c31e9561ca56b36c7e4138b Mon Sep 17 00:00:00 2001 From: Patrick Hiesel Date: Tue, 21 Mar 2017 09:40:03 +0100 Subject: [PATCH 2/3] Add reviewer.enableByEmail to ProjectConfig This change adds a setting to enable reviewers and CCs by email to the ProjectConfig. It also adds tests, docs, API and UI support. Bug: Issue 4134 Change-Id: Ibe6ccb80f9f71f2a430afcd6e12b3f27cf8fdbd6 --- Documentation/config-project-config.txt | 12 +++++ .../extensions/api/projects/ConfigInfo.java | 1 + .../client/admin/ProjectInfoScreen.java | 7 +++ .../gerrit/client/projects/ConfigInfo.java | 3 ++ .../gerrit/reviewdb/client/Project.java | 7 +++ .../gerrit/server/git/ProjectConfig.java | 54 ++++++++++++++++--- .../gerrit/server/project/ConfigInfoImpl.java | 3 ++ .../gerrit/server/project/ProjectState.java | 4 ++ .../gerrit/server/git/ProjectConfigTest.java | 6 ++- 9 files changed, 89 insertions(+), 8 deletions(-) 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/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-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/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/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 From 11873ef74eb5a8fbce6023a37ad762b3784c4449 Mon Sep 17 00:00:00 2001 From: Patrick Hiesel Date: Fri, 17 Mar 2017 17:36:05 +0100 Subject: [PATCH 3/3] Add reviewers by email to REST API and Email sender This change adds support for adding reviewers by email in PostReviewer and ChangeInfo. It also adds support for deleting reviewers using the ReviewerResource by adding Address to it. It further adds support for sending emails to reviewers that were added by email. This support includes emails for adding and removing a reviewer as well as for general change notifications. This change also adds tests and updates the documentation. Bug: Issue 4134 Change-Id: I901d711956ee69c25a05b455e068ec109821d79d --- Documentation/rest-api-changes.txt | 39 +++ .../gerrit/acceptance/AbstractDaemonTest.java | 17 +- .../rest/change/ChangeReviewersByEmailIT.java | 245 ++++++++++++++++++ .../extensions/api/changes/ReviewerInfo.java | 9 + .../gerrit/extensions/common/AccountInfo.java | 31 +++ .../gerrit/server/change/ChangeJson.java | 31 ++- .../gerrit/server/change/DeleteReviewer.java | 214 +-------------- .../change/DeleteReviewerByEmailOp.java | 96 +++++++ .../server/change/DeleteReviewerOp.java | 232 +++++++++++++++++ .../gerrit/server/change/ListReviewers.java | 12 +- .../server/change/ListRevisionReviewers.java | 12 +- .../google/gerrit/server/change/Module.java | 2 + .../gerrit/server/change/PostReview.java | 7 +- .../gerrit/server/change/PostReviewers.java | 183 +++++++++---- .../server/change/ReviewerResource.java | 46 +++- .../gerrit/server/change/Reviewers.java | 19 +- .../server/change/RevisionReviewers.java | 19 +- .../google/gerrit/server/mail/Address.java | 8 + .../gerrit/server/mail/send/ChangeEmail.java | 14 + .../mail/send/DeleteReviewerSender.java | 12 +- .../server/mail/send/NewChangeSender.java | 13 + .../server/mail/send/OutgoingEmail.java | 7 + .../server/notedb/ReviewerStateInternal.java | 8 +- 23 files changed, 995 insertions(+), 281 deletions(-) create mode 100644 gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewerByEmailOp.java create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewerOp.java 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/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-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/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/ReviewerStateInternal.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ReviewerStateInternal.java index 66c0c82e27..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(