commit 10f035a906e59c31465d0c867f9821ecaa9f1fe0 Author: Gal Paikin Date: Mon Oct 12 15:20:22 2020 +0200 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 diff --git a/java/com/google/gerrit/pgm/init/SitePathInitializer.java b/java/com/google/gerrit/pgm/init/SitePathInitializer.java index 8a71c1c..ddc4f79 100644 --- a/java/com/google/gerrit/pgm/init/SitePathInitializer.java +++ b/java/com/google/gerrit/pgm/init/SitePathInitializer.java @@ -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"); diff --git a/java/com/google/gerrit/server/mail/EmailSettings.java b/java/com/google/gerrit/server/mail/EmailSettings.java index c411af5..1ac6eb6 100644 --- a/java/com/google/gerrit/server/mail/EmailSettings.java +++ b/java/com/google/gerrit/server/mail/EmailSettings.java @@ -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); } } diff --git a/java/com/google/gerrit/server/mail/send/ChangeEmail.java b/java/com/google/gerrit/server/mail/send/ChangeEmail.java index 6637f99..8d76e23 100644 --- a/java/com/google/gerrit/server/mail/send/ChangeEmail.java +++ b/java/com/google/gerrit/server/mail/send/ChangeEmail.java @@ -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())); + } } /** diff --git a/java/com/google/gerrit/server/mail/send/MailSoySauceProvider.java b/java/com/google/gerrit/server/mail/send/MailSoySauceProvider.java index 623bdc2..1b58057 100644 --- a/java/com/google/gerrit/server/mail/send/MailSoySauceProvider.java +++ b/java/com/google/gerrit/server/mail/send/MailSoySauceProvider.java @@ -45,6 +45,8 @@ public class MailSoySauceProvider implements Provider { "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 { "InboundEmailRejectionHtml.soy", "Footer.soy", "FooterHtml.soy", - "HeaderHtml.soy", "HttpPasswordUpdate.soy", "HttpPasswordUpdateHtml.soy", "Merged.soy", diff --git a/java/com/google/gerrit/server/mail/send/OutgoingEmail.java b/java/com/google/gerrit/server/mail/send/OutgoingEmail.java index 1eb274b..bef5317 100644 --- a/java/com/google/gerrit/server/mail/send/OutgoingEmail.java +++ b/java/com/google/gerrit/server/mail/send/OutgoingEmail.java @@ -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()) { diff --git a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java index 08e70fb..61eef63 100644 --- a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java @@ -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; @@ -1363,6 +1364,75 @@ public class AttentionSetIT extends AbstractDaemonTest { } @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(); diff --git a/resources/com/google/gerrit/server/mail/ChangeHeader.soy b/resources/com/google/gerrit/server/mail/ChangeHeader.soy new file mode 100644 index 0000000..fde69f1 --- /dev/null +++ b/resources/com/google/gerrit/server/mail/ChangeHeader.soy @@ -0,0 +1,32 @@ +/** + * 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 .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} diff --git a/resources/com/google/gerrit/server/mail/ChangeHeaderHtml.soy b/resources/com/google/gerrit/server/mail/ChangeHeaderHtml.soy new file mode 100644 index 0000000..ea12455 --- /dev/null +++ b/resources/com/google/gerrit/server/mail/ChangeHeaderHtml.soy @@ -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} +

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} diff --git a/resources/com/google/gerrit/server/mail/HeaderHtml.soy b/resources/com/google/gerrit/server/mail/HeaderHtml.soy deleted file mode 100644 index 4710d8c..0000000 --- a/resources/com/google/gerrit/server/mail/HeaderHtml.soy +++ /dev/null @@ -1,20 +0,0 @@ -/** - * Copyright (C) 2016 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 .HeaderHtml} -{/template}