Skip to content
This repository has been archived by the owner. It is now read-only.

refactor(templates): Add more location data to emails #243

Merged
merged 3 commits into from Dec 23, 2016

Conversation

@vbudhram
Copy link
Contributor

@vbudhram vbudhram commented Dec 21, 2016

This PR adds adds a new verify account sync template. This does not change the verify account email for non-sync users. It also adds location data to

  • verify sync account email
  • password reset email
  • password change success email

It also contains some slight text tweaks to the emails.

Fixes #189

@vbudhram vbudhram added the WIP label Dec 21, 2016
@vbudhram vbudhram added this to the FxA-0: quality milestone Dec 21, 2016
@vbudhram vbudhram self-assigned this Dec 21, 2016
{{#if timestamp }}<span style="display:block;margin:2px 0;">{{ timestamp }}</span>{{/if}}


</p>

This comment has been minimized.

@vladikoff

vladikoff Dec 21, 2016
Contributor

do we need the 2 line breaks here in these?

This comment has been minimized.

@vbudhram

vbudhram Dec 21, 2016
Author Contributor

This is strange, these files are generated from grunt templates. I'll check to see what might be causing the extra breaks.

@vladikoff
Copy link
Contributor

@vladikoff vladikoff commented Dec 21, 2016

Fixes #189

{{ device }}
{{#if location}}{{ location }}{{/if}}
{{#if ip}}{{t "IP address: %(ip)s" }}{{/if}}
{{#if timestamp}}{{ timestamp }}{{/if}}

This comment has been minimized.

@vladikoff

vladikoff Dec 21, 2016
Contributor

let's not put the timestamp in the verify email because @ryanfeeley does not have that one in the template. Other 2 emails should have it though

{{#if ip}}{{t "IP address: %(ip)s" }}{{/if}}
{{#if timestamp}}{{ timestamp }}{{/if}}

{{t "Verify email →"}} {{{link}}}

This comment has been minimized.

@vladikoff

vladikoff Dec 21, 2016
Contributor

should this use an HTML right arrow &rarr; ?

@@ -35,7 +42,7 @@ <h1 style="font-family: sans-serif; font-weight: normal; margin: 0 0 24px 0; tex
<w:anchorlock/>
<center>
<![endif]-->
<a href="{{{link}}}" id="button-link" style="font-family:sans-serif; color: #fff; display: block; padding: 15px; text-decoration: none; width: 280px;">{{t "Activate now"}}</a>
<a href="{{{link}}}" id="button-link" style="font-family:sans-serif; color: #fff; display: block; padding: 15px; text-decoration: none; width: 280px;">{{t "Verify email →"}}</a>
<h1 style="font-family: sans-serif; font-weight: normal; margin: 0 0 24px 0; text-align: center;"><% block verify_title %>{{t "Ready, set sync"}}<% endblock %></h1>
<p class="primary" style="font-family: sans-serif; font-size: 14px; font-weight: normal; margin: 0 0 24px 0; text-align: center;"><% block verify_content %>{{t "Confirm you’ve received this email and we’ll help you install and sync Firefox on all your devices starting with:"}}<br/><br/>
<% include "./location/location.html" %>
<% endblock %></p>

This comment has been minimized.

@vladikoff

vladikoff Dec 21, 2016
Contributor

the <p> tag seems not closed

@vbudhram vbudhram changed the title refactor(templates): Add to more emails refactor(templates): Add more location data to emails Dec 21, 2016
@@ -11,7 +11,7 @@
<!--Logo-->
<tr style="page-break-before: always">
<td align="center" id="firefox-logo" style="padding: 20px 0;">
<img src="http://image.e.mozilla.org/lib/fe9915707361037e75/m/2/fxlogojg.gif" height="95" width="88" alt="" style="-ms-interpolation-mode: bicubic;" />
<img src="http://v14d.com/g/choose_what_to_sync_devices2x.jpg" height="152" width="310" alt="" style="-ms-interpolation-mode: bicubic;" />

This comment has been minimized.

@vladikoff

vladikoff Dec 21, 2016
Contributor

let's not forget to update that ;D

This comment has been minimized.

@vbudhram

vbudhram Dec 21, 2016
Author Contributor

Should this be updated before merge or could I file a follow up issue for it?

@vbudhram
Copy link
Contributor Author

@vbudhram vbudhram commented Dec 21, 2016

@ryanfeeley Here are the rendered out updates to the emails. Am I missing anything?

Password reset requested:
screen shot 2016-12-21 at 4 40 37 pm

Verify Sync Account:
screen shot 2016-12-21 at 4 39 59 pm

Password change completed:
screen shot 2016-12-21 at 4 40 19 pm

<img src="http://image.e.mozilla.org/lib/fe9915707361037e75/m/2/fxlogojg.gif" height="95" width="88" alt="" style="-ms-interpolation-mode: bicubic;" />
{{/if}}
{{#if sync}}
<img src="http://v14d.com/g/choose_what_to_sync_devices2x.jpg" height="152" width="310" alt="" style="-ms-interpolation-mode: bicubic;" />
@vbudhram vbudhram force-pushed the issue-189-add-location-to-more-emails branch from 3114e16 to 50e80aa Dec 22, 2016
@vbudhram
Copy link
Contributor Author

@vbudhram vbudhram commented Dec 22, 2016

@vladikoff Updated image and ran email on acid for new sync template, https://www.emailonacid.com/app/acidtest/display/summary/8tugyvPCXUR8ugpAaxuXcH4ZAfKjA6cDBPkdevhdhzQqC. Is there anything else I am missing, r?

return this.send({
acceptLanguage: message.acceptLanguage,
email: message.email,
headers: headers,
subject: gettext('Verify your Firefox Account'),
subject: gettext(subject),

This comment has been minimized.

@vladikoff

vladikoff Dec 23, 2016
Contributor

@vbudhram this gettext(subject) will not work for string extraction, the strings need to be written out as gettext('Stuff here') in the code

<img src="http://image.e.mozilla.org/lib/fe9915707361037e75/m/2/fxlogojg.gif" height="95" width="88" alt="" style="-ms-interpolation-mode: bicubic;" />
{{/if}}
{{#if sync}}
<img src="https://image.e.mozilla.org/lib/fe9915707361037e75/m/3/fxa_dec2016.jpeg" height="152" width="310" alt="" style="-ms-interpolation-mode: bicubic;" />

This comment has been minimized.

@vladikoff

vladikoff Dec 23, 2016
Contributor

yessss!

@@ -259,7 +259,7 @@ module.exports = function (log) {
log.trace({ op: 'mailer.verifyEmail', email: message.email, uid: message.uid })

var templateName = 'verifyEmail'
var subject = 'Verify your Firefox Account'
var subject = gettext('Verify your Firefox Account')

This comment has been minimized.

@vbudhram

vbudhram Dec 23, 2016
Author Contributor

👍

'unblockCodeEmail',
'recoveryEmail',
'verifyEmail',

This comment has been minimized.

@vladikoff

vladikoff Dec 23, 2016
Contributor

@vbudhram should the new "Verify Sync" template be in this array?

This comment has been minimized.

@vladikoff

vladikoff Dec 23, 2016
Contributor

from irc: no, same template, depends on service

@vladikoff vladikoff merged commit 1aa0464 into master Dec 23, 2016
3 checks passed
3 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 98.008%
Details
@vladikoff
Copy link
Contributor

@vladikoff vladikoff commented Dec 23, 2016

New Templates are Good 👍

@vladikoff vladikoff deleted the issue-189-add-location-to-more-emails branch Dec 23, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants