Add a read-only view to display unit details.

Add a read-only view to display unit details.

This branch adds a read-only view that displays unit details including charm,
machine info, and a listing of relations.

https://code.launchpad.net/~benji/juju-ui/unit-view/+merge/124051

(do not edit description out of merge proposal)

[benji, 2012-09-12 20:30:18]

Please take a look.

[benji, 2012-09-13 12:15:43]

Please take a look.

[gary.poster, 2012-09-13 15:03:44]

Hi Benji.  Thank you for the changes.  This looks very good to me.  The only
thing I'd like you to do now is use the new importable version of the socket
stub, as I mentioned.  It's ready for Ben or Kapil to take a look.

https://codereview.appspot.com/6503111/diff/2001/app/app.js
File app/app.js (right):

https://codereview.appspot.com/6503111/diff/2001/app/app.js#newcode155
app/app.js:155: this._prefetch_service(service);
It *might* be nice to say why prefetching the service is important.  Then again,
maybe it's obvious to everyone else. :-)

https://codereview.appspot.com/6503111/diff/2001/app/assets/stylesheets/juju-...
File app/assets/stylesheets/juju-gui.css (right):

https://codereview.appspot.com/6503111/diff/2001/app/assets/stylesheets/juju-...
app/assets/stylesheets/juju-gui.css:1: svg{border:2px solid black;}
I look forward to the generated files not being part of our diffs!

https://codereview.appspot.com/6503111/diff/2001/app/templates.js
File app/templates.js (right):

