Merge lp:~hatch/juju-gui/num-units-1253113 into lp:juju-gui/experimental

[Jeff Pihach]

Fix: Removing units throws notification error

Removing units in the sandbox and in a real environment no longer
always throws an error.

https://codereview.appspot.com/36690043/

[Jeff Pihach (hatch)]

Reviewers: mp+197597_code.launchpad.net,

Message:
Please take a look.

Description:
Fix: Removing units throws notification error

Removing units in the sandbox and in a real environment no longer
always throws an error.

To QA:
Sandbox:
Deploy a service with 10 units, change it to 1 unit, it should scale
down to 1.

Real environment:
Deploy a failtester service with 3 units in the GUI
after they are up scale it down to 1, they should scale down without
notifications.

https://code.launchpad.net/~hatch/juju-gui/num-units-1253113/+merge/197597

(do not edit description out of merge proposal)

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

Affected files (+118, -7 lines):
   [revision details]
   app/store/env/fakebackend.js
   app/store/env/sandbox.js
   test/test_fakebackend.js
   test/test_sandbox_go.js

[Madison Scott-Clary (makyo)]

LGTM, QA ok!

https://codereview.appspot.com/36690043/

[Jeff Pihach (hatch)]

Thanks for the review!

https://codereview.appspot.com/36690043/

[Jeff Pihach (hatch)]

*** Submitted:

Fix: Removing units throws notification error

Removing units in the sandbox and in a real environment no longer
always throws an error.

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

https://codereview.appspot.com/36690043/

=== modified file 'app/store/env/fakebackend.js'
--- app/store/env/fakebackend.js	2013-11-25 18:52:14 +0000
+++ app/store/env/fakebackend.js	2013-12-03 20:04:11 +0000
@@ -510,13 +510,13 @@
       // default.
       var unitNames = service.get('units').get('id');
       var result = this.removeUnits(unitNames);
