[Django] #21592: formset.ordered_forms should try to return ordered forms if is_valid() is false
Django
false
-------------------------------+--------------------
Reporter: nickname123 | Owner: nobody
Type: Uncategorized | Status: new
Component: Forms | Version: 1.6
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------+--------------------
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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21592>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
false
Reporter: nickname123 | Owner: nobody
Type: Uncategorized | Status: new
Component: Forms | Version: 1.6
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by nickname123):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
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
--
Ticket URL: <https://code.djangoproject.com/ticket/21592#comment:1>
Django
false
Reporter: nickname123 | Owner: nobody
Type: Uncategorized | Status: new
Component: Forms | Version: 1.6
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by nickname123):
It would make sense to include updating the deleted_forms property with
this patch
--
Ticket URL: <https://code.djangoproject.com/ticket/21592#comment:2>
Django
false
Reporter: nickname123 | Owner: pjrharley
Type: Uncategorized | Status: assigned
Component: Forms | Version: 1.6
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by pjrharley):
* status: new => assigned
* owner: nobody => pjrharley
* has_patch: 0 => 1
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21592#comment:3>
Django
false
Reporter: nickname123 | Owner: pjrharley
Type: Uncategorized | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21592#comment:4>
Django
false
Reporter: nickname123 | Owner: pjrharley
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+-------------------------------------
Changes (by timo):
* needs_better_patch: 0 => 1
* version: 1.6 => master
* type: Uncategorized => New feature
* easy: 1 => 0
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21592#comment:5>
Django
false
Reporter: nickname123 | Owner:
Type: New feature | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-----------------------------+------------------------------------
* status: assigned => new
* owner: pjrharley =>
Comment:
Replying to [comment:5 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21592#comment:6>
Django
false
Reporter: nickname123 | Owner:
Type: New feature | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-----------------------------+------------------------------------
Comment (by jaap3):
Isn't this a duplicate of #14238
--
Ticket URL: <https://code.djangoproject.com/ticket/21592#comment:7>
Django
false
Reporter: nickname123 | Owner:
Type: New feature | Status: closed
Component: Forms | Version: master
Severity: Normal | Resolution: duplicate
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* status: new => closed
* resolution: => duplicate
Comment:
Closing as duplicate of #8165.
--
Ticket URL: <https://code.djangoproject.com/ticket/21592#comment:8>