Shows the bundle topology on the bundle view

Shows the bundle topology on the bundle view



https://code.launchpad.net/~hatch/juju-gui/bundle-detail-view-2/+merge/189642

(do not edit description out of merge proposal)

[jeff.pihach, 2013-10-07 15:41:24]

Please take a look.

[gary.poster, 2013-10-07 15:59:39]

LGTM with a bunch of trivials (smoe more trivial than others). Thank you!

https://codereview.appspot.com/14454049/diff/1/app/config-debug.js
File app/config-debug.js (right):

https://codereview.appspot.com/14454049/diff/1/app/config-debug.js#newcode32
app/config-debug.js:32: charmworldURL: 'http://staging.jujucharms.com/',
Can we revert this change, or do we need it for demos until manage gets updated
again?

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

https://codereview.appspot.com/14454049/diff/1/app/store/charmworld.js#newcod...
app/store/charmworld.js:184: API call to fetch a charm's details.
Fetch a bundle's details?

https://codereview.appspot.com/14454049/diff/1/app/store/charmworld.js#newcod...
app/store/charmworld.js:200: Like the "bundle" method but returning a Promise.
Fetch a bundle's details, returning a promise.

https://codereview.appspot.com/14454049/diff/1/app/store/env/fakebackend.js
File app/store/env/fakebackend.js (right):

https://codereview.appspot.com/14454049/diff/1/app/store/env/fakebackend.js#n...
app/store/env/fakebackend.js:1349: // XXX name is not used.  See targetBundle.
Yeah, I tripped up on this too and had a similar comment somewhere.

I think we should either eliminate the argument or use the name if provided
(e.g. if (name) {options.targetBundle = name)}).  I lean towards the first
option.

https://codereview.appspot.com/14454049/diff/1/app/subapps/browser/views/bund...
File app/subapps/browser/views/bundle.js (right):

https://codereview.appspot.com/14454049/diff/1/app/subapps/browser/views/bund...
app/subapps/browser/views/bundle.js:72: prepaired to display in the local
topology instance.
prepared

https://codereview.appspot.com/14454049/diff/1/app/subapps/browser/views/bund...
app/subapps/browser/views/bundle.js:74: @property dataPrepaired
Prepared

https://codereview.appspot.com/14454049/diff/1/app/subapps/browser/views/bund...
app/subapps/browser/views/bundle.js:78: this.dataPrepaired = false;
Prepared

