Code

Opened 7 years ago

Closed 11 months ago

#5335 closed New feature (duplicate)

Add an append method to ErrorDict

Reported by: Thomas Güttler <hv@…> Owned by: jkocherhans
Component: Forms Version: master
Severity: Normal Keywords: append errodict
Cc: hv@…, simon@…, cahenan@…, miracle2k, bronger, alasdair Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Custom Validation: If you need to access other fields, you
need to overwrite form.clean(). If you want to have
the error message near a widget (not all), you need to
modify the _error dict of the form.

To make this more user friendly I wrote a small patch:

ErrorDict.append(fieldname, message)

Doc patch included.

Attachments (6)

form_error_append.diff (1.5 KB) - added by Thomas Güttler <hv@…> 7 years ago.
form_error_append2.diff (2.6 KB) - added by Thomas Güttler <hv@…> 7 years ago.
append-r9698.diff (4.4 KB) - added by kratorius 6 years ago.
updated patch, tests and documentation
append-take2-r9698.diff (4.3 KB) - added by kratorius 6 years ago.
updated patch using setdefault
append-take3-r9698.diff (4.3 KB) - added by kratorius 6 years ago.
Fixed patch, create an ErrorList instead of a simple list
form_error_append_r16741.diff (5.5 KB) - added by vbmendes 3 years ago.
Reviewed patch with unittests

Download all attachments as: .zip

Change History (39)

Changed 7 years ago by Thomas Güttler <hv@…>

comment:1 Changed 7 years ago by brosner

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to invalid
  • Status changed from new to closed

You can use the clean_FIELD() method validation hooks to get errors messages near fields.

comment:2 Changed 7 years ago by Thomas Güttler <hv@…>

  • Resolution invalid deleted
  • Status changed from closed to reopened

Thank you brosner, for looking at this patch. But I can't use clean_FIELD():

if clean_FIELD() requires to look at FIELD2, the dict self.cleaned_data does not contain
FIELD2. If you need the cleaned values of two fields for validation, you can not use clean_FIELD().

Nevertheless I want the error message to be near the field FIELD.

Here is how I use the patch:

    def clean(self):
        parent=self.cleaned_data.get("parent")
        name=self.cleaned_data.get("name")
        if ....:
            # Needs Django Patch #5335
            self.errors.append("name", u'...')
        return self.cleaned_data

Of course I could do this without the patch, too:

errorlist=self.errors.get(name) 
if errorlist==None: 
    errorlist=ErrorList() 
    self.errors[name]=errorlist 
errorlist.append(message) 

But that's too much code, I don't want to repeat. Maybe the documentation should
be updated, too.

comment:3 Changed 7 years ago by anonymous

  • Needs tests set
  • Triage Stage changed from Unreviewed to Design decision needed

Changed 7 years ago by Thomas Güttler <hv@…>

comment:4 Changed 7 years ago by Thomas Güttler <hv@…>

  • Needs tests unset

Added doctests

comment:5 Changed 7 years ago by Thomas Güttler <hv@…>

  • Cc hv@… added

comment:6 Changed 6 years ago by Simon Litchfield <simon@…>

  • Cc simon@… added

comment:7 Changed 6 years ago by anonymous

  • Cc cahenan@… added

comment:8 Changed 6 years ago by guettli

comment:9 Changed 6 years ago by Alex

Perhaps a better method would be to have the ErrorDict be a defaultdict, that way users can do form._errorsfield?.append('error message') without worrying about whether or not field is in the dict or not.

comment:10 Changed 6 years ago by kratorius

  • Keywords append errodict added
  • Summary changed from newforms: Custom validation. to Add an append method to ErrorDict

