Fix Service Destroy issue

Fix Service Destroy issue

Business logic in the view layer was messing up the
process of service destruction. This adds a notification
when a service is destroyed but waits for the delta to
update the canvas.

https://code.launchpad.net/~bcsaller/juju-gui/serviceUpdateOrdering/+merge/188183

(do not edit description out of merge proposal)

[bcsaller, 2013-09-27 23:53:33]

Please take a look.

[jeff.pihach, 2013-09-28 00:13:46]

Thanks for this fix - the error is gone but I am concerned about one of the
modifications.

https://codereview.appspot.com/14034045/diff/1/app/views/inspector.js
File app/views/inspector.js (left):

https://codereview.appspot.com/14034045/diff/1/app/views/inspector.js#oldcode461
app/views/inspector.js:461: } else {
With this gone the service icon hangs around in the canvas until the next delta
comes in - I would imagine it would be a longer delay with no indication that
something is happening on core.

https://codereview.appspot.com/14034045/diff/1/test/test_inspector_settings.js
File test/test_inspector_settings.js (left):

https://codereview.appspot.com/14034045/diff/1/test/test_inspector_settings.j...
test/test_inspector_settings.js:230: it('responds to service removal by cleaning
out the DB', function() {
See comment on previous file about this.

[benjamin.saller, 2013-09-28 00:44:54]

Thanks, feedback here for other reviewers even though we talked on IRC

https://codereview.appspot.com/14034045/diff/1/app/views/inspector.js
File app/views/inspector.js (left):

https://codereview.appspot.com/14034045/diff/1/app/views/inspector.js#oldcode461
app/views/inspector.js:461: } else {
On 2013/09/28 00:13:47, jeff.pihach wrote:
> With this gone the service icon hangs around in the canvas until the next
delta
> comes in - I would imagine it would be a longer delay with no indication that
> something is happening on core. 

Its not even the next delta, its the delta where the service was removed from
juju-core that we are waiting on. 

I think notifications make sense here as a UX around the pending action. We
could toggle some state on the model locally to indicate we expect it be Dying
or pending or somesuch, but to destroy it w/o  confirmation from the server is
an issue (see failing stop hooks talk from before).

[gary.poster, 2013-09-30 12:26:43]

LGTM with trivial.  Thank you!

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

https://codereview.appspot.com/14034045/diff/1/app/store/env/fakebackend.js#o...
app/store/env/fakebackend.js:1550: self.sendDelta();
I'm glad we don't have to do this.  I assume the reason is that the usual
gradual delta messages handle this fine.  Please let me know if that assumption
is incorrect.

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

https://codereview.appspot.com/14034045/diff/1/app/store/env/go.js#newcode261
app/store/env/go.js:261: // Reverse the sort order for removes.
Please add a comment as to why.

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

https://codereview.appspot.com/14034045/diff/1/app/store/env/sandbox.js#newco...
app/store/env/sandbox.js:273: deltas.push(delta);
I don't really care, but why did you make this change?  Do you think it is
clearer that the one-liner?

https://codereview.appspot.com/14034045/diff/1/app/views/inspector.js
File app/views/inspector.js (left):

https://codereview.appspot.com/14034045/diff/1/app/views/inspector.js#oldcode461
app/views/inspector.js:461: } else {
On 2013/09/28 00:44:54, benjamin.saller wrote:
> On 2013/09/28 00:13:47, jeff.pihach wrote:
> > With this gone the service icon hangs around in the canvas until the next
> delta
> > comes in - I would imagine it would be a longer delay with no indication
that
> > something is happening on core. 
> 
> Its not even the next delta, its the delta where the service was removed from
> juju-core that we are waiting on. 
> 
> I think notifications make sense here as a UX around the pending action. We
> could toggle some state on the model locally to indicate we expect it be Dying
> or pending or somesuch, but to destroy it w/o  confirmation from the server is
> an issue (see failing stop hooks talk from before).

Right.  I agree that this is what we agreed on.

[jeff.pihach, 2013-09-30 16:24:40]

LGTM with the new notification as per our chat.

Thanks for the fix!

[bcsaller, 2013-09-30 18:12:28]

*** Submitted:

Fix Service Destroy issue

Business logic in the view layer was messing up the
process of service destruction. This adds a notification
when a service is destroyed but waits for the delta to
update the canvas.

R=jeff.pihach, benjamin.saller, gary.poster
CC=
https://codereview.appspot.com/14034045