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 #192

Merged
merged 2 commits into from
Jan 4, 2024

Conversation

johanseto
Copy link
Contributor

Description

This changes the inheritance of the eox-tenant-auth-exception.

This is with the purpose of the exception to be handled by the social tpa middleware exception process.
This is to avoid this 502 error msg:
image
image

You can see here that the new inheritance class is also based on value error exception.

But the change is related to this eox-core PR:

So now the exception is family of the SocialAuthBaseException.

With that change now, the social_django middleware could manage it.

Keep in mind that this middleware is the parent of edx-platform middleware.

The middleware seems applied in the openedx tpa module.

Checklist for Merge

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

Copy link
Contributor

@andrey-canon andrey-canon left a comment

Choose a reason for hiding this comment

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

Why you are not adding this to the requirements ?
Tests failing

@johanseto
Copy link
Contributor Author

johanseto commented Dec 21, 2023

@andrey-canon For the test failing I made this PR to eox-tenant. But yes I can added also to this branch in other PR.
#193

@johanseto
Copy link
Contributor Author

@andrey-canon for the inclusion of the requirements. I avoid it because I want this PR to have the lowest lines and lowest impact.
Also, I want this to be moved to master, so then I would have a requirements incompatibility due to the large update...
Also some other eox`s like core have block code like that:
https://github.com/eduNEXT/eox-core/blob/master/eox_core/middleware.py#L31-L37
Also this eox:
https://github.com/eduNEXT/eox-tenant/blob/master/setup.py#L7-L10

@andrey-canon
Copy link
Contributor

@andrey-canon for the inclusion of the requirements. I avoid it because I want this PR to have the lowest lines and lowest impact. Also, I want this to be moved to master, so then I would have a requirements incompatibility due to the large update... Also some other eox`s like core have block code like that: https://github.com/eduNEXT/eox-core/blob/master/eox_core/middleware.py#L31-L37 Also this eox: https://github.com/eduNEXT/eox-tenant/blob/master/setup.py#L7-L10

in the past this was consider as the lazy way to pass the test if you are worried about the number of lines my suggestion is to do make upgrade and then discard all the changes that are not related with the social-core dependency

@johanseto
Copy link
Contributor Author

in the past this was consider as the lazy way to pass the test if you are worried about the number of lines my suggestion is to do make upgrade and then discard all the changes that are not related with the social-core dependency

4961cef

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
@johanseto
Copy link
Contributor Author

@andrey-canon I have added the dependency.
b306762

@andrey-canon
Copy link
Contributor

@johanseto could you open the PR against master, please

@johanseto johanseto merged commit c542749 into open-release/mango.nelp Jan 4, 2024
3 checks passed
@johanseto
Copy link
Contributor Author

PR opened against master
#195

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