Skip to content
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

fix: Github API endpoint #14874

Merged
merged 6 commits into from
Nov 10, 2021
Merged

Conversation

pateljannat
Copy link
Contributor

@pateljannat pateljannat commented Nov 3, 2021

Fixes: fossunited/mon_school#72 and #14810

Issue:

  1. Previously we used to hit the endpoint /user for GitHub login.
  2. This endpoint only returns public data of the user's profile.
  3. On GitHub users can opt to keep their email addresses private.

Screenshot 2021-11-03 at 3 17 12 PM

  1. In this case the /user endpoint returns the email as None.
  2. Hence the validation error mentioned in the above issues.

Fix:

  1. The endpoint /user/emails give us a list of email addresses from the user's profile even if it is private.

@pateljannat pateljannat requested review from a team and removed request for a team, leela and gavindsouza November 3, 2021 09:40
@codecov
Copy link

codecov bot commented Nov 3, 2021

Codecov Report

Merging #14874 (7771e0b) into develop (91f446e) will increase coverage by 0.61%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop   #14874      +/-   ##
===========================================
+ Coverage    49.57%   50.19%   +0.61%     
===========================================
  Files          748      744       -4     
  Lines        65606    65469     -137     
  Branches      5485     5428      -57     
===========================================
+ Hits         32527    32862     +335     
+ Misses       29171    28746     -425     
+ Partials      3908     3861      -47     
Flag Coverage Δ
server 56.95% <100.00%> (+0.24%) ⬆️
ui-tests 37.64% <ø> (+1.15%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Member

@surajshetty3416 surajshetty3416 left a comment

Choose a reason for hiding this comment

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

@pateljannat I encountered the following error while using Github Login

Traceback (most recent call last):
  File "/Users/sps/benches/develop/apps/frappe/frappe/app.py", line 66, in application
    response = frappe.api.handle()
  File "/Users/sps/benches/develop/apps/frappe/frappe/api.py", line 54, in handle
    return frappe.handler.handle()
  File "/Users/sps/benches/develop/apps/frappe/frappe/handler.py", line 31, in handle
    data = execute_cmd(cmd)
  File "/Users/sps/benches/develop/apps/frappe/frappe/handler.py", line 67, in execute_cmd
    return frappe.call(method, **frappe.form_dict)
  File "/Users/sps/benches/develop/apps/frappe/frappe/__init__.py", line 1213, in call
    return fn(*args, **newargs)
  File "/Users/sps/benches/develop/apps/frappe/frappe/www/login.py", line 85, in login_via_github
    login_via_oauth2("github", code, state)
  File "/Users/sps/benches/develop/apps/frappe/frappe/utils/oauth.py", line 109, in login_via_oauth2
    login_oauth_user(info, provider=provider, state=state)
  File "/Users/sps/benches/develop/apps/frappe/frappe/utils/oauth.py", line 186, in login_oauth_user
    if update_oauth_user(user, data, provider) is False:
  File "/Users/sps/benches/develop/apps/frappe/frappe/utils/oauth.py", line 267, in update_oauth_user
    user.set_social_login_userid(provider, userid=data["id"], username=data.get("login"))
KeyError: 'id'

Note: I did regular sign up (without Github) but used Github to sign in.

@surajshetty3416
Copy link
Member

surajshetty3416 commented Nov 9, 2021

@pateljannat can you also add test cases to validate these changes?
Suggestion: You can mock Github responses in the test.

@surajshetty3416 surajshetty3416 changed the title fix: github api endpoint fix: Github API endpoint Nov 9, 2021
@mergify mergify bot merged commit 318d4fa into frappe:develop Nov 10, 2021
@surajshetty3416
Copy link
Member

@Mergifyio backport version-13-hotfix

@mergify
Copy link
Contributor

mergify bot commented Nov 10, 2021

backport version-13-hotfix

✅ Backports have been created

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Github login giving error
2 participants