Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AO3-6017 Add role of commenter to comment notification emails #4683

Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
9642df9
AO3-6017 Add username and role of commenter to comment notification e…
Bilka2 Dec 4, 2023
35f9901
Merge branch 'master' into AO3-6017-notification-emails-for-pseud-for…
Bilka2 Dec 5, 2023
a14d55e
AO3-6017 Use same locale as guest comment name
Bilka2 Dec 5, 2023
1bd3197
Merge branch 'master' into AO3-6017-notification-emails-for-pseud-for…
Bilka2 Jan 4, 2024
66eb406
AO3-6017 Fix that color was set twice on the pseud
Bilka2 Jan 4, 2024
a66c3ab
AO3-6017 Add tests for guest and anon comment mails
Bilka2 Jan 4, 2024
8605ec3
AO3-6017 Change anon comment case to guard clause
Bilka2 Jan 4, 2024
7a71ae0
AO3-6017 Rubocop
Bilka2 Jan 4, 2024
1b6ece4
AO3-6017 Apply suggestions from code review
Bilka2 Feb 11, 2024
1909d9b
Merge branch 'master' into AO3-6017-notification-emails-for-pseud-for…
Bilka2 Feb 11, 2024
327085c
AO3-6017 Apply suggestions from code review everywhere
Bilka2 Feb 11, 2024
e36e64e
AO3-6017 Add tests for registered user with default pseud
Bilka2 Feb 11, 2024
5caaa04
AO3-6017 Add previews for comment reply sent and anon comment reply
Bilka2 Feb 11, 2024
3d17680
AO3-6017 Rubocop
Bilka2 Feb 11, 2024
8555721
AO3-6017 Fix copy paste error
Bilka2 Feb 11, 2024
87df0db
Merge branch 'master' into AO3-6017-notification-emails-for-pseud-for…
Bilka2 Mar 28, 2024
d492aa8
AO3-6017 Deduplicate comment notification tests in admin mailer spec
Bilka2 Mar 28, 2024
7db9b06
AO3-6017 Remove mistakenly added indentation
Bilka2 Mar 29, 2024
511a309
AO3-6017 Merge branch 'master'
Bilka2 Apr 14, 2024
a71500f
AO3-6017 Improve locale keys
Bilka2 Apr 14, 2024
f8d659f
AO3-6017 Add comments to mailer previews
Bilka2 Apr 14, 2024
7f79b61
Merge branch 'master' into AO3-6017-notification-emails-for-pseud-for…
Bilka2 May 5, 2024
11d5d85
AO3-6017 Merge fix
Bilka2 May 5, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 11 additions & 8 deletions app/helpers/mailer_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -181,22 +181,25 @@ def style_work_tag_metadata(tags)
end

def commenter_pseud_or_name_link(comment)
return style_bold(t("roles.anonymous_creator")) if comment.by_anonymous_creator?

if comment.comment_owner.nil?
t("roles.guest_commenter_name_html", name: style_bold(comment.comment_owner_name), role: style_role(t("roles.guest_with_parens")))
elsif comment.by_anonymous_creator?
style_bold(t("roles.anonymous_creator"))
t("roles.commenter_name_html", name: style_bold(comment.comment_owner_name), role: style_role(t("roles.guest_with_parens")))
else
style_pseud_link(comment.pseud)
role = comment.user.official ? t("roles.official_with_parens") : t("roles.registered_with_parens")
pseud_link = style_link(comment.pseud.byline, user_pseud_url(comment.user, comment.pseud))
t("roles.commenter_name_html", name: tag.strong(pseud_link), role: style_role(role))
brianjaustin marked this conversation as resolved.
Show resolved Hide resolved
end
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not 100% sold myself, but might be more manageable with smaller scope if like:

if comment.by_anonymous_creator?
  return style_bold(t("roles.anonymous_creator"))
end

