Opened 5 years ago

Closed 10 months ago

Last modified 10 months ago

#13147 closed Cleanup/optimization (fixed)

auth.UserCreationForm : unicity check on username not necessary

Reported by: peyman Owned by: nobody
Component: contrib.auth Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Hi,

In django.contrib.auth.UserCreationForm, there is clean_username

that checks if the username already exist in the db. This should not
be necessary because this form is a ModelForm and the associated model
User has unique=True.

Thanks
-- Peyman

Attachments (1)

13147.diff (1.1 KB) - added by jeffschenck 5 years ago.
Patch for 13147

Download all attachments as: .zip

Change History (14)

comment:1 Changed 5 years ago by kmtracey

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

And checking the queries issued when adding a user you can see that the existence check is done twice. There was a need, I believe, for the custom auth form to do that check manually when it was initially created, but that need is gone now that it's a model form.

Changed 5 years ago by jeffschenck

Patch for 13147

comment:2 Changed 4 years ago by lukeplant

The advantage to the current method is that the validation error issued, while very similar, is significantly nicer grammatically. Since the translated string is a custom one, rather than a generic one generated by the ORM, I imagine in other languages it could make a big difference, even to the level of affecting comprehensibility. I would be reluctant to do this change and produce a regression of usability for some people.

comment:3 Changed 4 years ago by lukeplant

  • Type set to Cleanup/optimization

comment:4 Changed 4 years ago by lukeplant

  • Severity set to Normal

comment:5 Changed 4 years ago by julien

  • Triage Stage changed from Accepted to Design decision needed

Marking as DDN to reflect Luke's comment.

comment:6 Changed 3 years ago by aaugustin

  • Easy pickings unset
  • Resolution set to wontfix
  • Status changed from new to closed
  • UI/UX unset

I agree with Luke.

comment:7 Changed 3 years ago by aaugustin

In [17225]:

Explained why UserCreationForm performs custom validation of usernames. Refs #13147.

comment:8 Changed 13 months ago by flisky

A real fix comes from https://github.com/django/django/pull/2521 with some cleanup.

comment:9 Changed 10 months ago by timo

  • Resolution wontfix deleted
  • Status changed from closed to new
  • Triage Stage changed from Design decision needed to Accepted
  • Version changed from 1.1 to master

The PR looks good to me. I'm going to add a release note to indicate that UserCreationForm.errors_messages['duplicate_username'] is no longer used just in case someone is customizing that.

comment:10 Changed 10 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 849538d03df21b69f0754a38ee4ec5f48fa02c52:

Fixed #13147 -- Moved User validation logic from form to model.

comment:11 Changed 10 months ago by Tim Graham <timograham@…>

In d5e1a2d5eb269df40979cda3eff3ed92c6d06d9c:

Added contrib.auth migration for refs #13147.

comment:12 Changed 10 months ago by Tim Graham <timograham@…>

In 7affb4ad58e7ee93e4cca0c350315c63035ed648:

Fixed/improved release note for refs #13147; thanks Loic.

comment:13 Changed 10 months ago by Loic Bistuer <loic.bistuer@…>

In 671e0c937c112b1908048b4d8deb14c0116b8946:

Further fix the release notes for refs #13147.

Mention of custom user models has been removed since UserCreationForm
didn't support custom user models anyway. Refs #19353.

Note: See TracTickets for help on using tickets.
Back to Top