Bug in v2.2.4 ?

[jedie]

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]

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

[ldeluigi]

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

[jrief]

@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]

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]

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]

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]

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]

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]

@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]

@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]

@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]

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.

[raphaelm]

For me, 2.2.4 is broken, too, without any ManyToMany shenanigans.

I have a straightforward ModelAdmin with a StackedInline. The model in the StackedInline has a position field with a default=0, as recommeded in the docs of django-admin-sortable2.

Now, I have an object with three dependent objects already listed. They have position=1, position=2, position=3. Then, I add one additional dependent object in the StackedInline.

With 2.2.3, the new object would have gotten position=4 and would have appeared at the end, as expected.

With 2.2.4, the new object is getting position=0.

I think the correct fix for #402 would have been

if order_field_value is None or order_field_value <= 0:

probably? Relying on the order_field_value is None case is in contrast to the recommended model field.

[ldeluigi]

See #416