AO3-4885 i18n challenge assignment #2773
Conversation
sarken
approved these changes
Mar 5, 2017
This looks good! It might be a good idea to add And the email should not contain "translation missing" in the test, though, just to be extra careful.
sarken
added Reviewed: Ready to Merge i18n
labels
Mar 5, 2017
|
Good idea, done. |
|
Now that we've merged the invitation email, this has merge conflicts that need to be resolved. |
sarken
added Reviewed: Needs Coder Action and removed Reviewed: Ready to Merge
labels
Apr 25, 2017
|
Merge conflicts sorted. |
sarken
requested changes
Apr 28, 2017
The merge conflict resolution looks good, but I'm afraid I've found a couple instances of HTML running around naked that need to be fixed.
| - <p> | ||
| - You have been assigned the following request in the <%= style_link(@collection.title, collection_url(@collection)) %> challenge at the Archive of Our Own! | ||
| - </p> | ||
| + <p><%= t ".html.part1", link: style_link(@collection.title, collection_url(@collection)) %></p> |
sarken
Apr 28, 2017
Owner
I took a quick look at this PR in my test logs, and this link appears to give exposed HTML: You have been assigned the following request in the <a href="http://archiveofourown.org/collections/Awesome_Gift_Exchange" style="color:#990000">Awesome Gift Exchange</a> challenge at the Archive of Our Own!
| - <p> | ||
| - You can look up this assignment from <%= style_link("your Assignments page", user_assignments_url(@assigned_user)) %>. | ||
| - </p> | ||
| + <p><%= t ".html.look_up", link: style_link(t(".html.look_up_link"), user_assignments_url(@assigned_user)) %></p> |
sarken
Apr 28, 2017
Owner
This also appears to be giving exposed HTML: You can look up this assignment from <a href="http://archiveofourown.org/users/myname4/assignments" style="color:#990000">your Assignments page</a>.
| @@ -197,4 +197,53 @@ | ||
| end | ||
| end | ||
| end | ||
| + | ||
| + describe "challenge assignment" do | ||
| + |
sarken
requested changes
May 1, 2017
It looks like there are still some merge conflicts on this as well.
| @@ -247,6 +247,26 @@ en: | ||
| para1: "The following abuse report has been sent to the Abuse team:" | ||
| url_para: "URL of the page the abuse is on:" | ||
| description_para: "Description of the abuse:" | ||
| + challenge_assignment_notification: | ||
| + subject: "[%{app_name}] [%{collection_title}] Your Assignment!" |
sarken
May 1, 2017
Owner
This didn't originally have a space between "[AO3][Collection Title]": "[#{ArchiveConfig.APP_SHORT_NAME}][#{@collection.title}] Your Assignment!"
Do the other collection emails have a space there?
cesy
May 1, 2017
Contributor
You're right, the challenge ones don't - I got confused by the other user mailer ones that do.
| + text: | ||
| + part1: "You have been assigned the following request in the %{collection_title} (%{collection_url}) challenge at the Archive of Our Own!" | ||
| + look_up: "You can look up this assignment from your Assignments page at %{link}." | ||
| + footer: "You're receiving this email because you signed up for the %{title} challenge (%{url}). For more information about this challenge and contact information for the moderators, please see %{profile_url}." |
sarken
May 1, 2017
Owner
We can make a separate issue for this if you prefer, but I just noticed the text email says "please see" while the HTML one says "please visit," which is probably better phrasing.
| @@ -220,7 +220,7 @@ def challenge_assignment_notification(collection_id, assigned_user_id, assignmen | ||
| @request = (assignment.request_signup || assignment.pinch_request_signup) | ||
| mail( | ||
| to: @assigned_user.email, | ||
| - subject: "[#{ArchiveConfig.APP_SHORT_NAME}][#{@collection.title}] Your Assignment!" | ||
| + subject: t("user_mailer.challenge_assignment_notification.subject", app_name: ArchiveConfig.APP_SHORT_NAME, collection_title: @collection.title).to_s |
sarken
May 1, 2017
Owner
I don't think we usually need to_s on the end -- did this not work without it?
| + let(:email) { UserMailer.challenge_assignment_notification(collection.id, otheruser.id, open_assignment.id).deliver } | ||
| + | ||
| + # Test the headers | ||
| + it 'should have a valid from line' do |
sarken
May 1, 2017
Owner
Not super important because the rest of this file seems to have it, but just an fyi: "should" isn't really necessary -- writing it like "has a valid from line" helps keeps things shorter.
cesy
May 1, 2017
Contributor
Easy enough to fix while I'm in the file :) and makes it easier to read in the future.
| + end | ||
| + | ||
| + describe "challenge assignment" do | ||
| + |
| @@ -245,4 +245,53 @@ | ||
| end | ||
| end | ||
| end | ||
| + | ||
| + describe "challenge assignment" do | ||
| + |
|
This should now be working and ready for another review. |
cesy commentedMar 4, 2017
https://otwarchive.atlassian.net/browse/AO3-4885
Internationalise the challenge assignment email
Testing
Check that the email you get with your assignment in a gift exchange is still readable in English. Check for "any" character etc. as well as correct pluralisation of e.g. "Fandom: SG-1" vs. "Fandoms: SG-1, SGA".
References
#2286
Credit
Cesy, she