Adds caching to interesting/search api calls

Adds caching to interesting/search api calls

* Caching mechanism added to search and editorial on their API calls.
* Editorial can always cache, as it won't change within a user's session.
* Search requires mechanism to see if search has changed, in which case we
need to hit the API again.

https://code.launchpad.net/~jcsackett/juju-gui/cache-api-calls/+merge/166051

(do not edit description out of merge proposal)

[j.c.sackett, 2013-05-28 13:40:22]

Please take a look.

[rharding, 2013-05-28 14:52:44]

I think there's a discussion to be had about making the cache a bit more common
and shared with the charmresults code instead of in each. There's also a lack of
tests that the new event is fired/caught correctly as well as a lack of tests
that the rendering takes place when the cache is passed in vs doing the mock
requests.

Please look over the notes and let me know if we should chat to go over it.

https://codereview.appspot.com/9828043/diff/1/app/subapps/browser/browser.js
File app/subapps/browser/browser.js (right):

https://codereview.appspot.com/9828043/diff/1/app/subapps/browser/browser.js#...
app/subapps/browser/browser.js:197: _searchChanged: function() {
Can we use this to simplify the _shouldShowSearch then? It seems there's some
dupe in there.

https://codereview.appspot.com/9828043/diff/1/app/subapps/browser/browser.js#...
app/subapps/browser/browser.js:453: this._cache.charms.reset(ev.cache.charms);
I worry about resetting the charms cache here. If you
- load the index, cache interesting, and those are cached into charms
- then go to search and the charms list is blown away with the search charms
- when we go back to the interesting/home does charms get refilled from the
interesting cache?

https://codereview.appspot.com/9828043/diff/1/app/subapps/browser/views/edito...
File app/subapps/browser/views/editorial.js (right):

https://codereview.appspot.com/9828043/diff/1/app/subapps/browser/views/edito...
app/subapps/browser/views/editorial.js:45: EV_CACHE_UPDATED: 'cache-updated',
Please update this with @event doc info.

https://codereview.appspot.com/9828043/diff/1/app/subapps/browser/views/edito...
app/subapps/browser/views/editorial.js:86: var featuredCharms =
this.get('store').resultsToCharmlist(
Can this part be moved to the ajax call version and we can just pass in a list
of charms into this? That way there's no need for the double
cache._intersting/cache._charms and the like. This always gets a list of charms
and we don't have to redo this resultsToCharmList even when we've already done
the work of hydrating the model objects from the result json.

https://codereview.appspot.com/9828043/diff/1/app/subapps/browser/views/edito...
app/subapps/browser/views/editorial.js:157: // Add the charms to the cache for
use in other views.
this post-result stuff is getting enough I wonder if it could be broken up to
some postResults or updateCache method.

https://codereview.appspot.com/9828043/diff/1/app/subapps/browser/views/edito...
app/subapps/browser/views/editorial.js:191: Destroy this view and clear from the
dom world.
These are one space too far in I think. Should be two spaces.

https://codereview.appspot.com/9828043/diff/1/app/subapps/browser/views/edito...
app/subapps/browser/views/editorial.js:205: Initializer
same 3 space issue here I think.

https://codereview.appspot.com/9828043/diff/1/app/subapps/browser/views/searc...
File app/subapps/browser/views/search.js (right):

https://codereview.appspot.com/9828043/diff/1/app/subapps/browser/views/searc...
app/subapps/browser/views/search.js:45: EV_CACHE_UPDATED: 'cache-updated',
If we only cache the list of models and not the actual api call results we
should be able to move this to the CharmResults and share the caching idea in
one place.

https://codereview.appspot.com/9828043/diff/1/app/subapps/browser/views/searc...
app/subapps/browser/views/search.js:138: var active = this.get('activeID');
There's an event in place to update active in charmresults. This should use that
instead of doing the work. It handles checking for fullscreen and such.

https://codereview.appspot.com/9828043/diff/1/app/subapps/browser/views/searc...
app/subapps/browser/views/search.js:209: if (cfg && cfg.search) {
I'm really not a fan of having an ATTR search on a view search that gets
assigned to a cache. Can this just be if cfg.cache or cfg.results?

https://codereview.appspot.com/9828043/diff/1/app/subapps/browser/views/view.js
File app/subapps/browser/views/view.js (right):

https://codereview.appspot.com/9828043/diff/1/app/subapps/browser/views/view....
app/subapps/browser/views/view.js:202: this._cache = {
why is this updated for insteresting/search? It should only case about the
charms cache and not need to know/worry about the other two.

[j.c.sackett, 2013-05-28 15:46:30]

https://codereview.appspot.com/9828043/diff/1/app/subapps/browser/browser.js
File app/subapps/browser/browser.js (right):

https://codereview.appspot.com/9828043/diff/1/app/subapps/browser/browser.js#...
app/subapps/browser/browser.js:197: _searchChanged: function() {
On 2013/05/28 14:52:44, rharding wrote:
> Can we use this to simplify the _shouldShowSearch then? It seems there's some
> dupe in there. 

Not really; the states being ANDed here are ORed there. This isn't really
answering the same question at all, it's just using the same information.

https://codereview.appspot.com/9828043/diff/1/app/subapps/browser/browser.js#...
app/subapps/browser/browser.js:453: this._cache.charms.reset(ev.cache.charms);
On 2013/05/28 14:52:44, rharding wrote:
> I worry about resetting the charms cache here. If you
> - load the index, cache interesting, and those are cached into charms
> - then go to search and the charms list is blown away with the search charms
> - when we go back to the interesting/home does charms get refilled from the
> interesting cache?

It does; both the search and the interesting cache reset the global cache when
they update, from their data. This *is* somewhat cyclical when the cache hasn't
updated, but the operation is relatively cheap. The alternative, not resetting,
means during a long session we're getting a larger and larger cache as we re-add
the interesting charms or the search charms over and over.

Since the charm cache is only going to be used for opening charms in charm
details, and the only way (sans URL hacking, and screw those guys) to open charm
details is from a token, we only need the current set of tokens in the cache.

https://codereview.appspot.com/9828043/diff/1/app/subapps/browser/views/edito...
File app/subapps/browser/views/editorial.js (right):

https://codereview.appspot.com/9828043/diff/1/app/subapps/browser/views/edito...
app/subapps/browser/views/editorial.js:45: EV_CACHE_UPDATED: 'cache-updated',
On 2013/05/28 14:52:44, rharding wrote:
> Please update this with @event doc info.

Done.

https://codereview.appspot.com/9828043/diff/1/app/subapps/browser/views/edito...
app/subapps/browser/views/editorial.js:86: var featuredCharms =
this.get('store').resultsToCharmlist(
On 2013/05/28 14:52:44, rharding wrote:
> Can this part be moved to the ajax call version and we can just pass in a list
> of charms into this? That way there's no need for the double
> cache._intersting/cache._charms and the like. This always gets a list of
charms
> and we don't have to redo this resultsToCharmList even when we've already done
> the work of hydrating the model objects from the result json.

This part I think we want to talk about; If I follow correctly, I think you mean
restructure _cache.interesting to be something like 
{
  featured: FeaturedBrowserList
  new: ...
  &c ...
}

This won't remove the .charms/.interesting stuff--.charms is a list of all
charms, and interesting, to be useful, should be grouped into the separate bits.

https://codereview.appspot.com/9828043/diff/1/app/subapps/browser/views/searc...
File app/subapps/browser/views/search.js (left):

https://codereview.appspot.com/9828043/diff/1/app/subapps/browser/views/searc...
app/subapps/browser/views/search.js:184: var active = this.get('activeID');
Here's the previous location of that same code.

https://codereview.appspot.com/9828043/diff/1/app/subapps/browser/views/searc...
File app/subapps/browser/views/search.js (right):

https://codereview.appspot.com/9828043/diff/1/app/subapps/browser/views/searc...
app/subapps/browser/views/search.js:45: EV_CACHE_UPDATED: 'cache-updated',
On 2013/05/28 14:52:44, rharding wrote:
> If we only cache the list of models and not the actual api call results we
> should be able to move this to the CharmResults and share the caching idea in
> one place. 

Let's put that off to when we're talking. .search and .interesting endpoints
seem different enough to me that I'm not sure they can be merged as I think
you're suggesting.

https://codereview.appspot.com/9828043/diff/1/app/subapps/browser/views/searc...
app/subapps/browser/views/search.js:138: var active = this.get('activeID');
On 2013/05/28 14:52:44, rharding wrote:
> There's an event in place to update active in charmresults. This should use
that
> instead of doing the work. It handles checking for fullscreen and such.

Please file that as a follow up, as it's not part of this branch--this was
simply moved from one place to another from the previous implementation. (see
note below)

I'm happy to land a branch towards that as clean up, but I think it's out of
scope here.

https://codereview.appspot.com/9828043/diff/1/app/subapps/browser/views/searc...
app/subapps/browser/views/search.js:209: if (cfg && cfg.search) {
On 2013/05/28 14:52:44, rharding wrote:
> I'm really not a fan of having an ATTR search on a view search that gets
> assigned to a cache. Can this just be if cfg.cache or cfg.results?

How about search_results? I would like the browser._cache and the view ._cache
to have the same names, and in the scope of browser your suggested names seem to
ambiguous.

https://codereview.appspot.com/9828043/diff/1/app/subapps/browser/views/view.js
File app/subapps/browser/views/view.js (right):

https://codereview.appspot.com/9828043/diff/1/app/subapps/browser/views/view....
app/subapps/browser/views/view.js:202: this._cache = {
On 2013/05/28 14:52:44, rharding wrote:
> why is this updated for insteresting/search? It should only case about the
> charms cache and not need to know/worry about the other two.

I don't actually know--I think I put this in here while exploring solutions and
forgot to take it out.

Looking at the code, I don't think this needs the _cache *at all*. The only
descendants of this object are Sidebar and Fullscreen, and neither use any cache
at all, including the charms.

[j.c.sackett, 2013-05-29 14:17:09]

Please take a look.

[rharding, 2013-05-29 14:29:36]

Thanks, this looks a lot nicer. I think there's even more code to go. Please
look at the comments and let me know if I'm mistaken.

https://codereview.appspot.com/9828043/diff/9001/app/subapps/browser/browser.js
File app/subapps/browser/browser.js (right):

https://codereview.appspot.com/9828043/diff/9001/app/subapps/browser/browser....
app/subapps/browser/browser.js:320: this._cache.charms.destroy();
should this check for interesting/search to either set to undefined, or unset as
well?

https://codereview.appspot.com/9828043/diff/9001/app/subapps/browser/browser....
app/subapps/browser/browser.js:435: if (this._cache.interesting) {
You've init'd _cache.interesting to null. can you just pass it in and it'll fail
any check for if(cache) later on. Then this if can go away and just call
.render(this._cache.interesting)

https://codereview.appspot.com/9828043/diff/9001/app/subapps/browser/browser....
app/subapps/browser/browser.js:476: if (this._cache.search &&
!this._searchChanged()) {
same if condition can go away here .

https://codereview.appspot.com/9828043/diff/9001/app/subapps/browser/views/ch...
File app/subapps/browser/views/charmresults.js (right):

https://codereview.appspot.com/9828043/diff/9001/app/subapps/browser/views/ch...
app/subapps/browser/views/charmresults.js:47: @event EV_CACHE_UPDATED when the
cache has been updated this is fired
please also describe the expected payload in the ev.

https://codereview.appspot.com/9828043/diff/9001/app/subapps/browser/views/ed...
File app/subapps/browser/views/editorial.js (right):

https://codereview.appspot.com/9828043/diff/9001/app/subapps/browser/views/ed...
app/subapps/browser/views/editorial.js:157: this.fire(this.EV_CACHE_UPDATED,
{cache: this._cache});
Does this need to track the _cache? It needs to fire the event with the data to
get the data to the subapp, but I don't think it needs to store the _cache at
all. The next call the subapp will pass down the cached data and it'll start all
over again.

https://codereview.appspot.com/9828043/diff/9001/app/subapps/browser/views/ed...
app/subapps/browser/views/editorial.js:177: this.get('store').interesting({
awesome, like this a lot better.

https://codereview.appspot.com/9828043/diff/9001/app/subapps/browser/views/ed...
app/subapps/browser/views/editorial.js:215: this._cache = {
so yea, if there's no need to hold the cache, only to fire the event with the
data, the init here gets simpler.

https://codereview.appspot.com/9828043/diff/9001/app/subapps/browser/views/se...
File app/subapps/browser/views/search.js (right):

https://codereview.appspot.com/9828043/diff/9001/app/subapps/browser/views/se...
app/subapps/browser/views/search.js:142: this._cache.charms.reset(results);
same thing here. I think it can just fire the event and not store the cache.

https://codereview.appspot.com/9828043/diff/9001/app/subapps/browser/views/se...
app/subapps/browser/views/search.js:209: initializer: function(cfg) {
same thing here.

https://codereview.appspot.com/9828043/diff/9001/app/subapps/browser/views/vi...
File app/subapps/browser/views/view.js (right):

https://codereview.appspot.com/9828043/diff/9001/app/subapps/browser/views/vi...
app/subapps/browser/views/view.js:194: this.set('store', new
Y.juju.Charmworld0({
this should never set the store. It gets it from the subapp. None of the views
should know about it.

[j.c.sackett, 2013-05-29 15:04:01]

Comments addressed, changes coming.

https://codereview.appspot.com/9828043/diff/9001/app/subapps/browser/browser.js
File app/subapps/browser/browser.js (right):

https://codereview.appspot.com/9828043/diff/9001/app/subapps/browser/browser....
app/subapps/browser/browser.js:320: this._cache.charms.destroy();
On 2013/05/29 14:29:36, rharding wrote:
> should this check for interesting/search to either set to undefined, or unset
as
> well?

Yup, done.

https://codereview.appspot.com/9828043/diff/9001/app/subapps/browser/browser....
app/subapps/browser/browser.js:435: if (this._cache.interesting) {
On 2013/05/29 14:29:36, rharding wrote:
> You've init'd _cache.interesting to null. can you just pass it in and it'll
fail
> any check for if(cache) later on. Then this if can go away and just call
> .render(this._cache.interesting)

Good call. Done.

https://codereview.appspot.com/9828043/diff/9001/app/subapps/browser/browser....
app/subapps/browser/browser.js:476: if (this._cache.search &&
!this._searchChanged()) {
On 2013/05/29 14:29:36, rharding wrote:
> same if condition can go away here .

Yes, though the !this._searchChanged must remain so we don't pass in a non-null,
but stale cache.

https://codereview.appspot.com/9828043/diff/9001/app/subapps/browser/views/ch...
File app/subapps/browser/views/charmresults.js (right):

https://codereview.appspot.com/9828043/diff/9001/app/subapps/browser/views/ch...
app/subapps/browser/views/charmresults.js:47: @event EV_CACHE_UPDATED when the
cache has been updated this is fired
On 2013/05/29 14:29:36, rharding wrote:
> please also describe the expected payload in the ev.

Per discussion, we don't need this.

https://codereview.appspot.com/9828043/diff/9001/app/subapps/browser/views/ed...
File app/subapps/browser/views/editorial.js (right):

https://codereview.appspot.com/9828043/diff/9001/app/subapps/browser/views/ed...
app/subapps/browser/views/editorial.js:157: this.fire(this.EV_CACHE_UPDATED,
{cache: this._cache});
On 2013/05/29 14:29:36, rharding wrote:
> Does this need to track the _cache? It needs to fire the event with the data
to
> get the data to the subapp, but I don't think it needs to store the _cache at
> all. The next call the subapp will pass down the cached data and it'll start
all
> over again.

Good call; killed the tracking. Now we just build a cache object and fire it
off.

https://codereview.appspot.com/9828043/diff/9001/app/subapps/browser/views/ed...
app/subapps/browser/views/editorial.js:215: this._cache = {
On 2013/05/29 14:29:36, rharding wrote:
> so yea, if there's no need to hold the cache, only to fire the event with the
> data, the init here gets simpler.

Actually, the init goes away; the remaining bit just does the "if a cache was
passed in as part of the extraCfg, add it" but we're not doing that anymore.

https://codereview.appspot.com/9828043/diff/9001/app/subapps/browser/views/se...
File app/subapps/browser/views/search.js (right):

https://codereview.appspot.com/9828043/diff/9001/app/subapps/browser/views/se...
app/subapps/browser/views/search.js:142: this._cache.charms.reset(results);
On 2013/05/29 14:29:36, rharding wrote:
> same thing here. I think it can just fire the event and not store the cache.

Yeah, again now we're just building a cache object to fire off in the event, but
not keeping it.

https://codereview.appspot.com/9828043/diff/9001/app/subapps/browser/views/se...
app/subapps/browser/views/search.js:209: initializer: function(cfg) {
On 2013/05/29 14:29:36, rharding wrote:
> same thing here.

Yup; whole thing is deleted.

https://codereview.appspot.com/9828043/diff/9001/app/subapps/browser/views/vi...
File app/subapps/browser/views/view.js (right):

https://codereview.appspot.com/9828043/diff/9001/app/subapps/browser/views/vi...
app/subapps/browser/views/view.js:194: this.set('store', new
Y.juju.Charmworld0({
On 2013/05/29 14:29:36, rharding wrote:
> this should never set the store. It gets it from the subapp. None of the views
> should know about it.

This showed up after I merged trunk; I'll kill it, since it's apparently not in
trunk, but I'm curious how it arrived.

https://codereview.appspot.com/9828043/diff/9001/test-server.sh
File test-server.sh (right):

https://codereview.appspot.com/9828043/diff/9001/test-server.sh#newcode11
test-server.sh:11: mocha-phantomjs -t 40000 http://0.0.0.0:8084/test/
I got no "w00t" for this? I was expecting more excitement, man. :-P

[j.c.sackett, 2013-05-29 15:17:16]

Please take a look.

[rharding, 2013-05-29 15:25:05]

LGTM thanks for working on this.

https://codereview.appspot.com/9828043/diff/17001/app/subapps/browser/views/e...
File app/subapps/browser/views/editorial.js (right):

https://codereview.appspot.com/9828043/diff/17001/app/subapps/browser/views/e...
app/subapps/browser/views/editorial.js:37: * Editorial view for landing pages.
this * goes away as well.

[matthew.scott, 2013-05-29 15:32:42]

LGTM, thanks!

https://codereview.appspot.com/9828043/diff/9001/app/subapps/browser/views/ed...
File app/subapps/browser/views/editorial.js (right):

https://codereview.appspot.com/9828043/diff/9001/app/subapps/browser/views/ed...
app/subapps/browser/views/editorial.js:37: * Editorial view for landing pages.
Trivial: I think if we're going to remove asterisks, this one should go too.

[j.c.sackett, 2013-05-29 16:39:41]

*** Submitted:

Adds caching to interesting/search api calls

* Caching mechanism added to search and editorial on their API calls.
* Editorial can always cache, as it won't change within a user's session.
* Search requires mechanism to see if search has changed, in which case we
need to hit the API again.

R=rharding, matthew.scott
CC=
https://codereview.appspot.com/9828043