Merge lp:~hatch/juju-gui/parse-pending-types into lp:juju-gui/experimental

[Jeff Pihach]

Adds pending and dying statuses to the inspector

A service in various states of pending are now properly represented in the
inspector.

Dying services are now left on the canvas with their inspectors until the service
is entirely destroyed from Juju.

https://codereview.appspot.com/36240043/

[Jeff Pihach (hatch)]

Reviewers: mp+197441_code.launchpad.net,

Message:
Please take a look.

Description:
Adds pending and dying statuses to the inspector

A service in various states of pending are now properly represented in
the
inspector.

Dying services are now left on the canvas with their inspectors until
the service
is entirely destroyed from Juju.

To QA:
Sandbox:
Deploy some services and make sure you can destroy them in various ways.
Deploy a service with a lot of units and wait for the simulator to show
   pending and installing statuses

Real Env:
Deploy 4 instances of failtester
Set one to fail on install hook
Open one of the pending services inspectors and then delete it from the
console
It should enter a 'dying' state then remove from the canvas when
finished
Wait for the remaining failtester services to come green/red
Destroy one from the GUI, it should enter a 'dying' state and then
remove from the canvas
   closing the inspector when it's done.
Open the other green one, destroy it from the console, it should enter a
'dying' state and
   then remove from the canvas
Open the red one's inspector, destroy it from the console, nothing
should happen in the GUI
Resolve the unit in error in the GUI and the service should enter a
'dying' state then
   remove from the canvas when done.

https://code.launchpad.net/~hatch/juju-gui/parse-pending-types/+merge/197441

(do not edit description out of merge proposal)

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

Affected files (+54, -71 lines):
   A [revision details]
   M app/models/handlers.js
   M app/models/models.js
   M app/store/env/simulator.js
   M app/views/environment.js
   M app/views/inspector.js
   M app/views/utils.js
   M app/views/viewlets/service-overview.js
   M test/test_model.js
   M test/test_service_module.js

[Madison Scott-Clary (makyo)]

LGTM, QA okay, one question, feel free to say no! Thanks.

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

https://codereview.appspot.com/36240043/diff/1/app/models/models.js#newcode205
app/models/models.js:205: if (life === ALIVE || life === 'dying') {
We have a constant ALIVE, might it make sense to do he samefor DYING?

https://codereview.appspot.com/36240043/

[Jeff Pihach (hatch)]

*** Submitted:

Adds pending and dying statuses to the inspector

A service in various states of pending are now properly represented in
the
inspector.

Dying services are now left on the canvas with their inspectors until
the service
is entirely destroyed from Juju.

R=matthew.scott
CC=
https://codereview.appspot.com/36240043

https://codereview.appspot.com/36240043/

=== modified file 'app/models/handlers.js'
--- app/models/handlers.js	2013-11-19 22:30:18 +0000
+++ app/models/handlers.js	2013-12-02 21:33:17 +0000
@@ -172,7 +172,10 @@
         public_address: change.PublicAddress
       };
       var service = db.services.getById(change.Name.split('/')[0]);
-      service.get('units').process_delta(action, unitData, db);
+      if (service) {
+        // The service can be destroyed before the last unit delta comes in.
+        service.get('units').process_delta(action, unitData, db);
+      }
       db.machines.process_delta(action, machineData, db);
     },
 
 