https://codereview.appspot.com/6503111/diff/2001/app/templates.js#newcode1
app/templates.js:1: /*
I, um, look forward to the generated files not being part of our diffs!

https://codereview.appspot.com/6503111/diff/2001/app/templates/unit.handlebars
File app/templates/unit.handlebars (right):

https://codereview.appspot.com/6503111/diff/2001/app/templates/unit.handlebar...
app/templates/unit.handlebars:21: <div class="span12">
I don't see "span12" in our less file.  (/me hunts around.) Oh, something about
a Twitter bootstrap grid system.  For my own edification, please lemme know (on
IRC or wherever) where should I read about this.

https://codereview.appspot.com/6503111/diff/2001/app/views/unit.js
File app/views/unit.js (right):

https://codereview.appspot.com/6503111/diff/2001/app/views/unit.js#newcode6
app/views/unit.js:6: models = Y.namespace('juju.models'),
I need to ask why this pattern is necessary.  We have followed it too.  I think
I know why, but I also think that it maybe is indicative of something we could
do better.  But anyway, continuing with it here for now is fine with me.

https://codereview.appspot.com/6503111/diff/2001/app/views/unit.js#newcode30
app/views/unit.js:30: container.setHTML('<div class="alert">Loading...</div>');
Ah, we should do something like that for our view too.

https://codereview.appspot.com/6503111/diff/2001/jshint.config
File jshint.config (right):

https://codereview.appspot.com/6503111/diff/2001/jshint.config#newcode8
jshint.config:8: "afterEach",
As I mentioned, we may have a conflict with our branch for this, but it should
be easy to resolve.

https://codereview.appspot.com/6503111/diff/2001/test/test_unit_view.js
File test/test_unit_view.js (right):

https://codereview.appspot.com/6503111/diff/2001/test/test_unit_view.js#newcode7
test/test_unit_view.js:7: var SocketStub = function () {
You can get the socket from juju-tests-utils now.  See our "before" function in
https://codereview.appspot.com/6496112/patch/1/17 .

https://codereview.appspot.com/6503111/diff/2001/test/test_unit_view.js#newco...
test/test_unit_view.js:73: service = new models.Service({
I wonder if we need a library of these test fixtures somewhere too.  The app
test has a gigantic collection of these.  (Nothing to do for now, just thinking)

[benji, 2012-09-13 15:49:10]

On 2012/09/13 15:03:44, gary.poster wrote:
> Hi Benji.  Thank you for the changes.  This looks very good to me.  The only
> thing I'd like you to do now is use the new importable version of the socket
> stub, as I mentioned.  It's ready for Ben or Kapil to take a look.
> 
> https://codereview.appspot.com/6503111/diff/2001/app/app.js
> File app/app.js (right):
> 
> https://codereview.appspot.com/6503111/diff/2001/app/app.js#newcode155
> app/app.js:155: this._prefetch_service(service);
> It *might* be nice to say why prefetching the service is important.  Then
again,
> maybe it's obvious to everyone else. :-)
> 
>
https://codereview.appspot.com/6503111/diff/2001/app/assets/stylesheets/juju-...
> File app/assets/stylesheets/juju-gui.css (right):
> 
>
https://codereview.appspot.com/6503111/diff/2001/app/assets/stylesheets/juju-...
> app/assets/stylesheets/juju-gui.css:1: svg{border:2px solid black;}
> I look forward to the generated files not being part of our diffs!
> 
> https://codereview.appspot.com/6503111/diff/2001/app/templates.js
> File app/templates.js (right):
> 
> https://codereview.appspot.com/6503111/diff/2001/app/templates.js#newcode1
> app/templates.js:1: /*
> I, um, look forward to the generated files not being part of our diffs!
> 
> https://codereview.appspot.com/6503111/diff/2001/app/templates/unit.handlebars
> File app/templates/unit.handlebars (right):
> 
>
https://codereview.appspot.com/6503111/diff/2001/app/templates/unit.handlebar...
> app/templates/unit.handlebars:21: <div class="span12">
> I don't see "span12" in our less file.  (/me hunts around.) Oh, something
about
> a Twitter bootstrap grid system.  For my own edification, please lemme know
(on
> IRC or wherever) where should I read about this.
> 
> https://codereview.appspot.com/6503111/diff/2001/app/views/unit.js
> File app/views/unit.js (right):
> 
> https://codereview.appspot.com/6503111/diff/2001/app/views/unit.js#newcode6
> app/views/unit.js:6: models = Y.namespace('juju.models'),
> I need to ask why this pattern is necessary.  We have followed it too.  I
think
> I know why, but I also think that it maybe is indicative of something we could
> do better.  But anyway, continuing with it here for now is fine with me.
> 
> https://codereview.appspot.com/6503111/diff/2001/app/views/unit.js#newcode30
> app/views/unit.js:30: container.setHTML('<div
class="alert">Loading...</div>');
> Ah, we should do something like that for our view too.
> 
> https://codereview.appspot.com/6503111/diff/2001/jshint.config
> File jshint.config (right):
> 
> https://codereview.appspot.com/6503111/diff/2001/jshint.config#newcode8
> jshint.config:8: "afterEach",
> As I mentioned, we may have a conflict with our branch for this, but it should
> be easy to resolve.
> 
> https://codereview.appspot.com/6503111/diff/2001/test/test_unit_view.js
> File test/test_unit_view.js (right):
> 
>
https://codereview.appspot.com/6503111/diff/2001/test/test_unit_view.js#newcode7
> test/test_unit_view.js:7: var SocketStub = function () {
> You can get the socket from juju-tests-utils now.  See our "before" function
in
> https://codereview.appspot.com/6496112/patch/1/17 .
> 
>
https://codereview.appspot.com/6503111/diff/2001/test/test_unit_view.js#newco...
> test/test_unit_view.js:73: service = new models.Service({
> I wonder if we need a library of these test fixtures somewhere too.  The app
> test has a gigantic collection of these.  (Nothing to do for now, just
thinking)

I've fixed all the things noted.

This branch is ready for non-trainee review now.

[hazmat, 2012-09-13 18:00:32]

lgtm with these minors, and gary's point regarding reusing socketstub from
test-utils.

https://codereview.appspot.com/6503111/diff/2001/app/templates/unit.handlebars
File app/templates/unit.handlebars (right):

https://codereview.appspot.com/6503111/diff/2001/app/templates/unit.handlebar...
app/templates/unit.handlebars:61: <td
class="relation-endpoint">{{endpoint}}</td>
this should link to the endpoint, see service relations details as an example.

https://codereview.appspot.com/6503111/diff/2001/app/views/unit.js
File app/views/unit.js (right):

https://codereview.appspot.com/6503111/diff/2001/app/views/unit.js#newcode47
app/views/unit.js:47: unit_ip_description = ip_description_chunks.join(' | ');
this sort of formatting in the view rather then the template seems odd, why not
push these named fields to the template and do presentation logic there?

https://codereview.appspot.com/6503111/diff/2001/test/test_unit_view.js
File test/test_unit_view.js (right):

https://codereview.appspot.com/6503111/diff/2001/test/test_unit_view.js#newco...
test/test_unit_view.js:105: conn.messages = [];
conn.close does this.

[benjamin.saller, 2012-09-13 18:13:51]

Thanks for this, I think the view template and render method need some work,
some of this is UX related, some about a lack of information and some of its
structural. That said I think this is pretty close :)

https://codereview.appspot.com/6503111/diff/2001/app/app.js
File app/app.js (right):

https://codereview.appspot.com/6503111/diff/2001/app/app.js#newcode155
app/app.js:155: this._prefetch_service(service);
On 2012/09/13 15:03:44, gary.poster wrote:
> It *might* be nice to say why prefetching the service is important.  Then
again,
> maybe it's obvious to everyone else. :-)

This is an area that should be better documented but its a pattern used in a
number of methods. The basic reason is this. From the environment view if you
navigate down the database naturally populates with the correct data needed for
the various detail views but if you load the application from a URL pointing to
a sub-view the data often won't be loaded in a state useful to the subview. This
says make sure the models for objects I intend to show are present and is most
useful in that re-load testing.

As an aside when authoring any view we currently must test that it functions on
navigation transitions (which is the simple case) and on reloads (which requires
the dance seen here)

https://codereview.appspot.com/6503111/diff/2001/app/templates/unit.handlebars
File app/templates/unit.handlebars (right):

https://codereview.appspot.com/6503111/diff/2001/app/templates/unit.handlebar...
app/templates/unit.handlebars:4: <b id="charm-uri">{{service.charm}}</b>
The charm object should be in the template name space and this should be a link
ala services view.

https://codereview.appspot.com/6503111/diff/2001/app/templates/unit.handlebar...
app/templates/unit.handlebars:5: {{#unit.is_subordinate}}
(subordinate){{/unit.is_subordinate}}
You using the block style for iteration rather than single element 'if's. 

This is done a number of places.

Following the style on the services page this could be
{{#if unit.public_address}
<b>Public Address:</b> {{unit.public_address}}
{{/if}

https://codereview.appspot.com/6503111/diff/2001/app/templates/unit.handlebar...
app/templates/unit.handlebars:7: {{unit_ip_description}}
This object doesn't need to be built in the render method. The above 'if's
should handle it just fine.

https://codereview.appspot.com/6503111/diff/2001/app/templates/unit.handlebar...
app/templates/unit.handlebars:10: <span style="color: green">&#x25B6;</span>
We shouldn't include inline style information like this. 

This should be in the stylesheet (using LESS)

#unit-status {
   .runnng {...}
}

or style="success" || style="error" which are standards in the Bootstrap CSS

https://codereview.appspot.com/6503111/diff/2001/app/templates/unit.handlebar...
app/templates/unit.handlebars:21: <div class="span12">
On 2012/09/13 15:03:44, gary.poster wrote:
> I don't see "span12" in our less file.  (/me hunts around.) Oh, something
about
> a Twitter bootstrap grid system.  For my own edification, please lemme know
(on
> IRC or wherever) where should I read about this.

http://twitter.github.com/bootstrap/scaffolding.html#gridSystem

Bootstrap is very simple (part of why its so popular) and well worth learning if
you're implementing views

https://codereview.appspot.com/6503111/diff/2001/test/test_unit_view.js
File test/test_unit_view.js (right):

https://codereview.appspot.com/6503111/diff/2001/test/test_unit_view.js#newcode7
test/test_unit_view.js:7: var SocketStub = function () {
On 2012/09/13 15:03:44, gary.poster wrote:
> You can get the socket from juju-tests-utils now.  See our "before" function
in
> https://codereview.appspot.com/6496112/patch/1/17 .

agreed, however I can't see that its actually used, it looks like you build the
models by hand.

https://codereview.appspot.com/6503111/diff/2001/test/test_unit_view.js#newco...
test/test_unit_view.js:134:
container.one('#machine-name').getHTML().should.contain(
If this pattern is repeated all over the test it might be worth adding a
declarative handler in utils

this.validate_content(container, {
"#machine-agent-state": {contain: "pending"},
"#machine-instance-id": {contain: "instance-0"},
...
}
If you like the sound of that I could cut a small branch for a it as a slack
task.

[hazmat, 2012-09-14 11:51:50]

https://codereview.appspot.com/6503111/diff/2001/app/templates/unit.handlebars
File app/templates/unit.handlebars (right):

https://codereview.appspot.com/6503111/diff/2001/app/templates/unit.handlebar...
app/templates/unit.handlebars:13: <span style="color: red">&#x2b1b;</span>
the boolean distinction here is a bit overly gestalt, we have multiple states of
interest, running/started, pending action, and error, so perhaps green, yellow,
red?

[benji, 2012-09-14 19:06:23]

I've fixed or otherwise responded to all the comments thus far.  As discussed in
the standup, I'll be merging this branch now.

https://codereview.appspot.com/6503111/diff/2001/app/app.js
File app/app.js (right):

https://codereview.appspot.com/6503111/diff/2001/app/app.js#newcode155
app/app.js:155: this._prefetch_service(service);
On 2012/09/13 15:03:44, gary.poster wrote:
> It *might* be nice to say why prefetching the service is important.  Then
again,
> maybe it's obvious to everyone else. :-)

Done.

https://codereview.appspot.com/6503111/diff/2001/app/templates/unit.handlebars
File app/templates/unit.handlebars (right):

https://codereview.appspot.com/6503111/diff/2001/app/templates/unit.handlebar...
app/templates/unit.handlebars:4: <b id="charm-uri">{{service.charm}}</b>
On 2012/09/13 18:13:52, benjamin.saller wrote:
> The charm object should be in the template name space and this should be a
link
> ala services view.

Done.

https://codereview.appspot.com/6503111/diff/2001/app/templates/unit.handlebar...
app/templates/unit.handlebars:5: {{#unit.is_subordinate}}
(subordinate){{/unit.is_subordinate}}
On 2012/09/13 18:13:52, benjamin.saller wrote:
> You using the block style for iteration rather than single element 'if's. 
> 
> This is done a number of places.
> 
> Following the style on the services page this could be
> {{#if unit.public_address}
> <b>Public Address:</b> {{unit.public_address}}
> {{/if}

Ah!  Very nice.  I was looking at the Moustache docs and they flat-out say that
there are no ifs.  Warning to future generations: Moustache and Handlebars are
only superficially similar.  Fixed.

https://codereview.appspot.com/6503111/diff/2001/app/templates/unit.handlebar...
app/templates/unit.handlebars:7: {{unit_ip_description}}
On 2012/09/13 18:13:52, benjamin.saller wrote:
> This object doesn't need to be built in the render method. The above 'if's
> should handle it just fine.

I would like to move the formatting to the template, but I don't see a (sane)
way to do this with just ifs.  Note the pipes separating the parts.  We could
use CSS and add an :after to each one to add the pipes or we could drop the
pipes and make a table like the rest of the data on the page.  I was trying to
be faithful to the mockup at
https://docs.google.com/a/canonical.com/file/d/0BwQq-CeM0YioeWxuMmRnQUdLNzA/edit.

https://codereview.appspot.com/6503111/diff/2001/app/templates/unit.handlebar...
app/templates/unit.handlebars:10: <span style="color: green">&#x25B6;</span>
On 2012/09/13 18:13:52, benjamin.saller wrote:
> We shouldn't include inline style information like this. 
> 
> This should be in the stylesheet (using LESS)
> 
> #unit-status {
>    .runnng {...}
> }
> 
> or style="success" || style="error" which are standards in the Bootstrap CSS

Bootstrap's "alert-error" was the closest I could find and it didn't produce
good results.  I added styles to our LESS stylesheet.

https://codereview.appspot.com/6503111/diff/2001/app/templates/unit.handlebar...
app/templates/unit.handlebars:13: <span style="color: red">&#x2b1b;</span>
On 2012/09/14 11:51:50, hazmat wrote:
> the boolean distinction here is a bit overly gestalt, we have multiple states
of
> interest, running/started, pending action, and error, so perhaps green,
yellow,
> red?

Sounds good.  I thought I heard someone say that we wanted to do it this way,
but I like yellow just fine. ;)

I've added a yellow circle for non-running, non-error states.  It looks good.

https://codereview.appspot.com/6503111/diff/2001/app/templates/unit.handlebar...
app/templates/unit.handlebars:61: <td
class="relation-endpoint">{{endpoint}}</td>
On 2012/09/13 18:00:32, hazmat wrote:
> this should link to the endpoint, see service relations details as an example.

Done.

Potential bug: can service names contain characters that need to be escaped?

https://codereview.appspot.com/6503111/diff/2001/app/views/unit.js
File app/views/unit.js (right):

https://codereview.appspot.com/6503111/diff/2001/app/views/unit.js#newcode30
app/views/unit.js:30: container.setHTML('<div class="alert">Loading...</div>');
On 2012/09/13 15:03:44, gary.poster wrote:
> Ah, we should do something like that for our view too.

Well, now that the top of every page isn't covered up by the banner, you'll
actually be able to see the loading message.

https://codereview.appspot.com/6503111/diff/2001/app/views/unit.js#newcode47
app/views/unit.js:47: unit_ip_description = ip_description_chunks.join(' | ');
On 2012/09/13 18:00:32, hazmat wrote:
> this sort of formatting in the view rather then the template seems odd, why
not
> push these named fields to the template and do presentation logic there?

I would like to, but I can't find a non-insane way to do so.  The interstitial
pipes are the issue.  Ben made a similar comment in the template if you want to
read the discussion there.

https://codereview.appspot.com/6503111/diff/2001/test/test_unit_view.js
File test/test_unit_view.js (right):

https://codereview.appspot.com/6503111/diff/2001/test/test_unit_view.js#newcode7
test/test_unit_view.js:7: var SocketStub = function () {
On 2012/09/13 15:03:44, gary.poster wrote:
> You can get the socket from juju-tests-utils now.  See our "before" function
in
> https://codereview.appspot.com/6496112/patch/1/17 .

Done.

https://codereview.appspot.com/6503111/diff/2001/test/test_unit_view.js#newcode7
test/test_unit_view.js:7: var SocketStub = function () {
On 2012/09/13 18:13:52, benjamin.saller wrote:
> On 2012/09/13 15:03:44, gary.poster wrote:
> > You can get the socket from juju-tests-utils now.  See our "before" function
> in
> > https://codereview.appspot.com/6496112/patch/1/17 .
> 
> agreed, however I can't see that its actually used, it looks like you build
the
> models by hand.

The connection is used to build an instance of juju.Environment which is in turn
used by the tests.

https://codereview.appspot.com/6503111/diff/2001/test/test_unit_view.js#newco...
test/test_unit_view.js:105: conn.messages = [];
On 2012/09/13 18:00:32, hazmat wrote:
> conn.close does this.

Ah, good to know.  I just cribbed this from another test.  I will fix all
instances of this. [later] Now the other test does not do this any more so this
test was the only one to fix.

https://codereview.appspot.com/6503111/diff/2001/test/test_unit_view.js#newco...
test/test_unit_view.js:134:
container.one('#machine-name').getHTML().should.contain(
On 2012/09/13 18:13:52, benjamin.saller wrote:
> If this pattern is repeated all over the test it might be worth adding a
> declarative handler in utils
> 
> this.validate_content(container, {
> "#machine-agent-state": {contain: "pending"},
> "#machine-instance-id": {contain: "instance-0"},
> ...
> }
> If you like the sound of that I could cut a small branch for a it as a slack
> task. 

Even though I'm a big proponent of DRY, the one less level of indirection makes
this approach is preferable.