Merge "Add a User Preference to Receive only Plaintext Emails"

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

@@ -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)

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

@@ -361,7 +361,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

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

@@ -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;

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

@@ -760,7 +760,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
@@ -837,7 +837,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
@@ -957,7 +957,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

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/NotificationMailFormatIT.java

@@ -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);
+  }
+}

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

@@ -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;

gerrit-server/BUILD

@@ -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",
         "//gerrit-test-util:test_util",

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

@@ -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();

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

@@ -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() {