opendev/gerrit
Code Issues Proposed changes

Add a User Preference to Receive only Plaintext Emails

Browse Source 
This change adds backend support for a new user preference that enables
users to specify that they want to receive plaintext emails only.
It also adds tests.

From now on, Gerrit will send out two emails: A multipart version and a
plaintext version if at least one user in the list of recipients has set
this user preference to plaintext.

For users who prefer plaintext, this comes at the cost of not being
listed in the multipart To and Cc headers. We work around this by adding
all users to the Reply-To address in both the plaintext and multipart
email.

The frontend work is done in a separate change.

Feature: Issue 5349
Change-Id: I190644732245cb4307f2ef098de173f3cc6209f2
This commit is contained in:
Patrick Hiesel
2017-02-27 16:24:26 +01:00
parent 8d30fa20dd
commit b5e5869409
9 changed files with 169 additions and 28 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
View File

@@ -1004,10 +1004,10 @@ public abstract class AbstractDaemonTest {
grant(Permission.CREATE_SIGNED_TAG, project, R_TAGS + "*");
}
protected void assertMailFrom(Message message, String email) throws Exception {
protected void assertMailReplyTo(Message message, String email) throws Exception {
assertThat(message.headers()).containsKey("Reply-To");
EmailHeader.String replyTo = (EmailHeader.String) message.headers().get("Reply-To");
assertThat(replyTo.getString()).isEqualTo(email);
assertThat(replyTo.getString()).contains(email);
}
protected ContributorAgreement configureContributorAgreement(boolean autoVerify)

2
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java
View File

@@ -351,7 +351,7 @@ public class AccountIT extends AbstractDaemonTest {
assertThat(messages).hasSize(1);
Message message = messages.get(0);
assertThat(message.rcpt()).containsExactly(user.emailAddress);
assertMailFrom(message, admin.email);
assertMailReplyTo(message, admin.email);
}
@Test

2
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/GeneralPreferencesIT.java
View File

@@ -26,6 +26,7 @@ import com.google.gerrit.extensions.client.GeneralPreferencesInfo.DateFormat;
import com.google.gerrit.extensions.client.GeneralPreferencesInfo.DefaultBase;
import com.google.gerrit.extensions.client.GeneralPreferencesInfo.DiffView;
import com.google.gerrit.extensions.client.GeneralPreferencesInfo.DownloadCommand;
import com.google.gerrit.extensions.client.GeneralPreferencesInfo.EmailFormat;
import com.google.gerrit.extensions.client.GeneralPreferencesInfo.EmailStrategy;
import com.google.gerrit.extensions.client.GeneralPreferencesInfo.ReviewCategoryStrategy;
import com.google.gerrit.extensions.client.GeneralPreferencesInfo.TimeFormat;
@@ -85,6 +86,7 @@ public class GeneralPreferencesIT extends AbstractDaemonTest {
i.dateFormat = DateFormat.US;
i.timeFormat = TimeFormat.HHMM_24;
i.emailStrategy = EmailStrategy.DISABLED;
i.emailFormat = EmailFormat.PLAINTEXT;
i.defaultBaseForMerges = DefaultBase.AUTO_MERGE;
i.expandInlineDiffs ^= true;
i.highlightAssigneeInChangeTable ^= true;

6
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
View File

@@ -759,7 +759,7 @@ public class ChangeIT extends AbstractDaemonTest {
assertThat(m.rcpt()).containsExactly(user.emailAddress);
assertThat(m.body()).contains(admin.fullName + " has uploaded this change for review");
assertThat(m.body()).contains("Change subject: " + PushOneCommit.SUBJECT + "\n");
assertMailFrom(m, admin.email);
assertMailReplyTo(m, admin.email);
}
@Test
@@ -836,7 +836,7 @@ public class ChangeIT extends AbstractDaemonTest {
assertThat(m.body()).contains("Hello " + user.fullName + ",\n");
assertThat(m.body()).contains("I'd like you to do a code review.");
assertThat(m.body()).contains("Change subject: " + PushOneCommit.SUBJECT + "\n");
assertMailFrom(m, admin.email);
assertMailReplyTo(m, admin.email);
}
@Test
@@ -956,7 +956,7 @@ public class ChangeIT extends AbstractDaemonTest {
assertThat(m.body()).contains("Hello " + user.fullName + ",\n");
assertThat(m.body()).contains("I'd like you to do a code review.");
assertThat(m.body()).contains("Change subject: " + PushOneCommit.SUBJECT + "\n");
assertMailFrom(m, admin.email);
assertMailReplyTo(m, admin.email);
ChangeInfo c = gApi.changes().id(r.getChangeId()).get();
// When NoteDb is enabled adding a reviewer records that user as reviewer

76
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/NotificationMailFormatIT.java Normal file
View File

@@ -0,0 +1,76 @@
// 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.server.mail;
import static com.google.common.truth.Truth.assertThat;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
import com.google.gerrit.extensions.client.GeneralPreferencesInfo.EmailFormat;
import com.google.gerrit.testutil.FakeEmailSender;
import org.junit.Test;
public class NotificationMailFormatIT extends AbstractDaemonTest {
@Test
public void userReceivesPlaintextEmail() throws Exception {
// Set user preference to receive only plaintext content
GeneralPreferencesInfo i = new GeneralPreferencesInfo();
i.emailFormat = EmailFormat.PLAINTEXT;
gApi.accounts().id(admin.getId().toString()).setPreferences(i);
// Create change as admin and review as user
PushOneCommit.Result r = createChange();
setApiUser(user);
gApi.changes()
.id(r.getChangeId())
.current()
.review(ReviewInput.recommend());
// Check that admin has received only plaintext content
assertThat(sender.getMessages()).hasSize(1);
FakeEmailSender.Message m = sender.getMessages().get(0);
assertThat(m.body()).isNotNull();
assertThat(m.htmlBody()).isNull();
assertMailReplyTo(m, admin.email);
assertMailReplyTo(m, user.email);
// Reset user preference
setApiUser(admin);
i.emailFormat = EmailFormat.HTML_PLAINTEXT;
gApi.accounts().id(admin.getId().toString()).setPreferences(i);
}
@Test
public void userReceivesHtmlAndPlaintextEmail() throws Exception {
// Create change as admin and review as user
PushOneCommit.Result r = createChange();
setApiUser(user);
gApi.changes()
.id(r.getChangeId())
.current()
.review(ReviewInput.recommend());
// Check that admin has received both HTML and plaintext content
assertThat(sender.getMessages()).hasSize(1);
FakeEmailSender.Message m = sender.getMessages().get(0);
assertThat(m.body()).isNotNull();
assertThat(m.htmlBody()).isNotNull();
assertMailReplyTo(m, admin.email);
assertMailReplyTo(m, user.email);
}
}

14
gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/GeneralPreferencesInfo.java
View File

@@ -87,6 +87,11 @@ public class GeneralPreferencesInfo {
DISABLED
}
public enum EmailFormat {
PLAINTEXT,
HTML_PLAINTEXT
}
public enum DefaultBase {
AUTO_MERGE(null),
FIRST_PARENT(-1);
@@ -150,6 +155,7 @@ public class GeneralPreferencesInfo {
public List<String> changeTable;
public Map<String, String> urlAliases;
public EmailStrategy emailStrategy;
public EmailFormat emailFormat;
public DefaultBase defaultBaseForMerges;
public boolean isShowInfoInReviewCategory() {
@@ -191,12 +197,20 @@ public class GeneralPreferencesInfo {
return emailStrategy;
}
public EmailFormat getEmailFormat() {
if (emailFormat == null) {
return EmailFormat.HTML_PLAINTEXT;
}
return emailFormat;
}
public static GeneralPreferencesInfo defaults() {
GeneralPreferencesInfo p = new GeneralPreferencesInfo();
p.changesPerPage = DEFAULT_PAGESIZE;
p.showSiteHeader = true;
p.useFlashClipboard = true;
p.emailStrategy = EmailStrategy.ENABLED;
p.emailFormat = EmailFormat.HTML_PLAINTEXT;
p.reviewCategoryStrategy = ReviewCategoryStrategy.NONE;
p.downloadScheme = null;
p.downloadCommand = DownloadCommand.CHECKOUT;

4
gerrit-server/BUILD
View File

@@ -88,6 +88,7 @@ java_library(
TESTUTIL_DEPS = [
":server",
"//gerrit-common:annotations",
"//gerrit-common:server",
"//gerrit-cache-h2:cache-h2",
"//gerrit-extension-api:api",
@@ -188,7 +189,6 @@ java_library(
":testutil",
"//gerrit-antlr:query_exception",
"//gerrit-antlr:query_parser",
"//gerrit-common:annotations",
"//gerrit-server/src/main/prolog:common",
"//lib/antlr:java_runtime",
],
@@ -203,7 +203,6 @@ junit_tests(
":testutil",
"//gerrit-antlr:query_exception",
"//gerrit-antlr:query_parser",
"//gerrit-common:annotations",
"//gerrit-server/src/main/prolog:common",
"//lib/antlr:java_runtime",
],
@@ -221,7 +220,6 @@ junit_tests(
deps = TESTUTIL_DEPS + [
":testutil",
"//gerrit-antlr:query_exception",
"//gerrit-common:annotations",
"//gerrit-patch-jgit:server",
"//gerrit-server/src/main/prolog:common",
"//lib:args4j",

64
gerrit-server/src/main/java/com/google/gerrit/server/mail/send/OutgoingEmail.java
View File

@@ -25,6 +25,7 @@ import com.google.gerrit.common.errors.EmailException;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.api.changes.RecipientType;
import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
import com.google.gerrit.extensions.client.GeneralPreferencesInfo.EmailFormat;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.UserIdentity;
import com.google.gerrit.server.account.AccountState;
@@ -50,6 +51,7 @@ import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.StringJoiner;
import org.apache.commons.lang.StringUtils;
import org.apache.velocity.Template;
import org.apache.velocity.VelocityContext;
@@ -126,6 +128,8 @@ public abstract class OutgoingEmail {
if (useHtml()) {
appendHtml(soyHtmlTemplate("FooterHtml"));
}
Set<Address> smtpRcptToPlaintextOnly = new HashSet<>();
if (shouldSendMessage()) {
if (fromId != null) {
final Account fromUser = args.accountCache.get(fromId).getAccount();
@@ -145,27 +149,48 @@ public abstract class OutgoingEmail {
}
// Check the preferences of all recipients. If any user has disabled
// his email notifications then drop him from recipients' list
// his email notifications then drop him from recipients' list.
// In addition, check if users only want to receive plaintext email.
for (Account.Id id : rcptTo) {
Account thisUser = args.accountCache.get(id).getAccount();
GeneralPreferencesInfo prefs = thisUser.getGeneralPreferencesInfo();
if (prefs == null || prefs.getEmailStrategy() == DISABLED) {
removeUser(thisUser);
} else if (useHtml() && prefs.getEmailFormat() == EmailFormat.PLAINTEXT) {
removeUser(thisUser);
smtpRcptToPlaintextOnly.add(
new Address(thisUser.getFullName(), thisUser.getPreferredEmail()));
}
if (smtpRcptTo.isEmpty()) {
if (smtpRcptTo.isEmpty() && smtpRcptToPlaintextOnly.isEmpty()) {
return;
}
}
}
// Set Reply-To only if it hasn't been set by a child class
// Reply-To will already be populated for the message types where Gerrit supports
// inbound email replies.
if (!headers.containsKey("Reply-To")) {
StringJoiner j = new StringJoiner(", ");
if (fromId != null) {
Address address = toAddress(fromId);
if (address != null) {
j.add(address.getEmail());
}
}
smtpRcptTo.stream().forEach(a -> j.add(a.getEmail()));
smtpRcptToPlaintextOnly.stream().forEach(a -> j.add(a.getEmail()));
setHeader("Reply-To", j.toString());
}
String textPart = textBody.toString();
OutgoingEmailValidationListener.Args va = new OutgoingEmailValidationListener.Args();
va.messageClass = messageClass;
va.smtpFromAddress = smtpFromAddress;
va.smtpRcptTo = smtpRcptTo;
va.headers = headers;
va.body = textPart;
if (useHtml()) {
va.htmlBody = htmlBody.toString();
} else {
@@ -180,7 +205,26 @@ public abstract class OutgoingEmail {
}
}
args.emailSender.send(va.smtpFromAddress, va.smtpRcptTo, va.headers, va.body, va.htmlBody);
if (!smtpRcptTo.isEmpty()) {
// Send multipart message
args.emailSender.send(va.smtpFromAddress, va.smtpRcptTo, va.headers, va.body, va.htmlBody);
}
if (!smtpRcptToPlaintextOnly.isEmpty()) {
// Send plaintext message
Map<String, EmailHeader> shallowCopy = new HashMap<>();
shallowCopy.putAll(headers);
// Remove To and Cc
shallowCopy.remove(HDR_TO);
shallowCopy.remove(HDR_CC);
for (Address a : smtpRcptToPlaintextOnly) {
// Add new To
EmailHeader.AddressList to = new EmailHeader.AddressList();
to.add(a);
shallowCopy.put(HDR_TO, to);
}
args.emailSender.send(va.smtpFromAddress, smtpRcptToPlaintextOnly, shallowCopy, va.body);
}
}
}
@@ -207,18 +251,6 @@ public abstract class OutgoingEmail {
add(recipientType, accountsToNotify.get(recipientType));
}
if (fromId != null) {
// If we have a user that this message is supposedly caused by
// but the From header on the email does not match the user as
// it is a generic header for this Gerrit server, include the
// Reply-To header with the current user's email address.
//
final Address a = toAddress(fromId);
if (a != null && !smtpFromAddress.getEmail().equals(a.getEmail())) {
setHeader("Reply-To", a.getEmail());
}
}
setHeader("X-Gerrit-MessageType", messageClass);
textBody = new StringBuilder();
htmlBody = new StringBuilder();

25
gerrit-server/src/test/java/com/google/gerrit/testutil/FakeEmailSender.java
View File

@@ -19,6 +19,7 @@ import static java.util.stream.Collectors.toList;
import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.errors.EmailException;
import com.google.gerrit.server.git.WorkQueue;
import com.google.gerrit.server.mail.Address;
@@ -58,9 +59,13 @@ public class FakeEmailSender implements EmailSender {
@AutoValue
public abstract static class Message {
private static Message create(
Address from, Collection<Address> rcpt, Map<String, EmailHeader> headers, String body) {
Address from,
Collection<Address> rcpt,
Map<String, EmailHeader> headers,
String body,
String htmlBody) {
return new AutoValue_FakeEmailSender_Message(
from, ImmutableList.copyOf(rcpt), ImmutableMap.copyOf(headers), body);
from, ImmutableList.copyOf(rcpt), ImmutableMap.copyOf(headers), body, htmlBody);
}
public abstract Address from();
@@ -70,6 +75,9 @@ public class FakeEmailSender implements EmailSender {
public abstract ImmutableMap<String, EmailHeader> headers();
public abstract String body();
@Nullable
public abstract String htmlBody();
}
private final WorkQueue workQueue;
@@ -95,7 +103,18 @@ public class FakeEmailSender implements EmailSender {
public void send(
Address from, Collection<Address> rcpt, Map<String, EmailHeader> headers, String body)
throws EmailException {
messages.add(Message.create(from, rcpt, headers, body));
send(from, rcpt, headers, body, null);
}
@Override
public void send(
Address from,
Collection<Address> rcpt,
Map<String, EmailHeader> headers,
String body,
String htmlBody)
throws EmailException {
messages.add(Message.create(from, rcpt, headers, body, htmlBody));
}
public void clear() {