=== modified file 'app/models/models.js'
--- app/models/models.js	2013-11-26 20:38:05 +0000
+++ app/models/models.js	2013-12-02 21:33:17 +0000
@@ -191,18 +191,21 @@
     },
 
     /**
-      Return true if this service life is "alive", false otherwise.
+      Return true if this service life is 'alive' or 'dying', false otherwise.
 
       A model instance is alive if its life cycle (i.e. the "life" attribute)
-      is set to "alive". Other possible values, as they arrive from the
-      juju-core delta stream, are "dying" and "dead", in which cases the
-      service is not considered alive.
+      is set to 'alive' or 'dying'. The other possible value, as they arrive
+      from the juju-core delta stream is 'dead'.
 
       @method isAlive
       @return {Boolean} Whether this service is alive.
      */
     isAlive: function() {
-      return this.get('life') === ALIVE;
+      var life = this.get('life');
+      if (life === ALIVE || life === 'dying') {
+        return true;
+      }
+      return false;
     },
 
     /**
@@ -413,14 +416,10 @@
     model: Service,
 
     /**
-      Return a list of visible model instances.
-
-      A model instance is visible when it is alive or when, even if it is dying
-      or dead, one or more of its units are in an error state.
-      In the latter case, we want to still display the service in order to
-      allow users to retry or resolve its units.
-
-      @method alive
+      Return a list of visible model instances. A model instance is visible
+      when it is alive or dying.
+
+      @method visible
       @return {Y.ModelList} The resulting visible model instances.
     */
     visible: function() {
@@ -593,10 +592,12 @@
     get_informative_states_for_service: function(service) {
       var aggregate_map = {},
           relationError = {},
-          units_for_service = service.get('units');
+          units_for_service = service.get('units'),
+          serviceLife = service.get('life');
 
       units_for_service.each(function(unit) {
-        var state = utils.determineCategoryType(utils.simplifyState(unit));
+        var state = utils.determineCategoryType(
+                              utils.simplifyState(unit, serviceLife));
         if (aggregate_map[state] === undefined) {
           aggregate_map[state] = 1;
         } else {
@@ -633,9 +634,7 @@
       var previous_unit_count = service.get('unit_count');
       service.set('unit_count', sum);
       service.set('aggregated_status', aggregate[0]);
-
       service.set('relationChangeTrigger', { error: aggregate[1] });
-
       // Set Google Analytics tracking event.
       if (previous_unit_count !== sum && window._gaq) {
         window._gaq.push(['_trackEvent', 'Service Stats', 'Update',
 
=== modified file 'app/store/env/simulator.js'
--- app/store/env/simulator.js	2013-11-22 22:52:30 +0000
+++ app/store/env/simulator.js	2013-12-02 21:33:17 +0000
@@ -157,18 +157,8 @@
               unit.agent_state_info = undefined;
               unit.agent_state_data = {};
             } else if (roll <= 0.7) {
-              if (window.flags && window.flags.simPend) {
-                // Because the GUI does not yet support parsing the 'life'
-                // status attribute from core and including it in the
-                // aggregate status this is here to simply simulate the
-                // functionality in the inspector unit list, until it is.
-                if (roll <= 0.4) {
-                  unit.agent_state = 'dying';
-                } else if (roll <= 0.5) {
-                  unit.agent_state = 'installing';
-                } else {
-                  unit.agent_state = 'pending';
-                }
+              if (roll <= 0.5) {
+                unit.agent_state = 'installing';
               } else {
                 unit.agent_state = 'pending';
               }
 
=== modified file 'app/views/environment.js'
--- app/views/environment.js	2013-11-13 15:41:46 +0000
+++ app/views/environment.js	2013-12-02 21:33:17 +0000
@@ -151,32 +151,11 @@
             this.topo.fire('hideServiceMenu');
           }, this);
 
-          model.get('units').on('deltaChange', function(e) {
-            var service = e.service;
-            var units = service.get('units');
-
-            var started = !units.some(function(unit) {
-              if ((/-?error$/).test(unit.agent_state)) {
-                return true;
-              }
-            });
-
-            if (started && service.get('life') === 'dying') {
-              var inspector = this.getInspector(service.get('packageName'));
-              if (inspector) { inspector.viewletManager.destroy(); }
-              this.topo.fire('hideServiceMenu');
-            }
-          }, this);
-
           // If the service is destroyed from the console then we need to
           // destroy the inspector and hide the service menu.
-          model.after(['lifeChange', 'destroy'], function(e) {
+          model.on('destroy', function(e) {
             var service = e.currentTarget;
-            // The user can put the service in a dying state from the console
-            // but it will not be destroyed if any of its units has errors.
-            if (service.isAlive() || service.hasErrors()) { return; }
-
-            var inspector = this.getInspector(service.get('packageName'));
+            var inspector = this.getInspector(service.get('id'));
             if (inspector) { inspector.viewletManager.destroy(); }
             this.topo.fire('hideServiceMenu');
           }, this);
 
=== modified file 'app/views/inspector.js'
--- app/views/inspector.js	2013-10-11 19:33:07 +0000
+++ app/views/inspector.js	2013-12-02 21:33:17 +0000
@@ -507,8 +507,8 @@
     */
     _onInitiateDestroy: function(evt) {
       evt.halt();
-      this.closeInspector();
       this.initiateServiceDestroy();
+      this._onCancelDestroy(evt);
       this.options.environment.topo.fire('clearState');
     },
 
 
=== modified file 'app/views/utils.js'
--- app/views/utils.js	2013-11-27 19:27:32 +0000
+++ app/views/utils.js	2013-12-02 21:33:17 +0000
@@ -1232,14 +1232,21 @@
 
     @method simplifyState
     @param {Object} unit A service unit.
+    @param {String} life The life status of the units service.
     @return {String} the filtered agent state of the unit.
   */
-  utils.simplifyState = function(unit) {
-    var state = unit.agent_state;
-    if (state === 'started') { return 'running'; }
-    if ((/-?error$/).test(state)) { return 'error'; }
-    // "pending", "installed", and "stopped", plus anything unforeseen
-    return state;
+  utils.simplifyState = function(unit, life) {
+    var state = unit.agent_state,
+        inError = (/-?error$/).test(state);
+    if (life === 'dying' && !inError) {
+      return 'dying';
+    } else {
+      if (state === 'started') { return 'running'; }
+      if (inError) { return 'error'; }
+      // "pending", "installed", and "stopped", plus anything unforeseen
+      return state;
+    }
+
   };
 
   /**
@@ -1267,7 +1274,8 @@
 
     @method determineCategoryType
     @param {String} category The category name to test.
-    @param {String} The category type 'error', 'landscape', 'pending', 'running'
+    @return {String} The category type
+      'error', 'landscape', 'pending', 'running'
   */
   utils.determineCategoryType = function(category) {
     if ((/fail|error/).test(category)) { return 'error'; }
 
=== modified file 'app/views/viewlets/service-overview.js'
--- app/views/viewlets/service-overview.js	2013-11-21 23:21:49 +0000
+++ app/views/viewlets/service-overview.js	2013-12-02 21:33:17 +0000
@@ -83,11 +83,14 @@
     [{ type: 'service', upgradeAvailable: true, upgradeTo: ...}].
     */
   function updateStatusList(unitList) {
+    // Disable: Possible strict violation for the entire function
+    /* jshint -W040 */
     var statuses = [],
-        unitByStatus = {};
+        unitByStatus = {},
+        serviceLife = this.model.get('life');
 
     unitList.each(function(unit) {
-      var category = utils.simplifyState(unit);
+      var category = utils.simplifyState(unit, serviceLife);
       // If the unit is in error we want it's category to be it's real error.
       if (category === 'error') { category = unit.agent_state_info; }
 
@@ -112,8 +115,7 @@
         units: unitByStatus[category]
       });
     });
-    // Disable: Possible strict violation
-    /* jshint -W040 */
+
     return sortStatuses(addCharmUpgrade(statuses, this.model));
   }
 
 
=== modified file 'test/test_model.js'
--- test/test_model.js	2013-11-26 22:30:42 +0000
+++ test/test_model.js	2013-12-02 21:33:17 +0000
@@ -1034,9 +1034,9 @@
       assert.isTrue(django.isAlive());
     });
 
-    it('instances identify if they are not alive (dying or dead)', function() {
-      assert.isFalse(rails.isAlive(), rails.get('id'));
-      assert.isFalse(wordpress.isAlive(), wordpress.get('id'));
+    it('instances identify if they are not alive (dead)', function() {
+      assert.isTrue(rails.isAlive(), rails.get('id'));
+      assert.isTrue(wordpress.isAlive(), wordpress.get('id'));
       assert.isFalse(mysql.isAlive(), mysql.get('id'));
     });
 
@@ -1052,8 +1052,8 @@
 
     it('can be filtered so that it returns only visible models', function() {
       var filtered = list.visible();
-      assert.strictEqual(2, filtered.size());
-      assert.deepEqual([django, wordpress], filtered.toArray());
+      assert.strictEqual(filtered.size(), 3);
+      assert.deepEqual(filtered.toArray(), [rails, django, wordpress]);
     });
   });
 });
 
=== modified file 'test/test_service_module.js'
--- test/test_service_module.js	2013-11-21 12:18:26 +0000
+++ test/test_service_module.js	2013-12-02 21:33:17 +0000
@@ -258,7 +258,7 @@
     // There are five services in total.
     assert.strictEqual(5, db.services.size(), 'total');
     // But only three of those are actually displayed.
-    assert.strictEqual(3, Y.Object.size(boxes), 'displayed');
+    assert.strictEqual(4, Y.Object.size(boxes), 'displayed');
     // And they are the visible ones.
     assert.deepPropertyVal(boxes, 'haproxy.model', haproxy);
     assert.deepPropertyVal(boxes, 'django.model', django);