Opened 15 years ago
Last modified 5 months ago
#13376 assigned New feature
Messages should have an "expire" flag
Reported by: | Owned by: | Andy Miller | |
---|---|---|---|
Component: | contrib.messages | Version: | 1.2-beta |
Severity: | Normal | Keywords: | messages, expire |
Cc: | Ryan Cheley | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
Messages should have a flag which sets their expiration behavior. In the current messages framework, messages are automatically expired as soon as they are iterated over. This can be overridden if you explicitly set their "used" flag to false after they have been iterated. It would be nice to be able to set a flag which tells the message never to expire unless explicitly told to do so.
Attachments (4)
Change History (19)
comment:1 by , 15 years ago
milestone: | 1.3 |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 14 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
by , 14 years ago
Attachment: | messages_expirity_capabilities.1.diff added |
---|
comment:3 by , 14 years ago
Has patch: | set |
---|
Pretty substantial patch, looks good at first sight, might take a deeper look soon.
comment:4 by , 14 years ago
Needs documentation: | set |
---|---|
Patch needs improvement: | set |
After inspection I have a couple of notes on the (otherwise good) patch:
- I do not completely agree with the chosen api-functions on restrictions;
on_iter
andto_remove
are too vague, I would use something along the lines ofon_display
since iteration could be happening at a lot of different points of time andis_expired
to put more emphasis on the conceptual state (restriction expired) instead of the procedural state (we have to remove it). - The changes in the API must be documented so people can actually make use of restrictions without having to read code.
- There are quite a few spelling errors and typos in the patch which give it a rushed feeling
comment:5 by , 14 years ago
After going through the "storage" discussion on the mailing list, I looked a bit further into the patch and spotted something related which should be improved: at the moment the encoding is done centrally in storage/cookie.py
with hardcoded checks for types. Maintaining different types of restrictions becomes a real pain in the backside this way. It should be de-centralised to the actual restriction class (which brings the problem of determining which restriction should be loaded for the decode).
by , 14 years ago
Attachment: | messages_expirity_capabilities.2.diff added |
---|
by , 14 years ago
Attachment: | messages_expirity_capabilities.3.diff added |
---|
comment:6 by , 14 years ago
Component: | Contrib apps → contrib.messages |
---|
comment:7 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → New feature |
by , 13 years ago
Attachment: | messages_expirity_capabilities.4.diff added |
---|
comment:8 by , 13 years ago
Easy pickings: | unset |
---|---|
Owner: | removed |
Status: | assigned → new |
UI/UX: | unset |
comment:9 by , 5 months ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:10 by , 5 months ago
Cc: | added |
---|
comment:11 by , 5 months ago
Draft PR: https://github.com/django/django/pull/18286
I need to review the current code and the patch file as some did not apply at all (mostly in tests since they have moved in the last 13 years)
Docs need to be added (I need to research this and consider what needs to be added)
Then I think some feedback would be useful to know if the previous authors were on the right track and what more needs to be done.
comment:12 by , 5 months ago
I have decided to split this ticket into 2 PRs as per the discussion I started on the forum here: https://forum.djangoproject.com/t/ticket-13376-messages-should-have-an-expire-flag/32190
The first of the 2 PRs is here: https://github.com/django/django/pull/18332 which modifies the python API which would allow this ticket/patch to then be implemented as a third-party package with minimal changes.
comment:13 by , 5 months ago
Patch needs improvement: | unset |
---|
comment:14 by , 5 months ago
Needs documentation: | unset |
---|
comment:15 by , 5 months ago
Patch needs improvement: | set |
---|
Getting the right API will be important here, but I can see how the idea could be useful. Expiry based on time, number of displays, or some other request/user based flag could all be useful.
I've removed the 1.3 milestone because it's a feature request without a patch.