Opened 8 years ago

Closed 7 years ago

#5923 closed (fixed)

required forms count field added into formsets

Reported by: jakub_vysoky Owned by: jkocherhans
Component: Forms Version: newforms-admin
Severity: Keywords: nfa-someday sprintdec01 edit inline add
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

We added var containing required forms count, instead of having it counted from ( total_forms - num_extra ). We needed it for javascript edit inline add.

Attachments (4)

required-forms-count-field.diff (3.0 KB) - added by jakub_vysoky 8 years ago.
required forms count field added into formsets
help-text-edit-inline-tests.diff (21 bytes) - added by jakub_vysoky 8 years ago.
sorry - i add this file by mistake
required-forms-count-field-tests.diff (6.1 KB) - added by jakub_vysoky 8 years ago.
django tests to work with this ticket
required-forms-count.diff (8.1 KB) - added by James Turnbull <james@…> 8 years ago.
Merged patch file that applies to nfa-trunk

Download all attachments as: .zip

Change History (12)

Changed 8 years ago by jakub_vysoky

required forms count field added into formsets

comment:1 Changed 8 years ago by tvrg

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

I'm voting for the addition of this field too.

It would probably need a design decision though

comment:2 Changed 8 years ago by jakub_vysoky

  • Keywords sprintdec01 added

Changed 8 years ago by jakub_vysoky

sorry - i add this file by mistake

Changed 8 years ago by jakub_vysoky

django tests to work with this ticket

comment:3 Changed 8 years ago by jkocherhans

  • Owner changed from nobody to jkocherhans
  • Status changed from new to assigned
  • Triage Stage changed from Unreviewed to Accepted

At first glance this looks like the right solution to me, but I need to take a closer look.

comment:4 Changed 8 years ago by brosner

  • Keywords nfa-someday added

This ticket isn't critical to the merge of newforms-admin. Tagging with nfa-someday.

comment:5 Changed 8 years ago by tvrg

I think the patch should be applied. You may be correct that it isn't critical to the merge of nfa but the patch looks ready for checking to me, and i've tested it extensively (aka using it in a production environment) without any issue whatsoever.

Without this patch I didn't find any way to reliably insert inline forms through ajax.
This patch is fully backwards compatible, works fine, has no risk, has tests, ...

Unless you want doc's for every hidden field i don't see why you should postpone applying the patch.

comment:6 Changed 8 years ago by brosner

  • Patch needs improvement set

Please create one patch that can be applied to the source tree. Then I will be able to bump this up to RFC.

Changed 8 years ago by James Turnbull <james@…>

Merged patch file that applies to nfa-trunk

comment:7 Changed 8 years ago by James Turnbull <james@…>

  • Patch needs improvement unset

I'm also rooting for this to ticket to be resolved before merge.

Have tested and uploaded a patch file which replaces the previous two.

comment:8 Changed 7 years ago by jkocherhans

  • Resolution set to fixed
  • Status changed from assigned to closed

This was fixed in [7270]. Thanks jakub_vysoky and James Turnbull.

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