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

Improvement Request for registration/serializers.py #672

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

ADRIDEV2024
Copy link

@ADRIDEV2024 ADRIDEV2024 commented Dec 18, 2024

Hi, I´m Adrian and I work as backend developer mostly with Python and frameworks like Django. I write this PR because I´ve been navigating this repo and looks great how is going forward, so I thought it would be worthwhile to contribute and support this project. Recently forked this repo and found some problems or inconsistencies inside the serializers file code ( registration folder ). I don´t pretend that all the changes I am going to suggest will be integrated into this project, but I would like at least a part of them to be taken into consideration.

PROBLEM:

  1. The _get_request function ( lines 45-49 )can be simplified with a ternary expression. We can save some unnecesary code lines for better function understanding as well as readibility. Anyways, I don´t see that like a critical problem, it´s only a tip. This is how looks this function without a oneliner:

code_getrequest

  1. Maybe the password validations functions (RegisterSerializer class) are too weak and doesn´t ensure safe handling that really protect users. The management for user passwords looks simple and insecure in many ways.

code_validate

  1. The validate function in SocialLoginSerializer class is too much large and performs multiple tasks (validation, authentication and user creation all in the same box). I would recommend splitting this logic into subfunctions and therefore smaller tasks.I think this should be a better option for long-term maintainability and faster performance.

PROPOSED CHANGES:

get_request function improvements:

  • Readability: The function is more concise with a oneliner expression, using a single line with getattr().
  • Efficiency: This little change reduces the number of lines of code and avoids explicit conditional evaluation.

getrequest(1)

Password validation logic:

Perhaps it will be better if password validation process is located in another place as separate logic that belongs to a new file named as password_valdations.py ( inside the registration folder). I removed the previous functions and made a complete file with only one responsability: password validations. All validation rules in that file can be customized through the constructor and it´s easy to add or modify them.

Validate function:

The validate() function was long and performed multiple tasks (validation, obtaining tokens, etc.).

The function can be split into this sub-methods:

_handle_code_flow(): Logic to handle the authorization flow by code.
_attempt_login(): Logic to attempt to login with social authentication.
_handle_new_user_registration(): Logic to register user if user does not exist.

Benefits:

Modularization: break logic into small, easy to understand tasks.
Reusability: Methods can be reused or tested separately.
Readability: The validate function is more readable and easier to follow.

@ADRIDEV2024 ADRIDEV2024 marked this pull request as ready for review December 18, 2024 09:12
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.

1 participant