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.

Sign up for GitHub

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

Jump to bottom

AO3-5610 notify users when a bookmark or series they have created is hidden #4760

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

AO3-5610 notify users when a bookmark or series they have created is hidden #4760

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
29bf722
Create series notification mailer action
walshyb Mar 1, 2024
a7130a0
Create series model admin_hidden update handler
walshyb Mar 1, 2024
ecd02f8
Add series mailer content and en translation
walshyb Mar 1, 2024
c64d3b8
remove unnecessary end
walshyb Mar 1, 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
12 changes: 12 additions & 0 deletions app/mailers/user_mailer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,18 @@ def admin_deleted_work_notification(user, work)
end
end

def admin_hidden_series_notification(creation_id, user_id)
Copy link
Contributor Author

@walshyb walshyb Mar 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

do we want to reutilize the admin_hidden_work_notification action (maybe call it admin_hidden_creation_notification), and update the email contents by passing in parameters?

or can we have separate mailer actions for when an admin hides works/series/bookmarks?

Copy link
Member

Choose a reason for hiding this comment

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

Personally, I like separate actions for each. I think the contents of the emails could potentially have enough differences that it could get kind of messy trying to account for all of the variations.

@user = User.find_by(id: user_id)
@series = Series.find_by(id: creation_id)

I18n.with_locale(@user.preference.locale.iso) do
mail(
to: @user.email,
subject: default_i18n_subject(app_name: ArchiveConfig.APP_SHORT_NAME)
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs a comment to help out i18n-tasks:

Suggested change
subject: default_i18n_subject(app_name: ArchiveConfig.APP_SHORT_NAME)
# i18n-tasks-use t('user_mailer.admin_hidden_series_notification.subject')
subject: default_i18n_subject(app_name: ArchiveConfig.APP_SHORT_NAME)

Copy link
Contributor

Choose a reason for hiding this comment

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

Update: The comment is no longer needed! Just merge the current master branch into this PR branch and i18n-tasks will recognize default_i18n_subject automatically.

)
end
end

# Sends email to creators when a creation is hidden by an admin
def admin_hidden_work_notification(creation_id, user_id)
@user = User.find_by(id: user_id)
Expand Down
11 changes: 11 additions & 0 deletions app/models/series.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
maximum: ArchiveConfig.TITLE_MAX,
too_long: ts("must be less than %{max} letters long.", max: ArchiveConfig.TITLE_MAX)

after_update :admin_hidden_series_notification, if: :hidden_by_admin_changed?

# return title.html_safe to overcome escaping done by sanitiser
def title
read_attribute(:title).try(:html_safe)
Expand Down Expand Up @@ -304,4 +306,13 @@
def work_types
works.map(&:work_types).flatten.uniq
end

private

def admin_hidden_series_notification
return unless hidden_by_admin?

Check warning on line 313 in app/models/series.rb

View workflow job for this annotation

GitHub Actions / Rubocop 

[rubocop] reported by reviewdog 🐶
Add empty line after guard clause.

Raw Output:
app/models/series.rb:313:5: C: Layout/EmptyLineAfterGuardClause: Add empty line after guard clause.
users.each do |user|
UserMailer.send_series_hidden_notification(id, user.id).deliver_later
end
end
end
14 changes: 14 additions & 0 deletions app/views/user_mailer/admin_hidden_series_notification.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<% content_for :message do %>
<p><%= t("mailer.general.greeting.formal", name: style_bold(@user.login)).html_safe %></p>

<p><%= t(".html.hidden", title: style_creation_link(@series.title, @series)).html_safe %></p>
Copy link
Contributor

Choose a reason for hiding this comment

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

The html in the locale keys should be at the end. Then, the interpolated links are automatically treated as html safe and we do not need the potentially unsafe html_safe call:

  <p><%= t(".hidden.html", title: style_creation_link(@series.title, @series)) %></p>

This applies to all keys you added in this file here, as well as the text version, where the key should be ".hidden.text" and so on.

Furthermore, the variable name for links should end in _link. In this key it would also be nice to use a more specific variable name:

Suggested change
<p><%= t(".html.hidden", title: style_creation_link(@series.title, @series)).html_safe %></p>
<p><%= t(".hidden.html", series_link: style_creation_link(@series.title, @series)) %></p>


<p><%= t(".access") %></p>

<p><%= t(".check_email") %></p>

<p><%= t(".html.tos_violation", tos_link: tos_link(t ".tos")).html_safe %></p>

Check failure on line 10 in app/views/user_mailer/admin_hidden_series_notification.html.erb

View workflow job for this annotation

GitHub Actions / ERB Lint runner 

[] reported by reviewdog 🐶
Style/NestedParenthesizedCalls: Add parentheses to nested method call `t ".tos"`.

Raw Output:
app/views/user_mailer/admin_hidden_series_notification.html.erb:10:53: Style/NestedParenthesizedCalls: Add parentheses to nested method call `t ".tos"`.
Copy link
Contributor

Choose a reason for hiding this comment

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

The ideal configuration of parentheses would be this:

Suggested change
<p><%= t(".html.tos_violation", tos_link: tos_link(t ".tos")).html_safe %></p>
<p><%= t(".tos_violation.html", tos_link: tos_link(t(".tos"))) %></p>

Similar for the abuse link below.


<p><%= t(".html.help", contact_abuse_link: abuse_link(t ".contact_abuse")).html_safe %></p>

Check failure on line 12 in app/views/user_mailer/admin_hidden_series_notification.html.erb

View workflow job for this annotation

GitHub Actions / ERB Lint runner 

[] reported by reviewdog 🐶
Style/NestedParenthesizedCalls: Add parentheses to nested method call `t ".contact_abuse"`.

Raw Output:
app/views/user_mailer/admin_hidden_series_notification.html.erb:12:56: Style/NestedParenthesizedCalls: Add parentheses to nested method call `t ".contact_abuse"`.
<% end %>

Check failure on line 14 in app/views/user_mailer/admin_hidden_series_notification.html.erb

View workflow job for this annotation

GitHub Actions / ERB Lint runner 

[] reported by reviewdog 🐶
Remove multiple trailing newline at the end of the file.

Raw Output:
app/views/user_mailer/admin_hidden_series_notification.html.erb:14:0: Remove multiple trailing newline at the end of the file.
14 changes: 14 additions & 0 deletions app/views/user_mailer/admin_hidden_series_notification.text.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<% content_for :message do %>
<%= t("mailer.general.greeting.formal", name: @user.login) %>

<%= t(".text.hidden", title: @series.title, series_url: series_url(@series)) %>

<%= t(".access") %>

<%= t(".check_email") %>

<%= t(".text.tos_violation", tos_url: tos_url) %>

<%= t(".text.help", contact_abuse_url: new_abuse_report_url) %>
<% end %>

Loading
Loading