Code review comment for lp://staging/~gary/juju-gui/core-sub-num-units

Reviewers: mp+176682_code.launchpad.net,

Message:
Please take a look.

Description:
Subordinates should not be deployed with unit ct.

Deploying a subordinate with a unit count causes an error in core. This
prevents one from entering a unit count in the charm panel (did not
touch ghost
inspector, that may also need it) and defaults to 0. This also affects
fakebackend.

https://code.launchpad.net/~gary/juju-gui/core-sub-num-units/+merge/176682

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M app/store/env/fakebackend.js
   M app/templates/charm-pre-configuration.handlebars
   M app/views/charm-panel.js
   M test/test_fakebackend.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_fakebackend.js
=== modified file 'test/test_fakebackend.js'
--- test/test_fakebackend.js 2013-07-09 15:11:25 +0000
+++ test/test_fakebackend.js 2013-07-24 13:06:18 +0000
@@ -168,7 +168,7 @@
        assert.isUndefined(charm.get('is_subordinate'));
        // The _set forces a change to a writeOnly attribute.
        charm._set('is_subordinate', true);
- fakebackend.deploy('cs:wordpress', callback);
+ fakebackend.deploy('cs:wordpress', callback, {unitCount: 0});
        assert.isUndefined(result.error);
        assert.strictEqual(
            fakebackend.db.charms.getById('cs:precise/wordpress-10'), charm);
@@ -859,6 +859,25 @@
            'Invalid number of units.');
      });

+ it('returns error for invalid number of subordinate units', function()
{
+ fakebackend.deploy('cs:puppet', callback);
+ assert.isUndefined(deployResult.error);
+ assert.equal(
+ fakebackend.addUnit('puppet', 'goyesca').error,
+ 'Invalid number of units.');
+ assert.equal(
+ fakebackend.addUnit('puppet', 1).error,
+ 'Invalid number of units.');
+ assert.equal(
+ fakebackend.addUnit('puppet', -1).error,
+ 'Invalid number of units.');
+ // It also ignores empty requests
+ assert.isUndefined(
+ fakebackend.addUnit('puppet', 0).error);
+ assert.isUndefined(
+ fakebackend.addUnit('puppet').error);
+ });
+
      it('returns an error if the service does not exist.', function() {
        assert.equal(
            fakebackend.addUnit('foo').error,

Index: app/templates/charm-pre-configuration.handlebars
=== modified file 'app/templates/charm-pre-configuration.handlebars'
--- app/templates/charm-pre-configuration.handlebars 2013-07-17 19:25:41
+0000
+++ app/templates/charm-pre-configuration.handlebars 2013-07-24 13:03:35
+0000
@@ -26,18 +26,22 @@
            </div>
          </div>
        </div>
- <div class="charm-entry">
- <div class="control-group">
- <div class="well control-description">
- Number of units to deploy for this service.
- </div>
- <div class="control-label" for="number-units">Number of
units</div>
- <div class="controls">
- <input class="config-field" type="text" name="number-units"
- id="number-units" value="1"/>
+ {{#if is_subordinate}}
+ <input type="hidden" id="number-units" value="0" />
+ {{else}}
+ <div class="charm-entry">
+ <div class="control-group">
+ <div class="well control-description">
+ Number of units to deploy for this service.
+ </div>
+ <div class="control-label" for="number-units">Number of
units</div>
+ <div class="controls">
+ <input class="config-field" type="text" name="number-units"
+ id="number-units" value="1"/>
+ </div>
            </div>
          </div>
- </div>
+ {{/if}}
        {{/with}}

        {{#if settings}}

Index: app/views/charm-panel.js
=== modified file 'app/views/charm-panel.js'
--- app/views/charm-panel.js 2013-07-16 22:05:06 +0000
+++ app/views/charm-panel.js 2013-07-24 03:22:41 +0000
@@ -629,11 +629,11 @@
            if (this.configFileContent) {
              config = null;
            }
- numUnits = parseInt(numUnits, 10);
+ numUnits = charm.get('is_subordinate') ? 0 : parseInt(numUnits,
10);
            env.deploy(url, serviceName, config, this.configFileContent,
                numUnits, function(ev) {
                  if (ev.err) {
- console.log(url + ' deployment failed');
+ console.log(url + ' deployment failed', ev.err);
                    db.notifications.add(
                        new models.Notification({
                          title: 'Error deploying ' + serviceName,

Index: app/store/env/fakebackend.js
=== modified file 'app/store/env/fakebackend.js'
--- app/store/env/fakebackend.js 2013-07-11 14:56:15 +0000
+++ app/store/env/fakebackend.js 2013-07-24 13:03:35 +0000
@@ -600,19 +600,19 @@
        if (!this.get('authenticated')) {
          return UNAUTHENTICATED_ERROR;
        }
+ var service = this.db.services.getById(serviceName);
+ if (!service) {
+ return {error: 'Service "' + serviceName + '" does not exist.'};
+ }
+ var is_subordinate = service.get('subordinate');
        if (Y.Lang.isUndefined(numUnits)) {
- numUnits = 1;
+ numUnits = is_subordinate ? 0 : 1;
        }
- if (!Y.Lang.isNumber(numUnits) || numUnits < 1) {
+ if (!Y.Lang.isNumber(numUnits) ||
+ (!is_subordinate && numUnits < 1 ||
+ (is_subordinate && numUnits !== 0))) {
          return {error: 'Invalid number of units.'};
        }
- var service = this.db.services.getById(serviceName);
- if (!service) {
- return {error: 'Service "' + serviceName + '" does not exist.'};
- }
- if (service && service.get('is_subordinate')) {
- return {error: serviceName + ' is a subordinate, cannot add
unit(s).'};
- }
        if (!Y.Lang.isValue(service.unitSequence)) {
          service.unitSequence = 0;
        }