New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add some brokers #1609

Merged
merged 5 commits into from Nov 4, 2014

Conversation

Projects
None yet
7 participants
@shane-tomlinson
Member

shane-tomlinson commented Aug 29, 2014

This is for @nchapman, @zaach, @vladikoff and @ckarlof. These are the first brokers, see what you think!

feat(client): Add some brokers!

  • Start to add brokers that know how to startup/complete flows.
  • 3 brokers to start, fx-desktop, redirect, web-channel.
  • Get rid of the Channels high level abstraction, these are used directly by the brokers if needed.
  • Move some code from service-mixin and various channels to brokers.
  • Update tests to make use of the brokers.
initializeBroker: function () {
if (this._searchParam('context') === 'fx_desktop_v1') {

This comment has been minimized.

@vladikoff

vladikoff Aug 29, 2014

Member

Didn't we have a constant for this?

This comment has been minimized.

@pdehaan

pdehaan Aug 29, 2014

Contributor

This seems awfully specific. Can we tweak this to use the Constants.FX_DESKTOP_CONTEXT const?

$ grep -irn "fx_desktop_v1" . | grep -v "./\(dist\|.tmp\)/"
./app/scripts/lib/constants.js:15:    FX_DESKTOP_CONTEXT: 'fx_desktop_v1',
./tests/server/metrics-collector-stderr.js:60:      assert.equal(loggedMetrics.context, 'fx_desktop_v1');
./tests/server/metrics-collector-stderr.js:89:      context: 'fx_desktop_v1',
./tests/tools/firefox_profile_creator.js:26:  myProfile.setPreference('identity.fxaccounts.remote.force_auth.uri', profile.fxaContentRoot + 'force_auth?service=sync&context=fx_desktop_v1');
./tests/tools/firefox_profile_creator.js:27:  myProfile.setPreference('identity.fxaccounts.remote.signin.uri', profile.fxaContentRoot + 'signin?service=sync&context=fx_desktop_v1');
./tests/tools/firefox_profile_creator.js:28:  myProfile.setPreference('identity.fxaccounts.remote.signup.uri', profile.fxaContentRoot + 'signup?service=sync&context=fx_desktop_v1');

$ grep -irn "Constants.FX_DESKTOP_CONTEXT" . | grep -v "./\(dist\|.tmp\)/"
./app/scripts/lib/session.js:140:      return this.get('context') === Constants.FX_DESKTOP_CONTEXT;
./app/scripts/views/settings.js:28:        showSignOut: Session.get('sessionTokenContext') !== Constants.FX_DESKTOP_CONTEXT
./app/scripts/views/sign_in.js:106:      if (Session.get('context') !== Constants.FX_DESKTOP_CONTEXT) {
./app/tests/spec/lib/app-start.js:29:      return '?context=' + Constants.FX_DESKTOP_CONTEXT;
./app/tests/spec/lib/router.js:67:        windowMock.location.search = '?context=' + Constants.FX_DESKTOP_CONTEXT;
./app/tests/spec/lib/router.js:69:        assert.equal(navigateUrl, '/forgot?context=' + Constants.FX_DESKTOP_CONTEXT);
./app/tests/spec/views/settings.js:78:          Session.set('sessionTokenContext', Constants.FX_DESKTOP_CONTEXT);
./tests/functional/settings.js:80:        .get(require.toUrl(SIGNIN_URL + '?context=' + Constants.FX_DESKTOP_CONTEXT))

This comment has been minimized.

@pdehaan

pdehaan Aug 29, 2014

Contributor

Plus, now looking through those beautiful greps, not sure if it would help to have some isDesktopContext() helper function since it seems like we're doing some variation of this in at least 3 places now:

if (this._searchParam('context') === 'fx_desktop_v1') {...}

Actually, it looks like we have one in /app/scripts/lib/session.js:139:

    isDesktopContext: function () {
      return this.get('context') === Constants.FX_DESKTOP_CONTEXT;
    },

This comment has been minimized.

@shane-tomlinson

shane-tomlinson Aug 29, 2014

Member

So, the idea is that the broker knows that it is in the desktop context and everything else has no need to know, and isDesktopContext can be ripped out of Session. I'll update to use the named string from Constants.

},
/**
* Initialize the Session. Perhaps should be merged into `fetch`

This comment has been minimized.

@pdehaan

pdehaan Aug 29, 2014

Contributor

Perhaps should be merged into fetch

My eyes say "docs comment", but my heart almost says "TODO".

ಠ_ಠ

This comment has been minimized.

@shane-tomlinson

shane-tomlinson Aug 29, 2014

Member

Good catch.

beforeEach(function () {
Session.clear();
email = 'testuser.' + Math.random() + '@testuser.com';

This comment has been minimized.

@pdehaan

pdehaan Aug 29, 2014

Contributor

Should be:

email = TestHelpers.createEmail();

See #1608

view.remove();
view.destroy();
document.cookie = 'tooyoung=1; expires=Thu, 01-Jan-1970 00:00:01 GMT';

This comment has been minimized.

@pdehaan

pdehaan Aug 29, 2014

Contributor

This looks very similar to line 439 above. Not sure if we should move this hardcoded cookie string somewhere higher into a constant or helper somewhere.

$ grep -irn "tooyoung=1" app

Results:

  1. app/scripts/views/clear_storage.js:24:

    document.cookie = 'tooyoung=1; expires=Thu, 01-Jan-1970 00:00:01 GMT';
  2. app/scripts/views/sign_up.js:173:

    document.cookie = 'tooyoung=1;';
  3. app/tests/spec/views/sign_up.js:53:

    document.cookie = 'tooyoung=1; expires=Thu, 01-Jan-1970 00:00:01 GMT';
  4. app/tests/spec/views/sign_up.js:76:

    document.cookie = 'tooyoung=1; expires=Thu, 01-Jan-1970 00:00:01 GMT';

Something like TOO_YOUNG_COOKIE = 'tooyoung=1; expires=Thu, 01-Jan-1970 00:00:01 GMT'; in /app/scripts/lib/constants.js and then in the src we could just use it as document.cookie = Constants.TOO_YOUNG_COOKIE; (or whatever).

[200, { 'Content-Type': 'application/json' }, JSON.stringify({ verfied: false })]);
var nowYear = (new Date()).getFullYear();
fillOutSignUp(email, 'password', { year: nowYear - 14, context: view });

This comment has been minimized.

@pdehaan

pdehaan Aug 29, 2014

Contributor

This also strikes me as a bit weird (all this nowYear - 14 and nowYear - 13 business in each test).
Not sure if we want to do this as constants at the top of the file or something. I counted 10 instances of var nowYear in this file alone.

(Probably out of the scope of this PR, but the eyes see what the eyes see.)

@shane-tomlinson shane-tomlinson force-pushed the add-a-broker branch 2 times, most recently from 2181d6d to dabf10e Sep 8, 2014

@@ -110,10 +115,26 @@ function (
},
initializeRelier: function () {
this._relier = new Relier();
this._relier = new Relier({

This comment has been minimized.

@shane-tomlinson

shane-tomlinson Sep 8, 2014

Member

Perhaps we should be able to pass in both a Relier and a Broker on object creation for testing.

this._window = options.window || window;
this._channel = new FxDesktopChannel();
this._channel.init({

This comment has been minimized.

@shane-tomlinson

shane-tomlinson Sep 8, 2014

Member

Probably not for this PR, but I'm starting to be confused by a mixture of startup paradigms. In some places our objects use init, in others initialize. When an object follows the Backbone conventions, it uses initialize and configuration is passed to the constructor instead of in a separate call to init.

init is my fault. We should probably standardize on initialize to make our lives easier.

@shane-tomlinson shane-tomlinson force-pushed the add-a-broker branch from 13ba35e to 4151dac Sep 8, 2014

@nchapman nchapman self-assigned this Sep 8, 2014

@nchapman nchapman modified the milestones: train-21 (Sep 8), train-22 (Sep 22) Sep 8, 2014

@pdehaan

This comment has been minimized.

Contributor

pdehaan commented Sep 8, 2014

Do we need to tweak the PR title for the benefit of our changelog?

@shane-tomlinson

This comment has been minimized.

Member

shane-tomlinson commented Sep 8, 2014

Do we need to tweak the PR title for the benefit of our changelog?

@pdehaan - ya. I'll do that once we move a bit further through this.

@ckarlof ckarlof added in review and removed in review labels Sep 10, 2014

@shane-tomlinson shane-tomlinson force-pushed the add-a-broker branch from 4151dac to 4bf50b2 Sep 15, 2014

@shane-tomlinson

This comment has been minimized.

Member

shane-tomlinson commented Sep 15, 2014

Remaining work:

  • - Do not stop app from starting if response to session_context is not received. This will maintain backwards compatibility.
'use strict';
define([

This comment has been minimized.

@shane-tomlinson

shane-tomlinson Sep 15, 2014

Member

This isn't used yet, so get rid of it.

@coveralls

This comment has been minimized.

coveralls commented Sep 15, 2014

Coverage Status

Coverage decreased (-0.06%) when pulling 4bf50b2 on add-a-broker into a23ca63 on master.

},
_isForceAuth: function () {
// XXX is this the correct place for this?

This comment has been minimized.

@nchapman

nchapman Sep 17, 2014

Member

It probably depends where all we need to answer this question. If it's only in this module, then this is probably fine, but it seems like we might need to answer the same question in different contexts.

_isForceAuth: function () {
// XXX is this the correct place for this?
return this._window.location.pathname === '/force_auth';

This comment has been minimized.

@nchapman

nchapman Sep 17, 2014

Member

I'm thinking it would be nice to figure out what kinds of data Brokers need and pass those into the constructor instead of it having to reach into window for random bits of data.

Session.clear();
// XXX does this go here, or into the Relier?
// forceAuth is only used for the FxDesktop Sync flow.
Session.set('forceAuth', true);

This comment has been minimized.

@nchapman

nchapman Sep 17, 2014

Member

What ends up consuming this from the Session?. It would be ideal if the consumer could query the Broker directly.

/**
* Transform the signin/signup links if necessary
*/
transformLink: function (link) {

This comment has been minimized.

@ckarlof

ckarlof Oct 30, 2014

Contributor

I prefer decorate vs transform here, personally.

@ckarlof

This comment has been minimized.

Contributor

ckarlof commented Oct 30, 2014

@shane-tomlinson, this needs a rebase.

It's looking good.

I ideally want to finishing reviewing this tomorrow, but I have meetings from 9am-5pm. I hopefully can get to it at 5pm. Otherwise, I'll fight to reclaim time! Friday I'm off, so I hope this doesn't drag on to next week.

@shane-tomlinson shane-tomlinson force-pushed the add-a-broker branch 2 times, most recently from b4b0292 to be2bd87 Oct 30, 2014

submit: function () {
var self = this;
return p().then(function () {
if (self.isOAuthSameBrowser()) {

This comment has been minimized.

@shane-tomlinson

shane-tomlinson Oct 30, 2014

Member

Check whether the Loop flow should finish the OAuth flow in the verification tab if the user closes/replaces the original tab. If so, we may need to re-introduce the afterSignUpVerified and afterResetPasswordVerified (or similar) functions to the brokers and let the brokers coordinate as they see fit.

shane-tomlinson added some commits Oct 31, 2014

feat(test): Add functional test for web channel flow when user signs …
…up, closes original tab, and opens the verification link in the same browser.

This is to add additional coverage to prepare for the add-a-broker branch.

Additional work:
* Reduce duplication in the web channel tests by calling the fillOutSignUp and fillOutSignIn FunctionalHelpers.
feat(client): Add some brokers!
* Start to add brokers that know how to startup/complete flows.
* 5 brokers to start, base, oauth, fx-desktop, redirect, web-channel.
* Get rid of the Channels high level abstraction, these are used directly by the brokers if needed.
* Move some code from service-mixin and various channels to brokers.
* Update tests to make use of the brokers.
* No longer save forceEmail to Session, get it from the relier.
* Remove the `signIn` from `completeResetPassword` in the FxaClient, this is now done in the views.
* No longer call `signIn` from `changePassword` in the FxaClient, this is now handled by the views. This allows the signature of `changePassword` to be much smaller.
* Move the re-link check from fxa-client to the views to allow finer grained control over when they are presented.
* Remove Session.forceAuth handling from fxa-client.js, this is now stored in relier.email
* Create a search-params mixin in models/mixins.
* Migrate signin/signup/verification finishing code from service-mixin to the brokers so each broker can do their own thing.
* Really shrink service-mixin to a bare bare minimum, fix the unit tests, make sure all the current functional tests pass.
* Extract a ChannelMixin for use by the brokers.
fix(client): Ensure the Loop initiated reset password verification fl…
…ow sends OAuth credentials after the reset password is complete when the original tab is closed.

fixes #1825
*
* @return {promise}
*/
afterResetPasswordConfirmationPoll: function () {

This comment has been minimized.

@ckarlof

ckarlof Nov 4, 2014

Contributor

Possible we can get rid of this: #1838

*
* @return {promise}
*/
afterSignUpConfirmationPoll: function () {

This comment has been minimized.

@ckarlof

ckarlof Nov 4, 2014

Contributor

Possible we can get rid of this: #1839

*
* @return {promise}
*/
afterCompleteSignUp: function () {

This comment has been minimized.

@ckarlof

ckarlof Nov 4, 2014

Contributor

Possible we can get rid of this: #1839

*
* @return {promise}
*/
afterCompleteResetPassword: function () {

This comment has been minimized.

@ckarlof

ckarlof Nov 4, 2014

Contributor

Possible we can get ride of this: #1838

This comment has been minimized.

@ckarlof

ckarlof Nov 4, 2014

Contributor

Maybe not because it usually does nothing, except for the WebChannel flow, in which case, it finishes the OAuth flow. Let's keep it.

oAuthClient: options.oAuthClient
});
},
afterRender: function () {

This comment has been minimized.

@ckarlof

ckarlof Nov 4, 2014

Contributor

Wow, these two oauth views are looking pretty slim. Do we have them just to call this.setupOAuthLinks()?

This comment has been minimized.

@ckarlof
client = new FxaClientWrapper({
channel: channelMock
});
client = new FxaClientWrapper();

This comment has been minimized.

@ckarlof

ckarlof Nov 4, 2014

Contributor

nice! terrible mixing of concerns previously.

@ckarlof

This comment has been minimized.

Contributor

ckarlof commented Nov 4, 2014

@shane-tomlinson, I have a few final comments, and I created issues for those that could be addressed after the initial merge.

INCOMING!

ckarlof added a commit that referenced this pull request Nov 4, 2014

@ckarlof ckarlof merged commit 04b372d into master Nov 4, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@ckarlof ckarlof deleted the add-a-broker branch Nov 4, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment