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-5610 notify users when a bookmark or series they have created is hidden #4760

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

walshyb
Copy link
Contributor

@walshyb walshyb commented Mar 1, 2024

Pull Request Checklist

Issue

https://otwarchive.atlassian.net/browse/AO3-5610 (Please fill in issue number and remove this comment.)

Purpose

What does this PR do?

Testing Instructions

How can the Archive's QA team verify that this is working as you intended?

If you have a Jira account with access, please update or comment on the issue
with any new or missing testing instructions instead.

References

Are there other relevant issues/pull requests/mailing list discussions?

Credit

What name and pronouns should we use to credit you in the Archive of Our Own's Release Notes?

If you have a Jira account, please include the same name in the "Full name"
field on your Jira profile, so we can assign you the issues you're working on.

Please note that if you do not fill in this section, we will use your GitHub account name and
they/them pronouns.

@walshyb
Copy link
Contributor Author

walshyb commented Mar 1, 2024

I haven't used Rails + i18n before. Is there a way to generate the translations from the yaml i added to config/locales/mailers/en.yml?

@sarken
Copy link
Collaborator

sarken commented Mar 2, 2024

If you mean a way to, e.g., translate the English text into Spanish and insert it in es.yml, nope! All our translations are done by the wonderful human translators on our Translation team. They enter it in Phrase and then export it to GitHub when it's ready (pull request #4517 is an example).

Copy link
Contributor

@Bilka2 Bilka2 left a comment

Choose a reason for hiding this comment

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

I left some comments regarding the localisation of the email. There are so many notes because the admin_hidden_work_notification that you based this of was using some outdated I18n standards.

@@ -6,20 +6,20 @@ en:
expiration:
one: If you do not use this link to reset your password within %{count} day, it will expire, and you will have to request a new one.
other: If you do not use this link to reset your password within %{count} days, it will expire, and you will have to request a new one.
intro: 'Someone has requested a password reset for your account. You can change your account password by following the link below and entering your new password:'
intro: "Someone has requested a password reset for your account. You can change your account password by following the link below and entering your new password:"
Copy link
Contributor

@Bilka2 Bilka2 Apr 13, 2024

Choose a reason for hiding this comment

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

The style guideline regarding " quotation marks being preferred does not apply to the locale yml files for strings that contain colons :. The normalized files are expected to use ' for these strings.

You can see this because the i18n test for this file fails. To have it show up as a failed test here on github, merge master into your branch so that you get this bug fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

