Implement flash messages #74

Closed
Techwraith opened this Issue Feb 1, 2012 · 23 comments

Projects

None yet

5 participants

@Techwraith
Contributor

Not sure if this is outside the scope of geddy core, but...

I'll be adding bootstrap to all newly generated apps, this should give apps a lot of functionality on the front end right out of the box. It would be cool if we made some optional components to be used on the server side to take advantage of some of these things. Flash messages would be a good place to start.

@djensen47

I created an issue on geddy-passport about the lack of error messaging on a failed login. Upon inspecting the code, I found that there was no "easy" way to do this, i.e., using a flash message.

I'm more than willing to implement this. The obvious place to put the flash message would be in the session. I'm thinking the format of the object would be as follows (but I'm open to alternative suggestions):

{ 
  'info': ['info message 1', 'info message 2'],
  'error': ['error message 1', 'error message 2']
} 

I'm thinking it would be set as follows. I like the idea (like in Rails) of having the type be flexible but also having a few defaults for error, info, etc. Since Bootstrap is the preferred CSS framework, then it seems appropriate to use the Bootstrap nomenclature: alert, error, success, info

geddy.flash('error', message)
// or
geddy.flashAlert(alertMsg);
geddy.flashError(errorMsg);
geddy.flashSuccess(successMsg);
geddy.flashInfo(infoMsg);

Calling any of these multiple times would simply add another item to the array of messages.

Finally, there should be two helpers. One that reads the flash from the session and outputs nice Bootstrap compatible html. Another one that just returns the flash object from the session allowing the end user to output it how they prefer (which is going to be damn problematic with handlebars).

I'm still learning the code base so I just need a pointer as to where to start. Where does geddy.flash belong? What about the helpers?

@Techwraith
Contributor

I'd love to see this get started!

Your assumptions seem pretty sound - we should definitely use boostrap conventions whenever we can for this.

View helpers should go in a file in here: https://github.com/mde/geddy/tree/master/lib/template/helpers

I'm thinking instead of geddy.flash, we should just implement it on the controller (this.flash()). You can implement that here: https://github.com/mde/geddy/blob/master/lib/controller/base_controller.js

@mde
Contributor
mde commented Jan 28, 2013

A couple of things to think about -- the error from an invalid Model instance is an object with keys matching the invalid fields, and values of the localized error-message for why the property is invalid. I guess any sort of flash-system would need to have a smart way to handle that pattern.

Also, if at all possible, I'd like to avoid stuffing things like error-messages into the session, particularly because the cookie session-store that lots of people will likely be using is extremely limited in size. What do Rails and Django do with this?

@Techwraith
Contributor

They stick it in the cookie ;)

http://guides.rubyonrails.org/action_controller_overview.html#the-flash

On Mon, Jan 28, 2013 at 12:09 PM, Matthew Eernisse <notifications@github.com

wrote:

A couple of things to think about -- the error from an invalid Model
instance is an object with keys matching the invalid fields, and values of
the localized error-message for why the property is invalid. I guess any
sort of flash-system would need to have a smart way to handle that pattern.

Also, if at all possible, I'd like to avoid stuffing things like
error-messages into the session, particularly because the cookie
session-store that lots of people will likely be using is _extremely_limited in size. What do Rails and Django do with this?


Reply to this email directly or view it on GitHubhttps://github.com/mde/geddy/issues/74#issuecomment-12801608.

@djensen47

IMO, flash messages shouldn't be used for field by field error messages but for something more general. For example, "Please fix the errors in the form below." Or, "Your login credentials were incorrect."

@mde
Contributor
mde commented Jan 28, 2013

Right on, we've been playing "WWRD" for a while, so it seems to make sense to do it here too -- looks like the flash object is a top-level construct, so yes, let's do it that way too. I can still be stored with everything else in the session, but just cleared out with every request. Looks like they accomplish this with the usual Ruby magic of exposed setters/getters for a private internal property. Basically we need to make sure that if a flash property hasn't been explicitly set during the previous request cycle, that it's empty for access. If we have to do that with method-calls instead of property-lookups, that's fine.

Also it looks like 4K is a nice rule-of-thumb size-limit for cookies, so let's implement something that will throw an error at the end of the response-cycle if you've put too much shit in. That would be nice to have anyhow.

@djensen47

How should we clear the flash when there is a redirect. If you set the flash, redirect, and clear on every request cycle, then the flash will never get to the template.

There needs to be a sweep function that marks unused flashes as used. The same function removes the flashes that were already marked as used.

Also, it looks like I need to insert the flash clearing code somewhere in lib/app.js

Is there a place that is better than another in this big function to do this?

Now the only problem I can see is if there were two redirects in a row. You lose the flash message again.

@mde
Contributor
mde commented Feb 1, 2013

Sounds like the flash object should include the previous response status. Any 2xx (maybe 4xx too?) for the previous response means you clear out the flash.

As far as how to organize the code, you could probably hook it up right after the session gets set up. You probably want to pass it the session object, since it's going to be stored in the session. Of course implement it in a separate file -- but yes, the hookup means a bit more code on the app.js pile.

I don't imagine you're interested in doing the big refactor of app.js that's we've been needing for a while now. I plan on working on that when I get some uninterrupted time, but it's going to be a pretty big job.

@der-On
Contributor
der-On commented Apr 11, 2013

In my app I created a simple "class" using the geddy session to store and retrieve flash messsages. Maybe you can use some of the code: https://gist.github.com/der-On/5361790

@mde
Contributor
mde commented Apr 11, 2013

This looks like a good start. How about putting together a pull request for this?

@der-On
Contributor
der-On commented Apr 12, 2013

Yeah. I need to find the time (maybe tonight). First I must setup a working dev env for a global geddy to test things.
How are you developing geddy which must be run globally (which means as root)?

@mde
Contributor
mde commented Apr 12, 2013

Working on Geddy is easy. Just git clone the repo, then instead of running your installed global geddy, you use the cli.js in your cloned repo, for example:

$ git clone git@github.com:mde/geddy.git
$ ./geddy/bin/cli.js app foo
$ cd foo
$ ../geddy/bin/cli.js
@der-On
Contributor
der-On commented Apr 12, 2013

=D

@MiguelMadero
Contributor

I actually changed the simlink to point it to my cloned version, so I don't
have to type the bath to cli.js, I just type geddy and it's always using
the version from trunk. You can do it manually or run ./make & sudo ./make install

Miguel

On Fri, Apr 12, 2013 at 1:44 AM, Ondrej notifications@github.com wrote:

=D


Reply to this email directly or view it on GitHubhttps://github.com/mde/geddy/issues/74#issuecomment-16282034
.

@mde
Contributor
mde commented Apr 12, 2013

That defniitely works too. :) OTOH, I do like to keep the global separate so I can test installation from NPM.

@MiguelMadero
Contributor

I hag geddy and ggeddy (for git geddy) on another box. I was always working
from git so I haven't installed from npm, but that won't work for most ppl.
I think having a second simlink is the best option, just to avoid the extra
typing.
On 12/04/2013 12:12 PM, "Matthew Eernisse" notifications@github.com wrote:

That defniitely works too. :) OTOH, I do like to keep the global separate
so I can test installation from NPM.


Reply to this email directly or view it on GitHubhttps://github.com/mde/geddy/issues/74#issuecomment-16311884
.

@mde
Contributor
mde commented Apr 17, 2013

This is now in master as of commit 3b40dfc. It includes @der-On's initial work, plus some rewriting to create an API closer to Rails flash-messages. This will be in the next release.

@der-On
Contributor
der-On commented Apr 17, 2013

mde: your current implementation only allows one message per type. Why not allow multiple messages of same type?

@mde
Contributor
mde commented Apr 17, 2013

Basically implementing the same API as Rails. You can store any objects you want in the Flash(es), as long as they're JSON-serializable. So you can store an array of messages if you want.

The ability to store small objects is important because the default 'errors' property that comes back from an invalid Model instance is an object literal with a key for each invalid field, and a value of the error-message. This will let us write our scaffolds to do flash by default on create and update for invalid instances.

@der-On
Contributor
der-On commented Apr 18, 2013

Okay. Going after an existing API is fine for me. If I have new ideas/proposals in future I will try to read the Rails API about it first. ;)

@mde
Contributor
mde commented Apr 19, 2013

@der-On, yes, we really try to observe the POLS (http://en.wikipedia.org/wiki/Principle_of_least_surprise), and stick to the well vetted Rails-style APIs where possible. But I do want to stress that your work here was critical -- this feature would not have happened without the work you did on it. :)

@mde
Contributor
mde commented May 14, 2013

Released in v0.8.

@mde mde closed this May 14, 2013
@ben-ng ben-ng pushed a commit to ben-ng/geddy that referenced this issue May 15, 2013
Ben Ng Store flash messages (#74) with their CSS display class so templates …
…don't have to be polluted with logic
8a10ee6
@ben-ng ben-ng pushed a commit to ben-ng/geddy that referenced this issue May 15, 2013
Ben Ng Display flash messages (#74) in layout templates 2710e41
@ben-ng ben-ng added a commit to ben-ng/geddy that referenced this issue Jun 6, 2013
@ben-ng Ben Ng + ben-ng Store flash messages (#74) with their CSS display class so templates …
…don't have to be polluted with logic
6ec73bf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment