Add support for watching progress of bundle import

Add support for watching progress of bundle import

- Change the bundle-import-extension to a helpers lib that doesn't need to be
mixed in. It didn't need any state info.
- Move everyone to going through the single deployBundle function in the
helpers vs their own calls to the env.deployerImport
- Provide the ability to start a watch on a deployment.
- Add the methods to update with new notifications on each change to the watch
status of the import.
- Start a new test file for the bundle-import-helpers tests
- Remove the dupe idea of adding import notifications from the view utils,
it's part of the helpers.

QA
----

There are four QA methods that need testing as there are four ways to deploy
a bundle in the code. In order to QA you need to bring up a live environment.
I suggest that you use `juju quickstart` without any parameters to bring up
the environment.

Once it's up, you need to change the source to this branch with:

juju set juju-gui juju-gui-source='lp:~rharding/juju-gui/bundle-progress'

Now this takes a while. I found the best way to monitor it was to

juju ssh juju-gui/0
watch 'ps aux | grep config'

You then need to wait for the config-changed hook to finish running.

Once the config change hook is done, you can start to QA the four methods of
deploying a bundle.


1. Search for 'jorge' in the browser and drag-n-drop the discource bundle onto
the canvas.
2. Search for 'jorge' and click on the discourse bundle to view the details.
Then click on the deploy button and press "Yes I'm sure".
3. Use the import icon to import a local bundle file.
4. Drag-n-Drop a bundle file onto the canvas.

In each test case, what you're watching for are notifications that the bundle
has been requested, started, and completed. If the bundle is large the final
step might take a while.

Let me know if you've got any questions or issues qa'ing.

https://code.launchpad.net/~rharding/juju-gui/bundle-progress/+merge/195084

(do not edit description out of merge proposal)

[rharding, 2013-11-14 16:08:53]

Please take a look.

[rharding, 2013-11-14 19:33:04]

Please take a look.

[rharding, 2013-11-14 19:35:58]

Please take a look.

[rharding, 2013-11-14 20:12:23]

Please take a look.

[rharding, 2013-11-14 20:17:25]

Please take a look.

[rharding, 2013-11-14 21:07:44]

Please take a look.

[jeff.pihach, 2013-11-14 21:30:52]

This is looking really good I have some small comments mostly related to naming
but I think that we need some integration tests for the go sandbox.

Do we have a story for the fakebackend for this functionality or if it's simply
supposed to go unanswered?

I'm curious as to why we can't initiate a watch rpc call and then have the
guiserver send fake deltas down for us to react to, this would get rid of
calling 'next' and essentially long-polling for updates. I'm sure there is a
good reason for it I just don't know what it is :)

https://codereview.appspot.com/26290043/diff/60001/app/app.js
File app/app.js (right):

https://codereview.appspot.com/26290043/diff/60001/app/app.js#newcode43
app/app.js:43: importHelpers = juju.BundleImport;
I'd probably change this to be something like `bundleHelpers`

https://codereview.appspot.com/26290043/diff/60001/app/app.js#newcode583
app/app.js:583: env,
this.env and this.db are available here because the callback is scoped to app.js
so you don't need the references above

