jrief/django-admin-sortable2

Bug in v2.2.4 ?

Opened this issue · 12 comments

jedie commented

I seems to me, that v2.2.3 -> v2.2.4 introduces a bug: Adding a new entry to a existing object will add it not always as a new last entry... Sometimes it will be added as the second one.

I found this bug, because a integration tests failed. It looks like:

#...

response = self.client.post(
    path='/admin/foo/add/',
    data=get_test_form_data(
        title='A Test Entry',
        **{
            'chapters-TOTAL_FORMS': '1',
            'chapters-0-title': 'Chapter One',
            'chapters-0-position': '0',
        },
    ),
)
test_item = Foo.objects.get()
self.assertEqual(test_item.title, 'A Test Entry')

#...

response = self.client.post(
    path=f'/admin/foo/{test_item.pk}/change/',
    data=get_test_form_data(
        title='A Test Entry',
        **{
            'chapters-TOTAL_FORMS': '2',
            'chapters-INITIAL_FORMS': '1',
            'chapters-0-id': str(chapter1.pk),
            'chapters-0-test': str(test_item.pk),
            'chapters-0-title': 'Chapter One',
            'chapters-0-position': '1',
            'chapters-1-test': str(test_item.pk),
            'chapters-1-title': 'Chapter Two',
            'chapters-1-position': '0',
            'chapters-__prefix__-test': str(test_item.pk),
            'chapters-__prefix__-position': '0',
        },
    ),
)

test_item.refresh_from_db()
self.assertEqual(
    list(test_item.chapters.values_list('title', flat=True)), ['Chapter One', 'Chapter Two']
)

#...

With v2.2.3 it's correct: ['Chapter One', 'Chapter Two']
With v2.2.4 it's: ['Chapter Two', 'Chapter One']

jrief commented

@ldeluigi may it be that this issue is a regression from merging PR #413?

ldeluigi commented

Didn't we just change the save_new method? It's not supposed to be invoked with updates I think

jrief commented

@jedie can you please test, if reverting PR #413 solves your problem? We then need a strategy for a unit test so that this regression does not occur anymore. Could you please adopt the current testapp to reflect this?

ldeluigi commented

Btw, now that I'm looking again it seems that the test @jedie is running is a false negative: in the form data, they are uploading "Chapter One" with position 1 and "Chapter Two" with position 0. Thus, the result containing "Chapter Two" first and "Chapter One" second should be considered correct.

jedie commented

This is exactly what the browser also send. Or exists the bug in the JavaScript part that adds the field?!?

Think the playwright tests should cover this, isn't it?

ldeluigi commented

It's not a bug, it's how the library works. Even if you change the order of an item in the admin page, "Chapter One" will always have index 0 written in the form field labels, while its position field will change value according to its position. This means that it's the position field to be responsible for determining the position of Chapter One, even if the index is of the item inside the form fields is still 0.

ldeluigi commented

From an HTML perspective, chapters-0-test is just a label and the 0 is not an index at all.

PS: I've tested the latest version myself and to me it's working as intended

tmsi10 commented

Changes in 2.2.4 causes "unexpected" ordering from user's perspective, in my opinion.

  1. Define a model with m2m fields to another model (incl. inline)
  2. Create a model instance with 1 inline object in admin site (now the ordering field is 0)
  3. Drag the inline object and save again (now the ordering field is 1)
  4. Add another inline object in the same model instance (in UI, it appears at the bottom)
  5. Click "Save and continue editing" (new object has the ordering field = 0)

The newly added row appears at the top.

models.py

class Child(models.Model):
    name = models.CharField(max_length=200)

class Parent(models.Model):
    children = models.ManyToManyField(Child, through='Through')

class Through(models.Model):
    child = models.ForeignKey(Child, on_delete=models.CASCADE)
    parent = models.ForeignKey(Parent, on_delete=models.CASCADE)
    order = models.PositiveIntegerField(default=0)

    class Meta:
        ordering = ['order']

admin.py

class ThroughInline(SortableStackedInline):
    model = Through
    readonly_fields = ['_order']

    def _order(self, obj):
        return obj.order

@admin.register(Parent)
class ParentAdmin(SortableAdminBase, admin.ModelAdmin):
    inlines = [ThroughInline]

@admin.register(Child)
class ChildAdmin(admin.ModelAdmin):
    pass
jrief commented

@tmsi10 I never intended admin-sortable to be able to sort many-to-many fields. Maybe for this we need another widget anyway. In one of my other projects I created such a widget:

https://django-formset.fly.dev/dual-selector/#sortable-dual-selector-widget

Would this help? If so, I might port it to django-admin-sortable2 since it also is based on the Sortable.js library.

tmsi10 commented

@jrief the mentioned case is similar to the example described in https://django-admin-sortable2.readthedocs.io/en/latest/usage.html#sortable-many-to-many-relations-with-sortable-inlines

Using inlines allows user to fill extra data when associating the records so I guess it cannot be replaced by the widget

ldeluigi commented

@tmsi10 to me your issue seems to be related to how the javascript sets the value of the ordering field when a new inline form gets added, which should equal one more the number of inlines present. Is that right?

In other words, I think that 2.2.4 fixed a bug that worked as a feature by hiding a javascript side bug

tmsi10 commented

JavaScript side issue, yes, if there is no intention to have (some) magic values, either 0, nullish or negative number, implicitly representing "let the python side append those records at the end (in sequence)".

It seems that it is pre-populated as 0 (field's default) if there is no dragging (the JavaScript sorting logic).

2.2.4 does fix a bug for the "save as new" scenario.

  • 👍1