Add support for related charm loading and display

Add support for related charm loading and display

- Adds a new store endpoint for the related charm api call
- Adds the idea of relatedCharms for model ATTRS
- Adds two helpers to take the api response data into something we can use in
the app
- Renders out the related charms in the template in fullscreen as an ajax
request
- Updates the charm details to handle click events on charm tokens 
- Adds tests for the model helpers and the details view
- Updates a bunch of tests. There were left over testing Nodes that caused
stray tests to fail. I fixed those and also went back and cleaned up the stray
test mess.
- Updates the css to work out so that the tokens can fit. There's a
redesign-a-coming so the work was to fit in currently and the redesign will
move the layout at a later date.

Notes:

- This does not currently add the related charm display to the interfaces tab.
It's a slightly different UX and this branch was taking too long as is.

- I'm not happy with the relatedCharms ATTR. It's not really an ATTR but I
wanted this cached into the model because we'll use that when we add related
charms to the interfaces tab. We don't want to re-request the data. However,
in sidebar move it will not have been requested yet. My plan is to re-evaluate
and possibly refactor when doing the interfaces tab display.

https://code.launchpad.net/~rharding/juju-gui/related-charm/+merge/170313

(do not edit description out of merge proposal)

[rharding, 2013-06-19 17:34:31]

Please take a look.

[rharding, 2013-06-19 17:39:40]

found one typo already wheeee

https://codereview.appspot.com/10416044/diff/1/app/subapps/browser/views/char...
File app/subapps/browser/views/charm.js (right):

https://codereview.appspot.com/10416044/diff/1/app/subapps/browser/views/char...
app/subapps/browser/views/charm.js:514: if (this.featuredCharmContainer) {
this is wrong, it's supposed to be relatedCharmContainer. Updating in current
branch.

[teknico, 2013-06-20 14:17:36]

LGTM, did not go through the big JSON file, as instructed. :-) Please find some
questions below.

https://codereview.appspot.com/10416044/diff/1/app/subapps/browser/views/char...
File app/subapps/browser/views/charm.js (right):

https://codereview.appspot.com/10416044/diff/1/app/subapps/browser/views/char...
app/subapps/browser/views/charm.js:387: // this.hideIndicato(node);
Is this line needed? Also, there's a typo in the method name.

https://codereview.appspot.com/10416044/diff/1/test/test_charm_store.js
File test/test_charm_store.js (left):

https://codereview.appspot.com/10416044/diff/1/test/test_charm_store.js#oldco...
test/test_charm_store.js:356: 
I'd prefer this blank line to remain here. It improves folding mode
visualization with Sublime Text, I don't know if that's the case with other
editors too.

https://codereview.appspot.com/10416044/diff/1/test/test_charm_store.js
File test/test_charm_store.js (right):

https://codereview.appspot.com/10416044/diff/1/test/test_charm_store.js#newco...
test/test_charm_store.js:280: });
I also had this same error in a branch, and the linter seemed unable to pick it
up. :-/

https://codereview.appspot.com/10416044/diff/1/test/test_model.js
File test/test_model.js (right):

https://codereview.appspot.com/10416044/diff/1/test/test_model.js#newcode740
test/test_model.js:740: // This must be converted properly.
What must be converted to what? What does "properly" mean? :-)

[rharding, 2013-06-20 14:22:23]

Thanks for the review. Questions hopefully answered and will push an update
shortly.

https://codereview.appspot.com/10416044/diff/1/app/subapps/browser/views/char...
File app/subapps/browser/views/charm.js (right):

https://codereview.appspot.com/10416044/diff/1/app/subapps/browser/views/char...
app/subapps/browser/views/charm.js:387: // this.hideIndicato(node);
On 2013/06/20 14:17:36, teknico wrote:
> Is this line needed? Also, there's a typo in the method name.

Ah no, this was moved elsewhere. I'll clean that up.

https://codereview.appspot.com/10416044/diff/1/app/subapps/browser/views/char...
app/subapps/browser/views/charm.js:514: if (this.featuredCharmContainer) {
On 2013/06/19 17:39:40, rharding wrote:
> this is wrong, it's supposed to be relatedCharmContainer. Updating in current
> branch.

Yep, noted it in the comment after I submitted it. Thanks for catching.

https://codereview.appspot.com/10416044/diff/1/test/test_charm_store.js
File test/test_charm_store.js (left):

https://codereview.appspot.com/10416044/diff/1/test/test_charm_store.js#oldco...
test/test_charm_store.js:356: 
On 2013/06/20 14:17:36, teknico wrote:
> I'd prefer this blank line to remain here. It improves folding mode
> visualization with Sublime Text, I don't know if that's the case with other
> editors too.

Sure thing, will back out the tweak.

https://codereview.appspot.com/10416044/diff/1/test/test_charm_store.js
File test/test_charm_store.js (right):

https://codereview.appspot.com/10416044/diff/1/test/test_charm_store.js#newco...
test/test_charm_store.js:280: });
On 2013/06/20 14:17:36, teknico wrote:
> I also had this same error in a branch, and the linter seemed unable to pick
it
> up. :-/

Hmm, I don't recall if my in-editor linter yelled at me or the make lint did.

https://codereview.appspot.com/10416044/diff/1/test/test_model.js
File test/test_model.js (right):

https://codereview.appspot.com/10416044/diff/1/test/test_model.js#newcode740
test/test_model.js:740: // This must be converted properly.
On 2013/06/20 14:17:36, teknico wrote:
> What must be converted to what? What does "properly" mean? :-)

Will clarify the comment. To sum up, the related api sends only a special subset
of the charm data down the pipe. Normally, we generate a BrowserCharmModel and
pass that into the charm-token widget for use. Due to the limited data set, we
have to do the things the model normally does for us and tweak the api data into
what the widget expects.

[rharding, 2013-06-20 14:27:48]

Please take a look.

[j.c.sackett, 2013-06-20 14:28:17]

https://codereview.appspot.com/10416044/diff/1/app/models/charm.js
File app/models/charm.js (right):

https://codereview.appspot.com/10416044/diff/1/app/models/charm.js#newcode391
app/models/charm.js:391: shouldShowIcon: data.has_icon,
Related charms aren't inherently reviewed, are they? shouldShowIcon is a
combination of a) having a charm and b) is_approved being true.

https://codereview.appspot.com/10416044/diff/1/app/models/charm.js#newcode454
app/models/charm.js:454: }, this);
These two functions are identical; might it be worth it to pull the shared bit
into a single function that can take face, key, and {required,provided}Charms
and just call that within the anon function passed to Object.each?

https://codereview.appspot.com/10416044/diff/1/app/subapps/browser/views/char...
File app/subapps/browser/views/charm.js (right):

https://codereview.appspot.com/10416044/diff/1/app/subapps/browser/views/char...
app/subapps/browser/views/charm.js:553: }
Do you need to provide this at all if it's empty?

https://codereview.appspot.com/10416044/diff/1/app/subapps/browser/views/char...
app/subapps/browser/views/charm.js:617:
this.showIndicator(Y.one('.related-charms'));
Given you call this.showIndicator here, I'm assuming you do want the
hideIndicator call that teknico pointed out above.

https://codereview.appspot.com/10416044/diff/1/test/test_browser_charm_detail...
File test/test_browser_charm_details.js (right):

https://codereview.appspot.com/10416044/diff/1/test/test_browser_charm_detail...
test/test_browser_charm_details.js:47: });
Is this totally safe? I don't see any cleanup of this later, and you are
redefining an existing helper.

https://codereview.appspot.com/10416044/diff/1/test/test_model.js
File test/test_model.js (right):

https://codereview.appspot.com/10416044/diff/1/test/test_model.js#newcode740
test/test_model.js:740: // This must be converted properly.
I'm not sure this comment is necessary at all; the private method still jumps at
me as something that should be tested, and I don't see anything inconsistent
with testing a private method in a unit test.

[rharding, 2013-06-20 14:51:58]

Please take a look.

https://codereview.appspot.com/10416044/diff/1/test/test_model.js
File test/test_model.js (right):

https://codereview.appspot.com/10416044/diff/1/test/test_model.js#newcode740
test/test_model.js:740: // This must be converted properly.
On 2013/06/20 14:28:18, j.c.sackett wrote:
> I'm not sure this comment is necessary at all; the private method still jumps
at
> me as something that should be tested, and I don't see anything inconsistent
> with testing a private method in a unit test.

Done.

[rharding, 2013-06-20 14:52:02]

