Merge into trunk : serviceUpdateOrdering : Code : juju-gui

Status:

Merged

Merged at revision:

1099

Proposed branch:

lp://staging/~bcsaller/juju-gui/serviceUpdateOrdering

Merge into:

lp://staging/juju-gui/experimental

Diff against target:

181 lines (+16/-70)

5 files modified

app/store/env/fakebackend.js (+7/-8)
app/store/env/go.js (+6/-1)
app/store/env/sandbox.js (+3/-3)
app/views/inspector.js (+0/-12)
test/test_inspector_settings.js (+0/-46)

To merge this branch:

bzr merge lp://staging/~bcsaller/juju-gui/serviceUpdateOrdering

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Juju GUI Hackers		2013-09-27	Pending
Review via email: mp+188183@code.staging.launchpad.net

Description of the change

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://codereview.appspot.com/14034045/

Revision history for this message

Benjamin Saller (bcsaller) wrote on 2013-09-27:

#

Download full text (7.5 KiB)

Reviewers: mp+188183_code.launchpad.net,

Message:
Please take a look.

Description:
Fix Service Destroy issue

Business logic in the view layer was messing up the
process of service destruction.

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

(do not edit description out of merge proposal)

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

Affected files (+18, -70 lines):
   A [revision details]
   M app/store/env/fakebackend.js
   M app/store/env/go.js
   M app/store/env/sandbox.js
   M app/views/inspector.js
   M test/test_inspector_settings.js

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: test/test_inspector_settings.js
=== modified file 'test/test_inspector_settings.js'
--- test/test_inspector_settings.js 2013-09-27 18:20:06 +0000
+++ test/test_inspector_settings.js 2013-09-27 23:41:44 +0000
@@ -227,52 +227,6 @@
assert.equal(typeof events['.cancel-destroy'].click, 'function');
});

- it('responds to service removal by cleaning out the DB', function() {
- // If destroying a service succeeds, the service is removed from the
- // database.
- var removeServiceCalled = false,
- removeRelationsCalled = false,
- destroyServiceCalled = false;
-
- inspector = setUpInspector();
-
- var service = {
- get: function(name) {
- assert.equal(name, 'id');
- return 'SERVICE-ID';
- },
- destroy: function() {
- destroyServiceCalled = true;
- }
- };
- var RELATIONS = 'all of the relations of the service being removed';
-
- var db = {
- services: {
- remove: function(serviceToBeRemoved) {
- assert.deepEqual(serviceToBeRemoved, service);
- removeServiceCalled = true;
- }
- },
- relations: {
- filter: function(predicate) {
- return RELATIONS;
- },
- remove: function(relationsToBeRemoved) {
- assert.deepEqual(relationsToBeRemoved, RELATIONS);
- removeRelationsCalled = true;
- }
- }
- };
-
- var evt = {err: false};
-
- inspector._destroyServiceCallback(service, db, evt);
- assert.isTrue(removeServiceCalled);
- assert.isTrue(removeRelationsCalled);
- assert.isTrue(removeServiceCalled);
- });
-
it('responds to service removal failure by alerting the user',
function() {
var notificationAdded;

Index: app/views/inspector.js
=== modified file 'app/views/inspector.js'
--- app/views/inspector.js 2013-09-27 18:20:06 +0000
+++ app/views/inspector.js 2013-09-27 23:41:44 +0000
@@ -458,18 +458,6 @@
                modelId: service
              })
          );
