Opened 7 years ago

Closed 6 years ago

Last modified 4 years ago

#9493 closed (fixed)

Formsets with data that violates unique constraints across forms are not marked invalid

Reported by: Alex Owned by: Alex
Component: Forms Version: 1.0
Severity: Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description

If I have a form with a unique name field, and I submit data that is a dupe of something in the db, it fails validation, however if I have a formset and I submit the same data 2x in the same formset, it passes validation and fails at the save level.

Attachments (14)

formset-unique.diff (2.9 KB) - added by Alex 7 years ago.
needs test for unique_together and docs
formset-unique.2.diff (3.6 KB) - added by Alex 7 years ago.
added tests for unique_together
formset-unique.3.diff (4.6 KB) - added by Alex 7 years ago.
made it a little more effiecient and add docs
formset_unique.4.diff (4.3 KB) - added by mrts 7 years ago.
Minor nitpicking
formset-unique.4.diff (4.5 KB) - added by Alex 7 years ago.
formset-unique.5.diff (4.5 KB) - added by Alex 7 years ago.
formset-unique.6.diff (5.1 KB) - added by Alex 7 years ago.
formset-unique.7.diff (8.3 KB) - added by Alex 6 years ago.
formset-unique.8.diff (7.8 KB) - added by Alex 6 years ago.
formset-unique.9.diff (7.8 KB) - added by Alex 6 years ago.
formset-unique.10.diff (11.3 KB) - added by Alex 6 years ago.
added the date stuff and removed code duplication. Not sure what to do to make the formset errors more user friendly. Also needs tests in the worst way (I'll do this during the EDC sprint in all likelyhood)
formset-unique.11.diff (13.4 KB) - added by Alex 6 years ago.
formset-unique.12.diff (14.6 KB) - added by Alex 6 years ago.
formset-unique.13.diff (16.0 KB) - added by Alex 6 years ago.

Download all attachments as: .zip

Change History (30)

comment:1 Changed 7 years ago by brosner

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from Alex to brosner
  • Patch needs improvement unset
  • Status changed from new to assigned
  • Summary changed from Formsets with data that violates unique constraints across forms are not mared invalid to Formsets with data that violates unique constraints across forms are not marked invalid
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 7 years ago by brosner

#8882 was really the original problem, but since that ticket grew a bit too big, lets use this ticket to track this individual problem.

Changed 7 years ago by Alex

needs test for unique_together and docs

Changed 7 years ago by Alex

added tests for unique_together

Changed 7 years ago by Alex

made it a little more effiecient and add docs

comment:3 Changed 7 years ago by mrts

Thanks to you both for working on this!

I've attached an updated patch with the following minor changes -- sorry for the nitpicking, the patch looks generally good and works well. Also, note that the updated patch has been tested (both by running the testcase and in admin).

1

Why do we need to copy the list on line 943 of the patch

unique_together = self.forms[0].instance._meta.unique_together[:]

and discard it right afterwards (by reusing the unique_together variable name on next line)? Wouldn't

unique_together = [check for check in self.forms[0].instance._meta.unique_together if [True for field in check if field in self.forms[0].fields]]

be more appropriate?

2

Similarly, wouldn't it make sense to avoid using the bad_fields intermediary and use errors directly instead?

3

If there is only one field (i.e. it is a unique, not unique together test), it probably makes sense to call force_unicode (or indeed, just unicode) directly instead of get_text_list:

if len(unique_check) == 1:
    errors.append(_(u"You have entered duplicate data for %(field)s, all %(field)ss should be unique." % { 'field': force_unicode(field), }))

4

I'd say creating tuple([form.cleaned_data[field] for field in unique_check]) twice in if tuple([form.cleaned_data[field] for field in unique_check]) in data: ... else: data.add(tuple([form.cleaned_data[field] for field in unique_check])) should be avoided.

Changed 7 years ago by mrts

Minor nitpicking

comment:4 Changed 7 years ago by Alex

1) The reason for this is it leads to long and unruly lines, I think this makes it a tad easier to follow.

2) Yes, this can be done, I don't think there is much of a speed difference, and I do think it's easier to read the other way. But however(FWIW this is how it's done for ModelForms as well).

3) Yeah, that should be changed, you need to do field[0] though, as field itself is a tuple.

4) Yes.

comment:5 Changed 7 years ago by mrts

Alex, if you have time please take a look at the attached patch and improve it further if you see something worth amending (it looks final-ish to me).

(As for The reason for this is it leads to long and unruly lines, I think this makes it a tad easier to follow, I mainly had in main avoiding copying (i.e. foo = a_list instead of foo = a_list[:]. Copying is usually required if the local copy is modified -- but not in this case.)

comment:6 Changed 7 years ago by mrts

3) Yeah, that should be changed, you need to do field[0] though, as field itself is a tuple. -- field is a field from unique_check (a tuple), not unique_checks (a list of tuples), thus not a . The patch has a bit different structure, although your idea remains exactly the same.

comment:7 Changed 7 years ago by mrts

... "thus not a tuple" :)

Changed 7 years ago by Alex

comment:8 Changed 7 years ago by Alex

  • Owner changed from brosner to Alex
  • Status changed from assigned to new

Changed 7 years ago by Alex

Changed 7 years ago by Alex

comment:9 Changed 6 years ago by mrts

  • Has patch set
  • milestone set to 1.1

It's a bug in both 1.0 and 1.1, marking as such.

Changed 6 years ago by Alex

Changed 6 years ago by Alex

comment:10 Changed 6 years ago by Alex

  • Triage Stage changed from Accepted to Ready for checkin

comment:11 Changed 6 years ago by mrts

#10769, #10770 were duplicates.

Changed 6 years ago by Alex

comment:12 Changed 6 years ago by russellm

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

On the whole, this patch looks good. However, I have two comments:

  1. The patch needs to be updated to include the formset analog of #10134/[10646] (avoiding duplication of logic, if possible).
  2. While this patch correctly catches the error condition, I'm not satisfied with the way the error is reported. Consider the use case where you have 100 inline forms - a top level "There's a duplicate" error doesn't really help identify the pair of forms that is actually causing the problem. In addition to the formset level error, the individual forms should be marked as being in error.

Changed 6 years ago by Alex

added the date stuff and removed code duplication. Not sure what to do to make the formset errors more user friendly. Also needs tests in the worst way (I'll do this during the EDC sprint in all likelyhood)

Changed 6 years ago by Alex

Changed 6 years ago by Alex

Changed 6 years ago by Alex

comment:13 Changed 6 years ago by jacob

  • Triage Stage changed from Accepted to Ready for checkin

comment:14 Changed 6 years ago by russellm

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

(In [10682]) Fixed #9493 -- Corrected error handling of formsets that violate unique constraints across the component forms. Thanks to Alex Gaynor for the patch.

comment:15 Changed 6 years ago by russellm

(In [10718]) [1.0.X] Fixed #9493 -- Corrected error handling of formsets that violate unique constraints across the component forms. Thanks to Alex Gaynor for the patch.

Merge of r10682 from trunk.

comment:16 Changed 4 years ago by jacob

  • milestone 1.1 deleted

Milestone 1.1 deleted

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