https://codereview.appspot.com/14454049/diff/1/app/subapps/browser/views/bund...
app/subapps/browser/views/bundle.js:97: this.fakebackend = new
Y.juju.environments.FakeBackend({
You know, alternatively, we only stash the db on the view.  The fakebackend is
only used in the initialize code.  Stashing the db on the view would be nicer. 
<shrug>

https://codereview.appspot.com/14454049/diff/1/app/subapps/browser/views/bund...
app/subapps/browser/views/bundle.js:104: if (entity) {
I know this part is just moving code around, but could you add a comment
explaining this logic branch?  When would you have an entityId but not an
entity?

https://codereview.appspot.com/14454049/diff/1/app/subapps/browser/views/bund...
app/subapps/browser/views/bundle.js:120: self.dataPrepaired = true;
Prepared

https://codereview.appspot.com/14454049/diff/1/app/subapps/browser/views/bund...
app/subapps/browser/views/bundle.js:136: if (!this.dataPrepaired ||
!this.rendered) {
Prepared

https://codereview.appspot.com/14454049/diff/1/app/subapps/browser/views/bund...
app/subapps/browser/views/bundle.js:138: 'Cannot render bundle data without
container or prepaired data.');
prepared (sorry) :-P

[jeff.pihach, 2013-10-07 16:20:03]

Thanks for the review. Changes completed!

https://codereview.appspot.com/14454049/diff/1/app/config-debug.js
File app/config-debug.js (right):

https://codereview.appspot.com/14454049/diff/1/app/config-debug.js#newcode32
app/config-debug.js:32: charmworldURL: 'http://staging.jujucharms.com/',
It will be left here because it's required for the new charmworld data. We will
have to remember to revert it once that code is moved to `manage`

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

https://codereview.appspot.com/14454049/diff/1/app/store/charmworld.js#newcod...
app/store/charmworld.js:184: API call to fetch a charm's details.
On 2013/10/07 15:59:40, gary.poster wrote:
> Fetch a bundle's details?

Done.

https://codereview.appspot.com/14454049/diff/1/app/store/charmworld.js#newcod...
app/store/charmworld.js:200: Like the "bundle" method but returning a Promise.
On 2013/10/07 15:59:40, gary.poster wrote:
> Fetch a bundle's details, returning a promise.

Done.

https://codereview.appspot.com/14454049/diff/1/app/store/env/fakebackend.js
File app/store/env/fakebackend.js (right):

https://codereview.appspot.com/14454049/diff/1/app/store/env/fakebackend.js#n...
app/store/env/fakebackend.js:1349: // XXX name is not used.  See targetBundle.
This is actually your comment - I just left it here :) 

I'll remove the argument

https://codereview.appspot.com/14454049/diff/1/app/subapps/browser/views/bund...
File app/subapps/browser/views/bundle.js (right):

https://codereview.appspot.com/14454049/diff/1/app/subapps/browser/views/bund...
app/subapps/browser/views/bundle.js:72: prepaired to display in the local
topology instance.
On 2013/10/07 15:59:40, gary.poster wrote:
> prepared

Done.

https://codereview.appspot.com/14454049/diff/1/app/subapps/browser/views/bund...
app/subapps/browser/views/bundle.js:74: @property dataPrepaired
On 2013/10/07 15:59:40, gary.poster wrote:
> Prepared

Done.

https://codereview.appspot.com/14454049/diff/1/app/subapps/browser/views/bund...
app/subapps/browser/views/bundle.js:78: this.dataPrepaired = false;
On 2013/10/07 15:59:40, gary.poster wrote:
> Prepared

Done.

https://codereview.appspot.com/14454049/diff/1/app/subapps/browser/views/bund...
app/subapps/browser/views/bundle.js:97: this.fakebackend = new
Y.juju.environments.FakeBackend({
On 2013/10/07 15:59:40, gary.poster wrote:
> You know, alternatively, we only stash the db on the view.  The fakebackend is
> only used in the initialize code.  Stashing the db on the view would be nicer.

> <shrug>

Done.

https://codereview.appspot.com/14454049/diff/1/app/subapps/browser/views/bund...
app/subapps/browser/views/bundle.js:104: if (entity) {
On 2013/10/07 15:59:40, gary.poster wrote:
> I know this part is just moving code around, but could you add a comment
> explaining this logic branch?  When would you have an entityId but not an
> entity?

Done.

https://codereview.appspot.com/14454049/diff/1/app/subapps/browser/views/bund...
app/subapps/browser/views/bundle.js:120: self.dataPrepaired = true;
On 2013/10/07 15:59:40, gary.poster wrote:
> Prepared

Done.

https://codereview.appspot.com/14454049/diff/1/app/subapps/browser/views/bund...
app/subapps/browser/views/bundle.js:136: if (!this.dataPrepaired ||
!this.rendered) {
On 2013/10/07 15:59:40, gary.poster wrote:
> Prepared

Done.

https://codereview.appspot.com/14454049/diff/1/app/subapps/browser/views/bund...
app/subapps/browser/views/bundle.js:138: 'Cannot render bundle data without
container or prepaired data.');
On 2013/10/07 15:59:40, gary.poster wrote:
> prepared (sorry) :-P

Done.

[jeff.pihach, 2013-10-07 16:31:30]

*** Submitted:

Shows the bundle topology on the bundle view

R=gary.poster
CC=
https://codereview.appspot.com/14454049