Merge lp:~gary/juju-gui/fixConflictResolution into lp:juju-gui/experimental

[Gary Poster]

Fix conflict resolution

Prior to this branch, conflict resolution did not play nicely with our new functionality to show modified fields and to hide/reveal save controls as appropriate. Specifically, these were two problems.

* If you set a model value but choose the user's value, the user's value will not be seen as a modified value.
* If you resolve a conflict that changes the DOM to match the model, the save dialog should disappear but does not.

This branch addresses these issues entirely within the databinding code.

In addition, I added a test of basic conflict handling. I cleaned up _getBinding further from my most recent branch. I bound the "resolve" method with the known arguments to make client code simpler.

https://codereview.appspot.com/13373052/

[Gary Poster (gary)]

Reviewers: mp+184916_code.launchpad.net,

Message:
Please take a look.

Description:
Fix conflict resolution

Prior to this branch, conflict resolution did not play nicely with our
new functionality to show modified fields and to hide/reveal save
controls as appropriate. Specifically, these were two problems.

* If you set a model value but choose the user's value, the user's value
will not be seen as a modified value.
* If you resolve a conflict that changes the DOM to match the model, the
save dialog should disappear but does not.

This branch addresses these issues entirely within the databinding code.

In addition, I added a test of basic conflict handling. I cleaned up
_getBinding further from my most recent branch. I bound the "resolve"
method with the known arguments to make client code simpler.

