Opened 15 months ago

Last modified 6 weeks ago

#21936 assigned New feature

Allow delete to provide a success message through a mixin.

Reported by: david.fischer.ch@… Owned by: auvipy
Component: contrib.messages Version: master
Severity: Normal Keywords: mixin
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Add a mixin to show a message on successful object deletion.

Attachments (1)

0001-Add-deletemessagemixin.patch (1.2 KB) - added by david.fischer.ch@… 15 months ago.
The patch for this contribution

Download all attachments as: .zip

Change History (14)

Changed 15 months ago by david.fischer.ch@…

The patch for this contribution

comment:1 Changed 14 months ago by AeroNotix

  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement unset

comment:2 Changed 13 months ago by aaugustin

  • Needs documentation set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to New feature

Your patch sets the message before the object is deleted. What if deleting the object fails?

Otherwise, this addition makes sense.

comment:3 Changed 12 months ago by CarolineSimpson

  • Owner changed from nobody to CarolineSimpson
  • Status changed from new to assigned

comment:4 Changed 12 months ago by CarolineSimpson

  • Summary changed from Add delete message mixin to Allow delete to provide a success message through a mixin.

Here is a pull request that approaches the problem from a different angle: https://github.com/django/django/pull/2585
Instead of adding a new mixin, the DeleteView is refactored to allow it to work with the existing SuccessMessageMixin.
Thanks to @charettes for the approach and much of the code.

comment:5 Changed 12 months ago by CarolineSimpson

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:6 Changed 12 months ago by bmispelon

  • Triage Stage changed from Accepted to Ready for checkin

The approach seems sensible to me and the patch looks quite good.

I'm going to mark this as ready for checkin so that we can get another set of eyes on this in case I've missed something.

Thanks!

comment:7 Changed 11 months ago by erikr

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

I see one small issue: the comment on line https://github.com/django/django/pull/2585/files#diff-2b2c9cb35ddf34bc38c90e322dcc39e8L201 still seems valid to me: the documented behaviour has changed, but I don't see a versionchanged annotation, which should be there in a case like this.

comment:8 Changed 11 months ago by olethanh

Bug #21926 was a duplicate of this one.

comment:9 Changed 11 months ago by CarolineSimpson

My main concern with the patch that I have provided is that there are changes related to using the DELETE method for deletions, but it doesn't work entirely as expected. So the documentation change were I changed it to read that you can use DELETE, and a few other changes in the code may not be valid.
There was a comment in the pull request that got buried I think:
"As I was updating the tests, I found that data cannot currently be sent with the DELETE method. When doing further research, I wasn't sure whether this should be allowed or not. The test client accepts a data parameter for DELETE, but the HTTP spec suggests that you shouldn't expect data, like you can for a POST: http://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html#sec9.7 If we want to I can figure out how to actually get data through the chain. Otherwise I can update the documentation to reflect the changes instead."

I wasn't sure how to proceed: to remove the parts related to supporting the DELETE method, or to try to figure out how to get the data through the DELETE chain.

Version 0, edited 11 months ago by CarolineSimpson (next)

comment:10 Changed 3 months ago by CarolineSimpson

  • Patch needs improvement unset

I've updated the patch with feedback from review.

comment:11 Changed 6 weeks ago by auvipy

  • Owner changed from CarolineSimpson to auvipy

comment:12 Changed 6 weeks ago by auvipy

new pull request send with moving the conflicting tests.

comment:13 Changed 6 weeks ago by erikr

  • Patch needs improvement set

Thanks for the new PR. For the record, this is https://github.com/django/django/pull/4256
I left a comment on GitHub regarding the code style issues.

Note: See TracTickets for help on using tickets.
Back to Top