Code

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#10512 closed (fixed)

Ordering of non-model fields in a ModelForm, after #8164

Reported by: mbi Owned by: Alex
Component: Forms Version: master
Severity: Keywords: modelform, field order
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: UI/UX:

Description

After #8164 was closed in [10062], it is not longer possible to define non-model fields in the fields list, as the ordering process raises a KeyError on forms/models.py:166

It would be great if the fields list order could be applied to non-model fields defined in the Form class as well.

Attachments (1)

modelforms-fields-order.diff (1.6 KB) - added by Alex 5 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 5 years ago by Alex

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

As I mentioned in IRC, the issue here is not that you can't have extra fields on the Form, it's that you can't use them in the fields ordering option.

Changed 5 years ago by Alex

comment:2 Changed 5 years ago by anonymous

  • Has patch set
  • Needs tests set

comment:3 Changed 5 years ago by korpios

fields_for_model is actually the wrong place to be sorting; you want to do that in ModelFormMetaclass.__new__, otherwise you'll miss sorting entirely on fields from declared_fields. Just reassign fields.keyOrder, e.g.:

        if opts.fields:
            fields.keyOrder = [f for f in opts.fields if f in fields.keyOrder]

But, yeah — the changes in r10062 made things go breaky over here. ^_^

comment:4 Changed 5 years ago by Alex

Nope this works fine(as the test show) since None will get put in the fields_dict and then get reassigned to the new value which doesn't change the ordering.

comment:5 Changed 5 years ago by Alex

Just to be clear I'm not necessarily saying this is the right place for it, just that it does work. I'm not sure why it would be causing a break for anyone, why do you have fields that aren't on the Model in the fields option?

comment:6 Changed 5 years ago by mbi

Thank you Alex, your patch worked as expected here.

comment:7 Changed 5 years ago by russellm

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

(In [10070]) Fixed #10512 -- Corrected the handling of extra fields on a ModelForm?. Thanks to Alex Gaynor for the patch.

comment:8 Changed 5 years ago by korpios

Alex: for the sake of ordering them. The fix in r10070 doesn't address that.

comment:9 Changed 5 years ago by Alex

Yes it does, the behavior of the SortedDict is that on update it doesn't reset the position of the key. So basically what happens is it tries to get a non Model field, it doesn't find it, it puts a None in the fields_dict, then later that gets updated which keeps that key position. And the tests demonstrate this.

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.