-      if (result.error.length > 0) {
+      if (result.error && result.error.length > 0) {
         console.log(result, result.error);
         return {
           error: 'Error removing units [' + unitNames.join(', ') +
               '] of ' + serviceName
         };
-      } else if (result.warning.length > 0) {
+      } else if (result.warning && result.warning.length > 0) {
         console.log(result, result.warning);
         return {
           error: 'Warning removing units [' + unitNames.join(', ') +
@@ -705,8 +705,7 @@
     */
     removeUnits: function(unitNames) {
       var service, removedUnit,
-          error = [],
-          warning = [];
+          warning, error;
 
       // XXX: BradCrittenden 2013-04-15: Remove units should optionally remove
       // the corresponding machines.
@@ -716,6 +715,7 @@
       Y.Array.each(unitNames, function(unitName) {
         service = this.db.services.getById(unitName.split('/')[0]);
         if (service && service.get('is_subordinate')) {
+          if (!Y.Lang.isArray(error)) { error = []; }
           error.push(unitName + ' is a subordinate, cannot remove.');
         } else {
           // For now we also need to clean up the services unit list but the
@@ -729,6 +729,7 @@
               return;
             }
           }
+          if (!Y.Lang.isArray(warning)) { warning = []; }
           warning.push(unitName + ' does not exist, cannot remove.');
         }
       }, this);

=== modified file 'app/store/env/sandbox.js'
--- app/store/env/sandbox.js	2013-11-26 02:52:25 +0000
+++ app/store/env/sandbox.js	2013-12-03 20:04:11 +0000
@@ -612,7 +612,7 @@
     */
     performOp_remove_units: function(data) {
       var res = this.get('state').removeUnits(data.unit_names);
-      if (res.error.length > 0) {
+      if (res.error && res.error.length > 0) {
         data.err = res.error;
         data.result = false;
       } else {

=== modified file 'test/test_fakebackend.js'
--- test/test_fakebackend.js	2013-11-25 18:52:14 +0000
+++ test/test_fakebackend.js	2013-12-03 20:04:11 +0000
@@ -1220,6 +1220,114 @@
 
   });
 
+  describe('FakeBackend.removeUnit', function() {
+    var requires = [
+      'node', 'juju-tests-utils', 'juju-models', 'juju-charm-models'];
+    var Y, fakebackend, utils, unitsRemoveData = '',
+        removeCalled = 0;
+
+    before(function(done) {
+      Y = YUI(GlobalConfig).use(requires, function(Y) {
+        utils = Y.namespace('juju-tests.utils');
+        done();
+      });
+    });
+
+    beforeEach(function() {
+      fakebackend = utils.makeFakeBackend();
+    });
+
+    afterEach(function() {
+      fakebackend.destroy();
+      removeCalled = 0;
+      unitsRemoveData = '';
+    });
+
+    function modifyDb(isSubordinate, unitIds) {
+      fakebackend.db.services.getById = function() {
+        return {
+          get: function(type) {
+            if (type === 'is_subordinate') { return isSubordinate; }
+            if (type === 'units') {
+              return {
+                remove: function(removeData) {
+                  unitsRemoveData += removeData;
+                  removeCalled += 1;
+                },
+                getById: function() {
+                  return unitIds;
+                }};
+            }}};
+      };
+    }
+
+    it('can remove a single unit', function() {
+      // This assumes that the addUnit tests above pass
+      fakebackend.deploy('cs:precise/wordpress-15', function() {
+        var unitId = 'wordpress/0';
+        modifyDb(false, unitId);
+        var result = fakebackend.removeUnits(unitId);
+        assert.deepEqual(result, {
+          error: undefined,
+          warning: undefined
+        });
+        assert.equal(removeCalled, 1);
+        assert.equal(unitsRemoveData, unitId);
+      });
+    });
+
+    it('can remove multiple units', function() {
+      // This assumes that the addUnit tests above pass
+      fakebackend.deploy('cs:precise/wordpress-15', function() {
+        var unitIds = ['wordpress/0', 'wordpress/1'];
+        modifyDb(false, unitIds);
+
+        var result = fakebackend.removeUnits(unitIds);
+        assert.deepEqual(result, {
+          error: undefined,
+          warning: undefined
+        });
+        assert.equal(removeCalled, 2);
+        // This join thing is kind of ugly but it's simply like this so that
+        // we can share the same simple modify method across all tests.
+        assert.equal(unitsRemoveData, unitIds.join(',') + unitIds.join(','));
+      });
+    });
+
+    it('returns an error when removing a subordinate', function() {
+      // This assumes that the addUnit tests above pass
+      fakebackend.deploy('cs:precise/wordpress-15', function() {
+        var unitId = 'wordpress/0';
+        modifyDb(true, unitId);
+        var result = fakebackend.removeUnits(unitId);
+        assert.deepEqual(result, {
+          error: [
+            'wordpress/0 is a subordinate, cannot remove.'
+          ],
+          warning: undefined
+        });
+        assert.equal(removeCalled, 0);
+        assert.equal(unitsRemoveData, '');
+      });
+    });
+
+    it('returns a warning when removing a service which doesn\'t exist',
+        function() {
+          // This assumes that the addUnit tests above pass
+          fakebackend.deploy('cs:precise/wordpress-15', function() {
+            modifyDb(false, null);
+            var result = fakebackend.removeUnits('wordpress/0');
+            assert.deepEqual(result, {
+              error: undefined,
+              warning: [
+                'wordpress/0 does not exist, cannot remove.'
+              ]
+            });
+          });
+        });
+
+  });
+
   describe('FakeBackend.next*', function() {
     var requires = [
       'node', 'juju-tests-utils', 'juju-models', 'juju-charm-models'];

=== modified file 'test/test_sandbox_go.js'
--- test/test_sandbox_go.js	2013-11-26 19:51:23 +0000
+++ test/test_sandbox_go.js	2013-12-03 20:04:11 +0000
@@ -478,7 +478,7 @@
           assert.equal(receivedData.RequestId, data.RequestId);
           // fakebackend defaults error and warning to [] which carries
           // through.
-          assert.deepEqual(receivedData.Error, []);
+          assert.isUndefined(receivedData.Error);
           assert.equal(state.db.services.item(0).get('units').size(), 0);
           done();
         };
@@ -493,7 +493,7 @@
         var callback = function(result) {
           // fakebackend defaults error and warning to [] which carries
           // through.
-          assert.deepEqual(result.err, []);
+          assert.isUndefined(result.err);
           assert.equal(state.db.services.item(0).get('units').size(), 0);
           done();
         };