Opened 7 months ago

Closed 7 months ago

Last modified 7 months ago

#33587 closed Bug (wontfix)

Improve SuccessMessageMixin working with BaseDeleteView

Reported by: Chris Chapman Owned by: Dulalet
Component: contrib.messages Version: 4.0
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (3)

comment:1 Changed 7 months ago by Dulalet

Owner: changed from nobody to Dulalet
Status: newassigned

comment:2 Changed 7 months ago by Carlton Gibson

Hi Chris.

I'm going to say wontfix here initially.

SuccessMessageMixin is essentially unchanged 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.

Last edited 7 months ago by Carlton Gibson (previous) (diff)

comment:3 Changed 7 months ago by Carlton Gibson

Resolution: wontfix
Status: assignedclosed
Note: See TracTickets for help on using tickets.
Back to Top