Opened 17 months ago

Closed 7 months 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: master
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 17 months ago.

Download all attachments as: .zip

Change History (9)

Changed 17 months ago by nickname123

comment:1 Changed 17 months ago by nickname123

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 Changed 17 months ago by nickname123

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

comment:3 Changed 17 months ago by pjrharley

  • Has patch set
  • Owner changed from nobody to pjrharley
  • Status changed from new to assigned
  • Triage Stage changed from Unreviewed to Accepted

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 Changed 17 months ago by nickname123

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 follow-up: Changed 16 months ago by timo

  • Easy pickings unset
  • Patch needs improvement set
  • Type changed from Uncategorized to New feature
  • Version changed from 1.6 to master

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.

comment:6 in reply to: ↑ 5 Changed 9 months ago by pjrharley

  • Owner pjrharley deleted
  • Status changed from assigned to new

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 Changed 8 months ago by jaap3

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 8 months ago by jaap3 (previous) (diff)

comment:8 Changed 7 months ago by timgraham

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

Closing as duplicate of #8165.

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