- } else {
- // If the removal succeeded on the server side, we need to remove
the
- // service from the database. (Why wouldn't we get an update from
the
- // server side that would do this for us?).
- db.services.remove(service);
...

Reviewers: mp+188183_code.launchpad.net,

Message:
Please take a look.

Description:
Fix Service Destroy issue

Business logic in the view layer was messing up the
process of service destruction.

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

(do not edit description out of merge proposal)

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

Affected files (+18, -70 lines):
   A [revision details]
   M app/store/env/fakebackend.js
   M app/store/env/go.js
   M app/store/env/sandbox.js
   M app/views/inspector.js
   M test/test_inspector_settings.js

Index: [revision details]
=== added file '[revision details]'
--- [revision details]	2012-01-01 00:00:00 +0000
+++ [revision details]	2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: jeff.pihach@canonical.com-20130927203033-89ltsmxuvlkrlnzx
+New revision: bcsaller@gmail.com-20130927234144-80y8yexfwgbcdggn

Index: test/test_inspector_settings.js
=== modified file 'test/test_inspector_settings.js'
--- test/test_inspector_settings.js	2013-09-27 18:20:06 +0000
+++ test/test_inspector_settings.js	2013-09-27 23:41:44 +0000
@@ -227,52 +227,6 @@
      assert.equal(typeof events['.cancel-destroy'].click, 'function');
    });

-  it('responds to service removal by cleaning out the DB', function() {
-    // If destroying a service succeeds, the service is removed from the
-    // database.
-    var removeServiceCalled = false,
-        removeRelationsCalled = false,
-        destroyServiceCalled = false;
-
-    inspector = setUpInspector();
-
-    var service = {
-      get: function(name) {
-        assert.equal(name, 'id');
-        return 'SERVICE-ID';
-      },
-      destroy: function() {
-        destroyServiceCalled = true;
-      }
-    };
-    var RELATIONS = 'all of the relations of the service being removed';
-
-    var db = {
-      services: {
-        remove: function(serviceToBeRemoved) {
-          assert.deepEqual(serviceToBeRemoved, service);
-          removeServiceCalled = true;
-        }
-      },
-      relations: {
-        filter: function(predicate) {
-          return RELATIONS;
-        },
-        remove: function(relationsToBeRemoved) {
-          assert.deepEqual(relationsToBeRemoved, RELATIONS);
-          removeRelationsCalled = true;
-        }
-      }
-    };
-
-    var evt = {err: false};
-
-    inspector._destroyServiceCallback(service, db, evt);
-    assert.isTrue(removeServiceCalled);
-    assert.isTrue(removeRelationsCalled);
-    assert.isTrue(removeServiceCalled);
-  });
-
    it('responds to service removal failure by alerting the user',  
function() {
      var notificationAdded;

Index: app/views/inspector.js
=== modified file 'app/views/inspector.js'
--- app/views/inspector.js	2013-09-27 18:20:06 +0000
+++ app/views/inspector.js	2013-09-27 23:41:44 +0000
@@ -458,18 +458,6 @@
                modelId: service
              })
          );
-      } else {
-        // If the removal succeeded on the server side, we need to remove  
the
-        // service from the database.  (Why wouldn't we get an update from  
the
-        // server side that would do this for us?).
-        db.services.remove(service);
-        service.destroy();
-        db.relations.remove(db.relations.filter(
-            function(r) {
-              return Y.Array.some(r.get('endpoints'), function(ep) {
-                return ep[0] === service.get('id');
-              });
-            }));
        }
      },

Index: app/store/env/fakebackend.js
=== modified file 'app/store/env/fakebackend.js'
--- app/store/env/fakebackend.js	2013-09-27 14:52:02 +0000
+++ app/store/env/fakebackend.js	2013-09-27 23:41:44 +0000
@@ -1344,9 +1344,9 @@
       returns a complex data structure which is used
       by importDeployer to enact the deploy.

-     @method injestDeployer
+     @method ingestDeployer
       */
