Skip to content
This repository has been archived by the owner on Jan 26, 2021. It is now read-only.

Added OAuth Settings for Facebook #551

Closed
wants to merge 1 commit into from
Closed

Added OAuth Settings for Facebook #551

wants to merge 1 commit into from

Conversation

ritwickraj78
Copy link
Contributor

Description

Add the Settings for OAuth(Facebook)

Fixes #533

Type of Change:

Delete irrelevant options.

  • Code

Code/Quality Assurance Only

  • New feature (non-breaking change which adds functionality pre-approved by mentors)

How Has This Been Tested?

The travis CI tests and all the local tests pass successfully and locally all third party authentications work properly.

Checklist:

Delete irrelevant options.

  • [*] My PR follows the style guidelines of this project
  • [*] I have performed a self-review of my own code or materials
  • [*] I have commented my code or provided relevant documentation, particularly in hard-to-understand areas
  • [*] Any dependent changes have been merged

Code/Quality Assurance Only

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

@satya7289
Copy link
Contributor

@ritwickraj78 just a suggestion do create a PR with branch other than develop.

@ritwickraj78 ritwickraj78 requested a review from SanketDG June 2, 2020 15:43
@sammy1997
Copy link
Contributor

@ritwickraj78 For the remaining PRs please create a new branch for the PR. Don't use develop

sammy1997
sammy1997 reviewed Jun 2, 2020
View reviewed changes
Copy link
Contributor

@sammy1997 sammy1997 left a comment

Choose a reason for hiding this comment

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

LGTM!

SanketDG
SanketDG suggested changes Jun 2, 2020
View reviewed changes
systers_portal/systers_portal/settings/base.py Outdated Show resolved Hide resolved
systers_portal/systers_portal/settings/base.py Outdated Show resolved Hide resolved
@ritwickraj78
Copy link
Contributor Author

Sure @sammy1997 I will keep that in mind. @SanketDG Could you have another look at it please?

SanketDG
SanketDG suggested changes Jun 3, 2020
View reviewed changes
Copy link
Contributor

@SanketDG SanketDG left a comment

Choose a reason for hiding this comment

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

After a fresh look, I have realized you don't need this setting at all.

Every setting just uses the default value except:

'AUTH_PARAMS': {'auth_type': 'reauthenticate'},

which just prompts for the password (I would personally vouch for not keeping this, since I have not seen this on popular sites' oAuth logins, but I am open to other views)

Unless we would want to implement specific niceties (good to have) of the oAuth implementation, it is mostly implemented and I have gone ahead and checked for atleast Facebook.

For production, all we need to is create the right SocialApp for the required providers, and all of it should be working.

Maybe others can pitch in on what needs to be done here, but you could write tests for the oAuth flow if there is nothing else you can work on right now.

@ritwickraj78
Copy link
Contributor Author

@SanketDG So what should be done? If we don't need the re-authentication then we can close the PR then.

@ritwickraj78 ritwickraj78 requested a review from SanketDG June 3, 2020 13:58
@SanketDG
Copy link
Contributor

SanketDG commented Jun 3, 2020

I am up for not making any changes at all and closing this, if others agree on this.

@sammy1997
Copy link
Contributor

@SanketDG Sure. But should we close the linked issue as well or wait till May completes her part? @sakshi1499?

@sakshi1499
Copy link

@sammy1997 I agree. I think we should wait for May.

@SanketDG
Copy link
Contributor

SanketDG commented Jun 7, 2020

Did we follow up on this?

@ritwickraj78
Copy link
Contributor Author

@SanketDG Not yet heard from May. Maybe tomorrow we can discuss in the meeting and then take the steps accordingly.

@sammy1997
Copy link
Contributor

@SanketDG I spoke to @mayburgos in my 1:1 and she said she will have to talk to the IT team. SO it might take a while. Let's move forward for now!

@LaibaBasit008 LaibaBasit008 added the Status: Needs Review PR needs an additional review or a maintainer's review. label Jun 11, 2020
@sakshi1499 sakshi1499 added Status: On Hold Issue or PR needs more info, a discussion, a review or approval from a Maintainer/Code Owner. and removed Status: Needs Review PR needs an additional review or a maintainer's review. labels Jun 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sakshi1499 sakshi1499 sakshi1499 left review comments

@sammy1997 sammy1997 sammy1997 left review comments

@SanketDG SanketDG Awaiting requested review from SanketDG

Assignees
No one assigned
Labels
Status: On Hold Issue or PR needs more info, a discussion, a review or approval from a Maintainer/Code Owner.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Third Party Authentication
6 participants
@ritwickraj78 @satya7289 @sammy1997 @SanketDG @sakshi1499 @LaibaBasit008