Merge lp:~rharding/juju-gui/fix-file-dnd into lp:juju-gui/experimental

[Richard Harding]

Fix bad call to bundle import helpers.

- Fixes call to drag and drop of deployer files
- Adds missing test for the if branch that called the bundle import helper
function.

https://codereview.appspot.com/29120043/

[Richard Harding (rharding)]

Reviewers: mp+195826_code.launchpad.net,

Message:
Please take a look.

Description:
Fix bad call to bundle import helpers.

- Fixes call to drag and drop of deployer files
- Adds missing test for the if branch that called the bundle import
helper
function.

https://code.launchpad.net/~rharding/juju-gui/fix-file-dnd/+merge/195826

(do not edit description out of merge proposal)

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

Affected files (+31, -4 lines):
   A [revision details]
   M app/assets/javascripts/bundle-import-helpers.js
   M app/views/topology/service.js
   M test/test_bundle_import_helpers.js
   M test/test_service_module.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_bundle_import_helpers.js
=== modified file 'test/test_bundle_import_helpers.js'
--- test/test_bundle_import_helpers.js 2013-11-18 18:25:08 +0000
+++ test/test_bundle_import_helpers.js 2013-11-19 16:54:54 +0000
@@ -127,7 +127,7 @@
        // the look in the watch.
        db.notifications.add = function(info) {
          if (callNumber === 0) {
- assert.equal('Updated status for deployment: 42', info.title);
+ assert.equal('Updated status for deployment id: 42', info.title);
            assert.equal(info.level, 'important');
            assert.isTrue(info.message.indexOf('scheduled') !== -1,
info.message);
          } else {

Index: test/test_service_module.js
=== modified file 'test/test_service_module.js'
--- test/test_service_module.js 2013-11-15 13:52:53 +0000
+++ test/test_service_module.js 2013-11-19 17:00:32 +0000
@@ -340,4 +340,29 @@
      serviceModule.set('component', view.topo);
      serviceModule.canvasDropHandler(fakeEventObject);
    });
+
+ it('should deploy a bundle on file drop events', function(done) {
+ var fakeEventObject = {
+ halt: function() {},
+ _event: {
+ dataTransfer: {
+ // All we need to fake things out is to have a file.
+ files: [1]
+ }
+ }
+ };
+
+ // mock out the Y.BundleHelpers call.
+ var _deployBundle = juju.BundleHelpers.deployBundleFiles;
+ juju.BundleHelpers.deployBundleFiles = function(files, env, db) {
+ assert.deepEqual(files, [1]);
+ // Restore the deployBundleFiles call for future tests.
+ juju.BundleHelpers.deployBundleFiles = _deployBundle;
+ done();
+ };
+
+ serviceModule.set('component', view.topo);
+ serviceModule.canvasDropHandler(fakeEventObject);
+ });
+
  });

