[patch] Eliminate unintuitive behavior when using edit_inline with no core fields specified

[rmunn@…]

Using Django revision 426, Apache+FCGI, MySQL 4.1.11.

I created the basic polls and choices models as per the second tutorial:

class Poll(meta.Model):
    fields = (
        meta.CharField('question', maxlength=200),
        meta.DateTimeField('pub_date', 'date published'),
    )
    admin = meta.Admin(
        fields = (
            (None, {'fields': ('question',)}),
            ('Date information', {'fields': ('pub_date',), 'classes': 'collapse'}),
        )
    )
    def __repr__(self):
        return self.question

class Choice(meta.Model):
    fields = (
        meta.ForeignKey(Poll, edit_inline=True, num_in_admin=3, num_extra_on_change=3),
        meta.CharField('choice', maxlength=200),
        meta.IntegerField('votes'),
    )
    def __repr__(self):
        return self.choice

I created a poll in admin interface, with question "What's up?" and no choices for the moment:

>>> from django.models.polls import polls, choices
>>> polls.get_list()
[What's up?]
>>> choices.get_list()
[]

I added three choices -- "one", "two", and "three" -- in the admin interface, choosing the "Save and continue editing" button:

>>> polls.get_list()
[What's up?]
>>> p = polls.get_object(question__exact="What's up?")
>>> p.get_choice_list()
[one, two, three]

In MySQL:

mysql> select * from polls_choices ;
+----+---------+--------+-------+
| id | poll_id | choice | votes |
+----+---------+--------+-------+
|  1 |       1 | one    |     0 |
|  2 |       1 | two    |     0 |
|  3 |       1 | three  |     0 |
+----+---------+--------+-------+
3 rows in set (0.00 sec)

The three choices I just added showed up just fine in the admin interface, along with three blank slots for more choices to be added. I added three more choices -- "four", "five", and "six" -- in the admin interface and clicked "Save and continue editing" again. Suddenly the first three choices disappeared, and only the latest three were showing in the admin interface, in the order "six", "five", and "four".

In Python:

>>> p.get_choice_list()
[six, five, four]

In MySQL:

mysql> select * from polls_choices ;
+----+---------+--------+-------+
| id | poll_id | choice | votes |
+----+---------+--------+-------+
|  6 |       1 | six    |     0 |
|  5 |       1 | five   |     0 |
|  4 |       1 | four   |     0 |
+----+---------+--------+-------+
3 rows in set (0.00 sec)

Adding more choices in the admin interface should add to, not replace, the already-existing choices.

[rmunn@…]

Correction: from looking at the MySQL ID's, it looks like the new choices didn't overwrite the old ones. Rather, it looks like they were added and the old choices were deleted. The net effect is the same, though: the old choices should not have gone away.

[hugo <gb@…>]

shouldn't some fields be marked with core=True if you use edit_inline=True? Nonetheless data shouldn't be removed, of course.

[rmunn@…]

You're right; setting one field to core=True got rid of the behavior. So it was working as designed, sort of. But the behavior when no fields are set to core=True is unintuitive and needs to be changed. I'll investigate and see if I can suggest a patch.

[rmunn@…]

The reason for the default behavior lies in the manipulator_save() function from django/core/meta/__init__.py. It sets two flags, all_cores_given and all_cores_blank, to True, and then once a core field is found, sets the appropriate flag to False depending on the state (blank or non-blank) of the core field. But if there are no core fields at all, then both flags end up being True.

One possible solution would be not to delete items if no core records have been specified. This would make the default behavior a lot less counter-intuitive. Here's a patch that would accomplish that:

Index: django/core/meta/__init__.py
===================================================================
--- django/core/meta/__init__.py        (revision 428)
+++ django/core/meta/__init__.py        (working copy)
@@ -1441,7 +1441,8 @@
                     except ObjectDoesNotExist:
                         pass

-            for f in rel_opts.fields:
+            core_fields = [f for f in rel_opts.fields if f.core]
+            for f in core_fields:
                 if f.core and not isinstance(f, FileField) and f.get_manipulator_new_data(rel_new_data, rel=True) in (None, ''):
                     all_cores_given = False
                 elif f.core and not isinstance(f, FileField) and f.get_manipulator_new_data(rel_new_data, rel=True) not in (None, ''):
@@ -1498,7 +1499,7 @@

             # If, in the change stage, all of the core fields were blank and
             # the primary key (ID) was provided, delete the item.
-            if change and all_cores_blank and rel_new_data.has_key(rel_opts.pk.name) and rel_new_data[rel_opts.pk.name][0]:
+            if change and all_cores_blank and core_fields and rel_new_data.has_key(rel_opts.pk.name) and rel_new_data[rel_opts.pk.name][0]:
                 new_rel_obj.delete()
                 self.fields_deleted.append('%s "%r"' % (rel_opts.verbose_name, old_rel_obj))

That patch leaves the redundant "if f.core" checks in the for loop, simply because that way it was a smaller patch. If this actually gets applied, those checks should probably be eliminated as well.

Another possibility would be that if no fields are specified as core fields, then all fields are treated as core fields. (E.g., if core_fields ends up being [] in the above patch, then you do core_fields = rel_opts.fields). That gets rid of the "and core_fields" check in the "if change and all_cores_blank" section.

I've updated the ticket description to better match my proposed solution.

[Adrian Holovaty]

The problem with "not to delete items if no core records have been specified" is: That makes it impossible to delete edit_inline items from the interface.

I'm closing this ticket because the "django-admin.py validate" command complains if you have an edit_inline object with no core=True fields.