name =
  if comment.comment_owner.nil?
    comment.comment_owner_name
  else
    style_link(comment.pseud.byline, user_pseud_url(comment.user, comment.pseud))
  end

role =
  if comment.comment_owner.nil?
    t("roles.guest_with_parens")
  elsif comment.user.official
    t("roles.official_with_parens")
  else
    t("roles.registered_with_parens")  
  end

t("roles.commenter_name_html", name: style_bold(name), role: style_role(role))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of turning the anon comment into a guard clause like

return style_bold(t("roles.anonymous_creator")) if comment.by_anonymous_creator?

but I don't know about the rest of the conditions.


def commenter_pseud_or_name_text(comment)
return t("roles.anonymous_creator") if comment.by_anonymous_creator?

if comment.comment_owner.nil?
t("roles.guest_commenter_name_text", name: comment.comment_owner_name, role: t("roles.guest_with_parens"))
elsif comment.by_anonymous_creator?
t("roles.anonymous_creator")
t("roles.commenter_name_text", name: comment.comment_owner_name, role: t("roles.guest_with_parens"))
else
text_pseud(comment.pseud)
role = comment.user.official ? t("roles.official_with_parens") : t("roles.registered_with_parens")
t("roles.commenter_name_text", name: text_pseud(comment.pseud), role: role)
end
end

Expand Down
4 changes: 2 additions & 2 deletions app/views/admin_mailer/comment_notification.text.erb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<% content_for :message do %>
<%= commenter_pseud_or_name_text(@comment) %> left the following comment on
<% if @comment.ultimate_parent.is_a?(Tag) then %>the tag "<%= @comment.ultimate_parent.commentable_name %>" (<%= url_for(:controller => :tags, :action => :show, :id => @comment.ultimate_parent, :only_path => false) %>) <% else %>"<%= @comment.ultimate_parent.commentable_name.html_safe %>" (<%= polymorphic_url(@comment.ultimate_parent, :only_path => false) %>)<% end %>:
<%= commenter_pseud_or_name_text(@comment) %> left the following comment on
<% if @comment.ultimate_parent.is_a?(Tag) then %>the tag "<%= @comment.ultimate_parent.commentable_name %>" (<%= url_for(:controller => :tags, :action => :show, :id => @comment.ultimate_parent, :only_path => false) %>) <% else %>"<%= @comment.ultimate_parent.commentable_name.html_safe %>" (<%= polymorphic_url(@comment.ultimate_parent, :only_path => false) %>)<% end %>:
<%= text_divider %>

<%= to_plain_text(raw @comment.sanitized_content) %>
Expand Down
4 changes: 2 additions & 2 deletions app/views/admin_mailer/edited_comment_notification.text.erb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<% content_for :message do %>
<%= commenter_pseud_or_name_text(@comment) %> edited the following comment on
<% if @comment.ultimate_parent.is_a?(Tag) then %>the tag "<%= @comment.ultimate_parent.commentable_name %>" (<%= url_for(:controller => :tags, :action => :show, :id => @comment.ultimate_parent, :only_path => false) %>) <% else %>"<%= @comment.ultimate_parent.commentable_name.html_safe %>" (<%= polymorphic_url(@comment.ultimate_parent, :only_path => false) %>)<% end %>:
<%= commenter_pseud_or_name_text(@comment) %> edited the following comment on
<% if @comment.ultimate_parent.is_a?(Tag) then %>the tag "<%= @comment.ultimate_parent.commentable_name %>" (<%= url_for(:controller => :tags, :action => :show, :id => @comment.ultimate_parent, :only_path => false) %>) <% else %>"<%= @comment.ultimate_parent.commentable_name.html_safe %>" (<%= polymorphic_url(@comment.ultimate_parent, :only_path => false) %>)<% end %>:
<%= text_divider %>

