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

Move email text to locale files #3383

Merged
merged 3 commits into from
Nov 13, 2018
Merged

Move email text to locale files #3383

merged 3 commits into from
Nov 13, 2018

Conversation

smithjp
Copy link
Contributor

@smithjp smithjp commented Nov 6, 2018

Fixes #3321

@samvera/hyrax-code-reviewers

@afred
Copy link
Contributor

afred commented Nov 7, 2018

@smithjp can we parameterize the :en so we're not hardcoding it when calling I18n.with_locale. That will make it easier to change once we have a mechanism for doing so. Thanks!

@smithjp
Copy link
Contributor Author

smithjp commented Nov 7, 2018

Would it make sense to set the locale in a hyrax config variable (in config/initializers/hyrax.rb)?

@jcoyne
Copy link
Member

jcoyne commented Nov 7, 2018

@smithjp no, the language is already set in I18n.locale, please use that.

@jcoyne
Copy link
Member

jcoyne commented Nov 7, 2018

I'm unclear why you need the I18n.with_locale(:en) call at all. Is there some reason not to use the currently set locale?

@smithjp
Copy link
Contributor Author

smithjp commented Nov 7, 2018

The feedback in #3321 indicated that the first step in having emails sent in a particular user's preferred language would be to move the email text to the locale files and always send in english. Further work will need to be done to either have emails sent in an application-wide preferred language or in the recipient's preferred language. Otherwise, emails may be sent based on a depositor's preferred language, which may not be a reviewer's preferred language.

@jcoyne
Copy link
Member

jcoyne commented Nov 7, 2018

@smithjp for now, the system default could be set to :fr, so it would be unexpected to send the emails in :en. You are doing extra work to make it send in :en, but I think just using the default behavior is preferable.

@no-reply
Copy link
Contributor

👍 to using the existing default I18n.t() values and considering that fulfillment of the first step in #3321.

Since that original discussion, I've learned that users already have a preferred_locale. An example usage is here:

I18n.t("hyrax.toolbar.notifications.zero", locale: locale_from_params || preferred_locale)

Copy link
Contributor

@no-reply no-reply left a comment

Choose a reason for hiding this comment

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

This looks like a great step. Thanks!

@no-reply no-reply merged commit 36a862f into master Nov 13, 2018
@no-reply no-reply deleted the 3321-email-text branch November 13, 2018 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants