Code

Opened 7 years ago

Closed 4 years ago

#3995 closed (wontfix)

Give the Message model a type (success/failure/generic)

Reported by: joe4444 Owned by: timo
Component: Core (Other) Version: master
Severity: Keywords: auth message
Cc: django@…, ross@…, xerdcire@…, jwm@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description

The message system is just a bit too light for me. Rather than assume all messages indicate a successful action I think they should also be able to point out errors (unrelated to form validation obviously) or just display generic notices. This patch makes it possible to create 3 types of messages: success (default), failure, and generic. Normal messages will remain the same (always success). For a failure/error or generic message you simply pass an extra argument like so:

    user.message_set.create(message=u'Something went wrong!', category='F')    # failure or error
    user.message_set.create(message=u'This is just a notice', category='G')    # generic message
    user.message_set.create(message=u'It worked!', category='S')    # explicit success
    user.message_set.create(message=u'It worked!')    # implicit success


Now in your template you can distinguish messages by type, and then use CSS to style them differently.

{% if messages %}
  <ul id="messages">
{% for message in messages %}
    <li class="{{ message.get_category_display }}">{{ message }}</li>
{% endfor %}
  </ul>
{% endif %}

Attachments (5)

message_type.diff (880 bytes) - added by joe4444 7 years ago.
Modify the Message model to categorize messages
message_type.2.diff (1.7 KB) - added by joe4444 7 years ago.
Same code; appended Message model docstring
message_type.3.diff (1.7 KB) - added by joe4444 7 years ago.
I think the others only worked for models.py from 0.96
3995.django.contrib.auth.diff (3.8 KB) - added by PJCrosier 7 years ago.
Updated messages with a type, with tests and documentation
3995.diff (5.2 KB) - added by timo 5 years ago.
updated patch to apply to trunk, added post_syncdb handler to add field

Download all attachments as: .zip

Change History (26)

Changed 7 years ago by joe4444

Modify the Message model to categorize messages

comment:1 Changed 7 years ago by joe4444

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Version changed from SVN to 0.96

Probably doesn't make a difference, but I patched 0.96 not trunk.

Changed 7 years ago by joe4444

Same code; appended Message model docstring

comment:2 Changed 7 years ago by joe4444

  • Needs documentation set
  • Needs tests set
  • Patch needs improvement set

Using a CharField with choices might not be the ideal solution. Any other suggestions?

comment:3 Changed 7 years ago by Simon G. <dev@…>

  • Triage Stage changed from Unreviewed to Design decision needed
  • Version changed from 0.96 to SVN

Interesting idea Joe, please bring it up on Django-Developers to get some discussion going (this is a backwards incompat. issue after all).

comment:4 Changed 7 years ago by Simon G. <dev@…>

  • Summary changed from Give the Message model a type to Give the Message model a type (success/failure/generic)

comment:5 Changed 7 years ago by joe4444

Yeah I guess you're right. I was thinking since code for creating new messages doesn't have to change then it is backwards compatible. But changing the Message model requires an update on the db table auth_message.

Changed 7 years ago by joe4444

I think the others only worked for models.py from 0.96

comment:6 Changed 7 years ago by ubernostrum

I'd be -1 on this because of the backwards incompatibility, but there have been various proposals to tie messages into sessions (for messages to anonymous users), and if we ever do that we should revisit this issue.

comment:7 Changed 7 years ago by jacob

  • Triage Stage changed from Design decision needed to Accepted

I think this is basically a good idea, but there needs to be a transition plan. I'd like to see a post-syncdb handler that automatically upgrades the table (if needed), and I'd like the type to just be a string field (instead of choices). And optional, of course.

comment:8 Changed 7 years ago by xian

I think type as a slugfield makes sense. That way you can use them as css classes and deal with them in the templates a bit easier. And it would be good to be able to get to them separately in the templates {{ messages }} all messages. {{ messages.warning }} just the messages with type='warning'.

comment:9 Changed 7 years ago by anonymous

  • Cc django@… added

comment:10 Changed 7 years ago by anonymous

  • Cc ross@… added

comment:11 Changed 7 years ago by PJCrosier

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to PJCrosier
  • Patch needs improvement unset

Patch attached as per Jacob's suggestion - just using a string (limited to 30 chars) rather than choices. Ignoring this patch, I think all calls to user.message_set.create need to use (message="blah") rather than just ("blah") - so I think it's backwards compatible (the type is optional in the patch)?

Changed 7 years ago by PJCrosier

Updated messages with a type, with tests and documentation

comment:12 Changed 7 years ago by PJCrosier

Quick patch update to use "category" rather than "type" attached, thanks to oebfare for pointing out problems and that the patch is indeed backwards incompatible without doing something post-syncdb.

comment:13 Changed 6 years ago by edrex

IMO this patch is pretty important since many people are hacking around the limitation. The patch looks good, RTBC?

comment:14 Changed 6 years ago by edrex

a nitpick, "type" seems more standard than "category"

comment:15 Changed 6 years ago by edrex

  • Cc xerdcire@… added

comment:16 Changed 5 years ago by jwm

  • Cc jwm@… added

comment:17 follow-up: Changed 5 years ago by m@…

What happened with this? It says "accepted," there's a patch from 2007 that I assume will no longer work, and there's no commentary beyond that. Are we just waiting on a new patch? I know it's almost 1.1 time, should this be brought up on django-dev during 1.2 feature selection, or was there some conscious decision to abandon this change?

comment:18 in reply to: ↑ 17 Changed 5 years ago by kmtracey

Replying to m@mihasya.com:

What happened with this? It says "accepted," there's a patch from 2007 that I assume will no longer work, and there's no commentary beyond that. Are we just waiting on a new patch? I know it's almost 1.1 time, should this be brought up on django-dev during 1.2 feature selection, or was there some conscious decision to abandon this change?

The most recent patch does not address Jacob's transition plan requirement mentioned in http://code.djangoproject.com/ticket/3995#comment:7. Any patch that changes the DB schema for a contrib app has to deal with automatically upgrading existing installs, and near as I can see there has been no effort to do that.

comment:19 Changed 5 years ago by kmtracey

  • Patch needs improvement set

Changed 5 years ago by timo

updated patch to apply to trunk, added post_syncdb handler to add field

comment:20 Changed 5 years ago by timo

  • Owner changed from PJCrosier to timo

I've brought the old patch up to date and added a first attempt at a post syncdb handler to add the field in the database. Not sure the best way to detect if the field currently exists in the db (just using try/catch currently). Any suggestions on this or any other aspects of the patch appreciated.

comment:21 Changed 4 years ago by timo

  • Resolution set to wontfix
  • Status changed from new to closed

Seems like this would be very difficult to pull off due to backwards compatibility, plus the new messages framework in 1.2 has message types.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.