Re: [Django] #32137: Change message describing the deletion of inline objects in admin has no id available
Django
available
-------------------------------+--------------------------------------
Reporter: Vlada Macek | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32137#comment:2>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
available
Reporter: Vlada Macek | Owner: nobody
Cleanup/optimization |
Component: contrib.admin | Version: master
Severity: Normal | Resolution: needsinfo
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* status: new => closed
* type: Bug => Cleanup/optimization
* resolution: => needsinfo
Comment:
Replying to [comment:2 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32137#comment:3>
Django
available
-------------------------------------+-------------------------------------
Reporter: Vlada Macek | Owner: nobody
Cleanup/optimization |
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Richard Laager (added)
* status: closed => new
* has_patch: 0 => 1
* resolution: needsinfo =>
Comment:
Replying to [comment:3 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)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32137#comment:4>
Django
available
-------------------------------------+-------------------------------------
Reporter: Vlada Macek | Owner: nobody
Cleanup/optimization |
Component: contrib.admin | Version: dev
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* has_patch: 1 => 0
* resolution: => needsinfo
Comment:
Replying to [comment:4 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32137#comment:5>