Adds relation details viewlet to inspector

Adds relation details viewlet to inspector

This branch adds the relation details viewlet to the inspector
and databinds the values to the relation interactions.

As with units we now keep a copy of the relations data on the
service and keep that up to date during the users interactions
and deltas to allow for easy data binding for the viewlet.

We no longer rely on the relation_error attribute and instead
have aggregateRelations and aggregateRelationError attributes.

https://code.launchpad.net/~hatch/juju-gui/inspector-relations/+merge/184831

(do not edit description out of merge proposal)

[jeff.pihach, 2013-09-10 17:03:49]

Please take a look.

[benjamin.saller, 2013-09-10 19:13:20]

In general good, I had a number of comments though and I haven't done the QA
yet.

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

https://codereview.appspot.com/13373050/diff/1/app/models/models.js#newcode140
app/models/models.js:140: service.set('units', modelList);
Should we move this into init now that we agree service.units is the proper way
to manage this and stop deferring it until the delta? I think we should. Then
most of this can go away.

https://codereview.appspot.com/13373050/diff/1/app/models/models.js#newcode164
app/models/models.js:164: // it's possible that a service will be null
We should fix those tests then, but fine for now.

https://codereview.appspot.com/13373050/diff/1/app/models/models.js#newcode168
app/models/models.js:168: tmp = new models.RelationList();
This path shouldn't happen with it in init, also can we call this serviceList or
something other than tmp.

