Code review comment for lp:~hatch/juju-gui/save-changed-1214087

Reviewers: mp+183048_code.launchpad.net,

Message:
Please take a look.

Description:
Only sends the changed config options.

1) There was a bug with the ghost constraints code which
    caused it to fail when deploying services on rapi.

2) The GUI now only sends configuration values to juju when
    those values differ from the defaults.

To QA: Deploy charms on sandbox, rapi, juju-core. Modifying and
        not modifying the default constraints and configuration.
        Then open the inspector and check to make sure the settings
        are appropriate and that the constraints page shows the
        proper constraints.

https://code.launchpad.net/~hatch/juju-gui/save-changed-1214087/+merge/183048

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M app/views/ghost-inspector.js
   M app/views/inspector.js
   M app/views/viewlets/service-ghost.js
   M test/test_ghost_inspector.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_ghost_inspector.js
=== modified file 'test/test_ghost_inspector.js'
--- test/test_ghost_inspector.js 2013-08-27 16:30:39 +0000
+++ test/test_ghost_inspector.js 2013-08-29 20:39:26 +0000
@@ -139,11 +139,7 @@
      var message = env.ws.last_message();
      var params = message.Params;
      var config = {
- admins: '',
- debug: 'false',
- logo: '',
- name: 'foo',
- skin: 'vector'
+ name: 'foo'
      };
      assert.equal('ServiceDeploy', message.Request);
      assert.equal('mediawiki', params.ServiceName);
@@ -189,7 +185,7 @@
      vmContainer.one('.viewlet-manager-footer
button.confirm').simulate('click');

      var message = env.ws.last_message();
- assert.equal(message.constraints.cpu, '2');
+ assert.deepEqual(message.constraints, ['cpu=2', 'mem=', 'arch=']);
    });

    it('deploys with constraints in go env', function() {
@@ -204,7 +200,8 @@
      vmContainer.one('.viewlet-manager-footer
button.confirm').simulate('click');

      var message = env.ws.last_message();
- assert.equal(message.Params.Constraints['cpu-power'], '2');
+ assert.deepEqual(message.Params.Constraints,
+ ['cpu-power=2', 'cpu-cores=', 'mem=', 'arch=']);
    });

    it('disables and resets input fields when \'use default config\' is
active',

Index: app/views/ghost-inspector.js
=== modified file 'app/views/ghost-inspector.js'
--- app/views/ghost-inspector.js 2013-08-26 14:16:34 +0000
+++ app/views/ghost-inspector.js 2013-08-29 20:39:26 +0000
@@ -121,9 +121,30 @@
              container, '.service-config .config-field');
        }

+ var charmOptions = options.model.get('options');
+ /*jshint -W089 */
+ // Tells jshint to ignore the lack of hasOwnProperty in forloops
+ for (var cfgOption in config) {
+ // Remove config options which are not different from the charm
defaults
+ // Intentionally letting the browser do the type coersion.
+ // The || check is to allow empty inputs to match an undefined
default
+ /* jshint -W116 */
+ if (config[cfgOption] == (charmOptions[cfgOption].default || '')) {
+ delete config[cfgOption];
+ }
+ /* jshint +W116 */
+ }
+
        // Deploy needs constraints in simple key:value object.
- var constraints = utils.getElementsValuesMapping(
+ var constraintMap = utils.getElementsValuesMapping(
            container, '.constraint-field');
+ // Format the constraints into the proper format.
+ var constraints = [];
+ /*jshint -W089 */
+ // Tells jshint to ignore the lack of hasOwnProperty in forloops
+ for (var constr in constraintMap) {
+ constraints.push(constr + '=' + constraintMap[constr]);
+ }

        options.env.deploy(
            model.get('id'),

Index: app/views/inspector.js
=== modified file 'app/views/inspector.js'
--- app/views/inspector.js 2013-08-29 17:10:19 +0000
+++ app/views/inspector.js 2013-08-29 22:10:02 +0000
@@ -734,7 +734,21 @@

        var newVals =
utils.getElementsValuesMapping(container, '.config-field');
        var errors = utils.validate(newVals, schema);
+
        if (Y.Object.isEmpty(errors)) {
+ /*jshint -W089 */
+ // Tells jshint to ignore the lack of hasOwnProperty in forloops
+ for (var cfgOption in newVals) {
+ // Remove config options which are not different from the charm
defaults
+ // Intentionally letting the browser do the type coersion.
+ // The || check is to allow empty inputs to match an undefined
default
+ /* jshint -W116 */
+ if (newVals[cfgOption] == (schema[cfgOption].default || '')) {
+ delete newVals[cfgOption];
+ }
+ /* jshint +W116 */
+ }
+
          env.set_config(
              service.get('id'),
              newVals,

Index: app/views/viewlets/service-ghost.js
=== modified file 'app/views/viewlets/service-ghost.js'
--- app/views/viewlets/service-ghost.js 2013-08-26 17:03:22 +0000
+++ app/views/viewlets/service-ghost.js 2013-08-29 20:07:54 +0000
@@ -41,6 +41,11 @@
            var newVal = (val['default'] === undefined) ? '' :
val['default'];
            node.set('value', newVal);
          }
+ },
+ 'constraints': {
+ 'update': function(node, val) {
+ node.set('value', val || '');
+ }
        }
      },
      'render': function(model, viewletMgrAttrs) {