-    injestDeployer: function(data, name, options) {
+    ingestDeployer: function(data, name, options) {
        if (!data) {return;}
        options = options || {};
        var db = this.db;
@@ -1482,15 +1482,15 @@
          data = YAMLData;
        }
        // XXX: The proper API doesn't allow options for the level
-      // of control that injest allows. This should be addressed
+      // of control that ingest allows. This should be addressed
        // in the future.
        var options = {};
        if (name) {
          options.targetBundle = name;
        }
-      var injestedData = this.injestDeployer(data, name, options);
+      var ingestedData = this.ingestDeployer(data, name, options);
        var servicePromises = [];
-      Y.each(injestedData.services, function(serviceData) {
+      Y.each(ingestedData.services, function(serviceData) {
          // Map the argument name from the deployer format
          // name for unit count.
          if (!serviceData.unitCount) {
@@ -1520,7 +1520,7 @@
                // doesn't handle
                var service = sdr.service;
                var serviceId = service.get('id');
-              var serviceData = injestedData.services[serviceId];
+              var serviceData = ingestedData.services[serviceId];

// Force the annotation update (deploy doesn't handle this).
                var annotiations = serviceData.annotations;
@@ -1537,7 +1537,7 @@
                  sdr.service, true];
              });

-            injestedData.relations.forEach(function(relationData) {
+            ingestedData.relations.forEach(function(relationData) {
                var relResult = self.addRelation(
                    relationData[0], relationData[1], true);
                self.changes.relations[relResult.relation.get('id')] = [
@@ -1547,7 +1547,6 @@
        .then(function() {
              deployStatus.Status = 'completed';
              callback({DeploymentId: self._deploymentId});
-            self.sendDelta();
            }, function(err) {
              deployStatus.Status = 'failed';
              callback({Error: err.toString()});

Index: app/store/env/go.js
=== modified file 'app/store/env/go.js'
--- app/store/env/go.js	2013-09-27 16:26:18 +0000
+++ app/store/env/go.js	2013-09-27 23:41:44 +0000
@@ -256,7 +256,12 @@
          if (!cmp[b[0]]) {
            return -1;
          }
-        return cmp[a[0]] - cmp[b[0]];
+        var scoreA = cmp[a[0]];
+        var scoreB = cmp[b[0]];
+        // Reverse the sort order for removes.
+        if (a[1] === 'remove') { scoreA = -scoreA; }
+         if (b[1] === 'remove') { scoreB = -scoreB; }
+        return scoreA - scoreB;
        });
        this.fire('delta', {data: {result: deltas}});
      },

Index: app/store/env/sandbox.js
=== modified file 'app/store/env/sandbox.js'
--- app/store/env/sandbox.js	2013-09-25 16:58:28 +0000
+++ app/store/env/sandbox.js	2013-09-27 23:41:44 +0000
@@ -242,8 +242,7 @@
    implentation, but the data within the individual deltas must be  
structured
    dependent on the backend.  This method is called using `apply` from  
within
    the appropriate sandbox so that `this._deltaWhitelist` and
-  `self._getDeltaAttrs` can structure the delta
-  according to the juju type.
+  `self._getDeltaAttrs` can structure the delta according to the juju type.

@method _prepareDelta
    @return {Array} An array of deltas events.
@@ -270,7 +269,8 @@
              // The unit changeType is actually "serviceUnit" in the Python
              // stream.  Our model code handles either, so we're not  
modifying
              // it for now.
-            deltas.push([changeType, action, attrs]);
+            var delta = [changeType, action, attrs];
+            deltas.push(delta);
            });
          }
          if (annotations) {

Revision history for this message

Jeff Pihach (hatch) wrote on 2013-09-28:

#

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.js#oldcode230
test/test_inspector_settings.js:230: it('responds to service removal by
cleaning out the DB', function() {
See comment on previous file about this.

https://codereview.appspot.com/14034045/

Revision history for this message

Benjamin Saller (bcsaller) wrote on 2013-09-28:

#

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).

https://codereview.appspot.com/14034045/

Revision history for this message

Gary Poster (gary) wrote on 2013-09-30:

#

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#oldcode1550
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#newcode273
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.

https://codereview.appspot.com/14034045/

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#oldcode1550
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#newcode273
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.

https://codereview.appspot.com/14034045/

Revision history for this message

Jeff Pihach (hatch) wrote on 2013-09-30:

#

LGTM with the new notification as per our chat.

Thanks for the fix!

https://codereview.appspot.com/14034045/

Revision history for this message

Benjamin Saller (bcsaller) wrote on 2013-09-30:

#

*** 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

https://codereview.appspot.com/14034045/

juju-gui

Merge lp://staging/~bcsaller/juju-gui/serviceUpdateOrdering into lp://staging/juju-gui/experimental

Commit message

Description of the change

Preview Diff

Subscribers