Opened 9 years ago

Closed 7 years ago

Last modified 5 years ago

#4604 closed Uncategorized (fixed)

session-based messages

Reported by: Sean Patrick Hogan <sp.hogan@…> Owned by: Tobias McNulty
Component: Contrib apps Version: master
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:

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)

__init__.py (327 bytes) - added by Sean Patrick Hogan <sp.hogan@…> 9 years ago.
FlashMiddleware and flash context processor
visitor_messages.patch (4.9 KB) - added by Chris Beaven 9 years ago.
visitor_messages_upd_6373.patch (6.6 KB) - added by Pedro Lima <pedro.lima@…> 9 years ago.
Update to visitor_messages patch to use session backends
visitor_messages_r6373.patch (6.6 KB) - added by Pedro Lima <pedro.lima@…> 9 years ago.
Updated patch to new session implementation and included the suggested documentation change
visitor_messages_r6596.upatch (5.9 KB) - added by Manuel Saelices 9 years ago.
Updated patch for revision 6596
visitor_messages_r6596.patch (6.3 KB) - added by Manuel Saelices 9 years ago.
Updated patch for revision 6596
messages.diff (11.9 KB) - added by Chris Beaven 9 years ago.
Rewritten version, new core context processor which grabs messages from both user and session
messages.2.diff (15.3 KB) - added by Chris Beaven 9 years ago.
More tests
messages.3.diff (15.1 KB) - added by Chris Beaven 9 years ago.
removed unnecessary imports
messages.4.diff (15.1 KB) - added by Ben Slavin 9 years ago.
Slight documentation change.
messages.5.diff (15.2 KB) - added by Chris Beaven 9 years ago.
Now with better backwards compatibility
messages.5.2.diff (15.2 KB) - added by Chris Beaven 9 years ago.
identical to previous, except has empty init.py and models.py files for tests
messages.6.diff (15.4 KB) - added by Chris Beaven 9 years ago.
identical to previous, except has empty init.py and models.py files for tests (no, really this time)
messages.7.diff (15.5 KB) - added by Chris Beaven 9 years ago.
implement __getitem__ too, some people are using stuff like {{ messages.0 }}
messages-r8347.diff (12.7 KB) - added by John Hensley 8 years ago.
Updated messages.7.diff to avoid rejects with django.core.context_processors.py
t4604-r9698.diff (14.7 KB) - added by Ramiro Morales 8 years ago.
Patch updated to r9698.
django-contrib-messages-9f54c0f8719c.diff (81.8 KB) - added by Tobias McNulty 7 years ago.
diff showing changes in django-contrib-messages branch as of changeset 9f54c0f8719c
django-contrib-messages-e4da706e1152.diff (96.5 KB) - added by Tobias McNulty 7 years ago.
diff showing changes in django-contrib-messages branch as of changeset e4da706e1152
django-contrib-messages-6399c12d1773.diff (98.0 KB) - added by Tobias McNulty 7 years ago.
changes in django-contrib-messages branch as of rev 6399c12d1773
django-contrib-messages.2.diff (98.5 KB) - added by Tobias McNulty 7 years ago.
django-contrib-messages.diff (98.5 KB) - added by Tobias McNulty 7 years ago.
final patch from django-contrib-messages branch

Download all attachments as: .zip

Change History (130)

Changed 9 years ago by Sean Patrick Hogan <sp.hogan@…>

Attachment: __init__.py added

FlashMiddleware and flash context processor

comment:1 Changed 9 years ago by Chris Beaven

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 Changed 9 years ago by Chris Beaven

Component: UncategorizedContrib apps
Needs documentation: set
Needs tests: set
Owner: changed from Jacob to Adrian Holovaty
Patch needs improvement: set
Triage Stage: UnreviewedDesign decision needed

comment:3 Changed 9 years ago by James Bennett

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.

Changed 9 years ago by Chris Beaven

Attachment: visitor_messages.patch added

comment:4 Changed 9 years ago by Chris Beaven

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

Here's my take on it, with tests and docs.

comment:5 Changed 9 years ago by anonymous

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:6 Changed 9 years ago by anonymous

Two users shouldn't be using the same account though :)

comment:7 Changed 9 years ago by anonymous

Cc: goliath.mailinglist@… added