https://codereview.appspot.com/13373050/diff/1/app/models/models.js#newcode502
app/models/models.js:502: if (unitList) {
This check would go away if you moved units into service.init

https://codereview.appspot.com/13373050/diff/1/app/models/models.js#newcode563
app/models/models.js:563: interfaceName = hook.slice(0, hook.length -
2)[0].trim();
This series of checks seems a little weak, error + relation & failed and the we
do splits... Even minor changes or additions to the error strings will break
this. We have that note to talk with William again about getting better
selectors into the error (category or whatnot) and we should follow up this
week.

https://codereview.appspot.com/13373050/diff/1/app/models/models.js#newcode704
app/models/models.js:704: // also need to remove the relation from those models.
I'd rather see the lookup code in this block moved to a method either on the
relationList or the db (for access to both rels and services)
with 'serviceRelations.remove(rel)' as the callback of the lookup method. Does
that make sense?

https://codereview.appspot.com/13373050/diff/1/app/models/models.js#newcode725
app/models/models.js:725: // If the relationLists weren't added to the services
after 'if action == remove' we can return or ifelse this block. Some of these
checks shouldn't need to be here as we create the list by default.

https://codereview.appspot.com/13373050/diff/1/app/views/viewlets/service-rel...
File app/views/viewlets/service-relations.js (right):

https://codereview.appspot.com/13373050/diff/1/app/views/viewlets/service-rel...
app/views/viewlets/service-relations.js:48: // Aggregate the unit statuses here
to display the relation status
Another branch? also you don't aggregate here, you just display here, the
aggregation occurs elsewhere.

https://codereview.appspot.com/13373050/diff/1/test/test_fakebackend.js
File test/test_fakebackend.js (right):

https://codereview.appspot.com/13373050/diff/1/test/test_fakebackend.js#newco...
test/test_fakebackend.js:120: // when doing a deep equals on an object which
contains a LazyModelList
frightening

https://codereview.appspot.com/13373050/diff/1/test/test_fakebackend.js#newco...
test/test_fakebackend.js:124: // JS engine - if there are failures here check
the order first.
I don't think that comment is fair, its guaranteed in an array and in an Object
I think it does the right thing?

[jeff.pihach, 2013-09-11 14:15:52]

Please take a look.

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

https://codereview.appspot.com/13373050/diff/1/app/models/models.js#newcode140
app/models/models.js:140: service.set('units', modelList);
Good idea, done!

https://codereview.appspot.com/13373050/diff/1/app/models/models.js#newcode164
app/models/models.js:164: // it's possible that a service will be null
right thanks

https://codereview.appspot.com/13373050/diff/1/app/models/models.js#newcode168
app/models/models.js:168: tmp = new models.RelationList();
On 2013/09/10 19:13:20, benjamin.saller wrote:
> This path shouldn't happen with it in init, also can we call this serviceList
or
> something other than tmp.

Done.

https://codereview.appspot.com/13373050/diff/1/app/models/models.js#newcode502
app/models/models.js:502: if (unitList) {
On 2013/09/10 19:13:20, benjamin.saller wrote:
> This check would go away if you moved units into service.init

Done.

https://codereview.appspot.com/13373050/diff/1/app/models/models.js#newcode563
app/models/models.js:563: interfaceName = hook.slice(0, hook.length -
2)[0].trim();
you bet!

https://codereview.appspot.com/13373050/diff/1/app/views/viewlets/service-rel...
File app/views/viewlets/service-relations.js (right):

https://codereview.appspot.com/13373050/diff/1/app/views/viewlets/service-rel...
app/views/viewlets/service-relations.js:48: // Aggregate the unit statuses here
to display the relation status
yup a follow-up this branch was getting long in the tooth already - a card has
been created.

https://codereview.appspot.com/13373050/diff/1/test/test_fakebackend.js
File test/test_fakebackend.js (right):

https://codereview.appspot.com/13373050/diff/1/test/test_fakebackend.js#newco...
test/test_fakebackend.js:120: // when doing a deep equals on an object which
contains a LazyModelList
yeah, took a while to figure this one out.

https://codereview.appspot.com/13373050/diff/1/test/test_fakebackend.js#newco...
test/test_fakebackend.js:124: // JS engine - if there are failures here check
the order first.
I assumed that it did a key/value comparison as well but this test fails if you
re-organize the 'expected' object.

[jeff.pihach, 2013-09-11 14:40:00]

Please take a look.

[rharding, 2013-09-11 15:06:48]

It seems there's a bunch of checks working around test limititations and they
all silently 'pass' if we were to hit them on accident in real life. There also
seems to be a bunch of missing tests for the new functions/code added. Comments
below.

https://codereview.appspot.com/13373050/diff/8001/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/13373050/diff/8001/app/models/models.js#newcod...
app/models/models.js:290: relations: { },
units ATTR set in initializer doesn't exist. No docs on these attrs.

https://codereview.appspot.com/13373050/diff/8001/app/models/models.js#newcod...
app/models/models.js:446: // for a valid service here.
should the tests be updated then? It seems this could hide bad calling code in
the future since it's not limited to a straight 'tests are running' flag.

https://codereview.appspot.com/13373050/diff/8001/app/models/models.js#newcod...
app/models/models.js:508: interfaceName = hook.slice(0, hook.length -
2)[0].trim();
does this work for both go/python?

https://codereview.appspot.com/13373050/diff/8001/app/models/models.js#newcod...
app/models/models.js:663: // The tests don't always add services so we check if
they exist
again, it seems the tests should be updated.

https://codereview.appspot.com/13373050/diff/8001/app/models/models.js#newcod...
app/models/models.js:682: // Because some of the tests add relations without
services
and another check

https://codereview.appspot.com/13373050/diff/8001/app/views/topology/relation.js
File app/views/topology/relation.js (right):

https://codereview.appspot.com/13373050/diff/8001/app/views/topology/relation...
app/views/topology/relation.js:582: // this check just makes sure it doesn't
blow up.
and more test exceptioning

https://codereview.appspot.com/13373050/diff/8001/app/views/viewlets/service-...
File app/views/viewlets/service-relations.js (right):

https://codereview.appspot.com/13373050/diff/8001/app/views/viewlets/service-...
app/views/viewlets/service-relations.js:48: // Aggregate the unit statuses here
to display the relation status
so there's a comment but no action? Was this meant to be imlpemented?

https://codereview.appspot.com/13373050/diff/12001/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/13373050/diff/12001/app/models/models.js#newco...
app/models/models.js:100: var getServicesfromDelta = function(modelInstance,
data, db) {
this function should have direct test casts for the if/else conditions below.

https://codereview.appspot.com/13373050/diff/12001/app/models/models.js#newco...
app/models/models.js:172: this._bindAttributes();
are there any new tests of the new data binding hooks into place here? I didn't
see them.

https://codereview.appspot.com/13373050/diff/12001/app/models/models.js#newco...
app/models/models.js:501: if (state === 'error') {
was there a test to verify the errors are parsed correctly?

https://codereview.appspot.com/13373050/diff/12001/app/models/models.js#newco...
app/models/models.js:674: });
I'd expect a test tha the model data is in fact removed.

https://codereview.appspot.com/13373050/diff/12001/app/templates/service-rela...
File app/templates/service-relations-list.handlebars (right):

https://codereview.appspot.com/13373050/diff/12001/app/templates/service-rela...
app/templates/service-relations-list.handlebars:8: {{/relations}}
Is there a test verifying the generated html for a couple of sample relations
from the viewlet/data binding?

[rharding, 2013-09-11 16:24:57]

QA ok in Go, sandbox in chrome and IE.

We agreed at least an ugly header on the tab for now so that the users can
understand what the tab is for until the content is designed better.

The one note is to add a bug/follow up card for the peer relation and add that
to visualization as well as to sandbox so we see/qa that as we develop.

[jeff.pihach, 2013-09-11 20:36:55]

Thanks for the reviews - as discussed in our call, changes will be coming soon.

https://codereview.appspot.com/13373050/diff/8001/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/13373050/diff/8001/app/models/models.js#newcod...
app/models/models.js:290: relations: { },
On 2013/09/11 15:06:48, rharding wrote:
> units ATTR set in initializer doesn't exist. No docs on these attrs.

Done.

https://codereview.appspot.com/13373050/diff/8001/app/models/models.js#newcod...
app/models/models.js:446: // for a valid service here.
log and flag added as discussed

https://codereview.appspot.com/13373050/diff/8001/app/models/models.js#newcod...
app/models/models.js:508: interfaceName = hook.slice(0, hook.length -
2)[0].trim();
No, the python backend does not provide a agent_state_info so the block never
gets parsed and won't aggregate unit statuses for the relations. This is
acceptable IMHO because it's simply an indicator in the relations list and we
are dropping support for pyjuju

https://codereview.appspot.com/13373050/diff/8001/app/models/models.js#newcod...
app/models/models.js:663: // The tests don't always add services so we check if
they exist
log and flag added as discussed

https://codereview.appspot.com/13373050/diff/8001/app/models/models.js#newcod...
app/models/models.js:682: // Because some of the tests add relations without
services
log and flag added as discussed

https://codereview.appspot.com/13373050/diff/8001/app/views/topology/relation.js
File app/views/topology/relation.js (right):

https://codereview.appspot.com/13373050/diff/8001/app/views/topology/relation...
app/views/topology/relation.js:582: // this check just makes sure it doesn't
blow up.
log and flag added as discussed

https://codereview.appspot.com/13373050/diff/8001/app/views/viewlets/service-...
File app/views/viewlets/service-relations.js (right):

https://codereview.appspot.com/13373050/diff/8001/app/views/viewlets/service-...
app/views/viewlets/service-relations.js:48: // Aggregate the unit statuses here
to display the relation status
Yes there is a follow-up card as this will require additional tests and
functionality

https://codereview.appspot.com/13373050/diff/12001/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/13373050/diff/12001/app/models/models.js#newco...
app/models/models.js:172: this._bindAttributes();
On 2013/09/11 15:06:49, rharding wrote:
> are there any new tests of the new data binding hooks into place here? I
didn't
> see them. 

Done.

https://codereview.appspot.com/13373050/diff/12001/app/templates/service-rela...
File app/templates/service-relations-list.handlebars (right):

https://codereview.appspot.com/13373050/diff/12001/app/templates/service-rela...
app/templates/service-relations-list.handlebars:8: {{/relations}}
There isn't but will be once the follow-up styling branch is finished.

[jeff.pihach, 2013-09-11 20:42:45]

Please take a look.

[benjamin.saller, 2013-09-11 21:07:56]

LGTM.

Thanks for this. I would like to see the service.relations manipulation moved
into the service model as a method. There is no case where we don't want those
associated cleanups and so its part of the model, not a view.

https://codereview.appspot.com/13373050/diff/23001/app/views/topology/relatio...
File app/views/topology/relation.js (right):

https://codereview.appspot.com/13373050/diff/23001/app/views/topology/relatio...
app/views/topology/relation.js:591: if (rel.get('relation_id') ===
relation.relation_id) {
This should be a method of service now, no? its not a method of the view to
remove the model, only to trigger that based on user interaction.

[jeff.pihach, 2013-09-11 21:15:21]

Moved the service relation remove onto the service.

Thanks for the reviews!

https://codereview.appspot.com/13373050/diff/23001/app/views/topology/relatio...
File app/views/topology/relation.js (right):

https://codereview.appspot.com/13373050/diff/23001/app/views/topology/relatio...
app/views/topology/relation.js:591: if (rel.get('relation_id') ===
relation.relation_id) {
Right sorry I meant to do this in the last branch.

[jeff.pihach, 2013-09-11 21:26:12]

*** Submitted:

Adds relation details viewlet to inspector

This branch adds the relation details viewlet to the inspector
and databinds the values to the relation interactions.

As with units we now keep a copy of the relations data on the
service and keep that up to date during the users interactions
and deltas to allow for easy data binding for the viewlet.

We no longer rely on the relation_error attribute and instead
have aggregateRelations and aggregateRelationError attributes.

R=benjamin.saller, rharding
CC=
https://codereview.appspot.com/13373050

[rharding, 2013-09-11 21:28:07]

LGTM

As noted, I would love to see new/process_delta and  getServicesFromDeltaChanged
work, but will take your word that it's covered by current functional-ish tests.

Other notes below.

https://codereview.appspot.com/13373050/diff/23001/app/views/topology/relatio...
File app/views/topology/relation.js (right):

https://codereview.appspot.com/13373050/diff/23001/app/views/topology/relatio...
app/views/topology/relation.js:591: if (rel.get('relation_id') ===
relation.relation_id) {
On 2013/09/11 21:07:56, benjamin.saller wrote:
> This should be a method of service now, no? its not a method of the view to
> remove the model, only to trigger that based on user interaction.

+1

https://codereview.appspot.com/13373050/diff/23001/test/test_model.js
File test/test_model.js (right):

https://codereview.appspot.com/13373050/diff/23001/test/test_model.js#newcode190
test/test_model.js:190: it('services have unit and relation modellists',
function() {
this tests the change in the initializer, cool

https://codereview.appspot.com/13373050/diff/23001/test/test_model.js#newcode196
test/test_model.js:196: it('relation changes on service update
aggregateRelations', function(done) {
this tests the _bindAttribute is setup correctly from what I see.

https://codereview.appspot.com/13373050/diff/23001/test/test_model.js#newcode205
test/test_model.js:205:
assert.equal(isObject(service.get('aggregateRelations')), true);
isObject seems to be bit of a light check. Recall the issues we had with null
returning true to this check.