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

Re-factored ResetPasswordConfirm using the open-closed principle #115

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

Conversation

Patrick-DS
Copy link

I would actually like to do bigger changes in the repository for the functionality that I need, but I wanted to initialize the discussion with this commit because it's the safest bet and shows an example of what I want to implement.

The views have a very bulky post method, so subclassing them to try to change a little bit of functionality ends up re-creating the class almost entirely and makes subclassing useless, going against the open-closed principle. I ended up re-writing a lot of code because of that.

The .post method in ResetPasswordConfirm was very bulky and much of the functionality inside the post method made more sense as a collection of smaller methods with a more precise task, because they could be re-defined for another developer's use-case (this is my use-case; I have three user types and boolean properties attached to the user model that I can use to tell the user types apart, and I needed to re-define "eligible_for_reset"). Functionality is left unchanged but the steps can now be adjusted if a developer subclasses this view. The post method is also now much more readable.

  • The token is only used to fetch the user from the DB, so we have the "get_user_from_token" method.
  • The purpose of the try-except block for the password validation is now clearer; isolated as a method, the method validates the password and converts the Django validation error into a DRF one
  • We used a contextmanager to trigger signals instead of just writing the signals. This gives a child class more control over what to do before and after the password change, improves readability, and puts all signal triggering in one place.
  • There's an actual "reset_password" method now.

The .post method in ResetPasswordConfirm was very bulky and much of the functionality inside the post method made more sense as a collection of smaller methods with a more precise task, because they could be re-defined for another developer's use-case (this is my case). Functionality is left unchanged but the steps can now be adjusted if a developer subclasses this view. The post method is also now much more readable.

- The token is only used to fetch the user from the DB, so we have the "get_user_from_token" method. 
- The purpose of the try-except block for the password validation is now clearer; isolated as a method, the method validates the password and converts the Django validation error into a DRF one
- We used a contextmanager to trigger signals instead of just writing the signals. This gives a child class more control over what to do before and after the password change, improves readability, and puts all signal triggering in one place.
- There's an actual "reset_password" method now.
- Replaced tabs by spaces
@nezhar nezhar self-requested a review August 18, 2021 17:50
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.

None yet

2 participants