github-mirror/discourse
2
0
Fork 0
mirror of https://github.com/discourse/discourse.git synced 2025-05-24 03:11:09 +08:00
Code Issues Actions 1 Packages Projects Releases Wiki Activity

FIX: Notification email CTA by system user (#31726)

Browse Source 
Followup https://github.com/discourse/discourse/pull/31505/

When sending notification emails for system user responses for PMs,
we removed the part of the CTA where it says "to respond to xyz" in a
previous commit.

This commit takes it slightly further -- we now only show a "Visit
Topic"
or "Visit Message" button if the PM notification is from a system user,
it's a bit cleaner.

This commit also adds more in-depth tests, and refactors the message
builder a little.
This commit is contained in:
Martin Brennan
2025-03-12 13:49:12 +10:00
committed by GitHub
parent 6e1954aa41
commit cf4d80d0b3
3 changed files with 211 additions and 50 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
config/locales/server.en.yml
View File

@ -4016,10 +4016,15 @@ en:
header_instructions: ""
reply_by_email: "[Visit Topic](%{base_url}%{url}) or reply to this email to respond."
reply_by_email_pm: "[Visit Message](%{base_url}%{url}) or reply to this email to respond to %{participants}."
reply_by_email_button_only: "[Visit Topic](%{base_url}%{url})"
reply_by_email_pm_button_only: "[Visit Message](%{base_url}%{url})"
only_reply_by_email: "Reply to this email to respond."
only_reply_by_email_pm: "Reply to this email to respond to %{participants}."
only_reply_by_email_pm_button_only: "[Visit Message](%{base_url}%{url})"
visit_link_to_respond: "[Visit Topic](%{base_url}%{url}) to respond."
visit_link_to_respond_pm: "[Visit Message](%{base_url}%{url}) to respond to %{participants}."
visit_link_to_respond_button_only: "[Visit Topic](%{base_url}%{url})"
visit_link_to_respond_pm_button_only: "[Visit Message](%{base_url}%{url})"
reply_above_line: "## Please type your reply above this line. ##"
posted_by: "Posted by %{username} on %{post_date}"

117
lib/email/message_builder.rb
View File

@ -7,6 +7,7 @@ module Email
attr_reader :template_args, :reply_by_email_key
ALLOW_REPLY_BY_EMAIL_HEADER = "X-Discourse-Allow-Reply-By-Email"
INSTRUCTIONS_SEPARATOR = "---\n"
def initialize(to, opts = nil)
@to = to
@ -18,65 +19,83 @@ module Email
user_preferences_url: "#{Discourse.base_url}/my/preferences",
hostname: Discourse.current_hostname,
}.merge!(@opts)
if @template_args[:url].present?
@template_args[:header_instructions] ||= I18n.t(
"user_notifications.header_instructions",
@template_args,
)
@visit_link_to_respond_key =
DiscoursePluginRegistry.apply_modifier(
:message_builder_visit_link_to_respond,
"user_notifications.visit_link_to_respond",
@opts,
@to,
)
@reply_by_email_key =
DiscoursePluginRegistry.apply_modifier(
:message_builder_reply_by_email,
"user_notifications.reply_by_email",
@opts,
@to,
)
if @opts[:include_respond_instructions] == false
@template_args[:respond_instructions] = ""
return if @template_args[:url].blank?
@template_args[:header_instructions] ||= I18n.t(
"user_notifications.header_instructions",
@template_args,
)
@visit_link_to_respond_key =
DiscoursePluginRegistry.apply_modifier(
:message_builder_visit_link_to_respond,
"user_notifications.visit_link_to_respond",
@opts,
@to,
)
@reply_by_email_key =
DiscoursePluginRegistry.apply_modifier(
:message_builder_reply_by_email,
"user_notifications.reply_by_email",
@opts,
@to,
)
if @opts[:include_respond_instructions] == false
if @opts[:private_reply]
@template_args[:respond_instructions] = I18n.t(
"user_notifications.pm_participants",
@template_args,
) if @opts[:private_reply]
)
else
if @opts[:only_reply_by_email]
string = +"user_notifications.only_reply_by_email"
if @opts[:private_reply] && @opts[:username] != Discourse.system_user.username
string << "_pm"
end
else
string =
(
if allow_reply_by_email?
+@reply_by_email_key
else
+@visit_link_to_respond_key
end
)
if @opts[:private_reply] && @opts[:username] != Discourse.system_user.username
string << "_pm"
@template_args[:respond_instructions] = ""
end
else
if @opts[:only_reply_by_email]
respond_instructions_key = +"user_notifications.only_reply_by_email"
if @opts[:private_reply]
if @opts[:username] == Discourse.system_user.username
respond_instructions_key << "_pm_button_only"
else
respond_instructions_key << "_pm"
end
end
@template_args[:respond_instructions] = "---\n" + I18n.t(string, @template_args)
end
if @opts[:add_unsubscribe_link]
unsubscribe_string =
if @opts[:mailing_list_mode]
"unsubscribe_mailing_list"
elsif SiteSetting.unsubscribe_via_email_footer
"unsubscribe_link_and_mail"
else
respond_instructions_key =
(
if allow_reply_by_email?
+@reply_by_email_key
else
+@visit_link_to_respond_key
end
)
if @opts[:private_reply]
if @opts[:username] == Discourse.system_user.username
respond_instructions_key << "_pm_button_only"
else
"unsubscribe_link"
respond_instructions_key << "_pm"
end
@template_args[:unsubscribe_instructions] = I18n.t(unsubscribe_string, @template_args)
end
end
@template_args[:respond_instructions] = (
if respond_instructions_key != ""
INSTRUCTIONS_SEPARATOR + I18n.t(respond_instructions_key, @template_args)
else
""
end
)
end
if @opts[:add_unsubscribe_link]
unsubscribe_string =
if @opts[:mailing_list_mode]
"unsubscribe_mailing_list"
elsif SiteSetting.unsubscribe_via_email_footer
"unsubscribe_link_and_mail"
else
"unsubscribe_link"
end
@template_args[:unsubscribe_instructions] = I18n.t(unsubscribe_string, @template_args)
end
end

