Get access token before importing users #267
Conversation
| } | ||
| }, | ||
| connection_id: data.connection_id | ||
| var promise = options.tokenProvider.getAccessToken().then(function(access_token) { |
There was a problem hiding this comment.
Before the existing request promise, we call getAccessToken from the provider we were passed on creation. This is then passed a function that wraps the existing code.
There was a problem hiding this comment.
I think this is the right way to do it, but I'm not an expert with the various ways this codebase handles authentication, so if it should do something different as well / instead, please let me know.
| { | ||
| url: url, | ||
| method: method, | ||
| headers: extend({ Authorization: `Bearer ${access_token}` }, headers), |
There was a problem hiding this comment.
The token is then added to the headers.
Floppy
changed the title
|
Why can't you just use the RestClient that is created in the constructor? |
|
I did wonder that, but was working on the assumption (before checking here) that this one rolls its own for some specific reason. Obviously that would be ideal, to use |
|
I suspect (though I've not tried it to check yet) that it's because this method submits a multipart form, which the base rest client doesn't support. But as I say I've not tried, was starting with a minimal change 😄 |
|
oh yeah, you're probably right. |
|
I think because it's quite a short diff, and one of the lines isn't covered (and wasn't before), so it fails the 95% diff coverage test. The coverage value overall has gone up though :) |
|
Found it. Can you write a test for when the request fails? |
|
Just realised that those last changes replicate some of what @mikemeerschaert did in #243. But now it has tests :) |
|
Thanks for the PR 🎉 |
Get an access token before running
importUsers. Resolves #266.The changes are more minimal than they look, because of the pre-commit formatter. Description of change added in comments on the relevant lines.