comment:8 Changed 9 years ago by Tai Lee

Cc: real.human@… 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 Changed 9 years ago by Ben Slavin <benjamin.slavin@…>

Nice patch, Chris. Two things I wanted to bring up though...

  1. 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.
  1. 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 Changed 9 years ago by Chris Beaven

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 Changed 9 years ago by Ben Slavin <benjamin.slavin@…>

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 Changed 9 years ago by Chris Beaven

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 Changed 9 years ago by John Shaffer <jshaffer2112@…>

Cc: jshaffer2112@… added

comment:14 Changed 9 years ago by Thomas Güttler <hv@…>

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 Changed 9 years ago by miracle2k <michael@…>

Cc: elsdoerfer@… added

Changed 9 years ago by Pedro Lima <pedro.lima@…>

Update to visitor_messages patch to use session backends

comment:16 Changed 9 years ago by Pedro Lima <pedro.lima@…>

Cc: pedro.lima@… added

Changed 9 years ago by Pedro Lima <pedro.lima@…>

Updated patch to new session implementation and included the suggested documentation change

comment:17 Changed 9 years ago by Thomas Güttler <hv@…>

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 Changed 9 years ago by Thomas Güttler <hv@…>

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 Changed 9 years ago by Ben Slavin

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 Changed 9 years ago by David Larlet

Cc: larlet@… added

Changed 9 years ago by Manuel Saelices

Updated patch for revision 6596

Changed 9 years ago by Manuel Saelices

Updated patch for revision 6596

comment:21 Changed 9 years ago by anonymous

Cc: hv@… added

comment:22 Changed 9 years ago by Thomas Steinacher <tom@…>

Cc: tom@… added

-1 for loglevels

comment:23 Changed 9 years ago by Andrew Badr <andrewbadr.etc@…>

Cc: goliath.mailinglist@… real.human@… jshaffer2112@… elsdoerfer@… pedro.lima@… larlet@… hv@… tom@… removed

comment:24 Changed 9 years ago by Andrew Badr <andrewbadr.etc@…>

Cc: goliath.mailinglist@… real.human@… jshaffer2112@… elsdoerfer@… pedro.lima@… larlet@… hv@… tom@… andrewbadr.etc@… added

comment:25 Changed 9 years ago by anonymous

Cc: ashanks@… added

comment:26 Changed 9 years ago by Niels Sandholt Busch

Cc: niels.busch@… added

comment:27 Changed 9 years ago by Ben Slavin

Owner: changed from nobody to Ben Slavin
Patch needs improvement: set
Status: newassigned
Triage Stage: Design decision neededAccepted

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:28 Changed 9 years ago by Chris Beaven

Hawkeye, wanna email me (@gmail) to discuss this?

comment:29 Changed 9 years ago by anonymous

Cc: django@… added

Changed 9 years ago by Chris Beaven

Attachment: messages.diff added

Rewritten version, new core context processor which grabs messages from both user and session

comment:30 Changed 9 years ago by Chris Beaven

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 Changed 9 years ago by Chris Beaven

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.

comment:32 Changed 9 years ago by Ben Slavin

Patch looks good overall.

Three quick points:

  1. The documentation seems to focus on authenticated versus anonymous. I see this as synchronous (session) versus asynchronous (user) messaging.
  2. You import memoize and lazy into django.core.context_processors, but it doesn't look like you're using either.
  3. There currently aren't any tests for the LazyMessages class. I'd feel more comfortable if they were there.

comment:33 in reply to:  32 Changed 9 years ago by Chris Beaven

Replying to __hawkeye__:

Patch looks good overall.

Three quick points:

  1. 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).

  1. You import memoize and lazy into django.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.

  1. 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.

Changed 9 years ago by Chris Beaven

Attachment: messages.2.diff added

More tests

Changed 9 years ago by Chris Beaven

Attachment: messages.3.diff added

removed unnecessary imports

comment:34 Changed 9 years ago by Chris Beaven

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.

Changed 9 years ago by Ben Slavin

Attachment: messages.4.diff added

Slight documentation change.

comment:35 Changed 9 years ago by Ben Slavin

Triage Stage: AcceptedReady 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 Changed 9 years ago by Chris Beaven

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 Changed 9 years ago by anonymous

