[Django] #21936: Add delete message mixin
Django
------------------------------------+--------------------
Reporter: david.fischer.ch@… | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.messages | Version: master
Severity: Normal | Keywords: mixin
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 0 | UI/UX: 0
------------------------------------+--------------------
Add a mixin to show a message on successful object deletion.
--
Ticket URL: <https://code.djangoproject.com/ticket/21936>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: david.fischer.ch@… | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.messages | Version: master
Keywords: mixin | Triage Stage: Unreviewed
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by AeroNotix):
* needs_better_patch: => 0
* needs_tests: => 1
* needs_docs: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/21936#comment:1>
Django
Reporter: david.fischer.ch@… | Owner: nobody
Type: New feature | Status: new
Component: contrib.messages | Version: master
Severity: Normal | Resolution:
Keywords: mixin | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* needs_better_patch: 0 => 1
* stage: Unreviewed => Accepted
* type: Uncategorized => New feature
* needs_docs: 0 => 1
Comment:
Your patch sets the message before the object is deleted. What if deleting
the object fails?
Otherwise, this addition makes sense.
--
Ticket URL: <https://code.djangoproject.com/ticket/21936#comment:2>
Django
Reporter: david.fischer.ch@… | Owner:
Type: New feature | CarolineSimpson
Component: contrib.messages | Status: assigned
Severity: Normal | Version: master
Keywords: mixin | Resolution:
Has patch: 1 | Triage Stage: Accepted
Needs tests: 1 | Needs documentation: 1
Easy pickings: 0 | Patch needs improvement: 1
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by CarolineSimpson):
* owner: nobody => CarolineSimpson
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/21936#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: david.fischer.ch@… | Owner:
Type: New feature | CarolineSimpson
Component: contrib.messages | Status: assigned
Severity: Normal | Version: master
Keywords: mixin | Resolution:
Has patch: 1 | Triage Stage: Accepted
Needs tests: 1 | Needs documentation: 1
Easy pickings: 0 | Patch needs improvement: 1
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by CarolineSimpson):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21936#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: david.fischer.ch@… | Owner:
Type: New feature | CarolineSimpson
Component: contrib.messages | Status: assigned
Severity: Normal | Version: master
Keywords: mixin | Resolution:
Has patch: 1 | Triage Stage: Accepted
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by CarolineSimpson):
* needs_better_patch: 1 => 0
* needs_tests: 1 => 0
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/21936#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: david.fischer.ch@… | Owner:
Type: New feature | CarolineSimpson
Component: contrib.messages | Status: assigned
Severity: Normal | Version: master
Keywords: mixin | Resolution:
Needs tests: 0 | checkin
Easy pickings: 0 | Needs documentation: 0
| Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
Comment:
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!
--
Ticket URL: <https://code.djangoproject.com/ticket/21936#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: david.fischer.ch@… | Owner:
Type: New feature | CarolineSimpson
Component: contrib.messages | Status: assigned
Severity: Normal | Version: master
Keywords: mixin | Resolution:
Needs tests: 0 | Needs documentation: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by erikr):
* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21936#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: david.fischer.ch@… | Owner:
Type: New feature | CarolineSimpson
Component: contrib.messages | Status: assigned
Severity: Normal | Version: master
Keywords: mixin | Resolution:
Has patch: 1 | Triage Stage: Accepted
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 1
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by olethanh):
Bug #21926 was a duplicate of this one.
--
Ticket URL: <https://code.djangoproject.com/ticket/21936#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: david.fischer.ch@… | Owner:
Type: New feature | CarolineSimpson
Component: contrib.messages | Status: assigned
Severity: Normal | Version: master
Keywords: mixin | Resolution:
Has patch: 1 | Triage Stage: Accepted
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 1
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21936#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: david.fischer.ch@… | Owner:
Type: New feature | Status: assigned
Component: contrib.messages | Version: master
Severity: Normal | Resolution:
Keywords: mixin | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by CarolineSimpson):
* needs_better_patch: 1 => 0
Comment:
I've updated the patch with feedback from review.
--
Ticket URL: <https://code.djangoproject.com/ticket/21936#comment:10>
Django
Reporter: david.fischer.ch@… | Owner: auvipy
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: mixin | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by auvipy):
* owner: CarolineSimpson => auvipy
--
Ticket URL: <https://code.djangoproject.com/ticket/21936#comment:11>
Django
------------------------------------+------------------------------------
Reporter: david.fischer.ch@… | Owner: auvipy
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: mixin | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by auvipy):
new pull request send with moving the conflicting tests.
--
Ticket URL: <https://code.djangoproject.com/ticket/21936#comment:12>
Django
------------------------------------+------------------------------------
Reporter: david.fischer.ch@… | Owner: auvipy
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: mixin | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by erikr):
* needs_better_patch: 0 => 1
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21936#comment:13>
Django
------------------------------------+------------------------------------
Reporter: david.fischer.ch@… | Owner: auvipy
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: mixin | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by JoseTomasTocino):
Is there any status update on this?
--
Ticket URL: <https://code.djangoproject.com/ticket/21936#comment:14>
Django
------------------------------------+------------------------------------
Reporter: david.fischer.ch@… | Owner: auvipy
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: mixin | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
We are waiting for someone to update the pull request as described in
comment 13.
--
Ticket URL: <https://code.djangoproject.com/ticket/21936#comment:15>
Django
------------------------------------+------------------------------------
Reporter: david.fischer.ch@… | Owner: auvipy
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: mixin | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by auvipy):
I will new PR
--
Ticket URL: <https://code.djangoproject.com/ticket/21936#comment:16>
Django
------------------------------------+------------------------------------
Reporter: david.fischer.ch@… | Owner: auvipy
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: mixin | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by auvipy):
new PR
https://github.com/django/django/pull/5992
--
Ticket URL: <https://code.djangoproject.com/ticket/21936#comment:17>
Django
Reporter: david.fischer.ch@… | Owner: Asif
| Saifuddin Auvi
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: mixin | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Asif Saifuddin Auvi):
re started working on it. will send the updated pr on master tomorrow
--
Ticket URL: <https://code.djangoproject.com/ticket/21936#comment:18>
Django
Reporter: david.fischer.ch@… | Owner: (none)
Type: New feature | Status: new
Component: contrib.messages | Version: master
Severity: Normal | Resolution:
Keywords: mixin | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Asif Saifuddin Auvi):
* owner: Asif Saifuddin Auvi => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/21936#comment:19>
Django
Reporter: david.fischer.ch@… | Owner: Craig
| Anderson
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: mixin | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Craig Anderson):
* owner: (none) => Craig Anderson
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/21936#comment:20>
Django
-------------------------------------+-------------------------------------
Reporter: david.fischer.ch@… | Owner: Craig
| Anderson
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: mixin | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by c0d5x):
Just wanted to remind this is an important feature/fix.
Thanks in advance
--
Ticket URL: <https://code.djangoproject.com/ticket/21936#comment:21>
Django
-------------------------------------+-------------------------------------
| Stavrou
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: mixin | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Demetris Stavrou):
* owner: Craig Anderson => Demetris Stavrou
--
Ticket URL: <https://code.djangoproject.com/ticket/21936#comment:22>
Django
-------------------------------------+-------------------------------------
Reporter: david.fischer.ch@… | Owner: Demetris
| Stavrou
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: mixin | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/21936#comment:23>
Django
-------------------------------------+-------------------------------------
Reporter: david.fischer.ch@… | Owner: Demetris
| Stavrou
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: mixin | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* needs_better_patch: 0 => 1
Comment:
Comments on PR. I think comment:9 is important: not clear adding the
delete handling makes too much sense.
--
Ticket URL: <https://code.djangoproject.com/ticket/21936#comment:24>
Django
-------------------------------------+-------------------------------------
Reporter: david.fischer.ch@… | Owner: Demetris
| Stavrou
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: mixin | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/21936#comment:25>
Django
-------------------------------------+-------------------------------------
Reporter: david.fischer.ch@… | Owner: Demetris
| Stavrou
Type: New feature | Status: assigned
Component: contrib.messages | Version: dev
Severity: Normal | Resolution:
Keywords: mixin | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Carlton Gibson):
[https://github.com/django/django/pull/14634 Updated PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/21936#comment:26>
Django
-------------------------------------+-------------------------------------
Reporter: david.fischer.ch@… | Owner: Demetris
| Stavrou
Type: New feature | Status: assigned
Component: contrib.messages | Version: dev
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/21936#comment:27>
Django
-------------------------------------+-------------------------------------
Reporter: david.fischer.ch@… | Owner: Demetris
| Stavrou
Component: contrib.messages | Version: dev
Severity: Normal | Resolution: fixed
Keywords: mixin | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"3a45fea0832c5910acee6e0d29f230f347a50462" 3a45fea0]:
{{{
#!CommitTicketReference repository=""
revision="3a45fea0832c5910acee6e0d29f230f347a50462"
Fixed #21936 -- Allowed DeleteView to work with custom Forms and
SuccessMessageMixin.
Thanks to Mariusz Felisiak for review.
Co-authored-by: Demetris Stavrou <deme...@gmail.com>
Co-authored-by: Caroline Simpson <git...@hoojiboo.com>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21936#comment:28>