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.

Sign up for GitHub

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

Jump to bottom

Added GitHub Oauth registration and login #109

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Added GitHub Oauth registration and login #109

wants to merge 3 commits into from

Conversation

codesankalp
Copy link
Member

@codesankalp codesankalp commented Mar 4, 2021 •
edited by isabelcosta
Loading

Description

Added GitHub Oauth which uses JWT REST API for authenticating users.
Make SECRET_KEY with default value.

Fixes #101

Type of Change:

  • Code
  • Quality Assurance
  • Documentation

Code/Quality Assurance Only

  • Bugfix (non-breaking change which fixes an issue)
  • This change requires a documentation update (software upgrade on readme file)
  • New feature (non-breaking change which adds functionality pre-approved by mentors)

How Has This Been Tested?

github

Checklist:

  • My PR follows the style guidelines of this project
  • I have performed a self-review of my own code or materials
  • I have commented on code or provided relevant documentation, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have updated dependencies in requirements.txt

Code/Quality Assurance Only

  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes

@codesankalp codesankalp requested a review from isabelcosta March 4, 2021 17:54
@codesankalp codesankalp added the Status: Needs Review PR needs an additional review or a maintainer's review. label Mar 4, 2021
@codesankalp
Copy link
Member Author

Passing test build: https://github.com/codesankalp/open-source-programs-backend/actions/runs/621291801

@codesankalp codesankalp mentioned this pull request Mar 4, 2021
Open
6 tasks
@codesankalp
Copy link
Member Author

@isabelcosta Can you review this PR?

isabelcosta
isabelcosta previously requested changes Mar 7, 2021
View reviewed changes
Copy link
Member

@isabelcosta isabelcosta left a comment

Choose a reason for hiding this comment

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

Could you please add any tests to the PR. I know this is using an external API, so you might need mocks @codesankalp

@codesankalp codesankalp force-pushed the github-auth branch 3 times, most recently from fab2538 to 7e96fdc Compare March 7, 2021 12:41
@codesankalp
Copy link
Member Author

@isabelcosta I have added tests for this.
Also, I changed the test workflow so that it does not depend on secrets.
Can you review it again?

@codesankalp codesankalp requested a review from isabelcosta March 10, 2021 06:56
@codesankalp codesankalp mentioned this pull request Mar 11, 2021
Closed
7 tasks
devkapilbansal
devkapilbansal suggested changes Mar 16, 2021
View reviewed changes
main/settings.py Outdated Show resolved Hide resolved
@devkapilbansal devkapilbansal added Status: Changes Requested Changes are required to be done by the PR author. and removed Status: Needs Review PR needs an additional review or a maintainer's review. labels Mar 16, 2021
@codecov-io
Copy link

codecov-io commented Mar 16, 2021 •
edited
Loading

Codecov Report

Merging #109 (b03c9ae) into develop (e2b0942) will increase coverage by 1.37%.
The diff coverage is 97.77%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #109      +/-   ##
===========================================
+ Coverage    64.85%   66.23%   +1.37%     
===========================================
  Files           64       66       +2     
  Lines         1030     1075      +45     
===========================================
+ Hits           668      712      +44     
- Misses         362      363       +1
Impacted Files Coverage Δ
main/settings.py 76.19% <83.33%> (+1.19%) ⬆️
tests/test_github_oauth.py 100.00% <100.00%> (ø)
token_auth/urls.py 100.00% <100.00%> (ø)
token_auth/views/github_oauth.py 100.00% <100.00%> (ø)

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 e2b0942...b03c9ae. Read the comment docs.

@codesankalp codesankalp mentioned this pull request Mar 20, 2021
Merged
3 tasks
@codesankalp codesankalp added Status: Needs Review PR needs an additional review or a maintainer's review. and removed Status: Changes Requested Changes are required to be done by the PR author. labels Mar 23, 2021
devkapilbansal
devkapilbansal previously approved these changes Mar 23, 2021
View reviewed changes
Copy link
Member

@devkapilbansal devkapilbansal left a comment

Choose a reason for hiding this comment

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

LGTM 👍
Will test it soon

keshakaneria
keshakaneria previously approved these changes Mar 29, 2021
View reviewed changes
Copy link
Member

@keshakaneria keshakaneria left a comment

Choose a reason for hiding this comment

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

Awesome! Looks good😃

@isabelcosta
Copy link
Member

@codesankalp could you promote this within the community, so we can have someone testing this :) It does not have to be always @devkapilbansal , so we can also see if more people get into QA contributions ;)

@isabelcosta isabelcosta dismissed their stale review April 2, 2021 11:54

cause its done I think

@isabelcosta
Copy link
Member

isabelcosta commented Apr 2, 2021 •
edited
Loading

@codesankalp could you create a separate PR for removing the secret key? In this way, the tests bit would not be so dependant on this PR. Let me know once you create I can get it approved and merged right away

@codesankalp
Added GitHub Oauth registration and login
8e927cf 
Added GitHub Oauth which uses JWT REST API for authenticating users.

Fixes anitab-org#101
@codesankalp
added tests and fixed name
b03c9ae
@codesankalp codesankalp dismissed stale reviews from keshakaneria and devkapilbansal via b03c9ae April 2, 2021 16:14
@codesankalp codesankalp mentioned this pull request Apr 2, 2021
Closed
6 tasks
@gaurivn gaurivn self-requested a review September 18, 2021 13:34
gaurivn
gaurivn reviewed Sep 18, 2021
View reviewed changes
Copy link
Member

@gaurivn gaurivn left a comment

Choose a reason for hiding this comment

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

LGTM 👍, Thank you @codesankalp, pls resolve the merge conflict.

@codecov-commenter
Copy link

Codecov Report

Merging #109 (2d5557c) into develop (426773d) will increase coverage by 1.37%.
The diff coverage is 97.77%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #109      +/-   ##
===========================================
+ Coverage    64.88%   66.26%   +1.37%     
===========================================
  Files           64       66       +2     
  Lines         1031     1076      +45     
===========================================
+ Hits           669      713      +44     
- Misses         362      363       +1
Impacted Files Coverage Δ
main/settings.py 76.19% <83.33%> (+1.19%) ⬆️
tests/test_github_oauth.py 100.00% <100.00%> (ø)
token_auth/urls.py 100.00% <100.00%> (ø)
token_auth/views/github_oauth.py 100.00% <100.00%> (ø)

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 426773d...2d5557c. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gaurivn gaurivn gaurivn left review comments

@isabelcosta isabelcosta Awaiting requested review from isabelcosta

@Aaishpra Aaishpra Awaiting requested review from Aaishpra

@devkapilbansal devkapilbansal Awaiting requested review from devkapilbansal

@keshakaneria keshakaneria Awaiting requested review from keshakaneria

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
Status: Needs Review PR needs an additional review or a maintainer's review.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add GitHub Oauth registration and login
7 participants
@codesankalp @codecov-io @isabelcosta @codecov-commenter @devkapilbansal @keshakaneria @gaurivn