Cc: ross@… added

comment:38 Changed 9 years ago by Malcolm Tredinnick

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

A couple of stylistic issues (all in the LazyMessages class):

  1. Subclass from django.utils.encoding.StrAndUnicode so that you get a __str__ method.
  2. Probably better form to call iter(self.messages), rather than calling self.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.
  3. 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 Changed 9 years ago by Chris Beaven

Owner: changed from Ben Slavin to Chris Beaven
Status: assignednew

comment:40 Changed 9 years ago by Guilherme M. Gondim (semente) <semente@…>

Cc: semente@… added

Changed 9 years ago by Chris Beaven

Attachment: messages.5.diff added

Now with better backwards compatibility

comment:41 in reply to:  38 Changed 9 years ago by Chris Beaven

Patch needs improvement: unset
Triage Stage: AcceptedReady 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.

Changed 9 years ago by Chris Beaven

Attachment: messages.5.2.diff added

identical to previous, except has empty init.py and models.py files for tests

Changed 9 years ago by Chris Beaven

Attachment: messages.6.diff added

identical to previous, except has empty init.py and models.py files for tests (no, really this time)

Changed 9 years ago by Chris Beaven

Attachment: messages.7.diff added

implement __getitem__ too, some people are using stuff like {{ messages.0 }}

comment:42 Changed 9 years ago by Manoj Govindan <egmanoj@…>

Cc: egmanoj@… added

comment:43 Changed 9 years ago by jedie

Cc: django@… added

comment:44 Changed 9 years ago by anonymous

Cc: grimdonkey@… added

comment:45 Changed 8 years ago by anonymous

milestone: 1.0 beta

comment:46 Changed 8 years ago by Malcolm Tredinnick

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 Changed 8 years ago by anonymous

Cc: robert.hopson@… added

comment:48 Changed 8 years ago by Niels Sandholt Busch

Cc: niels.busch@… removed

comment:49 Changed 8 years ago by technel

Cc: contact@… added

comment:50 Changed 8 years ago by Viktor

Cc: viktor.nagy@… added

comment:51 Changed 8 years ago by Ben Slavin

milestone: post-1.0

Spoke with Malcolm... looks like there's hesitation to do this currently (and somewhat in general).

comment:52 Changed 8 years ago by technel

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 Changed 8 years ago by Manuel Saelices

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.

Changed 8 years ago by John Hensley

Attachment: messages-r8347.diff added

Updated messages.7.diff to avoid rejects with django.core.context_processors.py

comment:54 Changed 8 years ago by John Hensley

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 Changed 8 years ago by Gonzalo Saavedra

Cc: gonz@… added

comment:56 Changed 8 years ago by Tobias McNulty

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 Changed 8 years ago by technel

Cc: contact@… removed

comment:58 Changed 8 years ago by 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/)

comment:59 Changed 8 years ago by Jakub Wilk <ubanus@…>

Cc: ubanus@… added

comment:60 Changed 8 years ago by anonymous

Cc: oliver@… added

comment:61 Changed 8 years ago by Lemuel Formacil

Cc: lemuelf@… added

comment:62 Changed 8 years ago by Dan Fairs

Cc: dan.fairs@… added

comment:63 Changed 8 years ago by Malcolm Tredinnick

milestone: post-1.0
Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

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 in reply to:  63 Changed 8 years ago by Chris Beaven

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 Changed 8 years ago by Chris Beaven

Summary: Message Passing For Anonymous Userssession-based messages

comment:66 Changed 8 years ago by Jarek Zgoda

Cc: jarek.zgoda@… added

comment:67 Changed 8 years ago by anonymous

Cc: hwaara@… added

comment:68 Changed 8 years ago by siddhi

Cc: siddhartag@… added

comment:69 Changed 8 years ago by anonymous

Cc: tonn81@… added

Changed 8 years ago by Ramiro Morales

Attachment: t4604-r9698.diff added

Patch updated to r9698.

comment:70 Changed 8 years ago by Ramiro Morales

I've updated SmileyChris' messages.7.diff to trunk as of now (messages-r8347.diff was missing three files).

comment:71 Changed 8 years ago by izibi

Cc: julian@… added

comment:72 Changed 8 years ago by daniel_martins

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.

comment:73 Changed 8 years ago by anonymous

