#4604 closed Uncategorized (fixed)
session-based messages
Reported by: | Owned by: | Tobias McNulty | |
---|---|---|---|
Component: | Contrib apps | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | goliath.mailinglist@…, real.human@…, jshaffer2112@…, pedro.lima@…, tom@…, andrewbadr.etc@…, ashanks@…, django@…, ross@…, semente@…, egmanoj@…, django@…, grimdonkey@…, robert.hopson@…, viktor.nagy@…, gonz@…, oliver@…, lemuelf@…, dan.fairs@…, hwaara@…, siddhartag@…, tonn81@…, julian@…, flosch@…, daniel.tritone@…, walter.php@…, mikecampo@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
Right now, in order to pass a message to the next view, you can use "User.message_set.create('Your whatever has been saved successfully!')"
That works for logged-in users, but doesn't help users who aren't logged in and it's probably more complicated than it needs to be.
By contrast, we could store messages in the user's session. Every user (logged in or not) has a session that uniquely identifies them. By creating a request processor and middleware, we can make this process work for all users.
With the attached patch, you can put the FlashMiddleware into the middleware tuple and the Flash context processor into the tuple for context processors and then simply do: "request.flash = 'Your whatever has been saved successfully!'
Since it runs off of the session data rather than the user data, it works without someone having to have logged in. This is very useful in many contexts. Also, I don't think we gain anything by storing it in the DB with a FK to User. Having it in the session just makes it nicely encapsulated to the browsing person.
Flash is what Rails calls this functionality, but if someone wants to rename it to 'message', that's fine by me. request.message = 'Apple' might be easier for people to understand.
Attachments (21)
Change History (130)
by , 17 years ago
Attachment: | __init__.py added |
---|
comment:1 by , 17 years ago
I definitely don't like the name, but the idea is interesting. It does seem like messages naturally fit the session (maybe moreso than the users)
This has to go to a design decision, I recommend you bring this up on the dev group.
comment:2 by , 17 years ago
Component: | Uncategorized → Contrib apps |
---|---|
Needs documentation: | set |
Needs tests: | set |
Owner: | changed from | to
Patch needs improvement: | set |
Triage Stage: | Unreviewed → Design decision needed |
comment:3 by , 17 years ago
I'm pretty sure this is a duplicate of a request that was filed a while back for the same thing (messages tied to sessions so they'd work with non-authenticated users), but I can't find it and I don't think the other one had a patch.
by , 17 years ago
Attachment: | visitor_messages.patch added |
---|
comment:4 by , 17 years ago
Needs documentation: | unset |
---|---|
Needs tests: | unset |
Patch needs improvement: | unset |
Here's my take on it, with tests and docs.
comment:5 by , 17 years ago
I would really replace the current Message-system with this one. Besides from beeing more generic it fixes a race-condition, that could happen when two users surf with the same account and both receive messages. Should be rare, but is fixed with this patch.
comment:7 by , 17 years ago
Cc: | added |
---|
comment:8 by , 17 years ago
Cc: | added |
---|
Two people may often share an account. For many applications there is no need to create a unique user for every person who will use the system, one superuser and one manager is often sufficient for many small projects.
I'd love to see messages attached to sessions instead of users, although it doesn't necesarily need to *replace* the user messages functionality which could also be useful (setting messages offline to be received at next login).
I also thought this issue had another ticket (with a patch) already.
comment:9 by , 17 years ago
Nice patch, Chris. Two things I wanted to bring up though...
- Looks like your patch is impacted by #4729. As a result, if the message is set and the session is persisted, the message will never be deleted by
get_and_delete_messages
.
- I wonder if the use of a LazyMessage style object might be helpful. This would ensure that the messages aren't actually purged until they are viewed.
comment:10 by , 17 years ago
Thx Ben. Yep, looks like it does rely on #4729 - that's a bit less straightforward than it actually looks however (see my comment there).
Regarding a LazyMessage, perhaps, but that's a full change of functionality. Things like that help leave tickets in DDR for much longer. Hmm, it does actually sound quite useful. Damn... :)
comment:11 by , 17 years ago
My apologies for the LazyMessage suggestion ;-)
I quickly threw together a context processor that uses lazy evaluation for both (or either) Session and User messages... I ended-up breaking it out of contrib (into my own project) though since it doesn't fit nicely in either contrib.sessions
or contrib.auth
.
If there's interest I can break it apart to handle only session messages (visitor_messages
as you've named them) and post it here... but I don't have the time to write the necessary tests right now.
comment:12 by , 17 years ago
I sat down and had a good think about the LazyMessage idea and I'm not sure that it is actually necessary. You don't have to use the context processor, you could use the django.core.context_processors.request
context processor and access the messages directly:
{% for message in request.session.get_and_delete_messages %}...{% endfor %}
So we're (thankfully) back to the only design decision left being whether the original idea itself is actually core-worthy...
comment:13 by , 17 years ago
Cc: | added |
---|
comment:14 by , 17 years ago
The visitor_messages.patch works for me.
One thing:
""The session message system provides a simple way to queue messages for
anonymous site visitors. To associate messages with users in the user database,
use the authentication message framework
_."""
I would say: ""The session message system provides a simple way to queue messages for
all (anonymous or authenticated) site visitors. If you want to associate messages
with users in the user database ..."""
comment:15 by , 17 years ago
Cc: | added |
---|
by , 17 years ago
Attachment: | visitor_messages_upd_6373.patch added |
---|
Update to visitor_messages patch to use session backends
comment:16 by , 17 years ago
Cc: | added |
---|
by , 17 years ago
Attachment: | visitor_messages_r6373.patch added |
---|
Updated patch to new session implementation and included the suggested documentation change
comment:17 by , 17 years ago
Patch needs improvement: | set |
---|
There is a bug in visitor_messages_r6373.patch
old:
def get_and_delete_messages(self): return self.pop(self.MESSAGES_NAME, [])
new:
def get_and_delete_messages(self): msgs = self.pop(self.MESSAGES_NAME, []) if msgs: self.modified = True return msgs
comment:18 by , 17 years ago
I think it would be nice to have a loglevel.
KNOWN_LOGLEVELS=["info", "error"] class Message(object): def __init__(self, message, loglevel="info"): self.message=message assert loglevel in KNOWN_LOGLEVEL self.loglevel=loglevel def __unicode__(self): return self.message def create_message(self, message, loglevel="info"): ... messages.append(Message(message, loglevel)) ...
comment:19 by , 17 years ago
Patch needs improvement: | unset |
---|
Hello Thomas,
The change you mention (self.modified = True
) is not a problem with this patch... this patch just reveals the problem.
The recent update of the session framework broke the functionality of pop
. Please see #4729 for more information.
As for the loglevels, I'm -1. I think that this was discussed on the django-dev list for the User-based message framework... you might want to check there. If not, you should bring it up. I think that it's important to keep both the Session message framework and User message framework functionality the same.
comment:20 by , 17 years ago
Cc: | added |
---|
comment:21 by , 17 years ago
Cc: | added |
---|
comment:23 by , 17 years ago
Cc: | removed |
---|
comment:24 by , 17 years ago
Cc: | added |
---|
comment:25 by , 17 years ago
Cc: | added |
---|
comment:26 by , 17 years ago
Cc: | added |
---|
comment:27 by , 17 years ago
Owner: | changed from | to
---|---|
Patch needs improvement: | set |
Status: | new → assigned |
Triage Stage: | Design decision needed → Accepted |
Marking this as Accepted
based on discussion at the 12/1/2007 sprint in Lawrence. I'm claiming it for now, and will work on it over the coming week or so.
We need to come up with an interface that gracefully allows use of both auth
messages and sessions
messages. I have some ideas on how to do this and will write them up here if I don't get around to implementing them in the next week or two.
comment:29 by , 17 years ago
Cc: | added |
---|
by , 17 years ago
Attachment: | messages.diff added |
---|
Rewritten version, new core context processor which grabs messages from both user and session
comment:30 by , 17 years ago
Patch needs improvement: | unset |
---|
Ok, here's a rewritten version which matches the requirements which hawkeye discussed with me via email.
If some people want to have a play with it and report back, that'd be swell :)
comment:31 by , 17 years ago
Note that if you want the admin messages to work still and you have specified a TEMPLATE_CONTEXT_PROCESSORS
in your own settings module, you'll need to ensure you add the new context processor to it.
This will need to be mentioned in BackwardsIncompatibleChanges.
follow-up: 33 comment:32 by , 17 years ago
Patch looks good overall.
Three quick points:
- The documentation seems to focus on authenticated versus anonymous. I see this as synchronous (session) versus asynchronous (user) messaging.
- You import
memoize
andlazy
intodjango.core.context_processors
, but it doesn't look like you're using either. - There currently aren't any tests for the
LazyMessages
class. I'd feel more comfortable if they were there.
comment:33 by , 17 years ago
Replying to __hawkeye__:
Patch looks good overall.
Three quick points:
- The documentation seems to focus on authenticated versus anonymous. I see this as synchronous (session) versus asynchronous (user) messaging.
It uses the terms "user messages" and "session messages". I'm not sure that calling this synchronous/asynchronous would make it any clearer.
The line "The session message system provides a simple way to queue messages for all (anonymous or authenticated) site visitors" is just there to explain synchronosity (my imaginary word of the day).
- You import
memoize
andlazy
intodjango.core.context_processors
, but it doesn't look like you're using either.
Indeed I'm not. I was going to use them but in the end just wrote the LazyMessages
class, due to the fact that lazy()
isn't useful for non-string classes.
- There currently aren't any tests for the
LazyMessages
class. I'd feel more comfortable if they were there.
Sure... now I just have to figure out where to stick them.
comment:34 by , 17 years ago
Ok, new tests in now. This patch also adds a new empty tests/middleware/models.py
file, since those tests (which do pass) weren't actually being run due to the lack of this file.
comment:35 by , 17 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
Made a slight documentation change, but this patch looks good to me. Patch applies cleanly, passes tests and works in my limited testing.
Thanks, SmileyChris, for this latest patch.
comment:36 by , 17 years ago
I have been considering the fact that this is quite a big incompatible change (if you have overridden the context processors setting, you'll have to add the messages processor in to retain your admin message functionality).
I was thinking that perhaps the admin processor should check to see if messages has been turned on, and if not then continue treating the (user) messages like it currently does. This would be tricky to document and I'm not sure how much I'm allowed to "rock the boat" with this change, so I'll wait for a committer comment before worrying about adding that (it wouldn't be too difficult, anyway)
comment:37 by , 17 years ago
Cc: | added |
---|
follow-up: 41 comment:38 by , 17 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Ready for checkin → Accepted |
A couple of stylistic issues (all in the LazyMessages class):
- Subclass from
django.utils.encoding.StrAndUnicode
so that you get a__str__
method. - Probably better form to call
iter(self.messages)
, rather than callingself.messages.__iter__()
directly. Use the interface, not the implementation (seems to be the python-dev-preferred approach, too). I prefer the way you've implemented__len__
to the__iter__
implementation, in other words. - Blank lines are needed between the methods in that class.
I don't have a strong opinion about the move to the context processor at the moment, although you're correct that it's of some concern. Adding code that tries to work with both old- and new-style is something we generally avoid doing, because in the long-term it's just bloat and there's never a good moment to remove it (particularly if 1.0 was released in the interim, so you might as well do it up-front). It's hard to make things fail with an exception if somebody forgets, too. I feel that this is a bit like the old "compulsory middleware" stuff: it's a bit of an oxymoron. If we really need that processor, which we do because admin needs it, then it shouldn't be in any way optional. I haven't wrapped my brain around why it can't be called all the time yet.
My secret hope is that another maintainer will adopt this, since this isn't too high on my priority list, but it looks like it's getting towards some sort of completion point, so we should keep it on the radar.
comment:39 by , 17 years ago
Owner: | changed from | to
---|---|
Status: | assigned → new |
comment:40 by , 17 years ago
Cc: | added |
---|
comment:41 by , 17 years ago
Patch needs improvement: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
Replying to mtredinnick:
A couple of stylistic issues (all in the LazyMessages class):
All addressed - thanks for the django.utils.encoding.StrAndUnicode
tip.
I don't have a strong opinion about the move to the context processor at the moment, although you're correct that it's of some concern.
I have now made the auth
processor work like it did before (in as much as it adds a messages
variable) and changed the messages
processor to not be enabled by default.
So now we have better backwards compatibility and everyone can hold hands and dance around together.
by , 17 years ago
Attachment: | messages.5.2.diff added |
---|
identical to previous, except has empty init.py and models.py files for tests
by , 17 years ago
Attachment: | messages.6.diff added |
---|
identical to previous, except has empty init.py and models.py files for tests (no, really this time)
by , 17 years ago
Attachment: | messages.7.diff added |
---|
implement __getitem__
too, some people are using stuff like {{ messages.0 }}
comment:42 by , 17 years ago
Cc: | added |
---|
comment:43 by , 17 years ago
Cc: | added |
---|
comment:44 by , 17 years ago
Cc: | added |
---|
comment:45 by , 16 years ago
milestone: | → 1.0 beta |
---|
comment:46 by , 16 years ago
milestone: | 1.0 beta |
---|
Removing 1.0-beta tag. It might make it in or it might not, but it's not on the "maybe" list and there's enough stuff to do that we have to guard that milestone setting carefully.
comment:47 by , 16 years ago
Cc: | added |
---|
comment:48 by , 16 years ago
Cc: | removed |
---|
comment:49 by , 16 years ago
Cc: | added |
---|
comment:50 by , 16 years ago
Cc: | added |
---|
comment:51 by , 16 years ago
milestone: | → post-1.0 |
---|
Spoke with Malcolm... looks like there's hesitation to do this currently (and somewhat in general).
comment:52 by , 16 years ago
hawkeye,
That is really disappointing to hear. What is the source of hesitation (beyond being a non-priority for 1.0)? Personally, I think it's pretty obvious that this is a necessary feature. (I mean, really it's just a better implementation of the currently existing message passing system.)
comment:53 by , 16 years ago
I think this feature is quite useful in every communication with anon users of your website: contact form (sent contact info sucessfully), newsletter (thanks for joining newletter), ...
Why don't commit this? it isn't a huge refactoring.
by , 16 years ago
Attachment: | messages-r8347.diff added |
---|
Updated messages.7.diff to avoid rejects with django.core.context_processors.py
comment:54 by , 16 years ago
Just chiming in to say that I find this really useful, and hope it does make it in after 1.0's done and dusted.
comment:55 by , 16 years ago
Cc: | added |
---|
comment:56 by , 16 years ago
To me it makes no sense to store messages with a short lifetime in user.message_set. user.message_set should be reserved for system or administrator messages to the user. The session is the right place to store feedback about the current session for the user.
What if, for example, the same user is logged in from two different browsers and completing two different tasks, simultaneously? When using user.message_set to store feedback for the user, the messages will be distributed on a first come first served basis, with no regard for what session actually generated what feedback. It's bad to get in the habit of using user.message_set for messages like "Article updated successfully," or other messages that really have no context outside the current session.
For these reasons, I think this patch erroneously conflates user messages and session messages.
comment:57 by , 16 years ago
Cc: | removed |
---|
follow-up: 96 comment:58 by , 16 years ago
FYI I took ideas from the patch and separated it into middleware: http://www.djangosnippets.org/snippets/1002/
Also added optional type for messages so you render them differently (e.g. http://traviscline.com/blog/2008/08/23/django-middleware-session-backed-messaging/)
comment:59 by , 16 years ago
Cc: | added |
---|
comment:60 by , 16 years ago
Cc: | added |
---|
comment:61 by , 16 years ago
Cc: | added |
---|
comment:62 by , 16 years ago
Cc: | added |
---|
follow-up: 64 comment:63 by , 16 years ago
milestone: | post-1.0 |
---|---|
Patch needs improvement: | set |
Triage Stage: | Ready for checkin → Accepted |
Moving back from "ready for checkin", because I don't think it is yet. I suspect the new lazy()
from r8120 is probably more reusable here. The duplicate lazy stuff makes me nervous.
I don't like the backwards compatibility this brings to the table. This is probably something that's relatively orthogonal to the existing stuff. I'm also not wild about it being in core, rather than a contrib app, since whilst it's useful sometimes and meets the "a reasonably common pattern", it's hardly impossible to use Django without it.
Right now, it's really suffering from not having any core maintainer having fallen in love with the idea and/or implementation enough. For me, a clean implementation that was low-impact on other pieces of code and backwards compatible and optional (part of contrib) would be more palatable. I haven't really seen much in the way of comment from any other core committer. I realise a fair bit of work has gone into this, but it doesn't feel like it's there yet. Solve the backwards incompatibility problem (not breaking existing admin, particularly) and work out how it could be part of contrib somewhere (contrib/auth maybe?) and it will have a better chance or proceeding. The fact that it impacts contrib/sessions also makes me wonder if the implementation is really correct. It's really being baked into the code in a few places. We must be able to do better than that.
comment:64 by , 16 years ago
Replying to mtredinnick:
Moving back from "ready for checkin", because I don't think it is yet. I suspect the new
lazy()
from r8120 is probably more reusable here. The duplicate lazy stuff makes me nervous.
That's fine - back when I started lazy()
didn't do a very good job.
I don't like the backwards compatibility this brings to the table. This is probably something that's relatively orthogonal to the existing stuff. I'm also not wild about it being in core, rather than a contrib app, since whilst it's useful sometimes and meets the "a reasonably common pattern", it's hardly impossible to use Django without it.
It's only "in core" as much as the auth context processor is. And that is only required if you want messages and aren't using the auth context processor. I guess it could have it's own contrib app but it would only consist of a context processor!
Solve the backwards incompatibility problem (not breaking existing admin, particularly)
What backwards incompatibility? It won't break existing admin.
... and work out how it could be part of contrib somewhere (contrib/auth maybe?) and it will have a better chance or proceeding. The fact that it impacts contrib/sessions also makes me wonder if the implementation is really correct.
The whole point is that this is a session message system. It seems obvious to bake it into contrib/sessions to me.
The context processor just provides a single message context variable containing both (existing) user messages and (new) session messages. This keeps backwards compatibility with the current message system (and admin). However, as it has been noted by tobias, the admin is currently doing the wrong thing and should really be updated to using a session based messaging system.
comment:65 by , 16 years ago
Summary: | Message Passing For Anonymous Users → session-based messages |
---|
comment:66 by , 16 years ago
Cc: | added |
---|
comment:67 by , 16 years ago
Cc: | added |
---|
comment:68 by , 16 years ago
Cc: | added |
---|
comment:69 by , 16 years ago
Cc: | added |
---|
comment:70 by , 16 years ago
I've updated SmileyChris' messages.7.diff
to trunk as of now (messages-r8347.diff
was missing three files).
comment:71 by , 16 years ago
Cc: | added |
---|
comment:72 by , 16 years ago
Maybe it's a good idea to take a look at Django-flash, an open source app that provides an implementation that works pretty much like the Rails one:
http://djangoflash.destaquenet.com/
I'm the Django-flash's lead developer, and I'd be more than happy to see it bundled with Django.
follow-up: 76 comment:73 by , 16 years ago
Cc: | added |
---|
comment:74 by , 16 years ago
milestone: | → 1.2 |
---|
comment:75 by , 16 years ago
Cc: | removed |
---|
comment:76 by , 16 years ago
Replying to anonymous:
I just posted to the mailing list without seeing that you've commented here: http://groups.google.com/group/django-developers/browse_thread/thread/1ad495e30f946b2c/f6501a9e2a66e1d0
I've been using django-flash and I like it a lot. Maybe an actual Django patch with flash as a contrib app and proper Django documentation and such would be helpful in getting more support for this implementation?
comment:77 by , 15 years ago
I realise now that my efforts towards integrating with current user messages in a backwards-compatible manor is a waste of time.
Since that was the only reason to have the code here I have now started a separate app to do session/cookie based user notifications. This app supersedes the patches in this ticket (and would be dead easy to convert to a contrib app if this is ever wanted - which I think it should be, as it is a good example for something which there should be one right way [for most cases] to do).
You can find it here: http://code.google.com/p/django-notify/
I haven't done tests yet, but am willing to accept contributions ;)
comment:78 by , 15 years ago
Cc: | added |
---|---|
Owner: | changed from | to
Status: | new → assigned |
Django-flash codebase is quite stable and it is fully documented and tested (all possible use cases are tested both in unit and integration tests), so I think the code is fine right now. I obviously don't know how many people are actually using it in their projects, but I know some of them who are quite happy with it. And I am too, since I created django-flash to scratch my own itch!
If you guys think django-flash would be a nice addition to Contrib (I think it is!), just get the code and use it the way you want, seriously. The project is currently licensed under LGPL, but I don't think it is a problem. (if it is, I can just dual license the project.)
As I said before, I'd be more than happy to see django-flash bundled with Django. Unfortunately, I will not send patches and work on a Django-style documentation before I get the green light.
comment:79 by , 15 years ago
Cc: | removed |
---|
comment:80 by , 15 years ago
The problem is that django-flash doesn't really encapsulate the requirements for a standard session based messaging system since it doesn't enforce a single way of using messages, so there is no way for third party apps to provide messages in a consistent manor.
This doesn't mean that I don't think it is a well developed or useful application, I'm just saying that I don't think on its own it works as a notification system.
As an unimportant personal aside, I don't like the name "flash".
comment:81 by , 15 years ago
I know what you mean. Although django-flash fulfills its purpose, this "flash" thing is probably just a subset of a full-fledged notification system.
Now I ask you: what exactly should a notification system do? Depending on the context, a notification system should do different things, at different levels of abstraction. So unless you define what a notification system should do, such term as "notification system" is just too general and creates more confusion than it helps.
I think we are missing the point here. The "flash" thing is part of Rails since the old days, and it continues to prove its usefulness. It is also general enough to cover the common cases. I might be biased, but I personally don't know anybody who dislikes the Rails' "flash" thing or thinks it's not great the way it is.
And, by the way, I don't like the name "flash" either, but that name happened to be used by several other web frameworks and became the standard way to describe such context or scope. So I think we should stick with that name and make everybody's life easier.
comment:82 by , 15 years ago
Oops, wasn't logged in before (in comment 80).
Actually, I'd say flash is a superset of the notification system that's being discussed in this ticket. All we're after is an alternative to storing messages in the User - so what we want is to encapsulate this behaviour, but in a standard way. The only frequently requested addition is the ability to have messages which can have different HTML classes.
Oh, and another request from several parties was to allow these messages without causing a hit on the database: django-notify comes with an alternate cookie storage backend you can use for this.
comment:83 by , 15 years ago
Owner: | changed from | to
---|---|
Status: | assigned → new |
I'm going to take a stab at converting django-flash + django-notify into something that looks like a contrib app with docs and tests. Let me know if you'd like to help! I think sticking with the "flash" name, although it sucks, will end up being a good thing.
comment:84 by , 15 years ago
leah: Personally I wouldn't bother the conversion until the issue is discussed a bit more in django-dev - and that discussion would best wait until after 1.1
But if you still need some help after then, I'd be more than happy. Let's just get a solution!
follow-up: 86 comment:85 by , 15 years ago
Ditto on helping out. I realize that django-flash may be closely following a convention with which I'm not familiar, but I don't like it for two reasons:
# It doesn't have an obvious, standard method for storing 2+ separate messages
# If the second request generates a redirect (or is another view that doesn't print session messages), and that view doesn't know about django-flash, you'll lose your messages
For these reasons I like django-notify better. It provides a standard way to store/retrieve multiple messages, and those messages aren't erased until they're actually printed in the template.
At least, that's my limited understanding of it. Just my two cents...
comment:86 by , 15 years ago
Replying to tobias:
# It doesn't have an obvious, standard method for storing 2+ separate messages
Just use Python! There's nothing more "obvious and standard" than a plain List to store several messages under a single key:
request.flash['message'] = ['Message one', 'Message two']
I like this approach because we are being explicit that there's multiple messages under the same key. (Remember the Zen of Python, which says "Explicit is better than implicit.")
# If the second request generates a redirect (or is another view that doesn't print session messages), and that view doesn't know about django-flash, you'll lose your messages
If you want flash messages to survive a redirect:
def view1(request): request.flash['message'] = 'My message' return HttpRedirectResponse(reverse(view2)) def view2(request): request.keep() # Keeps all messages stored inside the flash request.keep('message1', 'message2') # Only keeps the given messages return HttpRedirectResponse(reverse(final_view))
I personally think it's a feature, not a bug or something like that for the same reason: "Explicit is better than implicit".
Another reason is that we avoid propagating undesired messages by accident, since we might store stuff inside the flash to be retrieved by other views, not templates. It's rare, but it's possible.
comment:87 by , 15 years ago
Ooops, fixing some typos in the code:
def view1(request): request.flash['message1'] = 'My message' return HttpRedirectResponse(reverse(view2)) def view2(request): request.flash.keep() # Keeps all messages stored inside the flash request.flash.keep('message1', 'message2') # Only keeps the given messages return HttpRedirectResponse(reverse(final_view))
follow-up: 89 comment:88 by , 15 years ago
flash['message'] = ['1', '2']
is too flexible. You might have a project with one pluggable app that just setsflash['message'] = 'message'
, another that doesflash['message'].append('message')
, and yet another that doesflash['message'] = ('message 1', 'message 2')
. And if you want to append, who's responsible for creating the list in the first place? As far as I can tell from the docs, django-flash doesn't propose a convention for doing this. Maybe it's just me, butnotifications.add('message')
seems much more explicit...
- request.keep is all well and good for your own tightly integrated code, but what if you're dealing with a view in a pluggable app that you can't modify, and it doesn't know about django-flash?
- WRT propagating undesired messages, as far as I can tell django-notify (and I believe at least one of the patches on this ticket) only destroys messages when iterated (from a template OR a view). I'm not exactly clear about your concern but would that address it?
I tried to sum up some of my points about what would comprise a good contrib app here: http://www.caktusgroup.com/blog/2009/06/19/towards-a-standard-for-django-session-messages/
follow-up: 91 comment:89 by , 15 years ago
Replying to tobias:
flash['message'] = ['1', '2']
is too flexible. You might have a project with one pluggable app that just setsflash['message'] = 'message'
, another that doesflash['message'].append('message')
, and yet another that doesflash['message'] = ('message 1', 'message 2')
. And if you want to append, who's responsible for creating the list in the first place? As far as I can tell from the docs, django-flash doesn't propose a convention for doing this. Maybe it's just me, butnotifications.add('message')
seems much more explicit...
Storing several messages under the same key is not the rule, but the exception, at least for the projects I work on. But once you treat the flash as a regular dictionary (in fact their interface are very similar), things become easier to grasp. But, for the record, I have no problem with the ".add('message')" thing.
- request.keep is all well and good for your own tightly integrated code, but what if you're dealing with a view in a pluggable app that you can't modify, and it doesn't know about django-flash?
This is a very good point. I haven't thought about this until now. I'll try to come up with something.
- WRT propagating undesired messages, as far as I can tell django-notify (and I believe at least one of the patches on this ticket) only destroys messages when iterated (from a template OR a view). I'm not exactly clear about your concern but would that address it?
Yes it does, if that's really the case. The only thing I don't like about it is the idea of getting my messages removed when doing something like "request.flashmessage" in my views.
comment:90 by , 15 years ago
Cc: | added |
---|
comment:91 by , 15 years ago
Replying to daniel_martins:
Storing several messages under the same key is not the rule, but the exception, at least for the projects I work on. But once you treat the flash as a regular dictionary (in fact their interface are very similar), things become easier to grasp. But, for the record, I have no problem with the ".add('message')" thing.
It has nothing to do with being easier to grasp. The issue is that django-flash does not propose a singular, standard structure for creating and retrieving 2+ messages. The very fact that it's a dictionary leaves too much as an "exercise for the reader."
comment:92 by , 15 years ago
It has nothing to do with being easier to grasp. The issue is that django-flash
does not propose a singular, standard structure for creating and retrieving 2+
messages. The very fact that it's a dictionary leaves too much as an "exercise for the reader."
Without having actually tried django-flash: How so? If it's a dictionary, then the interface is pretty clear: It's the same interface as the standard python dict, possibly with some goodies on top of it. How does it matter if you use [] or update() to add messages?
I'm chiming in here because I would really like to see multiple types of messages supported, e.g. "error", "notice" etc. Using a dict seems fitting then.
comment:93 by , 15 years ago
Cc: | removed |
---|
comment:94 by , 15 years ago
Multiple message types sounds good to me. A dict probably makes sense behind the scenes, but I think it's exposing too much to make that the default interface for creating/retrieving messages.
comment:95 by , 15 years ago
Guys, might I suggest a different venue for this discussion, trac is a pretty bad place for it, as it's not as easily searchable, and it's visible to a smaller cut of individuals.
comment:96 by , 15 years ago
Replying to Travis Cline:
FYI I took ideas from the patch and separated it into middleware: http://www.djangosnippets.org/snippets/1002/
Also added optional type for messages so you render them differently (e.g. http://traviscline.com/blog/2008/08/23/django-middleware-session-backed-messaging/)
seems to be the same idea as 11617: Enhance User Message System
comment:98 by , 15 years ago
per Alex's request I started a conversation on django-developers here:
http://groups.google.com/group/django-developers/browse_thread/thread/eba93088c649022b
comment:100 by , 15 years ago
Leah, do you mind if I claim this ticket? Are you still working on it? Thanks -tobias
comment:102 by , 15 years ago
Status: | new → assigned |
---|
comment:103 by , 15 years ago
development branch for proposed messages app is here: http://bitbucket.org/tobias.mcnulty/django-contrib-messages/
by , 15 years ago
Attachment: | django-contrib-messages-9f54c0f8719c.diff added |
---|
diff showing changes in django-contrib-messages branch as of changeset 9f54c0f8719c
comment:104 by , 15 years ago
Cc: | added |
---|
by , 15 years ago
Attachment: | django-contrib-messages-e4da706e1152.diff added |
---|
diff showing changes in django-contrib-messages branch as of changeset e4da706e1152
by , 15 years ago
Attachment: | django-contrib-messages-6399c12d1773.diff added |
---|
changes in django-contrib-messages branch as of rev 6399c12d1773
by , 15 years ago
Attachment: | django-contrib-messages.2.diff added |
---|
by , 15 years ago
Attachment: | django-contrib-messages.diff added |
---|
final patch from django-contrib-messages branch
comment:105 by , 15 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
(In [11804]) Fixed #4604 - Configurable message passing system, supporting anonymous users
This deprecates User.message_set in favour of a configurable messaging
system, with backends provided for cookie storage, session storage and
backward compatibility.
Many thanks to Tobias McNulty for the bulk of the work here, with
contributions from Chris Beaven (SmileyChris) and lots of code review from
Russell Keith-Magee, and input from many others. Also credit to the authors
of various messaging systems for Django whose ideas may have been pinched
:-)
comment:106 by , 15 years ago
Took me a minute to find the docs, so here they are if anyone else is looking: http://docs.djangoproject.com/en/dev/ref/contrib/messages/
comment:107 by , 15 years ago
Cc: | removed |
---|
comment:108 by , 14 years ago
Cc: | removed |
---|---|
Easy pickings: | unset |
Severity: | → Normal |
Type: | → Uncategorized |
FlashMiddleware and flash context processor