Add a header about attention set for all change emails

While at it, also removed HeaderHtml.soy since it's anyway not used
and it's strange that there is no Header.soy. Once we start using
more headers, it may be needed to bring Header.soy back and do
similar logic to Footer.soy to loop over all the footers/headers
instead of adding them in format() as done now.

Ensured the attention set header is only visible if the attention
set is non-empty, and if the attention set is enabled.

Change-Id: I805c625e063ee56ba71588dd684b34cd5c318290
(cherry picked from commit 10f035a906)
This commit is contained in:
Gal Paikin
2020-10-12 15:20:22 +02:00
committed by Luca Milanesio
parent d5e4b054fb
commit b34fa48374
8 changed files with 135 additions and 7 deletions

View File

@@ -125,7 +125,8 @@ public class SitePathInitializer {
extractMailExample("DeleteVoteHtml.soy");
extractMailExample("Footer.soy");
extractMailExample("FooterHtml.soy");
extractMailExample("HeaderHtml.soy");
extractMailExample("ChangeHeader.soy");
extractMailExample("ChangeHeaderHtml.soy");
extractMailExample("HttpPasswordUpdate.soy");
extractMailExample("HttpPasswordUpdateHtml.soy");
extractMailExample("InboundEmailRejection.soy");

View File

@@ -38,6 +38,7 @@ public class EmailSettings {
public final Encryption encryption;
public final long fetchInterval; // in milliseconds
public final boolean sendNewPatchsetEmails;
public final boolean isAttentionSetEnabled;
@Inject
EmailSettings(@GerritServerConfig Config cfg) {
@@ -60,5 +61,6 @@ public class EmailSettings {
TimeUnit.MILLISECONDS.convert(60, TimeUnit.SECONDS),
TimeUnit.MILLISECONDS);
sendNewPatchsetEmails = cfg.getBoolean("change", null, "sendNewPatchsetEmails", true);
isAttentionSetEnabled = cfg.getBoolean("change", null, "enableAttentionSet", false);
}
}

View File

@@ -14,6 +14,7 @@
package com.google.gerrit.server.mail.send;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.gerrit.server.util.AttentionSetUtil.additionsOnly;
import com.google.common.base.Splitter;
@@ -128,6 +129,10 @@ public abstract class ChangeEmail extends NotificationEmail {
/** Format the message body by calling {@link #appendText(String)}. */
@Override
protected void format() throws EmailException {
if (useHtml()) {
appendHtml(soyHtmlTemplate("ChangeHeaderHtml"));
}
appendText(textTemplate("ChangeHeader"));
formatChange();
appendText(textTemplate("ChangeFooter"));
if (useHtml()) {
@@ -505,6 +510,13 @@ public abstract class ChangeEmail extends NotificationEmail {
for (Account.Id attentionUser : currentAttentionSet) {
footers.add(MailHeader.ATTENTION.withDelimiter() + getNameEmailFor(attentionUser));
}
// Since this would be user visible, only show it if attention set is enabled
if (args.settings.isAttentionSetEnabled && !currentAttentionSet.isEmpty()) {
// We need names rather than account ids / emails to make it user readable.
soyContext.put(
"attentionSet",
currentAttentionSet.stream().map(this::getNameFor).collect(toImmutableSet()));
}
}
/**

View File

@@ -45,6 +45,8 @@ public class MailSoySauceProvider implements Provider<SoySauce> {
"AddToAttentionSetHtml.soy",
"ChangeFooter.soy",
"ChangeFooterHtml.soy",
"ChangeHeader.soy",
"ChangeHeaderHtml.soy",
"ChangeSubject.soy",
"Comment.soy",
"CommentHtml.soy",
@@ -60,7 +62,6 @@ public class MailSoySauceProvider implements Provider<SoySauce> {
"InboundEmailRejectionHtml.soy",
"Footer.soy",
"FooterHtml.soy",
"HeaderHtml.soy",
"HttpPasswordUpdate.soy",
"HttpPasswordUpdateHtml.soy",
"Merged.soy",

View File

@@ -116,9 +116,6 @@ public abstract class OutgoingEmail {
if (messageId == null) {
throw new IllegalStateException("All emails must have a messageId");
}
if (useHtml()) {
appendHtml(soyHtmlTemplate("HeaderHtml"));
}
format();
appendText(textTemplate("Footer"));
if (useHtml()) {

View File

@@ -28,6 +28,7 @@ import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.UseClockStep;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.account.AccountOperations;
import com.google.gerrit.acceptance.testsuite.change.ChangeOperations;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
@@ -1362,6 +1363,75 @@ public class AttentionSetIT extends AbstractDaemonTest {
sender.clear();
}
@Test
@GerritConfig(name = "change.enableAttentionSet", value = "true")
public void attentionSetEmailHeader() throws Exception {
PushOneCommit.Result r = createChange();
TestAccount user2 = accountCreator.user2();
// Add user and user2 to the attention set.
change(r)
.current()
.review(
ReviewInput.create().reviewer(user.email()).reviewer(accountCreator.user2().email()));
assertThat(Iterables.getOnlyElement(sender.getMessages()).body())
.contains(
"Attention is currently required from: "
+ user2.fullName()
+ ", "
+ user.fullName()
+ ".");
assertThat(Iterables.getOnlyElement(sender.getMessages()).htmlBody())
.contains(
"Attention is currently required from: "
+ user2.fullName()
+ ", "
+ user.fullName()
+ ".");
sender.clear();
// Irrelevant reply, User and User2 are still in the attention set.
change(r).current().review(ReviewInput.approve());
assertThat(Iterables.getOnlyElement(sender.getMessages()).body())
.contains(
"Attention is currently required from: "
+ user2.fullName()
+ ", "
+ user.fullName()
+ ".");
assertThat(Iterables.getOnlyElement(sender.getMessages()).htmlBody())
.contains(
"Attention is currently required from: "
+ user2.fullName()
+ ", "
+ user.fullName()
+ ".");
sender.clear();
// Abandon the change which removes user from attention set; there is an email but without the
// attention footer.
change(r).abandon();
assertThat(Iterables.getOnlyElement(sender.getMessages()).body())
.doesNotContain("Attention is currently required");
assertThat(Iterables.getOnlyElement(sender.getMessages()).htmlBody())
.doesNotContain("Attention is currently required");
sender.clear();
}
@Test
@GerritConfig(name = "change.enableAttentionSet", value = "false")
public void noReferenceToAttentionSetInEmailsWhenDisabled() throws Exception {
PushOneCommit.Result r = createChange();
// Add user and to the attention set.
change(r).addReviewer(user.id().toString());
// Attention set is not referenced.
assertThat(Iterables.getOnlyElement(sender.getMessages()).body())
.doesNotContain("Attention is currently required");
assertThat(Iterables.getOnlyElement(sender.getMessages()).htmlBody())
.doesNotContain("Attention is currently required");
sender.clear();
}
@Test
public void attentionSetWithEmailFilter() throws Exception {
PushOneCommit.Result r = createChange();

View File

@@ -1,5 +1,5 @@
/**
* Copyright (C) 2016 The Android Open Source Project
* Copyright (C) 2020 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.
@@ -16,5 +16,17 @@
{namespace com.google.gerrit.server.mail.template}
{template .HeaderHtml}
{template .ChangeHeader kind="text"}
{@param attentionSet: ?}
{if $attentionSet}
Attention is currently required from:{sp}
{for $attentionSetUser in $attentionSet}
{$attentionSetUser}
// add commas or dot.
{if isLast($attentionSetUser)}.
{else},{sp}
{/if}
{/for}
{\n}
{/if}
{/template}

View File

@@ -0,0 +1,33 @@
/**
* Copyright (C) 2020 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.
*/
{namespace com.google.gerrit.server.mail.template}
{template .ChangeHeaderHtml}
{@param attentionSet: ?}
{if $attentionSet}
<p> Attention is currently required from:{sp}
{for $attentionSetUser in $attentionSet}
{$attentionSetUser}
//add commas or dot.
{if isLast($attentionSetUser)}.
{else},{sp}
{/if}
{/for} </p>
{\n}
{/if}
{/template}