Change message describing the deletion of inline objects in admin has no id available

[Vlada Macek]

Sometimes the developer wishes the id to be a part of the Model.__str__.
Such model could of course be as inline in other model admin.
When such inline object is deleted, the save_related() of the formset deletes it just before the first call of construct_change_message() in ​https://github.com/django/django/blob/master/django/contrib/admin/options.py#L1586 chimes in.

At that moment, the id is None and "None" makes it to the "Deleted ..." LogEntry.change_message for the formset.

[Vlada Macek]

I'm not entirely sure if mere shifting of construct_change_message() a few lines up is safe, hence I'm not providing a patch.

[Mariusz Felisiak]

Replying to Vlada Macek:

I'm not entirely sure if mere shifting of construct_change_message() a few lines up is safe, hence I'm not providing a patch.

Unfortunately, it's not, because new_objects will not exist, changed_objects will not take changes into account, and we will not have the list of deleted_objects. We could create a copy of obj before deletion:

                     return self.response_add(request, new_object)                                                                                     
diff --git a/django/forms/models.py b/django/forms/models.py                                                                                           
index 5d115458a1..eb9034fb43 100644                                                                                                                    
--- a/django/forms/models.py                                                                                                                           
+++ b/django/forms/models.py                                                                                                                           
@@ -790,7 +790,7 @@ class BaseModelFormSet(BaseFormSet):                                                                                               
             if obj.pk is None:                                                                                                                        
                 continue                                                                                                                              
             if form in forms_to_delete:                                                                                                               
-                self.deleted_objects.append(obj)                                                                                                      
+                self.deleted_objects.append(copy.deepcopy(obj))                                                                                       
                 self.delete_existing(obj, commit=commit)                                                                                              
             elif form.has_changed():                                                                                                                  
                 self.changed_objects.append((obj, form.changed_data))

but I'm not sure it's worth complexity. Closing as needsinfo, but I'm be happy to re-open if we will have a reasonable proposition.

[Richard Laager]

Replying to Mariusz Felisiak:

We could create a copy of obj before deletion:
...code snipped...
but I'm not sure it's worth complexity.

I just ran into this issue. That change (with the obvious addition of import copy) works as expected. Adding a copy doesn't seem particularly complex to me.

I don't see an easier way to fix it. save_existing_objects() takes a commit argument, but 1) passing down commit=False would be complicated (and it seems like that would end up breaking compatibility in ModelAdmin.save_formset()), 2) that probably has non-trivial consequences, and 3) the deletions still need to happen, so then something needs to do that (possibly a second call to save_existing_objects() with commit=True, but then we're creating another round of the first two concerns).

Note that the scenario under which this occurs ("Sometimes the developer wishes the id to be a part of the Model.str.") is literally the default. Model.__str__() is:

    def __str__(self):
        return '%s object (%s)' % (self.__class__.__name__, self.pk)

[Mariusz Felisiak]

Replying to Richard Laager:

I just ran into this issue. That change (with the obvious addition of import copy) works as expected. Adding a copy doesn't seem particularly complex to me.

Unfortunately it's complex. Creating a deep-copy is always complicated, will introduce a performance regression, and can create subsequent issues e.g. some objects may not support creating deep copies. Please if we will have a reasonable proposition.

Please first start a discussion (on the ​https://forum.djangoproject.com/ or DevelopersMailingList) about a doable solutions before reopening the ticket. Thanks.