feat(account): Move RequestPasswordReset.save impl into account adapter #4034
Conversation
mecampbellsoup
force-pushed
the
mc/account-adapter/request-password-reset
branch
3 times, most recently
from
e1258a0 to
6bc0f43
Compare
mecampbellsoup
force-pushed
the
mc/account-adapter/request-password-reset
branch
2 times, most recently
from
d633579 to
1c3c049
Compare
|
@pennersr would it be possible to get this one reviewed while you're at it? |
|
Must admit, I am still a bit at odds on how to proceed here. #4027 is starting to look more attractive given the impact of the change in this PR. WDYT? |
mecampbellsoup
force-pushed
the
mc/account-adapter/request-password-reset
branch
2 times, most recently
from
ec0f03d to
27910e1
Compare
I think I was able to improve this PR significantly after your review feedback - let me if the changes sway your opinion. I'm also fine w/ #4027. If we go that route, would you want to also expand/extend #4027 to include the other forms with the ✅ in my comment here while we're at it? |
mecampbellsoup
force-pushed
the
mc/account-adapter/request-password-reset
branch
2 times, most recently
from
372e22a to
6c6ae8a
Compare
|
@pennersr ping 😄 |
allauth/account/adapter.py
Outdated
| @@ -563,9 +563,20 @@ def is_safe_url(self, url): | |||
|
|
|||
| return url_has_allowed_host_and_scheme(url, allowed_hosts=allowed_hosts) | |||
|
|
|||
| def request_password_reset(self, users): | |||
There was a problem hiding this comment.
Thoughts:
- Naming -- it does not request the password reset
- Behavior -- if you filter users here the emails are silently skipped which is not very obvious.
Wouldn't it make be more useful to add this method to the adapter?:
def send_password_reset_mail(self, user, email)
Then, you are free to implement that as you see fit (send real reset, skip the mail, or send another mail).
mecampbellsoup
force-pushed
the
mc/account-adapter/request-password-reset
branch
from
6c6ae8a to
35a6aac
Compare
|
|
||
| def _send_password_reset_mail(self, request, email, users, **kwargs): | ||
| adapter: DefaultAccountAdapter = get_adapter() |
There was a problem hiding this comment.
I think it would be good to either abort the method inside the if above by returning return email, or, to reintroduce the else. As is now, both cases are executed (the if and the else part), and while that works because users is empty it did trip me over while reviewing.
There was a problem hiding this comment.
Ah, good catch though!
This new adapter method can be used to apply custom logic regarding whether a user should be able to initiate the reset password flow.
mecampbellsoup
force-pushed
the
mc/account-adapter/request-password-reset
branch
from
35a6aac to
f03eed7
Compare
Related to #4024.