<%= to_plain_text(raw @comment.sanitized_content) %>
Expand Down
6 changes: 4 additions & 2 deletions config/locales/mailers/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,11 @@ en:
support: The AO3 Support team
roles:
anonymous_creator: Anonymous Creator
guest_commenter_name_html: "%{name} %{role}"
guest_commenter_name_text: "%{name} %{role}"
commenter_name_html: "%{name} %{role}"
commenter_name_text: "%{name} %{role}"
Bilka2 marked this conversation as resolved.
Show resolved Hide resolved
guest_with_parens: "(Guest)"
official_with_parens: "(Official)"
registered_with_parens: "(Registered User)"
user_mailer:
abuse_report:
copy:
Expand Down
90 changes: 90 additions & 0 deletions spec/mailers/admin_mailer_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -172,4 +172,94 @@
end
end
end

let(:commenter) { create(:user, login: "Accumulator") }
let(:commenter_pseud) { create(:pseud, user: commenter, name: "Blueprint") }
let(:comment) { create(:comment, :on_admin_post, pseud: commenter_pseud) }

shared_examples "a notification email with the commenters pseud and username" do
Bilka2 marked this conversation as resolved.
Show resolved Hide resolved
describe "HTML email" do
it "has the pseud and username of the commenter" do
expect(email).to have_html_part_content(">Blueprint (Accumulator)</a></strong> <em><strong>(Registered User)</strong></em>")
expect(subject.html_part).to have_xpath(
"//a[@href=\"#{user_pseud_url(commenter, commenter_pseud)}\"]",
text: "Blueprint (Accumulator)"
)
end
end

describe "text email" do
it "has the pseud and username of the commenter" do
expect(subject).to have_text_part_content(
"Blueprint (Accumulator) (#{user_pseud_url(commenter, commenter_pseud)}) (Registered User)"
)
end
end
end

shared_examples "a notification email that marks the commenter as official" do
describe "HTML email" do
it "has the username of the commenter and the official role" do
expect(email).to have_html_part_content(">Centrifuge</a></strong> <em><strong>(Official)</strong></em>")
expect(subject.html_part).to have_xpath(
"//a[@href=\"#{user_pseud_url(commenter, commenter.default_pseud)}\"]",
text: "Centrifuge"
)
end
end

describe "text email" do
it "has the username of the commenter and the official role" do
expect(subject).to have_text_part_content(
"Centrifuge (#{user_pseud_url(commenter, commenter.default_pseud)}) (Official)"
)
end
end
end

describe "comment_notification" do
Bilka2 marked this conversation as resolved.
Show resolved Hide resolved
subject(:email) { AdminMailer.comment_notification(comment.id) }

it_behaves_like "an email with a valid sender"
it_behaves_like "a multipart email"
it_behaves_like "a notification email with the commenters pseud and username"

context "when the comment is by an official user using their default pseud" do
let(:commenter) { create(:official_user, login: "Centrifuge") }
let(:comment) { create(:comment, :on_admin_post, pseud: commenter.default_pseud) }

it_behaves_like "a notification email that marks the commenter as official"
end

context "when the commenter is a guest" do
let(:comment) { create(:comment, :on_admin_post, pseud: nil, name: "Defender", email: Faker::Internet.email) }

describe "HTML email" do
it "has the name of the guest and the guest role" do
expect(email).to have_html_part_content(">Defender</b> <em><strong>(Guest)</strong></em>")
end
end

describe "text email" do
it "has the name of the guest and the guest role" do
expect(subject).to have_text_part_content("Defender (Guest)")
end
end
end
end

describe "comment_edited_notification" do
subject(:email) { AdminMailer.edited_comment_notification(comment.id) }

it_behaves_like "an email with a valid sender"
it_behaves_like "a multipart email"
it_behaves_like "a notification email with the commenters pseud and username"

context "when the comment is by an official user using their default pseud" do
let(:commenter) { create(:official_user, login: "Centrifuge") }
let(:comment) { create(:comment, :on_admin_post, pseud: commenter.default_pseud) }

it_behaves_like "a notification email that marks the commenter as official"
end
end
end
Loading
Loading