Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#10512 closed (fixed)

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

Reported by: Marco Bonetti Owned by: Alex Gaynor
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:


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/

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 Gaynor 8 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 8 years ago by Alex Gaynor

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedAccepted
Version: 1.0SVN

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 8 years ago by Alex Gaynor

comment:2 Changed 8 years ago by anonymous

Has patch: set
Needs tests: set

comment:3 Changed 8 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 8 years ago by Alex Gaynor

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 8 years ago by Alex Gaynor

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 8 years ago by Marco Bonetti

Thank you Alex, your patch worked as expected here.

comment:7 Changed 8 years ago by Russell Keith-Magee

Resolution: fixed
Status: newclosed

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

comment:8 Changed 8 years ago by korpios

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

comment:9 Changed 8 years ago by Alex Gaynor

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.

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