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

refactor: inherit from social auth exception #195

Merged
merged 2 commits into from
Apr 11, 2024

Conversation

johanseto
Copy link
Contributor

@johanseto johanseto commented Jan 4, 2024

Description

  • refactor: inherit from social auth exception

This change the heritance of the eox-tenant-auth-exception.

This with the purpose of the exception to be handled by the social tpa middleware exception process.

You can see here that the new class is also based in value error exception. https://github.com/python-social-auth/social-core/blob/29cbbd22b98d81d569a886b1cc0bd9a316cd124f/social_core/exceptions.py#L1

But the change is related with this PR:
eduNEXT/eox-core#171
So now as the exception is family of the SocialAuthBaseException.

The the tpa middleware could managed it.
https://github.com/python-social-auth/social-app-django/blob/5.4.0/social_django/middleware.py#L35

Keep in mind that this middleware is parent of edx-platform middleware. https://github.com/openedx/edx-platform/blob/ebcbe1cd9208191c0589d7fe538c6ac13470abe6/common/djangoapps/third_party_auth/middleware.py#L18

  • refactor: social-auth-core pkg inclusion

(cherry picked from commit c542749)

Additional information

Please before review or approve the commitlint-PR or I can't add links or urls in the commits body.

PR related the change:
#192

Checklist for Merge

  • Tested in a remote environment
  • Updated documentation
  • Rebased master/main
  • Squashed commits

@johanseto
Copy link
Contributor Author

johanseto commented Feb 9, 2024

I am only adding social-auth-core in base.in

social-auth-core

and the running make upgrade.

For some unknown reason I had to change the setup.cfg to allow load the pytlint rules... that you can research what is happening with the pylint upgrade...
here are some rules that would be removed in the upgrade version pylint-dev/pylint#4942

@johanseto johanseto force-pushed the jlc/refactor-tpa-exception-social-core branch 2 times, most recently from 653cfe7 to 3806aaa Compare February 9, 2024 21:40
@bra-i-am
Copy link
Contributor

bra-i-am commented Mar 4, 2024

hi @johanseto, I hope you're well... could you please solve the conflicts to start with this PR's review?

Thank you :D

* refactor: inherit from social auth exception

This change the heritance of the eox-tenant-auth-exception.

This with the purpose of the exception to be handled by the social tpa middleware exception
process.

You can see here that the new class is also based in value error exception.
https://github.com/python-social-auth/social-core/blob/29cbbd22b98d81d569a886b1cc0bd9a316cd124f/social_core/exceptions.py#L1

But the change is related with this PR:
eduNEXT/eox-core#171
So now as the exception is family  of the SocialAuthBaseException.

The the tpa middleware could managed it.
https://github.com/python-social-auth/social-app-django/blob/5.4.0/social_django/middleware.py#L35

Keep in mind that this middleware is parent of edx-platform middleware.
https://github.com/openedx/edx-platform/blob/ebcbe1cd9208191c0589d7fe538c6ac13470abe6/common/djangoapps/third_party_auth/middleware.py#L18

* refactor: social-auth-core pkg inclusion

(cherry picked from commit c542749)

chore: run make upgrade
@johanseto johanseto force-pushed the jlc/refactor-tpa-exception-social-core branch 3 times, most recently from 80af81d to f0fc85a Compare March 4, 2024 15:19
add use-yield-from (R1737) due pylint update to 3.1.0
@johanseto johanseto force-pushed the jlc/refactor-tpa-exception-social-core branch from f0fc85a to b6301d9 Compare March 4, 2024 15:20
@johanseto
Copy link
Contributor Author

johanseto commented Mar 4, 2024

@bra-i-am nice confliicts resolved, I had to update a rule to the pyint disable due the update of plint and some yield config that is not under the scope of this PR.
image

Copy link
Contributor

@MaferMazu MaferMazu left a comment

Choose a reason for hiding this comment

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

This looks good to me.

Copy link
Contributor

@dcoa dcoa left a comment

Choose a reason for hiding this comment

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

looks good

@MaferMazu MaferMazu merged commit d0207e8 into master Apr 11, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants