Opened 8 years ago

Closed 6 years ago

Last modified 4 years ago

#4604 closed Uncategorized (fixed)

session-based messages

Reported by: Sean Patrick Hogan <sp.hogan@…> Owned by: tobias
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@…> 8 years ago.
FlashMiddleware and flash context processor
visitor_messages.patch (4.9 KB) - added by SmileyChris 8 years ago.
visitor_messages_upd_6373.patch (6.6 KB) - added by Pedro Lima <pedro.lima@…> 8 years ago.
Update to visitor_messages patch to use session backends
visitor_messages_r6373.patch (6.6 KB) - added by Pedro Lima <pedro.lima@…> 8 years ago.
Updated patch to new session implementation and included the suggested documentation change
visitor_messages_r6596.upatch (5.9 KB) - added by msaelices 8 years ago.
Updated patch for revision 6596
visitor_messages_r6596.patch (6.3 KB) - added by msaelices 8 years ago.
Updated patch for revision 6596
messages.diff (11.9 KB) - added by SmileyChris 8 years ago.
Rewritten version, new core context processor which grabs messages from both user and session
messages.2.diff (15.3 KB) - added by SmileyChris 8 years ago.
More tests
messages.3.diff (15.1 KB) - added by SmileyChris 8 years ago.
removed unnecessary imports
messages.4.diff (15.1 KB) - added by __hawkeye__ 8 years ago.
Slight documentation change.
messages.5.diff (15.2 KB) - added by SmileyChris 7 years ago.
Now with better backwards compatibility
messages.5.2.diff (15.2 KB) - added by SmileyChris 7 years ago.
identical to previous, except has empty init.py and models.py files for tests
messages.6.diff (15.4 KB) - added by SmileyChris 7 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 SmileyChris 7 years ago.
implement __getitem__ too, some people are using stuff like {{ messages.0 }}
messages-r8347.diff (12.7 KB) - added by john 7 years ago.
Updated messages.7.diff to avoid rejects with django.core.context_processors.py
t4604-r9698.diff (14.7 KB) - added by ramiro 7 years ago.
Patch updated to r9698.
django-contrib-messages-9f54c0f8719c.diff (81.8 KB) - added by tobias 6 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 6 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 6 years ago.
changes in django-contrib-messages branch as of rev 6399c12d1773
django-contrib-messages.2.diff (98.5 KB) - added by tobias 6 years ago.
django-contrib-messages.diff (98.5 KB) - added by tobias 6 years ago.
final patch from django-contrib-messages branch

Download all attachments as: .zip

Change History (130)

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

FlashMiddleware and flash context processor

comment:1 Changed 8 years ago by SmileyChris

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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

  • Component changed from Uncategorized to Contrib apps
  • Needs documentation set
  • Needs tests set
  • Owner changed from jacob to adrian
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Design decision needed

comment:3 Changed 8 years ago by ubernostrum

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

comment:4 Changed 8 years ago by SmileyChris

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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

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

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

comment:7 Changed 8 years ago by anonymous

  • Cc goliath.mailinglist@… added

comment:8 Changed 8 years ago by mrmachine

  • 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 8 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 8 years ago by SmileyChris

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

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

  • Cc jshaffer2112@… added

comment:14 Changed 8 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 8 years ago by miracle2k <michael@…>

  • Cc elsdoerfer@… added

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

Update to visitor_messages patch to use session backends

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

  • Cc pedro.lima@… added

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

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

comment:17 Changed 8 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 8 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 8 years ago by __hawkeye__

  • 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 8 years ago by david

  • Cc larlet@… added

Changed 8 years ago by msaelices

Updated patch for revision 6596

Changed 8 years ago by msaelices

Updated patch for revision 6596

comment:21 Changed 8 years ago by anonymous

  • Cc hv@… added

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

  • Cc tom@… added

-1 for loglevels

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

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

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

  • Cc ashanks@… added

comment:26 Changed 8 years ago by niels

  • Cc niels.busch@… added

comment:27 Changed 8 years ago by __hawkeye__

  • Owner changed from nobody to __hawkeye__
  • Patch needs improvement set
  • Status changed from new to assigned
  • Triage Stage changed from Design decision needed to 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:28 Changed 8 years ago by SmileyChris

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

comment:29 Changed 8 years ago by anonymous

  • Cc django@… added

Changed 8 years ago by SmileyChris

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

comment:30 Changed 8 years ago by SmileyChris

  • 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 8 years ago by SmileyChris

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 follow-up: Changed 8 years ago by __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.
  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 8 years ago by SmileyChris

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

More tests

Changed 8 years ago by SmileyChris

removed unnecessary imports

comment:34 Changed 8 years ago by SmileyChris

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

Slight documentation change.

comment:35 Changed 8 years ago by __hawkeye__

  • Triage Stage changed from Accepted to 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 Changed 8 years ago by SmileyChris

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

  • Cc ross@… added

comment:38 follow-up: Changed 8 years ago by mtredinnick

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

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

  • Owner changed from __hawkeye__ to SmileyChris
  • Status changed from assigned to new

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

  • Cc semente@… added

Changed 7 years ago by SmileyChris

Now with better backwards compatibility

comment:41 in reply to: ↑ 38 Changed 7 years ago by SmileyChris

  • Patch needs improvement unset
  • Triage Stage changed from Accepted to 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.

Changed 7 years ago by SmileyChris

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

Changed 7 years ago by SmileyChris

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

Changed 7 years ago by SmileyChris

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

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

  • Cc egmanoj@… added

comment:43 Changed 7 years ago by jedie

  • Cc django@… added

comment:44 Changed 7 years ago by anonymous

  • Cc grimdonkey@… added

comment:45 Changed 7 years ago by anonymous

  • milestone set to 1.0 beta

comment:46 Changed 7 years ago by mtredinnick

  • milestone 1.0 beta deleted

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

  • Cc robert.hopson@… added

comment:48 Changed 7 years ago by niels

  • Cc niels.busch@… removed

comment:49 Changed 7 years ago by technel

  • Cc contact@… added

comment:50 Changed 7 years ago by Viktor

  • Cc viktor.nagy@… added

comment:51 Changed 7 years ago by __hawkeye__

  • milestone set to post-1.0

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

comment:52 Changed 7 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 7 years ago by msaelices

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 7 years ago by john

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

comment:54 Changed 7 years ago by john

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 7 years ago by gonz

  • Cc gonz@… added

comment:56 Changed 7 years ago by tobias

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

  • Cc contact@… removed

comment:58 follow-up: Changed 7 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 7 years ago by Jakub Wilk <ubanus@…>

  • Cc ubanus@… added

comment:60 Changed 7 years ago by anonymous

  • Cc oliver@… added

comment:61 Changed 7 years ago by lemuelf

  • Cc lemuelf@… added

comment:62 Changed 7 years ago by danfairs

  • Cc dan.fairs@… added

comment:63 follow-up: Changed 7 years ago by mtredinnick

  • milestone post-1.0 deleted
  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to 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 in reply to: ↑ 63 Changed 7 years ago by SmileyChris

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 7 years ago by SmileyChris

  • Summary changed from Message Passing For Anonymous Users to session-based messages

comment:66 Changed 7 years ago by zgoda

  • Cc jarek.zgoda@… added

comment:67 Changed 7 years ago by anonymous

  • Cc hwaara@… added

comment:68 Changed 7 years ago by siddhi

  • Cc siddhartag@… added

comment:69 Changed 7 years ago by anonymous

  • Cc tonn81@… added

Changed 7 years ago by ramiro

Patch updated to r9698.

comment:70 Changed 7 years ago by ramiro

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

comment:71 Changed 6 years ago by izibi

  • Cc julian@… added

comment:72 Changed 6 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 follow-up: Changed 6 years ago by anonymous

  • Cc flosch@… added

comment:74 Changed 6 years ago by jacob

  • milestone set to 1.2

comment:75 Changed 6 years ago by david

  • Cc larlet@… removed

comment:76 in reply to: ↑ 73 Changed 6 years ago by leahculver

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 6 years ago by SmileyChris

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

  • Cc daniel.tritone@… added
  • Owner changed from SmileyChris to daniel_martins
  • Status changed from new to 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 Changed 6 years ago by miracle2k

  • Cc elsdoerfer@… removed

comment:80 Changed 6 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 6 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 6 years ago by SmileyChris

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 6 years ago by leahculver

  • Owner changed from daniel_martins to leahculver
  • Status changed from assigned to 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 Changed 6 years ago by SmileyChris

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 follow-up: Changed 6 years ago by tobias

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 6 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 6 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 follow-up: Changed 6 years ago by 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...
  • 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 ; follow-up: Changed 6 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 6 years ago by waltercruz

  • Cc walter.php@… added

comment:91 in reply to: ↑ 89 Changed 6 years ago by tobias

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 6 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 6 years ago by zgoda

  • Cc jarek.zgoda@… removed

comment:94 Changed 6 years ago by tobias

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

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 6 years ago by ckarrie

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 6 years ago by tobias

django-notify has a combo/fallback backend now:

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

comment:98 Changed 6 years ago by tobias

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

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

comment:101 Changed 6 years ago by leahculver

  • Owner changed from leahculver to tobias

Here you go! Nice wiki page summary.

comment:102 Changed 6 years ago by tobias

  • Status changed from new to assigned

comment:103 Changed 6 years ago by tobias

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

Changed 6 years ago by tobias

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

comment:104 Changed 6 years ago by mikecampo

  • Cc mikecampo@… added

Changed 6 years ago by tobias

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

Changed 6 years ago by tobias

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

Changed 6 years ago by tobias

Changed 6 years ago by tobias

final patch from django-contrib-messages branch

comment:105 Changed 6 years ago by lukeplant

  • Resolution set to fixed
  • Status changed from assigned to 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 Changed 5 years ago by andrewbadr

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 5 years ago by guettli

  • Cc hv@… removed

comment:108 Changed 4 years ago by ubanus@…

  • Cc ubanus@… removed
  • Easy pickings unset
  • Severity set to Normal
  • Type set to Uncategorized

comment:109 Changed 4 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

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