To QA, make a service and then open the inspector to the constraints
tab. Change the "CPU cores" field. Then in the console run something
like this (assuming that you did not type "8" in the CPU cores field:

app.db.services.item(0).set('constraints', {'cpu-cores': 8})

Select the second value (8) in the conflict resolution UX. The field
should be marked as unmodified and the save dialog should disappear.

Now change the CPU cores field to 4. In the console, set it the cores
to 6:

app.db.services.item(0).set('constraints', {'cpu-cores': 6})

Select the first value (4) in the conflict resolution UX. The conflict
marker should disappear but it should be replaced with the "modified"
asterisk, and the save button should still be visible (though it is
called "Overwrite," which is still technically accurate).

There are other edge cases I'm still planning on addressing in upcoming
branches, but these two are working, so I'm proposing the branch.

https://code.launchpad.net/~gary/juju-gui/fixConflictResolution/+merge/184916

(do not edit description out of merge proposal)

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

Affected files (+109, -25 lines):
   A [revision details]
   M app/views/databinding.js
   M app/views/inspector.js
   M test/test_databinding.js

[Jeff Pihach (hatch)]

Love the new interactions!

LGTM QA OK
IE OK

https://codereview.appspot.com/13373052/diff/1/app/views/databinding.js
File app/views/databinding.js (right):

https://codereview.appspot.com/13373052/diff/1/app/views/databinding.js#newcode814
app/views/databinding.js:814: Y.bind(resolve, self, binding.target,
binding.viewlet.name),
Nice cleanup!

https://codereview.appspot.com/13373052/

[Gary Poster (gary)]

*** Submitted:

Fix conflict resolution

Prior to this branch, conflict resolution did not play nicely with our
new functionality to show modified fields and to hide/reveal save
controls as appropriate. Specifically, these were two problems.

* If you set a model value but choose the user's value, the user's value
will not be seen as a modified value.
* If you resolve a conflict that changes the DOM to match the model, the
save dialog should disappear but does not.

This branch addresses these issues entirely within the databinding code.

In addition, I added a test of basic conflict handling. I cleaned up
_getBinding further from my most recent branch. I bound the "resolve"
method with the known arguments to make client code simpler.

R=jeff.pihach
CC=
https://codereview.appspot.com/13373052

https://codereview.appspot.com/13373052/

=== modified file 'app/views/databinding.js'
--- app/views/databinding.js	2013-09-11 02:09:14 +0000
+++ app/views/databinding.js	2013-09-11 03:35:27 +0000
@@ -664,22 +664,21 @@
       Find the binding for the given key (binding name).
 
       @method _getBinding
-      @param {String} key Binding key.
       @param {Y.Node} node The binding's target node.
       @return {Binding} Binding reference.
     */
-    BindingEngine.prototype._getBinding = function(key, node) {
+    BindingEngine.prototype._getBinding = function(node) {
       var binding;
-      if ( // Find the binding for the key, and break when found.
+      if ( // Find the binding for the node, and break when found.
           !this._bindings.some(function(b) {
-            if (b.name === key && b.target === node) {
+            if (b.target === node) {
               binding = b;
               return true;
             }})) {
         // We don't expect this to ever happen.  We should only be asking for
         // a key that has been bound.  If this does fail, let's be loud about
         // it ASAP so that we can fix it.
-        throw 'Programmer error: no binding found for ' + key;
+        throw 'Programmer error: no binding found for node';
       }
       return binding;
     };
@@ -708,7 +707,7 @@
       var key = node.getData('bind');
       var nodeHandler = this.getNodeHandler(node.getDOMNode());
       var model = viewlet.model;
-      var binding = this._getBinding(key, node);
+      var binding = this._getBinding(node);
       if (nodeHandler.eq(node, binding.get(model))) {
         delete viewlet.changedValues[key];
       } else {
@@ -812,7 +811,8 @@
           viewlet.unsyncedFields();
           binding.viewlet.conflict(
               binding.target, viewletModel, binding.viewlet.name,
-              Y.bind(resolve, self), binding);
+              Y.bind(resolve, self, binding.target, binding.viewlet.name),
+              binding);
         }
 
         var value = binding.get(viewletModel);
@@ -863,17 +863,19 @@
       @param {Any} value that the user has accepted to resolve with.
     */
     BindingEngine.prototype.resolve = function(node, viewletName, value) {
-      var key = node.getData('bind'),
-          viewlet = this._viewlets[viewletName];
-
-      delete viewlet.changedValues[key];
-      var field = this.getNodeHandler(node.getDOMNode());
-      field.set.call(this, node, value);
-      // If there are no more changed values then tell the
-      // the viewlet to update accordingly
-      if (Object.keys(viewlet.changedValues).length === 0) {
-        viewlet.syncedFields();
+      var nodeHandler = this.getNodeHandler(node.getDOMNode());
+      if (!nodeHandler.eq(node, value)) {
+        nodeHandler.set(node, value);
       }
+      // Case 1:
+      // The user chose the node value. It is still modified, so let the
+      // viewlet have a chance to reflect this again, using _nodeChanged.
+      // Case 2:
+      // The user chose the model value, or some other value.  Let
+      // _nodeChanged have a chance to update changedValues and possibly call
+      // syncedFields.
+      var viewlet = this._viewlets[viewletName];
+      this._nodeChanged(node, viewlet);
     };
 
     /**

=== modified file 'app/views/inspector.js'
--- app/views/inspector.js	2013-09-11 00:26:32 +0000
+++ app/views/inspector.js	2013-09-11 03:35:27 +0000
@@ -1229,9 +1229,9 @@
         resolver.addClass('hidden');
 
         if (e.currentTarget.hasClass('conflicted-env')) {
-          resolve(node, viewletName, modelValue);
+          resolve(modelValue);
         } else {
-          resolve(node, viewletName, formValue);
+          resolve(formValue);
         }
       }
 

=== modified file 'test/test_databinding.js'
--- test/test_databinding.js	2013-09-11 02:09:14 +0000
+++ test/test_databinding.js	2013-09-11 03:35:27 +0000
@@ -24,11 +24,21 @@
     container = utils.makeContainer();
     container.setHTML(input);
     viewlet = Object.create({
+      name: 'testViewlet',
       container: container,
       changedValues: {},
       _eventHandles: [],
       syncedFields: function() {
         this.calledSyncedFields = true;
+      },
+      unsyncedFields: function() {
+        this.calledUnsyncedFields = true;
+      },
+      conflict: function() {
+        this.conflictArgs = Array.prototype.slice.call(arguments);
+      },
+      changed: function() {
+        this.changedArgs = Array.prototype.slice.call(arguments);
       }
     });
     engine = new BindingEngine({interval: 0});
@@ -731,9 +741,9 @@
       var nodeA1 = container.one('textarea[data-bind="a"]');
       var nodeA2 = container.one('input[data-bind="a"]');
       var nodeB = container.one('[data-bind="b"]');
-      var bindingA1 = engine._getBinding('a', nodeA1);
-      var bindingA2 = engine._getBinding('a', nodeA2);
-      var bindingB = engine._getBinding('b', nodeB);
+      var bindingA1 = engine._getBinding(nodeA1);
+      var bindingA2 = engine._getBinding(nodeA2);
+      var bindingB = engine._getBinding(nodeB);
       assert.equal(bindingA1.name, 'a');
       assert.strictEqual(bindingA1.target, nodeA1);
       assert.equal(bindingA2.name, 'a');
@@ -743,10 +753,80 @@
     });
 
     it('should throw an error when a binding is not found', function() {
-      var nodeA1 = container.one('textarea[data-bind="a"]');
       assert.throws(
-          function() {engine._getBinding('c', nodeA1);},
-          'Programmer error: no binding found for c');
+          function() {engine._getBinding(container);},
+          'Programmer error: no binding found for node');
+    });
+  });
+
+  describe('_updateDOM tests', function() {
+    var model, node;
+
+    beforeEach(function() {
+      model = new Y.Model({a: undefined});
+      generateEngine(
+          '<textarea data-bind="a"></textarea>');
+      engine.bind(model, viewlet);
+      node = container.one('[data-bind="a"]');
+    });
+
+    it('reports conflicts', function() {
+      node.set('value', 'kumquat');
+      engine._nodeChanged(node, viewlet);
+      assert.deepEqual(viewlet.changedValues, {a: true});
+      assert.isUndefined(viewlet.conflictArgs);
+      model.set('a', 'rutebega');
+      // We have a conflict!
+      assert.isTrue(viewlet.calledUnsyncedFields);
+      assert.isDefined(viewlet.conflictArgs);
+      assert.strictEqual(viewlet.conflictArgs[0], node);
+      assert.strictEqual(viewlet.conflictArgs[1], model);
+      assert.strictEqual(viewlet.conflictArgs[2], 'testViewlet');
+      // argument 3 should be the bound resolve function.
+      // We'll assert this by showing that it works.
+      viewlet.conflictArgs[3]('rutebega');
+      assert.equal(node.get('value'), 'rutebega');
+      assert.strictEqual(viewlet.conflictArgs[4],
+                         engine._getBinding(node));
+    });
+
+  });
+
+  describe('resolve tests', function() {
+    var model, node;
+
+    beforeEach(function() {
+      model = new Y.Model({a: undefined});
+      generateEngine(
+          '<textarea data-bind="a"></textarea>');
+      engine.bind(model, viewlet);
+      node = container.one('[data-bind="a"]');
+      node.set('value', 'kumquat');
+      engine._nodeChanged(node, viewlet);
+      model.set('a', 'rutebega');
+      // Now we have a conflict.
+      delete viewlet.changedArgs;
+    });
+
+    afterEach(function() {
+      container.remove().destroy(true);
+      model.destroy(true);
+    });
+
+    it('resolves to model value', function() {
+      engine.resolve(node, viewlet.name, 'rutebega');
+      assert.equal(node.get('value'), 'rutebega');
+      assert.deepEqual(viewlet.changedValues, {});
+      assert.isDefined(viewlet.changedArgs);
+      assert.isTrue(viewlet.calledSyncedFields);
+    });
+
+    it('resolves to input value', function() {
+      engine.resolve(node, viewlet.name, 'kumquat');
+      assert.equal(node.get('value'), 'kumquat');
+      assert.deepEqual(viewlet.changedValues, {a: true});
+      assert.isDefined(viewlet.changedArgs);
+      assert.isUndefined(viewlet.calledSyncedFields);
     });
   });