Opened 11 years ago

Closed 10 years ago

#21592 closed New feature (duplicate)

formset.ordered_forms should try to return ordered forms if is_valid() is false

Reported by: nickname123 Owned by:
Component: Forms Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

I am not sure if this should be a bug report or a feature request.

I think that formset.ordered_forms should be usable even if the formset is invalid. This is particularly useful in conjunction with Form Wizard for me, but I think there are plenty of other cases where this would be nice.

See https://github.com/django/django/blob/stable/1.6.x/django/forms/formsets.py#L216 where is_valid() == False causes an AttributeError

It would be useful for rendering templates. The particular data I am working is much easier to understand if it is displayed in order.

I was attempting to allow ordering the forms without moving to the next step in a form wizard. So I overrode the formset is_valid method like below with the intentions that the user could post ordering without moving to the next step:

class BaseOrderFormSet(BaseFormSet):
    """
        THIS FORMSET RETURNS INVALID IF THE USER SUBMITTED A REQUEST
        TO ADD ADDITIONAL FORMS OR UPDATE THE FORMSET INSTEAD OF
        ACTUALLY SUBMITTING THE DATA FOR SAVING
    """
    can_add_form = True
    can_update_formset = True
    def is_valid(self):
        # do not validate if we need to add another row
        return not ADD_FORM_KEY in self.data and not UPDATE_FORMSET_KEY in self.data and super(BaseOrderFormSet, self).is_valid()

(note FunkyBob's first comment seems out of context because we were having a conversation in django-users first but it wasn't relevant enough to the ticket to copy over and it spanned a lot of other comments)

[18:57] <gp> Why does the ordered_forms property of a formset required is_valid() to be True? https://github.com/django/django/blob/stable/1.6.x/django/forms/formsets.py#L216
[18:58] <FunkyBob> isn't that a fundamental of the FormWizard ? you can't progress until each 'form' is valid?
[18:59] <gp> FunkyBob: I am trying to allow users to order the forms in the formset but I cannot output them using ordered_forms until the formset is valid
[18:59] <gp> I don't understand why is_valid is important for outputting the forms in an ordered fashion
[19:00] <FunkyBob> I see
[19:01] <gp> My use case is the following:  I have added "add another" function like the admin.  Then I have added an "update" button that rerenders the template.  I was hoping to easily allow ordering without javascript
[19:02] <gp> Which I can still do... that is just how I ran into this
[19:04] <gp> Well by still can do I mean I can do it if I reimplement the ordered_forms property.  Didn't know if there was a specific reason this was only allowed for valid forms
[19:04] <gp> Or if maybe I should submit a bug/feature request
[19:05] <+bmispelon> gp: fwiw, the full test suite still passes if self.is_valid() is removed
[19:08] <gp> Ty.  I will submit a ticket and override it for my formset

Attachments (1)

quick fix for #21592.txt (2.5 KB ) - added by nickname123 11 years ago.

Download all attachments as: .zip

Change History (9)

by nickname123, 11 years ago

Attachment: quick fix for #21592.txt added

comment:1 by nickname123, 11 years ago

I've made a quick fix and overridden the property on a BaseFormSet subclass. It appears to work fine so far. The prefix might need to be generated more "properly". I will post it as a patch against trunk if this gets accepted

comment:2 by nickname123, 11 years ago

It would make sense to include updating the deleted_forms property with this patch

comment:3 by pjrharley, 11 years ago

Has patch: set
Owner: changed from nobody to pjrharley
Status: newassigned
Triage Stage: UnreviewedAccepted

PR here https://github.com/django/django/pull/2093

I tried doing the same for deleted forms but it broke a whole load of other tests that were expecting an empty list. It would be really easy to just fix the tests but it makes me wonder if anyone is relying on that behaviour? I'd guess not, so if it is ok to change I'll submit that too.

comment:4 by nickname123, 11 years ago

I think the patch needs to do just a little more. If the form isn't valid then cleaned_data will raise an exception won't it? Unless that changed in more recent versions of django. I'm stuck on an older version with the project I am currently working with.

I got around this by falling back to using form.data. See line 33 of the file I attached. This still doesn't work for forms without data. It makes sense to me for initial forms to have an ordering as well as it might be nice for ordered forms to return all forms that aren't deleted for easy rendering in templates.

comment:5 by Tim Graham, 11 years ago

Easy pickings: unset
Patch needs improvement: set
Type: UncategorizedNew feature
Version: 1.6master

I guess I'm not entirely sold on the concept given the cleaned_data issues as noted above, but I've never used ordered_forms before so I don't want to speak definitely on the issue.

in reply to:  5 comment:6 by pjrharley, 10 years ago

Owner: pjrharley removed
Status: assignednew

Replying to timo:

I guess I'm not entirely sold on the concept given the cleaned_data issues as noted above, but I've never used ordered_forms before so I don't want to speak definitely on the issue.

To be honest I don't really have any opinions on it either. I've never used them, I was just looking for some tickets to work on

I'm not sure I really understand the issue though either - the test I added passes, and it accesses cleaned_data after asserting that the form isn't valid.

I'll unassign it from myself for now, but I'm happy to work on it again if there's a clear way forward.

comment:7 by Jaap Roes, 10 years ago

This seems to be a duplicate of #14238 and #8165. I'm also butting my head against ordered formsets (in my case combined with the admin) seems to be nearly impossible.

Last edited 10 years ago by Jaap Roes (previous) (diff)

comment:8 by Tim Graham, 10 years ago

Resolution: duplicate
Status: newclosed

Closing as duplicate of #8165.

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