On 2013/06/20 14:28:17, j.c.sackett wrote:
> https://codereview.appspot.com/10416044/diff/1/app/models/charm.js
> File app/models/charm.js (right):
> 
> https://codereview.appspot.com/10416044/diff/1/app/models/charm.js#newcode391
> app/models/charm.js:391: shouldShowIcon: data.has_icon,
> Related charms aren't inherently reviewed, are they? shouldShowIcon is a
> combination of a) having a charm and b) is_approved being true.

Yes, thanks! Updated.

> 
> https://codereview.appspot.com/10416044/diff/1/app/models/charm.js#newcode454
> app/models/charm.js:454: }, this);
> These two functions are identical; might it be worth it to pull the shared bit
> into a single function that can take face, key, and {required,provided}Charms
> and just call that within the anon function passed to Object.each?

I thought it was messier but very fair and updated. 

> 
>
https://codereview.appspot.com/10416044/diff/1/app/subapps/browser/views/char...
> File app/subapps/browser/views/charm.js (right):
> 
>
https://codereview.appspot.com/10416044/diff/1/app/subapps/browser/views/char...
> app/subapps/browser/views/charm.js:553: }
> Do you need to provide this at all if it's empty?

Nope, originally I was going to introduce a 'tiny' size but didn't need it.
Thanks for the catch. 


>
https://codereview.appspot.com/10416044/diff/1/test/test_browser_charm_detail...
> File test/test_browser_charm_details.js (right):
> 
>
https://codereview.appspot.com/10416044/diff/1/test/test_browser_charm_detail...
> test/test_browser_charm_details.js:47: });
> Is this totally safe? I don't see any cleanup of this later, and you are
> redefining an existing helper.

Thanks for the catch. In other places I use this I added an unset in the
after(). I've updated it. 

> 
> https://codereview.appspot.com/10416044/diff/1/test/test_model.js
> File test/test_model.js (right):
> 
> https://codereview.appspot.com/10416044/diff/1/test/test_model.js#newcode740
> test/test_model.js:740: // This must be converted properly.
> I'm not sure this comment is necessary at all; the private method still jumps
at
> me as something that should be tested, and I don't see anything inconsistent
> with testing a private method in a unit test.

I'm usually anti testing private methods as they're private and not to be
dependent upon. I think this one can really help cut down debugging time if the
api data changes so I wanted to add it. I'll leave it for now if that's ok. I
like to leave comments behind when there might be a question for others.

[j.c.sackett, 2013-06-20 16:02:31]

LGTM, thanks.

On 2013/06/20 14:52:02, rharding wrote:
> I thought it was messier but very fair and updated. 

This wasn't quite the way I envisioned the refactor, but definitely cuts down on
the repetition; thanks.

> I'm usually anti testing private methods as they're private and not to be
> dependent upon. I think this one can really help cut down debugging time if
the
> api data changes so I wanted to add it. I'll leave it for now if that's ok. I
> like to leave comments behind when there might be a question for others.

Oh, I'm totally fine with leaving the comment; given that it caused some
confusion, I was merely pointing out that I had no problem with the comment just
being removed, if that was easier.

[rharding, 2013-06-21 11:05:23]

*** Submitted:

Add support for related charm loading and display

- Adds a new store endpoint for the related charm api call
- Adds the idea of relatedCharms for model ATTRS
- Adds two helpers to take the api response data into something we can use in
the app
- Renders out the related charms in the template in fullscreen as an ajax
request
- Updates the charm details to handle click events on charm tokens 
- Adds tests for the model helpers and the details view
- Updates a bunch of tests. There were left over testing Nodes that caused
stray tests to fail. I fixed those and also went back and cleaned up the stray
test mess.
- Updates the css to work out so that the tokens can fit. There's a
redesign-a-coming so the work was to fit in currently and the redesign will
move the layout at a later date.

Notes:

- This does not currently add the related charm display to the interfaces tab.
It's a slightly different UX and this branch was taking too long as is.

- I'm not happy with the relatedCharms ATTR. It's not really an ATTR but I
wanted this cached into the model because we'll use that when we add related
charms to the interfaces tab. We don't want to re-request the data. However,
in sidebar move it will not have been requested yet. My plan is to re-evaluate
and possibly refactor when doing the interfaces tab display.

R=
CC=
https://codereview.appspot.com/10416044