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: david.fischer.ch@… 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)

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

Download all attachments as: .zip

Change History (29)

by david.fischer.ch@…, 11 years ago

The patch for this contribution

comment:1 by AeroNotix, 11 years ago

Needs tests: set

comment:2 by Aymeric Augustin, 11 years ago

Needs documentation: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted
Type: UncategorizedNew 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 Caroline Simpson, 11 years ago

Owner: changed from nobody to Caroline Simpson
Status: newassigned

comment:4 by Caroline Simpson, 11 years ago

Summary: Add delete message mixinAllow 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 Caroline Simpson, 11 years ago

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

comment:6 by Baptiste Mispelon, 11 years ago

Triage Stage: AcceptedReady 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 Sasha Romijn, 11 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

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 by Olivier Le Thanh Duong, 11 years ago

Bug #21926 was a duplicate of this one.

comment:9 by Caroline Simpson, 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. For example, if an application uses a form to verify that a field should be deleted. If the view were to have code that checks that a confirmation box is checked or something similar, and they used the DELETE HTTP method, then the view would not work correctly, as data is not passed through with the request in the case of DELETE. So the documentation change where 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.

Last edited 11 years ago by Caroline Simpson (previous) (diff)

comment:10 by Caroline Simpson, 10 years ago

Patch needs improvement: unset

I've updated the patch with feedback from review.

comment:11 by Asif Saifuddin Auvi, 10 years ago

Owner: changed from Caroline Simpson to Asif Saifuddin Auvi

comment:12 by Asif Saifuddin Auvi, 10 years ago

new pull request send with moving the conflicting tests.

comment:13 by Sasha Romijn, 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:14 by JoseTomasTocino, 9 years ago

Is there any status update on this?

comment:15 by Tim Graham, 9 years ago

We are waiting for someone to update the pull request as described in comment 13.

comment:16 by Asif Saifuddin Auvi, 9 years ago

I will new PR

comment:18 by Asif Saifuddin Auvi, 8 years ago

re started working on it. will send the updated pr on master tomorrow

comment:19 by Asif Saifuddin Auvi, 7 years ago

Owner: Asif Saifuddin Auvi removed
Status: assignednew

comment:20 by Craig Anderson, 6 years ago

Owner: set to Craig Anderson
Status: newassigned

comment:21 by c0d5x, 5 years ago

Just wanted to remind this is an important feature/fix.
Thanks in advance

comment:22 by Demetris Stavrou, 4 years ago

Owner: changed from Craig Anderson to Demetris Stavrou

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

Last edited 4 years ago by Demetris Stavrou (previous) (diff)

comment:23 by Mariusz Felisiak, 4 years ago

Patch needs improvement: unset

comment:24 by Carlton Gibson, 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 Jacob Walls, 4 years ago

Patch needs improvement: unset

comment:26 by Carlton Gibson, 3 years ago

comment:27 by Mariusz Felisiak, 3 years ago

Triage Stage: AcceptedReady for checkin

comment:28 by Carlton Gibson <carlton.gibson@…>, 3 years ago

Resolution: fixed
Status: assignedclosed

In 3a45fea0:

Fixed #21936 -- Allowed DeleteView to work with custom Forms and SuccessMessageMixin.

Thanks to Mariusz Felisiak for review.

Co-authored-by: Demetris Stavrou <demestav@…>
Co-authored-by: Caroline Simpson <github@…>

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