Opened 9 years ago

Last modified 4 years ago

#25139 assigned New feature

ModelFormSet: allow swapping unique values

Reported by: Jon Dufresne Owned by: Parth Patil
Component: Forms Version: dev
Severity: Normal Keywords:
Cc: johannes.linke@…, Sergey Fedoseev, Marco Beri Triage Stage: Someday/Maybe
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

As a user, I would expect to be able to swap unique values in an HTML form backed by a ModelFormSet. Turns out, this is not the case. The user must do a two step process of first setting the values to temporary value, then swapping the values. I have created a test case in the Django project that demonstrates my expectation in tests/model_formsets/tests.py:

    def test_swap_unique_values(self):
        p0 = Product.objects.create(slug='car-red')
        p1 = Product.objects.create(slug='car-blue')
        FormSet = modelformset_factory(Product, fields="__all__")
        data = {
            'form-TOTAL_FORMS': '2',
            'form-INITIAL_FORMS': '2',
            # Swap slug values.
            'form-0-id': p0.pk,
            'form-0-slug': 'car-blue',
            'form-1-id': p1.pk,
            'form-1-slug': 'car-red',
        }
        formset = FormSet(data)
        self.assertTrue(formset.is_valid())
        formset.save()

        p0 = Product.objects.get(pk=p0.pk)
        self.assertEquals(p0.slug, 'car-blue')
        p1 = Product.objects.get(pk=p1.pk)
        self.assertEquals(p1.slug, 'car-red')

However, the form fails to validate on the is_valid() line with the message "Product with this Slug already exists.".

When thinking about the details, I understand why this happens. But I don't think it should error in this way. Swapping values is a convenience that a program can handle without the intervention of a human.

Knowing that in the end all values will be unique, I think the Django form engine should go ahead with the steps necessary to get there. I think it is reasonable for a user to expect that they could swap unique values through an HTML form in this way.

As a point of reference, the following test passes. It demonstrates a user using temporary values.

    def test_swap_unique_values_two_step(self):
        p0 = Product.objects.create(slug='car-red')
        p1 = Product.objects.create(slug='car-blue')
        FormSet = modelformset_factory(Product, fields="__all__")
        data = {
            'form-TOTAL_FORMS': '2',
            'form-INITIAL_FORMS': '2',
            'form-0-id': p0.pk,
            'form-0-slug': 'temp0',
            'form-1-id': p1.pk,
            'form-1-slug': 'temp1',
        }
        formset = FormSet(data)
        self.assertTrue(formset.is_valid())
        formset.save()

        data = {
            'form-TOTAL_FORMS': '2',
            'form-INITIAL_FORMS': '2',
            # Swap slug values.
            'form-0-id': p0.pk,
            'form-0-slug': 'car-blue',
            'form-1-id': p1.pk,
            'form-1-slug': 'car-red',
        }
        formset = FormSet(data)
        self.assertTrue(formset.is_valid())
        formset.save()

        p0 = Product.objects.get(pk=p0.pk)
        self.assertEquals(p0.slug, 'car-blue')
        p1 = Product.objects.get(pk=p1.pk)
        self.assertEquals(p1.slug, 'car-red')

Change History (8)

comment:1 by Claude Paroz, 9 years ago

Triage Stage: UnreviewedAccepted

Yes, ideally this should work. Now the implementation might not be trivial, as unique checking exists also at the database level.

comment:2 by karyon, 8 years ago

i hit the same bug, and i wasn't able to come up with a workaround that covers all the edge cases and that's not manually fiddling with that data dictionary and parsing the strings in there.

any fix or pointers how to fix would be appreciated, as this is a user-facing bug that we have no acceptable workaround for right now.

comment:3 by karyon, 8 years ago

Cc: johannes.linke@… added

comment:4 by Parth Patil, 5 years ago

Owner: changed from nobody to Parth Patil
Status: newassigned

I don't think so this is feasible, this will require n(n-1)/2 comparisons to determine whether any two of the models are swapped.
This looks easy in the above case but won't be a good idea for a general case.

Correct me if I'm wrong or if anyone has a better approach for implementation of this feature. I would like to work on it.

But as of now, I feel this ticket should be closed

comment:5 by Carlton Gibson, 5 years ago

Triage Stage: AcceptedSomeday/Maybe

Comment on the mailing list:

What would it take: fetching the set of to_be_unique values and comparing it to the set of values submitted and then assigning both within a transaction... — meh, possible but I'm not sure it'd be clean, or something we'd want to bundle-in, even if we had the solution available, vs, putting it in a third-party package...

I'll bump this to Someday/Maybe for now. Seems like a nice-to-have but easier to think about with a proposal in hand.

comment:6 by Sergey Fedoseev, 4 years ago

Cc: Sergey Fedoseev added

comment:7 by Mariusz Felisiak, 4 years ago

#31932 is a related ticket for excluding objects marked for deletion from unique checks. It looks more feasible.

comment:8 by Marco Beri, 4 years ago

Cc: Marco Beri added
Note: See TracTickets for help on using tickets.
Back to Top