feat(brokers): Add brokers for mob_android_v1 and mob_ios_v1
#5084
Conversation
shane-tomlinson
force-pushed
the
issue-5029-mobile-lib-contexts
branch
from
e1d8d25
to
41c70b6
Compare
shane-tomlinson
force-pushed
the
issue-5029-mobile-lib-contexts
branch
4 times, most recently
from
308428a
to
7d75dec
Compare
shane-tomlinson
force-pushed
the
issue-5029-mobile-lib-contexts
branch
from
569a01c
to
a8142d4
Compare
shane-tomlinson
changed the title
mob_android_v1 and mob_ios_v1mob_android_v1 and mob_ios_v1
| return broker.afterSignInConfirmationPoll(account) | ||
| .then((result) => { | ||
| assert.isTrue(channelMock.send.calledOnce); | ||
| assert.isTrue(channelMock.send.calledWith('email_verified')); |
There was a problem hiding this comment.
Ensure login data is sent too.
| return broker.afterSignUpConfirmationPoll(account) | ||
| .then((result) => { | ||
| assert.isTrue(channelMock.send.calledOnce); | ||
| assert.isTrue(channelMock.send.calledWith('email_verified')); |
There was a problem hiding this comment.
Ensure login data is sent too.
shane-tomlinson
force-pushed
the
issue-5029-mobile-lib-contexts
branch
from
b84b7ad
to
e00f8da
Compare
| @@ -40,7 +41,9 @@ define(function (require, exports, module) { | |||
| commands: null, | |||
|
|
|||
| defaultCapabilities: _.extend({}, proto.defaultCapabilities, { | |||
| sendChangePasswordNotice: true | |||
| sendAfterSignInConfirmationPollNotice: false, | |||
| sendAfterSignUpConfirmationPollNotice: false, | |||
There was a problem hiding this comment.
A spot of bike-shedding here with my "ubiquitous language" hat on:
I thought we named sign-in confirmation to differentiate it from the email verification step that happens after sign-up. Are we calling them both "confirmation" everywhere now?
There was a problem hiding this comment.
No, this is the semantics of the content server. "Confirm" occurs in the tab the user initiated the action. "verify" occurs in the verification tab.
There was a problem hiding this comment.
The above comment is not to mean that the semantics the content server uses are not totally messed up and confusing. The are both. I'd love help renaming these methods/concepts internally, every person that comes into the project is confused by the names & corresponding URL scheme.
| .then((behavior) => { | ||
| if (this.hasCapability('sendAfterSignInConfirmationPollNotice')) { | ||
| const loginData = this._getLoginData(account); | ||
| return this.send(this.getCommand('EMAIL_VERIFIED'), loginData) |
There was a problem hiding this comment.
Once upon a time we had plans to replace the current sign-in confirmation flow with other approaches, Google Authenticator etc. Would this command name seem weird if/when that happens?
There was a problem hiding this comment.
Once upon a time we had plans to replace the current sign-in confirmation flow with other approaches, Google Authenticator etc. Would this command name seem weird if/when that happens?
Possibly. We could just call it "VERIFIED" and ignore the verification method.
There was a problem hiding this comment.
Pulling over the comment from the issue:
@mcomella - if we changed the WebChannel message from fxaccounts:email_verified to fxaccounts:verified, would that cause havoc for you?
There was a problem hiding this comment.
@mcomella says "@shane-tomlinson Nope! Go for it."
|
LGTM! r+ |
* Add lots of unit and functional tests. * Add docs.
To force signin confirmation, the email needs a `sync` prefix.
shane-tomlinson
force-pushed
the
issue-5029-mobile-lib-contexts
branch
from
e00f8da
to
45fe4e5
Compare
fixes #5029