Flash helper needs improvement :)

[dyatlov]

https://github.com/plataformatec/devise/issues/1777

Devise adds :timedout = true value there and it breaks flash messages. It should be filtered out somehow. Better of course is to have configurable allowed keys for which we render messages.

[dgmstuart]

Hi @dyatlov, thanks for raising this. Not sure I quite follow the issue - how does it break the messages? Can you describe the scenario which breaks and what the desired behaviour is? Is there a failing spec you can add?

[dyatlov]

Hi @dgmstuart Devise inserting this: flash[:timedout] = true So, when you render then messages in a loop, you add

           (value + link_to("×".html_safe, "#", :class => :close)).html_safe

but value there is true (boolean type) and therefore we have an error from rails which is telling us about incompatible types.

You can try it yourself.. simply put flash[:timedout] = true in one of your actions.. you will see how it impacts :)

[dyatlov]

Desired behaviour is to ignore such keys.. or maybe check if value has type of string.. not sure. In my clone I did this: https://github.com/dyatlov/foundation_rails_helper/commit/5acf532664dfe78bebcb82dea1fba0e5f8daf7c6

[dgmstuart]

Sorry - I should have been more specific: by 'behaviour' I mean what does the user see? From your description I guess desired behaviour is to do nothing?

[dyatlov]

@dgmstuart That's right. Desired behaviour is to ignore such values..

[dgmstuart]

Lol - you're still talking about code (ignoring values) and not what the user sees ;)

[dyatlov]

:) did you read original issue and comments I've posted? https://github.com/plataformatec/devise/issues/1777 Devise adds custom data to flash hash for their internal needs. They tell that flash thing is not for just messages but for everything which should be kept temporarily. So according to their response we shouldn't put everything what is in flash hash to user's screen but only data which we expect will be meaningful to user.

you're still talking about code (ignoring values) and not what the user sees ;)

that's internal stuff which users shouldn't see of course. And current behavior of flash helper is to die with such flash hashes which should be fixed.

[dyatlov]

Added spec for this: https://github.com/sgruhier/foundation_rails_helper/pull/99

[dsandstrom]

@dgmstuart In the production environment, we would see a 500, in the development environment we would see undefined method+' for true:TrueClass`.

This is annoying, but I don't see anything in the Flash documentation from stopping devise, or any other gem, from using the flash hash for something besides UI messages. Instead of blacklisting :timedout, we should blacklist all keys, and whitelist the keys that would be used for our needs (notice, alert, success, etc.). In addition, add an option to whitelist other keys that might pertain to a certain apps's requirements.

[dgmstuart]

@dsandstrom I think the approach you describe would result in the helper breaking people's sites a hard to detect ways: custom flash types would silently fail to be rendered, so if they're not covered by the user's specs, and they're not commonly generated in normal usage, then this would be undetected.

How about instead we just reject flashes unless the value is of a compatible type. Would value.respond_to?(:+) suffice?

[dyatlov]

I think value.is_a?(String) is better

+ not necessarily allows joining with strings which is required

[dsandstrom]

Yeah, it might be confusing for some users. However, there is nothing stopping the FooBarWidget gem from setting flash[:timedout] = 'widgets' and telling us it should only be used for internal purposes and not exposed to app users.

@dyatlov I was just going to say that.

[dsandstrom]

Sorry, wrong button.

[dyatlov]

@dsandstrom agree. There must be an optional config option to limit allowed keys.

[dgmstuart]

@dyatlov typechecks are evil. value.respond_to?(:to_s) would be better if that's what we're checking for.

I guess what you're suggesting then is allowing a config to specify a blacklist of keys which are never intended to be displayed to the user. I can agree with that: these cases are most likely the exceptions. And will fail noisily when they're not excluded. @dsandstrom - do you agree

[dyatlov]

@dgmstuart no, I suggest to optionaly turn on or off feature to enable just a subset of keys. I personally need just :error and :notice.. though I can see that blacklisting can help too.

Regarding :to_s - it wont help against true type. If value is not string then it's not a message we expect, right?

[dyatlov]

It's better to miss some flash keys than to not blacklist a key which will crash your app in a moment you won't expect.

[dgmstuart]

I disagree: I think it's better to fail noisily - which allows the developer to fix things - than to fail silently and make it very difficult to identify why a particular flash doesn't display

— Sent from Mailbox

On Wed, Apr 29, 2015 at 6:28 PM, Vitaly Dyatlov notifications@github.com wrote:

It's better to miss some flash keys than to not blacklist a key which will crash your app in a moment you won't expect.

Reply to this email directly or view it on GitHub: https://github.com/sgruhier/foundation_rails_helper/issues/98#issuecomment-97512200

[dyatlov]

Ok, blacklist would work then. Just in case of that Timeoutable from devise I didn't even test it (as normally it's not required when you turn it on) besides checking that app still works and sent it to server where a client said cool! but then in 10 minutes after timeout: wtf.. site doesnt work anymore!!! (because every page now tries to render that flash) and client couldn't work at all until I released new version which took hours.

[dgmstuart]

@dyatlov I've done some refactoring to do away with the += (which was dodgy anyway) in favour of capture/concat:

https://github.com/dgmstuart/foundation_rails_helper/commit/7773b35b8474c85a574603e709f31cc141d82a4b

With that change, the app doesn't blow up anymore on keys with non-string values - it just display an alert with a value of "true". Next step is to actually exclude that list of internal keys. (working on that now)

[dyatlov]

thanks @dgmstuart awesome!

[dsandstrom]

Fix has been merged, thanks @dgmstuart.