(You can normalize the file again with bundle exec i18n-tasks normalize -l en, from https://github.com/otwcode/otwarchive/wiki/Internationalization-%28i18n%29-Standards#i18n-tasks.)

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 ran bundle exec i18n-tasks normalize -l en # Normalize formatting of en.yml in my docker container and it didn't appear to change the file (in the container, nor in my local codebase).

I can make the changes manually, though. Just to be clear, all instances of " (double quotes) should be ' (single quotes)?

Thank you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually maybe that command fixed it because I just checked and most of the cases where " was used now it's '. So we may be good!

app/mailers/user_mailer.rb Show resolved Hide resolved
@Bilka2
Copy link
Contributor

Bilka2 commented Jun 29, 2024

Noting that besides the i18n review comments, the notification for bookmarks still needs to be added.

@walshyb walshyb marked this pull request as ready for review October 12, 2024 17:14
@walshyb
Copy link
Contributor Author

walshyb commented Oct 12, 2024

Hello!

I've made the following changes per the above comments:

  • Added an admin_hidden_series_notification mailer preview
  • Added an admin_hidden_bookmark_notification mailer action, lifecycle callback, view, preview, and locale stuff
  • Updated the respective locale content to fit the current standard

Questions:

  • If possible, would you like me to write explicit tests to ensure the mails are getting sent (or I guess added to the queue) when an admin hides the respective models?
  • Can you double check the verbiage used in the new mailers?
  • Did I follow all the i18n standards?

Thank you!

Copy link
Contributor

@Bilka2 Bilka2 left a comment

Choose a reason for hiding this comment

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

The additions look good, there are just some details and tests left.

If possible, would you like me to write explicit tests to ensure the mails are getting sent (or I guess added to the queue) when an admin hides the respective models?

That would be nice! That would have to be cucumber tests. It'd be even nicer if you could check that the emails end up getting translated in those tests, by hiding the bookmark/series belonging to two users where one of them has translated emails enabled. An example of a test for translated emails can be seen in #4921.

Furthermore, we usually add rspec tests for new emails, you can take a look at what was done for the admin hidden work notification or in the recent #4952.

One more thing, there is this note on Jira:

The footer should no longer say, “If you've received this message in error, please contact Support.”

The current email still says this. This footer text is defined in app/views/layouts/mailer.html.erb and app/views/layouts/mailer.text.erb. It needs to be suppressed for the emails here and admin_hidden_work_notification.

I noted the few remaining i18n problems in separate review comments.

@user = User.find_by(id: user_id)
@bookmark = Bookmark.find_by(id: creation_id)

I18n.with_locale(@user.preference.locale.iso) do
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be in the model where admin_hidden_bookmark_notification is called and surround that call there, to make the locale selector in the preview work.

Same for the other mail.

app/models/series.rb Outdated Show resolved Hide resolved
app/models/bookmark.rb Outdated Show resolved Hide resolved
<% content_for :message do %>
<%= t("mailer.general.greeting.formal_html", name: @user.login) %>

<%= t(".hidden.text", bookmark_url: bookmark_url(@bookmark)) %>
Copy link
Contributor

Choose a reason for hiding this comment

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

The text here should be Your bookmark of "<%= @bookmark.bookmarkable.title %>" (<%= bookmark_url(@bookmark) %>) has ... so something similar to what you did for the series.

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 did the same thing that I did for the series mailer. Is this new change good?

Copy link
Contributor

Choose a reason for hiding this comment

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

The text is good now, but the variable name is incorrect. I left a comment on the line in the yml file.

config/locales/mailers/en.yml Outdated Show resolved Hide resolved
test/mailers/previews/user_mailer_preview.rb Outdated Show resolved Hide resolved
test/mailers/previews/user_mailer_preview.rb Outdated Show resolved Hide resolved
walshyb and others added 9 commits January 10, 2025 14:49
Co-authored-by: Bilka <Bilka2@users.noreply.github.com>
Co-authored-by: Bilka <Bilka2@users.noreply.github.com>
Co-authored-by: Bilka <Bilka2@users.noreply.github.com>
…ve-created-is-hidden' of github.com:walshyb/otwarchive into AO3-5610-Notify-users-when-a-bookmark-or-series-they-have-created-is-hidden
html: If you are uncertain why your bookmark was hidden, and you have not received further communication regarding this matter, please %{contact_abuse_link} directly.
text: "If you are uncertain why your bookmark was hidden, and you have not received further communication regarding this matter, please contact Policy & Abuse directly: %{contact_abuse_url}."
hidden:
html: Your bookmark of %{title} has been hidden by the Policy & Abuse team and is no longer publicly accessible.
Copy link
Contributor

Choose a reason for hiding this comment

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

According to Jira, the entire "bookmark of X" text should be the link, so this needs to be split up:

Suggested change
html: Your bookmark of %{title} has been hidden by the Policy & Abuse team and is no longer publicly accessible.
bookmark_of: bookmark of %{title}
html: Your %{bookmark_of_link} has been hidden by the Policy & Abuse team and is no longer publicly accessible.

For why I named the link variable the way I did, refer to https://github.com/otwcode/otwarchive/wiki/Internationalization-%28i18n%29-Standards#hyperlinks

<% content_for :message do %>
<p><%= t("mailer.general.greeting.formal_html", name: style_bold(@user.login)) %></p>

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

Choose a reason for hiding this comment

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

And then to match the change in the yml file and also conform to this from Jira:

In the HTML email, the creation title inside the bookmark link should be bold and italicized.

Suggested change
<p><%= t(".hidden.html", title: style_creation_link(@bookmark.bookmarkable.title, @bookmark)) %></p>
<p><%= t(".hidden.html", bookmark_of_link: link_to(t(".hidden.bookmark_of", title: tag.em(tag.strong(@bookmark.bookmarkable.title))), @bookmark)) %></p>

text: "If you are uncertain why your bookmark was hidden, and you have not received further communication regarding this matter, please contact Policy & Abuse directly: %{contact_abuse_url}."
hidden:
html: Your bookmark of %{title} has been hidden by the Policy & Abuse team and is no longer publicly accessible.
text: Your bookmark of "%{title}" (%{bookmark_link}) has been hidden by the Policy & Abuse team and is no longer publicly accessible.
Copy link
Contributor

Choose a reason for hiding this comment

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

In the text version of the email this is a plain URL so the variable name should bookmark_url, not _link

(Needs the change in admin_hidden_bookmark_notification.text.erb too)

<% content_for :message do %>
<%= t("mailer.general.greeting.formal_html", name: @user.login) %>

<%= t(".hidden.text", bookmark_url: bookmark_url(@bookmark)) %>
Copy link
Contributor

Choose a reason for hiding this comment

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

The text is good now, but the variable name is incorrect. I left a comment on the line in the yml file.

Copy link
Contributor

@Bilka2 Bilka2 left a comment

Choose a reason for hiding this comment

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

I think an autoformatter decided to change all the quotes in the yml file, please normalize the file again.

To make tracking a bit easier, still missing from my previous review: Cucumber and rspec tests, moving the I18n.with_locale call, removing the notice to contact Support from the footer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants