Render the configuration tab in charm details.

Render the configuration tab in charm details.

- Add a `config` attr to the Charm model so that we can loop over the options
from the api.
- Display them as a definition list out on render

https://code.launchpad.net/~rharding/juju-gui/config_tab/+merge/156565

Requires: https://code.launchpad.net/~rharding/juju-gui/qa_tab/+merge/156392

(do not edit description out of merge proposal)

[rharding, 2013-04-02 17:29:16]

Please take a look.

[rharding, 2013-04-02 17:36:58]

Please take a look.

[jeff.pihach, 2013-04-02 18:14:36]

Thanks this is looking good. I am not sure about the location of your getter -
feel free to discuss with me on IRC if you don't agree.

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

https://codereview.appspot.com/8268043/diff/3001/app/models/charm.js#newcode317
app/models/charm.js:317: config: {
On Friday I'm going to propose that we always define and document attributes - I
am glad that you're defining them but could you also start documenting them
using yuidoc too please? :-)

https://codereview.appspot.com/8268043/diff/3001/app/models/charm.js#newcode327
app/models/charm.js:327: getter: function(val) {
I think defining a getter as a string allows the attribute to be lazily created.
Is it beneficial here to have this attribute lazy loaded?

https://codereview.appspot.com/8268043/diff/3001/app/models/charm.js#newcode339
app/models/charm.js:339: return options;
In just reading this code (without the big picture) it looks to me like this
getter should actually be in options as you aren't actually augmenting anything
from config for the return value.

https://codereview.appspot.com/8268043/diff/3001/test/test_model.js
File test/test_model.js (right):

https://codereview.appspot.com/8268043/diff/3001/test/test_model.js#newcode556
test/test_model.js:556: after(function(done) {
It looks to me like this should actually be afterEach() as you're creating a new
instance in every test.

[rharding, 2013-04-02 18:29:21]

Comments replied to, updated code coming.

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

https://codereview.appspot.com/8268043/diff/3001/app/models/charm.js#newcode317
app/models/charm.js:317: config: {
On 2013/04/02 18:14:36, jeff.pihach wrote:
> On Friday I'm going to propose that we always define and document attributes -
I
> am glad that you're defining them but could you also start documenting them
> using yuidoc too please? :-)

Sorry, you're right. I generally do document attrs and since I just moved this
one I didn't add docs. I'll add those in.

https://codereview.appspot.com/8268043/diff/3001/app/models/charm.js#newcode327
app/models/charm.js:327: getter: function(val) {
On 2013/04/02 18:14:36, jeff.pihach wrote:
> I think defining a getter as a string allows the attribute to be lazily
created.
> Is it beneficial here to have this attribute lazy loaded?

Yes, I suppose lazy loading it will be good. I can make that change.

https://codereview.appspot.com/8268043/diff/3001/app/models/charm.js#newcode339
app/models/charm.js:339: return options;
On 2013/04/02 18:14:36, jeff.pihach wrote:
> In just reading this code (without the big picture) it looks to me like this
> getter should actually be in options as you aren't actually augmenting
anything
> from config for the return value.

If there are options, we're turning an object into an array. Each item in the
array has a new .name attribute that is the object's key. This lets us
loop/output the object data in array form in the template in this MP. 

So config == array of object, options == nested object provided by the api
calls.

https://codereview.appspot.com/8268043/diff/3001/test/test_model.js
File test/test_model.js (right):

https://codereview.appspot.com/8268043/diff/3001/test/test_model.js#newcode556
test/test_model.js:556: after(function(done) {
On 2013/04/02 18:14:36, jeff.pihach wrote:
> It looks to me like this should actually be afterEach() as you're creating a
new
> instance in every test.

Correct, thanks!

[rharding, 2013-04-02 18:59:19]

Please take a look.

[jeff.pihach, 2013-04-02 19:15:56]

LGTM with trivial - Thanks for that change! I think this approach is a lot
cleaner.

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

https://codereview.appspot.com/8268043/diff/9001/app/subapps/browser/views/ch...
app/subapps/browser/views/charm.js:282: config: charm.getOptionsAsArray()
Maybe config here should be changed to `options`

[j.c.sackett, 2013-04-02 19:28:24]

LGTM, with two minor points to address.

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

https://codereview.appspot.com/8268043/diff/9001/app/models/charm.js#newcode321
app/models/charm.js:321: return options;
This is a little confusing. It seems to read "if no options return options". Is
this so it returns undefined if there aren't options? Could we just explicitly
return undefined in this case?

https://codereview.appspot.com/8268043/diff/9001/app/subapps/browser/template...
File app/subapps/browser/templates/browser_charm.handlebars (right):

https://codereview.appspot.com/8268043/diff/9001/app/subapps/browser/template...
app/subapps/browser/templates/browser_charm.handlebars:61: {{#each config}}
You can actually skip the `each` here, I think. {{#config}}{{/config}} will
iterate over the config data. Not sure which is preferred handlebars syntax,
though.

[rharding, 2013-04-02 19:35:40]

Thanks, comments below.

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

https://codereview.appspot.com/8268043/diff/9001/app/models/charm.js#newcode321
app/models/charm.js:321: return options;
On 2013/04/02 19:28:24, j.c.sackett wrote:
> This is a little confusing. It seems to read "if no options return options".
Is
> this so it returns undefined if there aren't options? Could we just explicitly
> return undefined in this case?

Ok, I can manually return undefined. It felt ugly so this 'hid' it.

https://codereview.appspot.com/8268043/diff/9001/app/subapps/browser/template...
File app/subapps/browser/templates/browser_charm.handlebars (right):

https://codereview.appspot.com/8268043/diff/9001/app/subapps/browser/template...
app/subapps/browser/templates/browser_charm.handlebars:61: {{#each config}}
On 2013/04/02 19:28:24, j.c.sackett wrote:
> You can actually skip the `each` here, I think. {{#config}}{{/config}} will
> iterate over the config data. Not sure which is preferred handlebars syntax,
> though.

Thanks, yea I noticed that as an option but prefer the explicit myself. A quick
grep though doesn't find much use of #each though so I guess I'll change it to
conform.

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

https://codereview.appspot.com/8268043/diff/9001/app/subapps/browser/views/ch...
app/subapps/browser/views/charm.js:282: config: charm.getOptionsAsArray()
On 2013/04/02 19:15:56, jeff.pihach wrote:
> Maybe config here should be changed to `options`

I want to make sure it's different from options so that there's no confusion
that this is a different bit of data than the options from the model. It's been
transformed.

[rharding, 2013-04-02 19:39:43]

*** Submitted:

Render the configuration tab in charm details.

- Add a `config` attr to the Charm model so that we can loop over the options
from the api.
- Display them as a definition list out on render

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