[Django] #33587: Improve SuccessMessageMixin working with BaseDeleteView

[Django]

#33587: Improve SuccessMessageMixin working with BaseDeleteView
--------------------------------------------+------------------------
Reporter: Chris Chapman | Owner: nobody
Type: Bug | Status: new
Component: contrib.messages | Version: 4.0
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
--------------------------------------------+------------------------
Currently a developer is blocked from easily using values from the object
before deleting it. This is because `BaseDeleteView.form_valid` is where
the object is deleted, but this is called before constructing the success
message. This could be easily improved if `SuccessMessageMixin.form_valid`
method was slightly modified. The following suggestion just switches the
order of the first two lines in the method:

{{{
def form_valid(self, form):
success_message = self.get_success_message(form.cleaned_data)
response = super().form_valid(form)
if success_message:
messages.success(self.request, success_message)
return response

}}}

This change would allow the following to work:
{{{
MyDeleteView(SuccessMessageMixin, DeleteView):
"""Delete object and give user feedback on success."""

success_message = "Successfully deleted %(name)s"
...

def get_success_message(self, cleaned_data):
data = {**cleaned_data, "name": str(self.object)}
return super().get_success_message(data)
}}}

Just as before, the message will only be applied if the call to
`super().form_valid(form)` is successful.
This change would allow access to the object as typically expected with
any class inheriting from `SingleObjectMixin`.

--
Ticket URL: <https://code.djangoproject.com/ticket/33587>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#33587: Improve SuccessMessageMixin working with BaseDeleteView

----------------------------------+--------------------------------------
Reporter: Chris Chapman | Owner: Dulalet
Type: Bug | Status: assigned
Component: contrib.messages | Version: 4.0
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

----------------------------------+--------------------------------------
Changes (by Dulalet):

* owner: nobody => Dulalet
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/33587#comment:1>

[Django]

#33587: Improve SuccessMessageMixin working with BaseDeleteView
----------------------------------+--------------------------------------
Reporter: Chris Chapman | Owner: Dulalet
Type: Bug | Status: assigned
Component: contrib.messages | Version: 4.0
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 Carlton Gibson):

Hi Chris.

I'm going to say `wontfix` here initially.

`SuccessMessageMixin` is [essentially unchanged
https://github.com/django/django/blame/9c19aff7c7561e3a82978a272ecdaad40dda5c00/django/contrib/messages/views.py
since it was introduced 9 years ago]. In `get_success_message()` you still
have `self.object` available, with everything but the `pk` (that was
nulled during the `delete()` call):

{{{
>>> from django.contrib.auth.models import Group
>>> g = Group(name="Hello")
>>> g.save()
>>> g
<Group: Hello>
>>> g.id
1
>>> g.delete()
(1, {'auth.Group': 1})
>>> g.id
>>> g.name
'Hello'
}}}

Even the `pk` available from `self.kwargs` in the usual case, so you
already have everything available.

The example you gave — `data = {**cleaned_data, "name":
str(self.object)}` — I guess would be relying on the default `__str__`
implementation using the `pk` but you're already overriding
`get_success_message` so there's no reason not to do something better,
like using a `.name` for the `name` key, or implementing a `__str__` that
doesn't depend in `pk`, or using `self.kwargs` directly if that's what you
really want.

As such, I can't see that it's worth the code change adding other ways to
achieve the same thing.
I hope that makes sense.
Happy to consider if there's a use-case that can't be addressed in this
way.

--
Ticket URL: <https://code.djangoproject.com/ticket/33587#comment:2>

[Django]

#33587: Improve SuccessMessageMixin working with BaseDeleteView
----------------------------------+--------------------------------------
Reporter: Chris Chapman | Owner: Dulalet

Type: Bug | Status: closed
Component: contrib.messages | Version: 4.0
Severity: Normal | Resolution: wontfix

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 Carlton Gibson):

* status: assigned => closed
* resolution: => wontfix

--
Ticket URL: <https://code.djangoproject.com/ticket/33587#comment:3>