Skip to content

Adding GameCenter auth #4946

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

Closed
wants to merge 4 commits into from
Closed

Adding GameCenter auth #4946

wants to merge 4 commits into from

Conversation

njt1982
Copy link

@njt1982 njt1982 commented Aug 10, 2018

This is basically a copy and paste of just the GameCenter code from #4484 by @kenglou

I've not tested and I have no idea how to, but felt this was more "bite size" and likely to progress without 2 other auth's mixed in...

Copy link
Author

@njt1982 njt1982 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess these need fixing...

// Returns a promise that fulfills if this user id is valid.
function validateAuthData(authData) {
return new Promise(function (resolve, reject)
{
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this should be on the line above?

};

return verify(identity, function (err, token)
{
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this...

return verify(identity, function (err, token)
{
if(err)
return reject('Failed to validate this access token with Game Center.');
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a coding standard about if/else statements like this?

@codecov
Copy link

codecov bot commented Aug 10, 2018

Codecov Report

Merging #4946 into master will decrease coverage by 1.73%.
The diff coverage is 9.09%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #4946      +/-   ##
=========================================
- Coverage   94.24%   92.5%   -1.74%     
=========================================
  Files         121     120       -1     
  Lines        8820    8913      +93     
=========================================
- Hits         8312    8245      -67     
- Misses        508     668     +160
Impacted Files Coverage Δ
src/Adapters/Auth/index.js 92.45% <100%> (+0.14%) ⬆️
src/Adapters/Auth/gcenter.js 7.4% <7.4%> (ø)
src/Adapters/Auth/wechat.js 15.78% <0%> (-84.22%) ⬇️
src/Adapters/Auth/weibo.js 14.81% <0%> (-76.86%) ⬇️
src/Adapters/Auth/qq.js 12.5% <0%> (-75%) ⬇️
src/Adapters/Auth/spotify.js 12% <0%> (-68%) ⬇️
src/Adapters/Auth/vkontakte.js 33.33% <0%> (-51.67%) ⬇️
src/Adapters/Auth/janraincapture.js 55.55% <0%> (-35.36%) ⬇️
src/Adapters/Auth/google.js 67.85% <0%> (-32.15%) ⬇️
src/Adapters/Auth/facebookaccountkit.js 75.75% <0%> (-15.91%) ⬇️
... and 101 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d83a0b6...b4d276d. Read the comment docs.

@flovilmart
Copy link
Contributor

I've not tested and I have no idea how to

We should probably test it before we merge it in.

@stale
Copy link

stale bot commented Oct 18, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Oct 18, 2018
@njt1982
Copy link
Author

njt1982 commented Oct 18, 2018

Looks like a test needs fixing...

@stale stale bot removed the wontfix label Oct 18, 2018
const authenticationHandler = authenticationLoader({
gcenter: path.resolve('./src/Adapters/Auth/gcenter.js')
});
validateAuthenticationHandler(authenticationHandler);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ReferenceError: validateAuthenticationHandler is not defined

This is causing tests to fail...

@stale
Copy link

stale bot commented Dec 2, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Dec 2, 2018
@stale stale bot closed this Dec 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants