search

kittoframework / kitto

Kitto is a framework for interactive dashboards written in Elixir
http://kitto.io/dashboards/sample
MIT License
956 stars 58 forks source link

string to atom to fix API post on widgets so cache map is updated pro… #109

Closed aroyer closed 7 years ago

aroyer commented 7 years ago

…perly

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.3%) to 88.636% when pulling abb15f9801d8187d2cda440952e8f65c9acca078 on aroyer:a_atom into 8c3af7d8260b004b81f03a8e3822d2f1b8ddf1eb on kittoframework:master.

davejlong commented 7 years ago

Can you describe in more detail what the issue is that this is solving exactly, including a test case?

aroyer commented 7 years ago

the internal Kitto.Notifier.cache has all keys as atoms for some reason by design (https://github.com/kittoframework/kitto/blob/master/lib/kitto/notifier.ex#L58). when you post using the API to a widget the json comes in as strings so the cache map can only update string keys but they are all internally stored as atoms. to conform with design on cache map with keys as atoms changing strings to atoms on json api posts. if you want to change internal data storage to strings that sounds better.

simple test to reproduce is post to widget with a key and try and get the value posted at that key back with a get and will notice its returning what was internally set by the job. in other words the atom key is fetched and can see in Kitto.Notifier.cache that there are 2 entries one as a string key and the other atom key. added a test here https://github.com/kittoframework/kitto/pull/109/files#diff-06b39c5def3218a2fa4fd995a7db8d99R338

davejlong commented 7 years ago

I would suggest that we change the cache to use string based keys instead of atoms if we're to allow interacting with them through a public interface.

zorbash commented 7 years ago

@aroyer First I want to thank you for spotting this and trying to fix it. We have concluded to change the cache keys to strings. I've made the required changes on a branch. Can you adapt your PR to these changes?

zorbash commented 7 years ago

@aroyer I made the required changes to have this resolved (see: d91d015d).

Thanks again for getting involved!

aroyer commented 7 years ago

thank you much for the fix and the great framework.