Fix for authenticating with instagram#7173
Conversation
There was a problem hiding this comment.
This looks like the proper fix after looking at the API
https://developers.facebook.com/docs/instagram-basic-display-api/reference/user
src/Adapters/Auth/instagram.js
Outdated
| const path = `${apiURL}me?fields=id&access_token=${authData.access_token}`; | ||
| return httpsRequest.get(path).then(response => { | ||
| if (response && response.data && response.data.id == authData.id) { | ||
| if (response && response.id == authData.id) { |
There was a problem hiding this comment.
The CI build is failing. Can you update the tests to match your change?
There was a problem hiding this comment.
Are you using any API urls or just the default graph.instagram.com? You could easily support both response and response.data.
const user = response.data ? response.data : response;
There was a problem hiding this comment.
I use defaultURL.
OK I will apply support for both response, and response.data
I will try to figure out how to update the tests. In the meantime I would appreciate it if you can refer me to some instructions on how to update the test.
There was a problem hiding this comment.
You can checkout the Contributing Guide
You can see that tests are failing in the CI.
The tests you are looking for should be here
There was a problem hiding this comment.
Thanks very much!
I will update the test. But you mentioned earlier that I can support response and response.data. Are you sure it would be a good practice?. In the test you send it can only cover one of the cases
There was a problem hiding this comment.
Just copy the existing test and create a new test one. I think its best practice as I don't want a breaking change with older urls.
There was a problem hiding this comment.
Sorry for the late response, but I really spent many hours trying to find the older urls where the response includes data child object, but I didn't find any. Are you sure this was working at one point? Anyway I pushed new changes, for covering also older urls
OverDriveGain
force-pushed
the
master
branch
from
5a7c7e4 to
2302d50
Compare
Codecov Report
@@ Coverage Diff @@
## master #7173 +/- ##
==========================================
- Coverage 94.00% 94.00% -0.01%
==========================================
Files 172 172
Lines 12835 12868 +33
==========================================
+ Hits 12066 12097 +31
- Misses 769 771 +2
Continue to review full report at Codecov.
|
OverDriveGain
force-pushed
the
master
branch
from
2302d50 to
8a4915b
Compare
…nted in the response
dplewis
left a comment
dplewis
left a comment
There was a problem hiding this comment.
LGTM! Thank you contributing!
* Fix for authenticating with instagram * Change tests for instagram authentication * Instagram authentication for the case when data child object is presented in the response
|
🎉 This change has been released in version 5.0.0-beta.1 |
|
🎉 This change has been released in version 5.0.0 |
3 participants
New Pull Request Checklist
Issue Description
Instagram authentication is not working, it always returns 'Instagram auth is invalid for this user.'
The response of function httpsRequest.get(path) does not include the data object.
The user id is directly presented in the response
Approach
Remove the 'data' part from 'response.data'
TODOs before merging