Added support for client credentials flow #133
Conversation
glena
changed the title
glena
force-pushed
the
add-client-credentials-support
branch
5 times, most recently
from
22528ab
to
0e8ca35
Compare
glena
changed the title
| @@ -28,6 +30,8 @@ var OAuthAuthenticator = function (options) { | |||
|
|
|||
| this.oauth = new RestClient(oauthUrl); | |||
| this.clientId = options.clientId; | |||
| this.clientSecret = options.clientSecret; | |||
| this.domain = options.domain; | |||
There was a problem hiding this comment.
where is the domain used? not familiar with the code base, but no change in the PR seems to be using it. if it is in this.oauth then 👍
There was a problem hiding this comment.
forgot to remove that. I made it different at first
| it('should perform a POST request to ' + path, function (done) { | ||
| var request = this.request; | ||
|
|
||
| this |
There was a problem hiding this comment.
you can return the promise and avoid calling done() inside then.
I would not add catch(done). If this results in an error, the test should not pass
glena
force-pushed
the
add-client-credentials-support
branch
from
0e8ca35
to
3fd2466
Compare
| .authenticator | ||
| .clientCredentialsGrant(options) | ||
| .then(done.bind(null, null)) | ||
| .catch(done.bind(null, null)); |
There was a problem hiding this comment.
if the operation fails the test should not pass, remove the catch. you can return the promise directly, remove the done parameter and the then call
There was a problem hiding this comment.
right, we should check the rest of the tests then
| }) | ||
| .reply(200); | ||
|
|
||
| this |
There was a problem hiding this comment.
for all of these, same thing: return the promise and don't handle the catch, no need for that
No description provided.