Index: app/assets/javascripts/bundle-import-helpers.js
=== modified file 'app/assets/javascripts/bundle-import-helpers.js'
--- app/assets/javascripts/bundle-import-helpers.js 2013-11-18 17:45:56
+0000
+++ app/assets/javascripts/bundle-import-helpers.js 2013-11-19 16:54:54
+0000
@@ -213,7 +213,7 @@
            // The change could be the result of an error.
            if (newChange.Error !== undefined) {
              notification...

[Richard Harding (rharding)]

Reviewer comments added.

https://codereview.appspot.com/29120043/diff/1/app/assets/javascripts/bundle-import-helpers.js
File app/assets/javascripts/bundle-import-helpers.js (right):

https://codereview.appspot.com/29120043/diff/1/app/assets/javascripts/bundle-import-helpers.js#newcode216
app/assets/javascripts/bundle-import-helpers.js:216: title: 'Updated
status for deployment id: ' +
drive by suggestion from another dev to make this more explicit.

https://codereview.appspot.com/29120043/

[Jeff Pihach (hatch)]

Code LGTM

QA NOT OK

When dragging and dropping a bundle yaml file I get the following error:

Uncaught TypeError: Property 'handleDeployerWatch' of object
#<GoJujuAPI> is not a function sandbox.js:800
Y.extend.receive sandbox.js:800
Y.extend.send sandbox.js:114
Y.extend._send_rpc go.js:195
Y.extend.deployerWatch go.js:521
ns.BundleHelpers._watchDeployment bundle-import-helpers.js:161
(anonymous function) bundle-import-helpers.js:126
Y.extend.handleDeployerImport go.js:460
(anonymous function) oop-debug.js:378
Y.extend.dispatch_result go.js:167
CEProto.fireComplex event-custom-complex-debug.js:340
Y.CustomEvent._fire event-custom-base-debug.js:1068
ET.fire event-custom-base-debug.js:2230
Y.extend.on_message base.js:251
(anonymous function) oop-debug.js:378
Y.extend.receiveNow sandbox.js:83
(anonymous function)

https://codereview.appspot.com/29120043/

[Jeff Pihach (hatch)]

QA OK on real environment.

Previous QA failure is being fixed in another branch.

https://codereview.appspot.com/29120043/

[Richard Harding (rharding)]

*** Submitted:

Fix bad call to bundle import helpers.

- Fixes call to drag and drop of deployer files
- Adds missing test for the if branch that called the bundle import
helper
function.

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

https://codereview.appspot.com/29120043/

=== modified file 'app/assets/javascripts/bundle-import-helpers.js'
--- app/assets/javascripts/bundle-import-helpers.js	2013-11-18 17:45:56 +0000
+++ app/assets/javascripts/bundle-import-helpers.js	2013-11-19 17:03:22 +0000
@@ -213,7 +213,7 @@
           // The change could be the result of an error.
           if (newChange.Error !== undefined) {
             notifications.add({
-              title: 'Updated status for deployment: ' +
+              title: 'Updated status for deployment id: ' +
                   newChange.DeploymentId,
               message: 'The deployment errored: ' +
                   newChange.Error,
@@ -221,7 +221,7 @@
             });
           } else {
             notifications.add({
-              title: 'Updated status for deployment: ' +
+              title: 'Updated status for deployment id: ' +
                   newChange.DeploymentId,
               message: 'The deployment is currently: ' +
                   newChange.Status,

=== modified file 'app/views/topology/service.js'
--- app/views/topology/service.js	2013-11-15 13:46:35 +0000
+++ app/views/topology/service.js	2013-11-19 17:03:22 +0000
@@ -640,7 +640,7 @@
       var env = topo.get('env');
       var db = topo.get('db');
       if (fileSources && fileSources.length) {
-        importHelpers.sendToDeployer(fileSources, env, db);
+        importHelpers.deployBundleFiles(fileSources, env, db);
       } else {
         // Handle dropping charm/bundle tokens from the left side bar.
         var dragData = JSON.parse(dataTransfer.getData('Text'));

=== modified file 'test/test_bundle_import_helpers.js'
--- test/test_bundle_import_helpers.js	2013-11-18 18:25:08 +0000
+++ test/test_bundle_import_helpers.js	2013-11-19 17:03:22 +0000
@@ -127,7 +127,7 @@
       // the look in the watch.
       db.notifications.add = function(info) {
         if (callNumber === 0) {
-          assert.equal('Updated status for deployment: 42', info.title);
+          assert.equal('Updated status for deployment id: 42', info.title);
           assert.equal(info.level, 'important');
           assert.isTrue(info.message.indexOf('scheduled') !== -1, info.message);
         } else {

=== modified file 'test/test_service_module.js'
--- test/test_service_module.js	2013-11-15 13:52:53 +0000
+++ test/test_service_module.js	2013-11-19 17:03:22 +0000
@@ -340,4 +340,29 @@
     serviceModule.set('component', view.topo);
     serviceModule.canvasDropHandler(fakeEventObject);
   });
+
+  it('should deploy a bundle on file drop events', function(done) {
+    var fakeEventObject = {
+      halt: function() {},
+      _event: {
+        dataTransfer: {
+          // All we need to fake things out is to have a file.
+          files: [1]
+        }
+      }
+    };
+
+    // mock out the Y.BundleHelpers call.
+    var _deployBundle = juju.BundleHelpers.deployBundleFiles;
+    juju.BundleHelpers.deployBundleFiles = function(files, env, db) {
+      assert.deepEqual(files, [1]);
+      // Restore the deployBundleFiles call for future tests.
+      juju.BundleHelpers.deployBundleFiles = _deployBundle;
+      done();
+    };
+
+    serviceModule.set('component', view.topo);
+    serviceModule.canvasDropHandler(fakeEventObject);
+  });
+
 });