From 1d31699e037218fd0d931bac9377054ddba84768 Mon Sep 17 00:00:00 2001 From: Patrick Hiesel Date: Wed, 15 Mar 2017 08:46:18 +0100 Subject: [PATCH] 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;