Attached patch implements ErrorDict as a defaultdict (can't inherit directly from defaultdict since it's new in 2.5 or 2.4, can't remember, but definitely not in 2.3). It would be helpful if someone could review the documentation part of the patch since I'm not a native english speaker.

Changed 6 years ago by kratorius

updated patch, tests and documentation

comment:11 Changed 6 years ago by Alex

The docs look ok by me, 2 small things though, instead of self.has_key(field) do field in self . Also there's no need to use super() for setitem since it isn't overode in the subclass, you can actually just use setattr(self, field, []).

Changed 6 years ago by kratorius

updated patch using setdefault

comment:12 Changed 6 years ago by kratorius

Even better, I used setdefault so in the end is a one-line patch.

comment:13 Changed 6 years ago by Alex

There's actually a slight disconnect between the docs and the code, the docs say an ErrorList is created, but getitem adds a list([]), I think an ErrorList would be the correct behavior.

Changed 6 years ago by kratorius

Fixed patch, create an ErrorList instead of a simple list

comment:14 Changed 5 years ago by mrts

  • milestone set to 1.1

According to Version1.1Features seems to be in scope for 1.1?

comment:15 Changed 5 years ago by jkocherhans

  • milestone changed from 1.1 to 1.2

Bumping to 1.2. For the record, I have to do this all the time and I'd like to see something like this, but I'm not entirely sold on this way yet.

comment:16 Changed 5 years ago by simon29

Definitely. Often views need to perform more involved steps *after* the form is validated; which may fail, and ultimately mean there was a problem with what was entered in the form.

There needs to be an easy, documented, encouraged way of appending errors to NON_FIELD_ERRORS as well as to specific fields; post-validation (ie from the view).

comment:17 Changed 5 years ago by Honza_Kral

This approach is in conflict with #4752 ([6142]).

The method would either have to be on the Form class (not entirely insane). Another way how to propagate your error onto a field is using ComplexValidators present in model-validation branch, that way you don't have to worry about the internals of ErrorDict...

comment:18 Changed 4 years ago by miracle2k

  • Cc miracle2k added

comment:19 Changed 4 years ago by jkocherhans

  • milestone changed from 1.2 to 1.3
  • Owner changed from nobody to jkocherhans
  • Status changed from reopened to new

This a a feature, so I'm bumping it to 1.3. Sorry. I'm assigning it to myself it to increase the chances that I'll remember it for the next cycle.

comment:20 Changed 4 years ago by Alex

  • Triage Stage changed from Design decision needed to Accepted

comment:21 Changed 3 years ago by lukeplant

  • milestone changed from 1.3 to 1.4
  • Patch needs improvement set

Looks like Joseph didn't remember :-( This is a feature, so will have to wait again. Patch also needs updating (especially to use unittest rather than doctests).

comment:22 Changed 3 years ago by gabrielhurley

  • Severity set to Normal
  • Type set to New feature

Changed 3 years ago by vbmendes

Reviewed patch with unittests

comment:23 Changed 3 years ago by julien

  • Easy pickings unset
  • UI/UX unset

Thanks a lot for updating the patch. I showed it to Alex and he wondered if you could write a test that verifies that doing errorsfoo? without actually altering it doesn't cause any problem?

Version 0, edited 3 years ago by julien (next)

comment:24 Changed 3 years ago by bronger

  • Cc bronger added

comment:25 Changed 3 years ago by kmtracey

Current patch (form_error_append_r16741.diff) will auto-create an ErrorList() object for a field's error list even if the including form has over-ridden the form's error class (https://docs.djangoproject.com/en/1.3/ref/forms/api/#customizing-the-error-list-format). If we are going to add this (and it would be nice) it needs to be done in a way that will use the form's custom error class if it has been defined. This case should also be tested, in a test similar to: https://code.djangoproject.com/browser/django/tags/releases/1.3.1/tests/regressiontests/forms/tests/error_messages.py#L199

comment:26 Changed 3 years ago by jacob

  • milestone 1.4 deleted

Milestone 1.4 deleted

comment:27 Changed 11 months ago by loic84

The problem is that ErrorDict should be ErrorList agnostic. The latest patch hardcodes the ErrorList type which can be customized at the form level with the error_class argument of Form.__init__() .

The Form.add_errors() method discussed here is IMO a much better approach.

comment:28 Changed 11 months ago by loic84

Actually there is merit in making ErrorDict inherit from defaultdict.

PR https://github.com/django/django/pull/1481

comment:29 Changed 11 months ago by alasdair

Looping through defaultdicts in templates can cause problems. See #16335. I believe the pull request 1481 would stop the following template snippet from working.

    <ul>
      {% for field, errors in form.errors.items %}
       <li>{{ field }}: {{errors}}</li>
      {% endfor %}
    </ul>

I realise it's a slightly artificial example, and that usually you would use {{ form.errors }}.

Last edited 11 months ago by alasdair (previous) (diff)

comment:30 Changed 11 months ago by simon29

Check out the related discussion and also ticket #20867

comment:31 Changed 11 months ago by loic84

@alasdair: that looks like a nasty incompatibility with no obvious workaround.

The only reason I fixed this despite #20867 was so internally it wouldn't be needed to check if keys existed, it's clearly not worth breaking existing templates over this.

I think we should close this ticket as wontfix, especially now that another ticket addresses the main issue.

comment:32 Changed 11 months ago by alasdair

  • Cc alasdair added

comment:33 Changed 11 months ago by mjtamlyn

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

I agree that this is not the best way forwards. The ticket #20867 is now covering the Form.add_errors() approach. I'm going to close this ticket as a "duplicate" cos I think that's closest to what's happening.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.