Merge lp://staging/~hatch/juju-gui/detail-hang into lp://staging/juju-gui/experimental

Reviewers: mp+160228_code.launchpad.net,

Message:
Please take a look.

Description:
Fix GUI hanging when service viewed is deleted

WIP BRANCH
The UI will now display a notification if the user is:
1) Viewing a service and it gets destroyed. The user will
   be redirected to the env view.
2) Viewing a unit and it gets destroyed. The user will be
   redirected to the service view.
3) Viewing a unit and its service gets destroyed. The
   user will be redirected to the env view.

The service views have been converted to use promises to
pull the service data in.

To Do:
The unit views still need to be converted to use promises.

Tests need to be written.

This branch cannot land until the multiple 'login' event
bug has been fixed as it causes the application to break
if the user navigates to a service view before the second
'login' event has been dispatched.

https://code.launchpad.net/~hatch/juju-gui/detail-hang/+merge/160228

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/8782046/

Affected files:
   A [revision details]
   M app/app.js
   M app/assets/javascripts/ns-routing-app-extension.js
   M app/views/service.js
   M app/views/topology/service.js
   M app/views/unit.js
   M app/views/utils.js
   M test/test_application_notifications.js
   M test/test_service_view.js
   M undocumented

I'm skimming and didn't look at the service view promise support, but I
had some comments. I'm happy to talk through this tomorrow or pair if
you like.

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

https://codereview.appspot.com/8782046/diff/1/app/app.js#newcode582
app/app.js:582: _prefetch_service: function(serviceId, callback,
useCallback) {
You should be able to simplify this method drastically by always using
your promise code--or maybe you can even eliminate it?

https://codereview.appspot.com/8782046/diff/1/app/app.js#newcode638
app/app.js:638: @method _servicePromise
As noted later, this is @static as written.

https://codereview.appspot.com/8782046/diff/1/app/app.js#newcode654
app/app.js:654: service.set('prefetch', true);
Hm. This kind of implies a tri-state to me: not prefetched,
prefetching, and prefetched. If I called getServiceModel twice in rapid
succession, the second one would be called immediately(-ish) because
prefetch would be true the second time around. ISTM that instead you
want to look for an active "prefetching" state; and if the service is in
that state, wait to call resolve or reject until the prefetch attribute
is changed.

I'm not saying I love what I propose. I'd think about it. I am saying
that the current approach isn't quite right.

https://codereview.appspot.com/8782046/diff/1/app/app.js#newcode657
app/app.js:657: db.charms.add({id: charm_id}).load(env,
Hm. Make sure that you are not duplicating code in
app/store/endpoints.js (handleServiceEvent and setupCharmOnceLoad). I
am pretty sure you are.

https://codereview.appspot.com/8782046/diff/1/app/app.js#newcode661
app/app.js:661: }
You never resolve in this branch.

https://codereview.appspot.com/8782046/diff/1/app/app.js#newcode677
app/app.js:677: resolve(service);
You never get the charm in this branch (except by accident, thanks to
the endpoints code).

https://codereview.appspot.com/8782046/diff/1/app/app.js#newcode689
app/app.js:689: @return {Y.Model | Y.Promise} If the service model is
readily available
The usual pattern IME is that you *always* return a promise. The
promise might already be fulfilled! In that case when you add a
callback, it will be called very, very soon. :-)

var service = this.db.services.getById(serviceId);
if (service && service.get('prefetch')) {
   return Y.when(service);
} else {
   return new Y.Promise(...);
}

Or, actually, given the way you have written _servicePromise, you could
just say

getServiceModel: function(serviceId) {
   return new Y.Promise(...);
}

This simplifies calling code, because it always deals with a promise.

https://codereview.appspot.com/8782046/diff/1/app/app.js#newcode699
app/app.js:699: Y.bind(this._servicePromise, null, serviceId, this.db,
this.env));
I'm not convinced of the value of static methods like this. I think we
should either make it a function (inline? or not?) or a method. <shrug>
I don't feel strongly about this.

https://codereview.appspot.com/8782046/