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)
Change History (9)
by , 11 years ago
Attachment: | quick fix for #21592.txt added |
---|
comment:1 by , 11 years ago
comment:2 by , 11 years ago
It would make sense to include updating the deleted_forms property with this patch
comment:3 by , 11 years ago
Has patch: | set |
---|---|
Owner: | changed from | to
Status: | new → assigned |
Triage Stage: | Unreviewed → 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 by , 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.
follow-up: 6 comment:5 by , 11 years ago
Easy pickings: | unset |
---|---|
Patch needs improvement: | set |
Type: | Uncategorized → New feature |
Version: | 1.6 → 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 by , 10 years ago
Owner: | removed |
---|---|
Status: | assigned → 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 by , 10 years ago
comment:8 by , 10 years ago
Resolution: | → duplicate |
---|---|
Status: | new → closed |
Closing as duplicate of #8165.
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