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

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

needs test for unique_together and docs

added tests for unique_together

made it a little more effiecient and add docs

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.

Minor nitpicking

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.

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.)

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.

... "thus not a tuple" :)

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

#10769, #10770 were duplicates.

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.

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)

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

(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.