Sharing password management functionality within auth forms

Django Internals
1

Hello everyone!

While continuing with the review for the new feature proposed in PR #16942, I stumbled with the following question for which I’m not 100% sure about the answer, so I’m reaching out to you all to hear/read your thoughts (@adamchainz @carltongibson I’m particularly interested in your view).

Intro

The new feature is about allowing admin users to set unusable password for users. This work requires changes in every “form that deals with passwords”, so a few review iterations ago I asked Fabian to define a SetPasswordMixin to isolate and unify this non trivial (and new) logic. I also requested that we split the creation and usage of the mixin in two commits to ease reviewing.

Challenge

Now, I’m reviewing those two commits and while they match my request, it bothered me that SetPasswordMixin inherits from forms.Form. So, I started making changes locally to make the mixin a “pure” mixin, removing the inheritance on forms.Form and adjusting the methods so they make sense for the “real” forms (BaseUserCreationForm, AdminPasswordChangeForm, and tangentially SetPasswordForm). This is what I ended up with:

Is this a “sufficiently idiomatic” proposal? Is there a better/cleaner way? Would inheriting from forms.Form be preferred?

1 Like
2

Hi @nessita.

I had a quick look at the PR.

I haven’t been following it closely, so I’d defer to those already involved, but the mixin change looked reasonable enough on the surface. Certainly given the amount of thought that’s gone in, it seems likely so. (I’m not sure I can say more without spending quite a bit more time so let me know if that’s sufficient.)

(I thought the release note could do with a rephrase…)

HTH.
C.

1 Like