Mwalsh/social signup #95
Conversation
|
Should be merged at same time as: |
app/authenticators/torii.js
Outdated
| if (response && response.ok) { | ||
| resolve(authData); | ||
| // if we get a 401 Unauthorized, create a user | ||
| } else if (response && response.status === 401 && userAttrs) { |
There was a problem hiding this comment.
Just checking: this is the part that the auth service is going to handle now, right? Perhaps this will remove the need to set this cross-concern 'isNewSocialUser' flag?
app/services/current-user.js
Outdated
| // this access token has since been revoked | ||
| this.get('session').invalidate(); | ||
| }); | ||
| } else { | ||
| return RSVP.resolve(); |
app/services/session.js
Outdated
| import fetch from 'fetch'; | ||
| import getOwner from 'ember-owner/get'; | ||
|
|
||
| export default SessionService.extend({ | ||
| store: service(), |
There was a problem hiding this comment.
Don't see this used anywhere
|
|
||
| export default FacebookConnectProvider.extend({ | ||
| open() { | ||
| return new RSVP.Promise((resolve, reject) => { | ||
| this._super().then((data) => { |
|
Love those test helpers! Definitely the right move putting them in their own spot. And that registerMovkOnInstance is seriously paying dividends. Great work! |
walsh9
force-pushed
the
mwalsh/social-signup
branch
from
e9bd13f
to
0c9788f
Compare
app/authenticators/torii.js
Outdated
| }) | ||
| .catch(reject); | ||
| }) | ||
| .catch(reject); |
There was a problem hiding this comment.
what if you only rejected in this outermost catch? I think errors will bubble.
There was a problem hiding this comment.
I think I'm just going to rewrite this so it doesn't use nested promises.
app/services/current-user.js
Outdated
| @@ -9,7 +9,10 @@ export default Service.extend({ | |||
| if (this.get('session.isAuthenticated')) { | |||
| let user = this.get('store').queryRecord('user', {me: true}); | |||
| this.set('user', user); | |||
| user.catch(() => { | |||
| return user.then((user) => { | |||
| this.set('user', user); | |||
There was a problem hiding this comment.
what's this all about? you're setting the user as the value of the promise and then overriding that with the resolve value? won't ember do the fulfillment behind the scenes?
There was a problem hiding this comment.
Yeah I think this was leftover from a weird issue when we were doing that weird double authentication thing and can be removed now. I'll clean it up and make sure it still works.
package.json
Outdated
| "nypr-ads": "nypublicradio/nypr-ads", | ||
| "nypr-player": "nypublicradio/nypr-player", | ||
| "nypr-ui": "nypublicradio/nypr-ui", | ||
| "nypr-ads": "nypublicradio/nypr-ads", | ||
| "nypr-ui": "nypublicradio/nypr-ui", |
There was a problem hiding this comment.
ui and ads wound up in here twice
package.json
Outdated
| "nypr-ads": "nypublicradio/nypr-ads", | ||
| "nypr-player": "nypublicradio/nypr-player", | ||
| "nypr-ui": "nypublicradio/nypr-ui", | ||
| "nypr-account-settings": "nypublicradio/nypr-account-settings#d4a20e76df91c1150e581a16bcf47420b38d98d8", |
There was a problem hiding this comment.
what is this hash? should it be removed before merging?
There was a problem hiding this comment.
after merging nypublicradio/nypr-account-settings#8 i should be able to remove the hash and this and nypublicradio/wqxr-web-client#73 can be merged
There was a problem hiding this comment.
nypublicradio/nypr-account-settings#8 looks like it's good to merge?
There was a problem hiding this comment.
Yeah was just waiting for this to be ready so I can merge them around the same time. I can start now.
Expect creation of third-party users to happen on the server side.
Move display of account-based flash messages to the account-wrapper template
Tests for Facebook signup
https://jira.wnyc.org/browse/WE-6588
https://jira.wnyc.org/browse/WE-6869