https://codereview.appspot.com/26290043/diff/60001/app/app.js#newcode598
app/app.js:598: cfg.deployBundle = function(bundle) {
if you use .bind() for this function you do not need to maintain the reference
above for env, db

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

https://codereview.appspot.com/26290043/diff/60001/app/assets/javascripts/bun...
app/assets/javascripts/bundle-import-helpers.js:55:
ns.BundleImport._watchDeployment(result.DeploymentId, env, db);
When I see utility methods that are directly tied together this it makes me
think this is a class trying really hard to be a utility object. Everything in
this object is directly related to a single instance but instead of relying on
that instance everything needs to be passed around. I don't see the advantage to
this structure vs relying on the primary instance. 

For testing you could unit test the class simply by creating an instance of it
with stubbed env/db like you do now. If you didn't want to create a new class
this would be handled by converting it to an extension and then testing would be
the same technique as prev mentioned.

https://codereview.appspot.com/26290043/diff/60001/app/assets/javascripts/bun...
app/assets/javascripts/bundle-import-helpers.js:105:
ns.BundleImport.deployBundle(
I know this is repurposed but it feels like we are duplicating work here just to
supply different notification messages. I'd try to move these to be handled in
the deployBundle method, or offload both notifications to a separate
showNotification(file Boolean) {} method

https://codereview.appspot.com/26290043/diff/60001/app/assets/javascripts/bun...
app/assets/javascripts/bundle-import-helpers.js:188:
_processWatchDeploymentUpdates: function(watchId, env, db) {
this is really a watchDeploymentUpdates method

https://codereview.appspot.com/26290043/diff/60001/app/store/env/go.js
File app/store/env/go.js (right):

https://codereview.appspot.com/26290043/diff/60001/app/store/env/go.js#newcod...
app/store/env/go.js:528: Wrapper for the deployerWatch call.
I don't think this description is representative of what this fn actually is
doing.

https://codereview.appspot.com/26290043/diff/60001/test/test_service_module.js
File test/test_service_module.js (right):

https://codereview.appspot.com/26290043/diff/60001/test/test_service_module.j...
test/test_service_module.js:332: juju.BundleImport.deployBundle =
function(deployerData, env, db) {
This should probably be set back to it's original value avoid any oddities in
future tests.

https://codereview.appspot.com/26290043/diff/90001/test/test_bundle_import_he...
File test/test_bundle_import_helpers.js (right):

https://codereview.appspot.com/26290043/diff/90001/test/test_bundle_import_he...
test/test_bundle_import_helpers.js:82: assert.equal(info.level, 'important');
There is no guarantee that this will be hit you could add a flag which gets set
to 1 when this is called and then check it in the same callback where your
done() is

[rharding, 2013-11-15 12:16:08]

On 2013/11/14 21:30:52, jeff.pihach wrote:
> This is looking really good I have some small comments mostly related to
naming
> but I think that we need some integration tests for the go sandbox.

Thanks, I should have brought this up. The next card is to figure out how to
implement this in the sandbox. This branch is purely about making it work for
users in a live environment.

> 
> Do we have a story for the fakebackend for this functionality or if it's
simply
> supposed to go unanswered?

That is for the follow up card. My first instinct is to not show all of these
notifications to the sandbox user, but only the final "Competed" one. 


> I'm curious as to why we can't initiate a watch rpc call and then have the
> guiserver send fake deltas down for us to react to, this would get rid of
> calling 'next' and essentially long-polling for updates. I'm sure there is a
> good reason for it I just don't know what it is :)

I'll ask around on this one. Not sure about the delta stuff myself.

> 
> https://codereview.appspot.com/26290043/diff/60001/app/app.js
> File app/app.js (right):
> 
> https://codereview.appspot.com/26290043/diff/60001/app/app.js#newcode43
> app/app.js:43: importHelpers = juju.BundleImport;
> I'd probably change this to be something like `bundleHelpers`

Sure thing, thanks.


> 
> https://codereview.appspot.com/26290043/diff/60001/app/app.js#newcode583
> app/app.js:583: env,
> this.env and this.db are available here because the callback is scoped to
app.js
> so you don't need the references above

The trouble is that these callbacks get bound in the go env wrappings and I
preferred to be explicit. I'll test it out and make sure, but then add a
comment/note.



> https://codereview.appspot.com/26290043/diff/60001/app/app.js#newcode598
> app/app.js:598: cfg.deployBundle = function(bundle) {
> if you use .bind() for this function you do not need to maintain the reference
> above for env, db
> 
>
https://codereview.appspot.com/26290043/diff/60001/app/assets/javascripts/bun...
> File app/assets/javascripts/bundle-import-helpers.js (right):
> 
>
https://codereview.appspot.com/26290043/diff/60001/app/assets/javascripts/bun...
> app/assets/javascripts/bundle-import-helpers.js:55:
> ns.BundleImport._watchDeployment(result.DeploymentId, env, db);
> When I see utility methods that are directly tied together this it makes me
> think this is a class trying really hard to be a utility object. Everything in
> this object is directly related to a single instance but instead of relying on
> that instance everything needs to be passed around. I don't see the advantage
to
> this structure vs relying on the primary instance. 
> 
> For testing you could unit test the class simply by creating an instance of it
> with stubbed env/db like you do now. If you didn't want to create a new class
> this would be handled by converting it to an extension and then testing would
be
> the same technique as prev mentioned.

I thought about a class but then I stopped since this makes testing really easy.
Passing in arguments ftw which allowed me to write a specific test case for the
update method without dealing with anything else. They all share the same method
signature of really 'data, env, db' so it's not that they're confusing or mixing
things up. We'd not really be carrying around multiple instances of a class with
different env/db instances so a class seemed to add complexity/overkill for no
real gain. 

> 
>
https://codereview.appspot.com/26290043/diff/60001/app/assets/javascripts/bun...
> app/assets/javascripts/bundle-import-helpers.js:105:
> ns.BundleImport.deployBundle(
> I know this is repurposed but it feels like we are duplicating work here just
to
> supply different notification messages. I'd try to move these to be handled in
> the deployBundle method, or offload both notifications to a separate
> showNotification(file Boolean) {} method

Sure thing, this was just copied over and adjusted to use the deployBundle
method. I think the different notifications are valuable and the work outside of
this look needs to stay as you can have multiple files selected by the user. 


> 
>
https://codereview.appspot.com/26290043/diff/60001/app/assets/javascripts/bun...
> app/assets/javascripts/bundle-import-helpers.js:188:
> _processWatchDeploymentUpdates: function(watchId, env, db) {
> this is really a watchDeploymentUpdates method
> 
> https://codereview.appspot.com/26290043/diff/60001/app/store/env/go.js
> File app/store/env/go.js (right):
> 
>
https://codereview.appspot.com/26290043/diff/60001/app/store/env/go.js#newcod...
> app/store/env/go.js:528: Wrapper for the deployerWatch call.
> I don't think this description is representative of what this fn actually is
> doing.


I'll double check the names. thanks.


> https://codereview.appspot.com/26290043/diff/60001/test/test_service_module.js
> File test/test_service_module.js (right):
> 
>
https://codereview.appspot.com/26290043/diff/60001/test/test_service_module.j...
> test/test_service_module.js:332: juju.BundleImport.deployBundle =
> function(deployerData, env, db) {
> This should probably be set back to it's original value avoid any oddities in
> future tests.
> 
>
https://codereview.appspot.com/26290043/diff/90001/test/test_bundle_import_he...
> File test/test_bundle_import_helpers.js (right):
> 
>
https://codereview.appspot.com/26290043/diff/90001/test/test_bundle_import_he...
> test/test_bundle_import_helpers.js:82: assert.equal(info.level, 'important');
> There is no guarantee that this will be hit you could add a flag which gets
set
> to 1 when this is called and then check it in the same callback where your
> done() is

I'll update the tests.

[rharding, 2013-11-15 14:03:26]

Please take a look.

[jeff.pihach, 2013-11-15 14:14:16]

LGTM no qa yet

[jeff.pihach, 2013-11-15 20:01:27]

QA OK Let-r-rip!

[matthew.scott, 2013-11-15 20:20:25]

LGTM!  Thanks!

[rharding, 2013-11-15 20:43:32]

*** Submitted:

Add support for watching progress of bundle import

- Change the bundle-import-extension to a helpers lib that doesn't need to be
mixed in. It didn't need any state info.
- Move everyone to going through the single deployBundle function in the
helpers vs their own calls to the env.deployerImport
- Provide the ability to start a watch on a deployment.
- Add the methods to update with new notifications on each change to the watch
status of the import.
- Start a new test file for the bundle-import-helpers tests
- Remove the dupe idea of adding import notifications from the view utils,
it's part of the helpers.

QA
----

There are four QA methods that need testing as there are four ways to deploy
a bundle in the code. In order to QA you need to bring up a live environment.
I suggest that you use `juju quickstart` without any parameters to bring up
the environment.

Once it's up, you need to change the source to this branch with:

juju set juju-gui juju-gui-source='lp:~rharding/juju-gui/bundle-progress'

Now this takes a while. I found the best way to monitor it was to

juju ssh juju-gui/0
watch 'ps aux | grep config'

You then need to wait for the config-changed hook to finish running.

Once the config change hook is done, you can start to QA the four methods of
deploying a bundle.


1. Search for 'jorge' in the browser and drag-n-drop the discource bundle onto
the canvas.
2. Search for 'jorge' and click on the discourse bundle to view the details.
Then click on the deploy button and press "Yes I'm sure".
3. Use the import icon to import a local bundle file.
4. Drag-n-Drop a bundle file onto the canvas.

In each test case, what you're watching for are notifications that the bundle
has been requested, started, and completed. If the bundle is large the final
step might take a while.

Let me know if you've got any questions or issues qa'ing.

R=jeff.pihach, matthew.scott
CC=
https://codereview.appspot.com/26290043