Code

Opened 6 years ago

Closed 10 months ago

Last modified 2 months ago

#9532 closed New feature (fixed)

Add min_num and validate_min on formsets

Reported by: gsf Owned by: yokomizor
Component: Forms Version: master
Severity: Normal Keywords:
Cc: mathijs@…, tgecho, kitsunde@…, timograham@…, me@…, tchristiansen Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: yes UI/UX: no

Description

Formsets have a max_num for limiting the number of forms, but no way to ensure a minimum.

Use case: I want an address formset that displays all addresses that have been created and no extras, unless there are no addresses, then one empty form should be displayed. I would set extra=0 and min_num=1.

Patch included is just a beginning. I may have missed a function or two and I haven't touched docs or tests.

Attachments (3)

formsets_min_num.diff (3.1 KB) - added by gsf 6 years ago.
9532.patch (11.7 KB) - added by michal@… 2 years ago.
Added min_num argument to formset_factory, updated docs
9532_2.patch (11.7 KB) - added by michal@… 2 years ago.
Removed maxDiff = None from test case

Download all attachments as: .zip

Change History (36)

Changed 6 years ago by gsf

comment:1 Changed 5 years ago by jacob

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

comment:2 follow-up: Changed 3 years ago by charliebubblegum

double +1. This was very hard for me to work around, and would be a very logical addition.

comment:3 in reply to: ↑ 2 Changed 3 years ago by anonymous

Replying to charliebubblegum:

double +1. This was very hard for me to work around, and would be a very logical addition.

agreed. will like to hear inputs from devs.

comment:4 Changed 3 years ago by dokterbob

  • Cc mathijs@… added

Bump! It would make a lot of sense to have this kind of functionality built into Django, and the effort would not be gigantic.

comment:5 Changed 3 years ago by lukeplant

  • Severity set to Normal
  • Type set to New feature

comment:6 Changed 3 years ago by tgecho

  • Cc tgecho added
  • Easy pickings unset
  • UI/UX unset

comment:7 Changed 2 years ago by ted.tieken@…

  • Easy pickings set

Changed 2 years ago by michal@…

Added min_num argument to formset_factory, updated docs

Changed 2 years ago by michal@…

Removed maxDiff = None from test case

comment:8 Changed 22 months ago by kitsunde

  • Cc kitsunde@… added

comment:9 Changed 22 months ago by thejaswi_puthraya

  • Has patch set

comment:10 Changed 18 months ago by claudep

  • Triage Stage changed from Design decision needed to Accepted

#17642 has a patch to apply this feature to admin inlines.

comment:11 Changed 18 months ago by wim@…

Hi, probably it is a good idea to add min_num to modelformset_factory and inlineformset_factory as well.

comment:12 Changed 17 months ago by AnkitBagaria

  • Owner changed from nobody to AnkitBagaria
  • Status changed from new to assigned

comment:13 Changed 17 months ago by tback

Hi AnkitBagria, I just started working on this too. I'm in Utrecht right now, I'm also in #django-sprint. My nick is tback. Maybe we can work on this together.

Last edited 17 months ago by tback (previous) (diff)

comment:15 Changed 16 months ago by tback

Sorry, it's not fixed yet. BaseModelFormsets are still broken. The current state of my work is here: https://github.com/tback/django/tree/formset-min-num

comment:16 Changed 16 months ago by AnkitBagaria

  • Owner AnkitBagaria deleted
  • Status changed from assigned to new

comment:17 Changed 16 months ago by AnkitBagaria

  • Owner set to AnkitBagaria
  • Status changed from new to assigned

comment:18 Changed 16 months ago by AnkitBagaria

  • Owner AnkitBagaria deleted
  • Status changed from assigned to new

comment:19 Changed 14 months ago by yokomizor

  • Owner set to yokomizor
  • Status changed from new to assigned

comment:20 Changed 14 months ago by yokomizor

comment:21 Changed 12 months ago by timo

  • Cc timograham@… added
  • Needs documentation set
  • Patch needs improvement set
  • Summary changed from min_num on formsets to Add min_num on formsets

Thanks, the patch no longer merges cleanly so it needs to be rebased. It also needs documentation including a mention in the 1.7 release notes. If you could add that, I will try to review and commit it.

comment:22 Changed 12 months ago by mjtamlyn

I'm pretty concerned about what the implementation for this looks like. It seems to me that every formset on every Django site would now have to have the extra "MIN" field. Whilst in theory this shouldn't break anyone's sites, it will almost certainly break everyone's tests. I'd prefer a less backwardsly problematic patch for a relatively minor feature.

That said, I think the feature is probably useful. Perhaps a patch could be devised which means the "MAX" field is not required either?

comment:23 Changed 11 months ago by Rogério Yokomizo <me@…>

  • Cc me@… added
  • Summary changed from Add min_num on formsets to Add min_num and validate_min on formsets

Patch rebased.

Could someone please review?

https://github.com/django/django/pull/1444

About backward problematic, I don't know how can I do that. Any suggestions?

comment:24 Changed 10 months ago by timo

  • Needs documentation unset

I left comments on the pull request. I'm not sure there's actually any backwards compatibility concerns as the parameters aren't actually required in POST data as far as I can tell.

comment:25 Changed 10 months ago by yokomizor

Thanks timo!

I adjust the pull request based on your comments.

I rebased it. The old commit with comments can be seen at https://github.com/yokomizor/django/commit/35d40939f89990c01f4c31ac10a29e3ec809d26e.

Could someone please review?

https://github.com/django/django/pull/1444

About the backward problematic, maybe the only backwards compatibility concerns are in tests that test formset.as_X() return.

comment:26 Changed 10 months ago by Tim Graham <timograham@…>

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

In df27803a55ffea71d468a6aab0d897caa2b7a797:

Fixed #9532 -- Added min_num and validate_min on formsets.

Thanks gsf for the suggestion.

comment:27 Changed 10 months ago by timo

It would also be useful to add this to modelformset_factory, inlineformset_factory, etc. Ticket #17642 is open for that issue -- it has a patch and the branch from tback above also looks relevant.

comment:28 Changed 8 months ago by tchristiansen

  • Cc tchristiansen added

Why is min_num added to extra?
This makes no sense to me, and makes extra useless in combination with min_num.

extra=3 and min_num=3 resulting 6 forms, which is absolutely not what I would expect (3 forms).

comment:29 Changed 8 months ago by yokomizor

I think this is there because when you set min_num as 3 you want at least 3 extra forms.

Maybe we should remove it, or just clarify in the docs.

What about set extra equals to min_num when min_num is greater than extra?

comment:30 Changed 7 months ago by timo

I agree a documentation improvement would be welcome.

@tchristiansen, is there a case you foresee where min_num=X and you wouldn't want at least X extra forms?

comment:31 Changed 2 months ago by melinath

@timo I would set min_num=3 if there should be at least three things. I would set extra=3 if I wanted three blank forms presented.

The implementation here means that there will *always* be six blank forms - existing objects are *not* taken into account.

To put it another way, if min_num is 3, and extra is 2, and there are already 2 existing objects, I would expect to have either 4 forms (2 extra on top of existing objects) or 5 forms (2 extra on top of the minimum.) With the code as it is, I will instead get seven forms - two for the existing objects, and an additional seven from extra.

Ran into this behavior while working on #17642. Possible this is only a modelformset issue.

comment:32 Changed 2 months ago by timo

I agree existing objects should be taken into account so the number of blank forms should be (min_num + extra - existing objects). Let's call it a bug and fix it now.

comment:33 Changed 2 months ago by melinath

Opened a new ticket for this: #22628

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.