Cc: flosch@… added

comment:74 Changed 8 years ago by Jacob

milestone: 1.2

comment:75 Changed 8 years ago by David Larlet

Cc: larlet@… removed

comment:76 in reply to:  73 Changed 8 years ago by Leah Culver

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 Changed 8 years ago by Chris Beaven

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 Changed 8 years ago by daniel_martins

Cc: daniel.tritone@… added
Owner: changed from Chris Beaven to daniel_martins
Status: newassigned

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 Changed 8 years ago by miracle2k

Cc: elsdoerfer@… removed

comment:80 Changed 8 years ago by anonymous

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 Changed 8 years ago by daniel_martins

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 Changed 8 years ago by Chris Beaven

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 Changed 7 years ago by Leah Culver

Owner: changed from daniel_martins to Leah Culver
Status: assignednew

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 Changed 7 years ago by Chris Beaven

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!

comment:85 Changed 7 years ago by Tobias McNulty

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 in reply to:  85 Changed 7 years ago by daniel_martins

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 Changed 7 years ago by daniel_martins

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))

comment:88 Changed 7 years ago by Tobias McNulty

  • flash['message'] = ['1', '2'] is too flexible. You might have a project with one pluggable app that just sets flash['message'] = 'message', another that does flash['message'].append('message'), and yet another that does flash['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, but notifications.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/

comment:89 in reply to:  88 ; Changed 7 years ago by daniel_martins

Replying to tobias:

  • flash['message'] = ['1', '2'] is too flexible. You might have a project with one pluggable app that just sets flash['message'] = 'message', another that does flash['message'].append('message'), and yet another that does flash['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, but notifications.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 Changed 7 years ago by Walter Rodrigo de Sá Cruz

Cc: walter.php@… added

comment:91 in reply to:  89 Changed 7 years ago by Tobias McNulty

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 Changed 7 years ago by miracle2k

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 Changed 7 years ago by Jarek Zgoda

Cc: jarek.zgoda@… removed

comment:94 Changed 7 years ago by Tobias McNulty

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 Changed 7 years ago by Alex Gaynor

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 in reply to:  58 Changed 7 years ago by Christian Karrié

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:97 Changed 7 years ago by Tobias McNulty

django-notify has a combo/fallback backend now:

http://code.google.com/p/django-notify/issues/detail?id=5

comment:98 Changed 7 years ago by Tobias McNulty

per Alex's request I started a conversation on django-developers here:

http://groups.google.com/group/django-developers/browse_thread/thread/eba93088c649022b

comment:99 Changed 7 years ago by Tobias McNulty

comment:100 Changed 7 years ago by Tobias McNulty

Leah, do you mind if I claim this ticket? Are you still working on it? Thanks -tobias

comment:101 Changed 7 years ago by Leah Culver

Owner: changed from Leah Culver to Tobias McNulty

Here you go! Nice wiki page summary.

comment:102 Changed 7 years ago by Tobias McNulty

Status: newassigned

comment:103 Changed 7 years ago by Tobias McNulty

development branch for proposed messages app is here: http://bitbucket.org/tobias.mcnulty/django-contrib-messages/

Changed 7 years ago by Tobias McNulty

diff showing changes in django-contrib-messages branch as of changeset 9f54c0f8719c

comment:104 Changed 7 years ago by Michael Campagnaro

Cc: mikecampo@… added

Changed 7 years ago by Tobias McNulty

diff showing changes in django-contrib-messages branch as of changeset e4da706e1152

Changed 7 years ago by Tobias McNulty

changes in django-contrib-messages branch as of rev 6399c12d1773

Changed 7 years ago by Tobias McNulty

Changed 7 years ago by Tobias McNulty

final patch from django-contrib-messages branch

comment:105 Changed 7 years ago by Luke Plant

Resolution: fixed
Status: assignedclosed

(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 Changed 7 years ago by Andrew Badr

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 Changed 7 years ago by Thomas Güttler

Cc: hv@… removed

comment:108 Changed 6 years ago by ubanus@…

Cc: ubanus@… removed
Easy pickings: unset
Severity: Normal
Type: Uncategorized

comment:109 Changed 5 years ago by Jacob

milestone: 1.2

Milestone 1.2 deleted

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