fix(email): Add ability for content server to delegate sending emails #4155
Conversation
… to the auth-server
|
|
||
| if (! account.get('verified') && isSignUp) { | ||
| // Only send account verification email, it signing up |
shane-tomlinson
Sep 16, 2016
Member
it signing => if signing
it signing => if signing
| beforeEach(function () { | ||
| account = user.initAccount({ email: 'email', uid: 'uid' }); | ||
| // Ensure that content-server can correctly handle any emailSent response | ||
| var emailSentTestCases = [{ |
shane-tomlinson
Sep 16, 2016
Member
Can you use const instead of var here? We're living in the (mostly) present in the content server!
Can you use const instead of var here? We're living in the (mostly) present in the content server!
| } else if (property === 'verificationReason') { | ||
| return VerificationReasons.SIGN_UP; | ||
| } | ||
| sinon.stub(account, 'get', function (property) { |
shane-tomlinson
Sep 16, 2016
Member
Is there a reason to use a stub and not a simple set on the account?
account.set({
emailSent: testCase.emailSent,
verificationReason: VerificationReasons.SIGN_UP,
verified: false
});
I don't see the stub checked for whether it's called, so I'm not sure overriding it provides any value.
Is there a reason to use a stub and not a simple set on the account?
account.set({
emailSent: testCase.emailSent,
verificationReason: VerificationReasons.SIGN_UP,
verified: false
});I don't see the stub checked for whether it's called, so I'm not sure overriding it provides any value.
shane-tomlinson
Sep 16, 2016
Member
It seems like in your account.signIn stub, you should set these values to mimic real life.
It seems like in your account.signIn stub, you should set these values to mimic real life.
| @@ -360,8 +360,11 @@ define(function (require, exports, module) { | |||
| .then(function () { | |||
| var isSignUp = | |||
| account.get('verificationReason') === VerificationReasons.SIGN_UP; | |||
| var emailSent = !! account.get('emailSent'); | |||
shane-tomlinson
Sep 16, 2016
Member
Might as well const these two variables too.
Might as well const these two variables too.
| if (! account.get('verified') && isSignUp) { | ||
| // Only send account verification email, it signing up | ||
| // and a verification email has not been sent by auth-server. | ||
| if (! account.get('verified') && isSignUp && ! emailSent) { |
shane-tomlinson
Sep 16, 2016
Member
Are you going to send the sendEmailIfUnverified directly from the fxa-js-client? It'd probably be best to only use the fxa-js-client as a pipe that transfers sendEmailIfUnverified from the fxa-js-client consumer to the auth server to avoid making an assumption about who is consuming the client, we don't want to inadvertently break anyone.
Are you going to send the sendEmailIfUnverified directly from the fxa-js-client? It'd probably be best to only use the fxa-js-client as a pipe that transfers sendEmailIfUnverified from the fxa-js-client consumer to the auth server to avoid making an assumption about who is consuming the client, we don't want to inadvertently break anyone.
vbudhram
Sep 16, 2016
Author
Contributor
That was my original plan, but decided against it for that reason, the js-client sets the default of sendEmailIfUnverified=false if it is not passed. Will update so that value gets propagated to client.
That was my original plan, but decided against it for that reason, the js-client sets the default of sendEmailIfUnverified=false if it is not passed. Will update so that value gets propagated to client.
|
|
||
| sinon.spy(notifier, 'triggerRemote'); | ||
| sinon.stub(user, 'setSignedInAccount', function () { |
shane-tomlinson
Sep 16, 2016
Member
Is it possible to use a spy instead of a stub here? setSignedInAccount doesn't seem like it has any side effects that should cause problems elsewhere.
Is it possible to use a spy instead of a stub here? setSignedInAccount doesn't seem like it has any side effects that should cause problems elsewhere.
| if (! account.get('verified') && isSignUp) { | ||
| // Only send account verification email, it signing up | ||
| // and a verification email has not been sent by auth-server. | ||
| if (! account.get('verified') && isSignUp && ! emailSent) { |
shane-tomlinson
Sep 16, 2016
Member
Can you update the comment that once we are reasonably sure all consumers delegate email sending to the auth-server, this branch can be removed?
Can you update the comment that once we are reasonably sure all consumers delegate email sending to the auth-server, this branch can be removed?
|
This PR looks a lot like what I hacked together to test, which is good! Most of the comments are minor nits. The one major change I'd make is to have fxa-client.signIn send the new query parameter the fxa-js-client.signIn to avoid making assumptions about who is consuming the fxa-js-client as well as prevent spreading that branching knowledge around too much. Thanks @vbudhram! |
| } else if (property === 'verificationReason') { | ||
| return VerificationReasons.SIGN_UP; | ||
| } | ||
| sinon.stub(account, 'get', function (property) { |
shane-tomlinson
Sep 16, 2016
Member
It seems like in your account.signIn stub, you should set these values to mimic real life.
It seems like in your account.signIn stub, you should set these values to mimic real life.
|
@shane-tomlinson Updated from review, sending back to you! |
|
LGTM, once the fxa-js-client merges and we can run the full functional test suite, let's test and merge! |
|
Needs a new fxa-js-client |
|
Thanks! @shane-tomlinson @vladikoff |
Fixes #4105