Feature.add additional email #4996
Conversation
|
@vbudhram If I try to |
…condary email (#1874) r=vladikoff This PR fixes mozilla/fxa-content-server#4996 (comment) by sending a unique error if a user is attempting to reset an account from a secondary email.
b80a6b8 to
6a6c5c6
Compare
|
The |
app/scripts/models/email.js
Outdated
|
|
||
| var Email = Backbone.Model.extend({ | ||
| defaults: { | ||
| email: undefined, |
There was a problem hiding this comment.
this needs to use null like other files in models/
| // Return a promise delayed by ms | ||
| function delay(ms) { | ||
| var deferred = p.defer(); | ||
| setTimeout(deferred.resolve, ms); |
There was a problem hiding this comment.
other files use this.setTimeout or use the timer-mixin. we need to add that
| }, | ||
|
|
||
| displaySuccess (msg) { | ||
| displaySuccess (msg, options = { closePanel: true}) { |
There was a problem hiding this comment.
nit : style near clonePanel
app/scripts/views/ready.js
Outdated
| headerTitle: t('Account verified'), | ||
| readyToSyncText: t('You are now ready to use %(serviceName)s') | ||
| } | ||
| }, |
There was a problem hiding this comment.
need to remove dangling comma
app/scripts/views/settings/emails.js
Outdated
| const account = this.getSignedInAccount(); | ||
| return account.resendEmailCode(email) | ||
| .then(() => { | ||
| this.displaySuccess(t('Verification emailed to ') + email, { |
There was a problem hiding this comment.
This is not gonna work, this needs to string interpolate the translate string
app/scripts/views/settings/emails.js
Outdated
| const account = this.getSignedInAccount(); | ||
| return account.recoveryEmailCreate(newEmail) | ||
| .then(() => { | ||
| this.displaySuccess(t('Verification emailed to ') + newEmail, { |
There was a problem hiding this comment.
This is not gonna work, this needs to string interpolate the translate string
|
Moved the comment up |
app/scripts/lib/auth-errors.js
Outdated
| errno: 136, | ||
| message: t('This email was already verified by another user') | ||
| }, | ||
| EMAIL_DELETE_PRIMARY: { |
There was a problem hiding this comment.
from mtg: we probably do not need this one
app/scripts/lib/auth-errors.js
Outdated
| }, | ||
| SESSION_UNVERIFIED: { | ||
| errno: 138, | ||
| message: t('Unable to perform action, unverified session') |
There was a problem hiding this comment.
also might not need this in the content-server, just like https://github.com/mozilla/fxa-content-server/pull/4996/files#r115769238 . At least we can remove the translation bit.
app/scripts/lib/auth-errors.js
Outdated
| }, | ||
| SECONDARY_EMAIL_UNKNOWN: { | ||
| errno: 143, | ||
| message: t('Cannot use this email to sign in') |
There was a problem hiding this comment.
we need to check this one, is this user facing? from mtg: it should probably say 'Primary account email required for sign-in'
There was a problem hiding this comment.
Nope, looks like it is not user facing. Will remove 👍
app/scripts/lib/auth-errors.js
Outdated
| }, | ||
| RESET_PASSWORD_WITH_SECONDARY_EMAIL: { | ||
| errno: 145, | ||
| message: t('Reset with this email type is not currently supported. Use the primary email of this account') |
There was a problem hiding this comment.
need to change: 'Primary account email required for reset'
[no ci]
…pen on unverified emails
…wouldn't load if feature disabled
90fd10d to
bc917b6
Compare
shane-tomlinson
left a comment
There was a problem hiding this comment.
A couple of small comments, nothing major.
| }, | ||
| EMAIL_VERIFIED_PRIMARY_EXISTS: { | ||
| errno: 140, | ||
| message: t('Account already exists') |
There was a problem hiding this comment.
Should this and the next error have different strings so we can differentiate in Sentry?
There was a problem hiding this comment.
Sentry has the errno AFAIK
| }, | ||
| VERIFIED_SECONDARY_EMAIL_EXISTS: { | ||
| errno: 144, | ||
| message: t('Address in use by another account') |
There was a problem hiding this comment.
What's the difference between this and # 136?
| <div class="address">{{email}}</div> | ||
| <div class="details"> | ||
| {{#verified}} | ||
| <div class="verified"> {{#t}}verified{{/t}}</div> |
There was a problem hiding this comment.
Do you want the extra space in front of the {?
app/scripts/views/settings/emails.js
Outdated
| this.displaySuccess(Strings.interpolate(t('Verification emailed to %(email)s'), { email: email }), { | ||
| closePanel: false | ||
| }); | ||
| this.render(); |
There was a problem hiding this comment.
Do you need to render if you are immediately navigating?
app/scripts/views/settings/emails.js
Outdated
| const account = this.getSignedInAccount(); | ||
| return account.resendEmailCode(email) | ||
| .then(() => { | ||
| this.displaySuccess(Strings.interpolate(t('Verification emailed to %(email)s'), { email: email }), { |
There was a problem hiding this comment.
I personally find this string slightly confusing.
There was a problem hiding this comment.
@shane-tomlinson what do you mean? the wording?
There was a problem hiding this comment.
@shane-tomlinson what do you mean? the wording?
Verification emailed to...
When sending a signup email, we say:
A verification link has been sent to %(escapedEmail)s
Fixes #4756
Reopening for docker stuff feature.add-additional-email branch name / tag
TODO
opened: Add secondary email signin confirmation and unblock code tests #5054
@mozilla, @softvision, @restmail)- echo "Tests totally passed ;)"in circle.ymlDONE
“Add” button should be blue in settings
The Secondary email section should always be open if the secondary email is not verified
The section should say “cancel” instead of done
Emails have wrong footer compared to the mock up from Ryan:
undefinedin email: in theverify secondary emailtemplate we have<a undefined>please change your password.</a>Customs configs on docker builds-> We need to disable customs on emails3.dev.lcip.org
IP_PROFILE need to disable it on emails3.dev.lcip.org
input placeholder is wrong in the secondary email section
In the email: please change password link doesn’t link. check footer of the email in the design. That link is not there.See AboveAlways enable “Refresh” button
Delete account didn’t delete the secondary email
DONE button should be grey if email is waiting to be verified.
Should reset password -> send to secondary?
remind Ryan F: "need to make an error for users trying to register a with secondary email" // review all new errors with Ryan F.
MINOR: verification link says “Damaged” when verifying after some other user took the email... Should it say "Expired" ?