Update the Charmworld store to use Api1 vs Api0

Update the Charmworld store to use Api1 vs Api0

- The new API changes some responses, update the store to handle these
responses.
- Update all code to change from the Charmworl0 store to the Charmworld1 store
and update tests.
- Pull down new test/sample json responses for the tests to reference.
- Change some of the tests to verify against a known value vs whatever the
sample api response pulls down. Should make less work to update again.
- Update how the icons are displayed. They are no longer in the API response
and so we need to generate links to them using the existing file api.
- Add a handlebars helper to aid in generating those urls and knows about the
store instance used.
- Add a new tests for the new file path generation.

https://code.launchpad.net/~rharding/juju-gui/charmworld-api1/+merge/166284

(do not edit description out of merge proposal)

[rharding, 2013-05-29 15:28:39]

Please take a look.

[jeff.pihach, 2013-05-29 15:47:48]

LGTM with a few changes.

Thanks for upgrading to the next version this should be much faster!

Because we aren't creating a new class to handle the api changes I feel that it
should be renamed to simply be Charmworld so that it doesn't need to be renamed
throughout the application every time.

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

https://codereview.appspot.com/9841044/diff/1/app/models/charm.js#newcode467
app/models/charm.js:467: 
@attribute is missing

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

https://codereview.appspot.com/9841044/diff/1/app/store/charm.js#newcode164
app/store/charm.js:164: ns.Charmworld1 = Y.Base.create('charmworld0', Y.Base,
[], {
If every time we upgrade the api we change the class name should this not just
be called `Charmworld`?

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

https://codereview.appspot.com/9841044/diff/1/app/subapps/browser/browser.js#...
app/subapps/browser/browser.js:140: _registerSubappHelpers: function() {
The majority of the helpers are in views/utils.js it would probably make sense
to put this in there as well to avoid having to create a new one in each test

https://codereview.appspot.com/9841044/diff/1/app/subapps/browser/browser.js#...
app/subapps/browser/browser.js:688: @type {Charmworld1}
Charmworld

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

https://codereview.appspot.com/9841044/diff/1/app/subapps/browser/views/charm...
app/subapps/browser/views/charm.js:629: * @type {Charmworld1}
Charmworld

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

https://codereview.appspot.com/9841044/diff/1/test/test_charm_store.js#newcod...
test/test_charm_store.js:337: var hostname = 'http://localhost';
what? not 0.0.0.0? ;)

[rharding, 2013-05-29 15:51:34]

On 2013/05/29 15:47:48, jeff.pihach wrote:
> LGTM with a few changes.
> 
> Thanks for upgrading to the next version this should be much faster!
> 
> Because we aren't creating a new class to handle the api changes I feel that
it
> should be renamed to simply be Charmworld so that it doesn't need to be
renamed
> throughout the application every time.

I'm going to respectively disagree. This points out we are able to have multiple
versions if required. It also makes updating a LOT nicer since a simple bzr grep
points out where to head for things. I found it really nice in doing this
branch. It also makes it perfectly clear to any dev what api they're running
against. 


> https://codereview.appspot.com/9841044/diff/1/app/subapps/browser/browser.js
> File app/subapps/browser/browser.js (right):
> 
>
https://codereview.appspot.com/9841044/diff/1/app/subapps/browser/browser.js#...
> app/subapps/browser/browser.js:140: _registerSubappHelpers: function() {
> The majority of the helpers are in views/utils.js it would probably make sense
> to put this in there as well to avoid having to create a new one in each test

So this helper is specific to the subapp. It needs a reference to the api store
used so it went into the Browser subapp which is the one place that knows what
store is instantiated. I thought about putting it in utils, but it makes
creating this helper with knowledge about a store instance a lot harder.


> https://codereview.appspot.com/9841044/diff/1/test/test_charm_store.js
> File test/test_charm_store.js (right):
> 
>
https://codereview.appspot.com/9841044/diff/1/test/test_charm_store.js#newcod...
> test/test_charm_store.js:337: var hostname = 'http://localhost';
> what? not 0.0.0.0? ;)

Hey, that's someone else's branch.

[jeff.pihach, 2013-05-29 16:20:33]

On 2013/05/29 15:51:34, rharding wrote:
> On 2013/05/29 15:47:48, jeff.pihach wrote:
> > LGTM with a few changes.
> > 
> > Thanks for upgrading to the next version this should be much faster!
> > 
> > Because we aren't creating a new class to handle the api changes I feel that
> it
> > should be renamed to simply be Charmworld so that it doesn't need to be
> renamed
> > throughout the application every time.
> 
> I'm going to respectively disagree. This points out we are able to have
multiple
> versions if required. It also makes updating a LOT nicer since a simple bzr
grep
> points out where to head for things. I found it really nice in doing this
> branch. It also makes it perfectly clear to any dev what api they're running
> against. 

If the plan is to eventually support multiple api charm classes then I feel that
we
should have a base charm class to which api specific versions can extend. That
way
things like attribute types and documentation can be instances of the base charm
api class.


> > https://codereview.appspot.com/9841044/diff/1/app/subapps/browser/browser.js
> > File app/subapps/browser/browser.js (right):
> > 
> >
>
https://codereview.appspot.com/9841044/diff/1/app/subapps/browser/browser.js#...
> > app/subapps/browser/browser.js:140: _registerSubappHelpers: function() {
> > The majority of the helpers are in views/utils.js it would probably make
sense
> > to put this in there as well to avoid having to create a new one in each
test
> 
> So this helper is specific to the subapp. It needs a reference to the api
store
> used so it went into the Browser subapp which is the one place that knows what
> store is instantiated. I thought about putting it in utils, but it makes
> creating this helper with knowledge about a store instance a lot harder.

Understandable

[j.c.sackett, 2013-05-29 16:52:18]

LGTM, with one easy fix in comments.

I am a little surprised we're just doing s/charmworld0/charmworld1/g ... I
thought the original plan was to keep charmworld0 but create a charmworld1 and
update things to use that. I don't actually care one way or another, since
there's nothing you can do with API0 that you can't with API1, I'm just
wondering if my understanding was wrong, or if our plans changed.

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

https://codereview.appspot.com/9841044/diff/1/app/store/charm.js#newcode164
app/store/charm.js:164: ns.Charmworld1 = Y.Base.create('charmworld0', Y.Base,
[], {
The name passed into create should be 'charmworld1', not 'charmworld0'

[rharding, 2013-05-29 17:42:47]

On 2013/05/29 16:52:18, j.c.sackett wrote:
> LGTM, with one easy fix in comments.
> 
> I am a little surprised we're just doing s/charmworld0/charmworld1/g ... I
> thought the original plan was to keep charmworld0 but create a charmworld1 and
> update things to use that. I don't actually care one way or another, since
> there's nothing you can do with API0 that you can't with API1, I'm just
> wondering if my understanding was wrong, or if our plans changed.

It was the plan to support that use case. This change doesn't need to support
multiple versions at once. In talking with Jeff, I don't know what would cause
us to need to support multiple versions at once. At this point, the numbering is
left for clarity and easy to updating things more then anything else.

> 
> https://codereview.appspot.com/9841044/diff/1/app/store/charm.js
> File app/store/charm.js (right):
> 
> https://codereview.appspot.com/9841044/diff/1/app/store/charm.js#newcode164
> app/store/charm.js:164: ns.Charmworld1 = Y.Base.create('charmworld0', Y.Base,
> [], {
> The name passed into create should be 'charmworld1', not 'charmworld0'

Thanks for the catch.

[rharding, 2013-05-29 18:36:11]

*** Submitted:

Update the Charmworld store to use Api1 vs Api0

- The new API changes some responses, update the store to handle these
responses.
- Update all code to change from the Charmworl0 store to the Charmworld1 store
and update tests.
- Pull down new test/sample json responses for the tests to reference.
- Change some of the tests to verify against a known value vs whatever the
sample api response pulls down. Should make less work to update again.
- Update how the icons are displayed. They are no longer in the API response
and so we need to generate links to them using the existing file api.
- Add a handlebars helper to aid in generating those urls and knows about the
store instance used.
- Add a new tests for the new file path generation.

R=jeff.pihach, j.c.sackett
CC=
https://codereview.appspot.com/9841044