Opened 11 years ago
Closed 3 years ago
#21936 closed New feature (fixed)
Allow delete to provide a success message through a mixin.
Reported by: | Owned by: | Demetris Stavrou | |
---|---|---|---|
Component: | contrib.messages | Version: | dev |
Severity: | Normal | Keywords: | mixin |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Add a mixin to show a message on successful object deletion.
Attachments (1)
Change History (29)
by , 11 years ago
Attachment: | 0001-Add-deletemessagemixin.patch added |
---|
comment:1 by , 11 years ago
Needs tests: | set |
---|
comment:2 by , 11 years ago
Needs documentation: | set |
---|---|
Patch needs improvement: | set |
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → 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 by , 11 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:4 by , 11 years ago
Summary: | Add delete message mixin → 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 by , 11 years ago
Needs documentation: | unset |
---|---|
Needs tests: | unset |
Patch needs improvement: | unset |
comment:6 by , 11 years ago
Triage Stage: | Accepted → 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 by , 11 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Ready for checkin → 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:9 by , 11 years ago
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.
comment:10 by , 10 years ago
Patch needs improvement: | unset |
---|
I've updated the patch with feedback from review.
comment:11 by , 10 years ago
Owner: | changed from | to
---|
comment:13 by , 10 years ago
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.
comment:15 by , 9 years ago
We are waiting for someone to update the pull request as described in comment 13.
comment:19 by , 7 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:20 by , 6 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:21 by , 5 years ago
Just wanted to remind this is an important feature/fix.
Thanks in advance
comment:22 by , 4 years ago
Owner: | changed from | to
---|
Thanks to all for their work on this. I did some minor changes to the original pull request that become inactive, and submitted a new pull request here https://github.com/django/django/pull/13362
comment:23 by , 4 years ago
Patch needs improvement: | unset |
---|
comment:24 by , 4 years ago
Patch needs improvement: | set |
---|
Comments on PR. I think comment:9 is important: not clear adding the delete handling makes too much sense.
comment:25 by , 4 years ago
Patch needs improvement: | unset |
---|
comment:27 by , 3 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
The patch for this contribution