139
spec/lib/email/message_builder_spec.rb
View File

@ -111,6 +111,143 @@ RSpec.describe Email::MessageBuilder do
expect(header_args["X-Auto-Response-Suppress"]).to eq("All")
end
describe "include_respond_instructions" do
context "when include_respond_instructions is false" do
let(:private_reply) { false }
let(:builder) do
Email::MessageBuilder.new(
"to@to.com",
subject: "test",
body: "test",
include_respond_instructions: false,
url: "/t/123",
participants: %w[moe joe],
private_reply: private_reply,
)
end
it "does not include any instructions" do
expect(builder.template_args[:respond_instructions]).to eq("")
end
context "for a private_reply" do
let(:private_reply) { true }
it "includes the pm_participants instruction" do
expect(builder.template_args[:respond_instructions]).to eq(
I18n.t("user_notifications.pm_participants", builder.template_args),
)
end
end
end
context "when include_respond_instructions is true" do
let(:other_opts) { {} }
let(:builder) do
Email::MessageBuilder.new(
"to@to.com",
{
subject: "test",
body: "test",
include_respond_instructions: true,
allow_reply_by_email: true,
participants: %w[moe joe],
url: "/t/123",
}.merge(other_opts),
)
end
context "when only_reply_by_email" do
let(:other_opts) { { only_reply_by_email: true } }
it "includes the correct instructions" do
expect(builder.template_args[:respond_instructions]).to eq(
Email::MessageBuilder::INSTRUCTIONS_SEPARATOR +
I18n.t("user_notifications.only_reply_by_email", builder.template_args),
)
end
context "for private_reply to regular users" do
let(:other_opts) do
{ private_reply: true, username: "someguy", only_reply_by_email: true }
end
it "includes the correct instructions" do
expect(builder.template_args[:respond_instructions]).to eq(
Email::MessageBuilder::INSTRUCTIONS_SEPARATOR +
I18n.t("user_notifications.only_reply_by_email_pm", builder.template_args),
)
end
end
context "for private_reply to system users" do
let(:other_opts) do
{
private_reply: true,
username: Discourse.system_user.username,
only_reply_by_email: true,
}
end
it "only includes a button for respond_instructions" do
expect(builder.template_args[:respond_instructions]).to eq(
Email::MessageBuilder::INSTRUCTIONS_SEPARATOR +
I18n.t(
"user_notifications.only_reply_by_email_pm_button_only",
builder.template_args,
),
)
end
end
end
context "when not only_reply_by_email" do
it "includes the correct instructions when allowing reply by email" do
SiteSetting.manual_polling_enabled = true
SiteSetting.reply_by_email_address = "test+%{reply_key}@test.com"
SiteSetting.reply_by_email_enabled = true
expect(builder.template_args[:respond_instructions]).to eq(
Email::MessageBuilder::INSTRUCTIONS_SEPARATOR +
I18n.t("user_notifications.reply_by_email", builder.template_args),
)
end
it "includes the correct instructions when not allowing reply by email" do
expect(builder.template_args[:respond_instructions]).to eq(
Email::MessageBuilder::INSTRUCTIONS_SEPARATOR +
I18n.t("user_notifications.visit_link_to_respond", builder.template_args),
)
end
context "for private_reply to regular users" do
let(:other_opts) { { private_reply: true, username: "someguy" } }
it "includes the correct instructions" do
expect(builder.template_args[:respond_instructions]).to eq(
Email::MessageBuilder::INSTRUCTIONS_SEPARATOR +
I18n.t("user_notifications.visit_link_to_respond_pm", builder.template_args),
)
end
end
context "for private_reply to system users" do
let(:other_opts) { { private_reply: true, username: Discourse.system_user.username } }
it "only includes a button for respond_instructions" do
expect(builder.template_args[:respond_instructions]).to eq(
Email::MessageBuilder::INSTRUCTIONS_SEPARATOR +
I18n.t(
"user_notifications.visit_link_to_respond_pm_button_only",
builder.template_args,
),
)
end
end
end
end
end
describe "reply by email" do
context "without allow_reply_by_email" do
it "does not have a X-Discourse-Reply-Key" do
@ -165,7 +302,7 @@ RSpec.describe Email::MessageBuilder do
end
end
context "with allow_reply_by_email" do
context "with allow_reply_by_email and private_reply" do
let(:reply_by_email_builder) do
Email::MessageBuilder.new(
to_address,