Sandbox import/export w/File DnD support

Sandbox import/export w/File DnD support

Very basic import/export (doesn't use all the fakebackend method
directly as it should)

File Drag and Drop support. To test open a blank sandbox and a nautilus 
window (with test/data opened) and drag one of the samples to the canvas, 
sample-improv.json is taken from the rapi-delta branch and can be used
directly.

https://code.launchpad.net/~bcsaller/juju-gui/sandbox-simulator/+merge/161788

(do not edit description out of merge proposal)

[bcsaller, 2013-05-01 07:19:04]

Please take a look.

[gary.poster, 2013-05-01 19:29:18]

Very cool, thank you.

I still need to qa.  I have a call and will do so after that.  Notice my concern
about charms and relations: I suspect that would have a qa problem.

https://codereview.appspot.com/9070043/diff/1/app/config-debug.js
File app/config-debug.js (right):

https://codereview.appspot.com/9070043/diff/1/app/config-debug.js#newcode27
app/config-debug.js:27: sandbox: true,
...the usual... :-)

I wonder if we ought to have a :dev: namespace where we can change some config
values like this for testing?

https://codereview.appspot.com/9070043/diff/1/app/store/env/fakebackend.js
File app/store/env/fakebackend.js (right):

https://codereview.appspot.com/9070043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:1178: // Might have to check for float and sub '.'
with '_'.
I'm fine with [a-zA-Z0-9_]+ at the moment.

https://codereview.appspot.com/9070043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:1217: importEnvironment_v1: function(data) {
I worry somewhat that there are multiple versions of this import/export
functionality.  Maybe we should say that our version is "GUI_1" or something
right now?  Namespacing our approach from others (like juju core) is my concern.
 Although there is no telling that they will use "exportFormat"...maybe that
should be the namespace instead? "GUIexportFormat"?

https://codereview.appspot.com/9070043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:1227: // real promise chain.
+1.  However, we do need to get the charms loaded, or else there will be errors
when people try to make new relations to the imported charms.  A relatively
trivial change to what you have here would be to use Y.Promise to aggregate
calling getCharm for all of the service's charms.  With that, importEnvironment
would need to take a callback (or return a promise) to let the caller know when
the environment is fully loaded.

You could also use Y.Promise to aggregate the deploy and addRelation calls, but
that might be a larger refactoring, and I would like to see this land.

Note that I think we should be comfortable gradually moving this interface over
to promises, not requiring ourselves to to do it wholesale at once.

https://codereview.appspot.com/9070043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:1230: // TODO: assign new ids for import data?
I assume you mean for handling conflicts with existing data?  Perhaps import
should refuse to proceed if there are any services in the db, for now?

https://codereview.appspot.com/9070043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:1235: s.id = s.name;
If we ever do need to handle conflicts with a db's existing services, we'd need
to do this work here anyway, so I think it is quite reasonable to do it here.

https://codereview.appspot.com/9070043/diff/1/app/store/env/python.js
File app/store/env/python.js (right):

https://codereview.appspot.com/9070043/diff/1/app/store/env/python.js#newcode271
app/store/env/python.js:271: exportEnvironment: function(callback) {
With some reservations, I think it would be nice if methods only appear on the
environment if they are available.  The Go environment will gain conditional
methods, and I think the natural JS'y way to handle different capabilities is to
see if the env has the associated method.  On the Go side, for now we might
simply try to call methods with an invalid signature on the environment, and if
they are recognized but invalid then we add them to the environment.  Roger has
an idea of exposing the API capabilities from a single call at a future date. 
Perhaps we could leverage one of those ideas here, when it is being initialized?
 If the operation exists, it says "this.exportEnvironment =
this._exportEnvironment; this.importEnvironment = this._importEnvironment;"

Maybe don't do this now.  I'd rather have this landed.

https://codereview.appspot.com/9070043/diff/1/app/views/topology/importexport.js
File app/views/topology/importexport.js (right):

https://codereview.appspot.com/9070043/diff/1/app/views/topology/importexport...
app/views/topology/importexport.js:16: * Handle ImportExport from the Topology.
Nice use of the topology, so we can turn this on and off.

https://codereview.appspot.com/9070043/diff/1/test/test_sandbox.js
File test/test_sandbox.js (right):

https://codereview.appspot.com/9070043/diff/1/test/test_sandbox.js#newcode1377
test/test_sandbox.js:1377: * It doesn't this by supplying a simple
doesn't? :-)

https://codereview.appspot.com/9070043/diff/1/test/test_sandbox.js#newcode1417
test/test_sandbox.js:1417: // jshint is insisting on these 'new' prefixes.
Because of initial capital letter, maybe?  Maybe if you called it promise it
would remove the unnecessary "new" and be a bit easier to read?

[bcsaller, 2013-05-01 20:47:21]

Thanks for the review, I pushed some of the changes and will eval what it takes
to move the import logic in the direction mentioned.

Also pushed changes to add a notification on import success (but not on failure
which I still should)

https://codereview.appspot.com/9070043/diff/1/app/config-debug.js
File app/config-debug.js (right):

https://codereview.appspot.com/9070043/diff/1/app/config-debug.js#newcode27
app/config-debug.js:27: sandbox: true,
On 2013/05/01 19:29:18, gary.poster wrote:
> ...the usual... :-)
> 
> I wonder if we ought to have a :dev: namespace where we can change some config
> values like this for testing?

I added a card with a proposed solution to this.

https://codereview.appspot.com/9070043/diff/1/app/store/env/fakebackend.js
File app/store/env/fakebackend.js (right):

https://codereview.appspot.com/9070043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:1178: // Might have to check for float and sub '.'
with '_'.
On 2013/05/01 19:29:18, gary.poster wrote:
> I'm fine with [a-zA-Z0-9_]+ at the moment.
In this case the intention was only a Number for versions. The plan was to take
whatever input format we have and pass it from vCurrent to vN massaging the data
as needed prior to invoking the backend methods to generate state.

https://codereview.appspot.com/9070043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:1217: importEnvironment_v1: function(data) {
On 2013/05/01 19:29:18, gary.poster wrote:
> I worry somewhat that there are multiple versions of this import/export
> functionality.  Maybe we should say that our version is "GUI_1" or something
> right now?  Namespacing our approach from others (like juju core) is my
concern.
>  Although there is no telling that they will use "exportFormat"...maybe that
> should be the namespace instead? "GUIexportFormat"?

I assumed we would propose they version their exports and we assign v2 to
whatever that is.

https://codereview.appspot.com/9070043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:1230: // TODO: assign new ids for import data?
On 2013/05/01 19:29:18, gary.poster wrote:
> I assume you mean for handling conflicts with existing data?  Perhaps import
> should refuse to proceed if there are any services in the db, for now?

Maybe for now, but looking towards bundles it will be the common case that some
of the services in a bundle may already be present in the env. I can picture a
future in which conflict resolution is interactive though the UI around that
could be quite complex. 

"You already have a mysql database deployed, should Juju relate to that service
with the Mediawiki bundle or create a new service?" Where this would impact the
proposed "chain of generated promises" plan. We don't need a full solution to
this now but thinking about it seems valuable.

https://codereview.appspot.com/9070043/diff/1/test/test_sandbox.js
File test/test_sandbox.js (right):

https://codereview.appspot.com/9070043/diff/1/test/test_sandbox.js#newcode1377
test/test_sandbox.js:1377: * It doesn't this by supplying a simple
On 2013/05/01 19:29:18, gary.poster wrote:
> doesn't? :-)

Oh no I didn't!

https://codereview.appspot.com/9070043/diff/1/test/test_sandbox.js#newcode1417
test/test_sandbox.js:1417: // jshint is insisting on these 'new' prefixes.
On 2013/05/01 19:29:18, gary.poster wrote:
> Because of initial capital letter, maybe?  Maybe if you called it promise it
> would remove the unnecessary "new" and be a bit easier to read?

That appears to have been it. Overcome by convention. Thanks.

[bcsaller, 2013-05-02 15:59:27]

Please take a look.

[gary.poster, 2013-05-02 17:53:01]

(for the record, qa'd and still seeing some issues; discussed on IRC)

[bcsaller, 2013-05-02 18:58:51]

Please take a look.

[bcsaller, 2013-05-02 19:01:23]

This version should fix the relation issues by updating the charm/service info
to be in sync on the backend.

[jeff.pihach, 2013-05-02 19:42:31]

LGTM - Thanks for doing this it's such a cool feature.

There are a number of comments but most are trivial.

https://codereview.appspot.com/9070043/diff/15001/app/config-debug.js
File app/config-debug.js (right):

https://codereview.appspot.com/9070043/diff/15001/app/config-debug.js#newcode27
app/config-debug.js:27: sandbox: true,
You probably didn't mean to commit this change.

https://codereview.appspot.com/9070043/diff/15001/app/store/env/fakebackend.js
File app/store/env/fakebackend.js (right):

https://codereview.appspot.com/9070043/diff/15001/app/store/env/fakebackend.j...
app/store/env/fakebackend.js:1111: _promiseCharmForService:
function(serviceData) {
the model_controller code already has a method for a promise for charm data.
Maybe you could use that one instead?

https://codereview.appspot.com/9070043/diff/15001/app/store/env/fakebackend.j...
app/store/env/fakebackend.js:1134: * Export environment state
These comment blocks seem to have incorrect indentation

https://codereview.appspot.com/9070043/diff/15001/app/store/env/python.js
File app/store/env/python.js (right):

https://codereview.appspot.com/9070043/diff/15001/app/store/env/python.js#new...
app/store/env/python.js:269: * @return {String} Sends a message to the server
only.
This will return undefined

https://codereview.appspot.com/9070043/diff/15001/app/store/env/python.js#new...
app/store/env/python.js:281: * @return {String} Sends a message to the server
only.
will return undefined

https://codereview.appspot.com/9070043/diff/15001/app/views/environment.js
File app/views/environment.js (right):

https://codereview.appspot.com/9070043/diff/15001/app/views/environment.js#ne...
app/views/environment.js:83: topo.addModule(views.ImportExportModule);
Isn't this going to add it to the environment for all backend types?

https://codereview.appspot.com/9070043/diff/15001/app/views/topology/importex...
File app/views/topology/importexport.js (right):

https://codereview.appspot.com/9070043/diff/15001/app/views/topology/importex...
app/views/topology/importexport.js:37: var evt = d3.event;
This seems kind of odd, shouldn't the event be passed into the handler?

https://codereview.appspot.com/9070043/diff/15001/app/views/topology/importex...
app/views/topology/importexport.js:47: handleFileDrop: function(box, self) {
should probably be private _handleFileDrop because it's a callback

https://codereview.appspot.com/9070043/diff/15001/app/views/topology/importex...
app/views/topology/importexport.js:53: var evt = d3.event,
same as above.

https://codereview.appspot.com/9070043/diff/15001/test/test_fakebackend.js
File test/test_fakebackend.js (right):

https://codereview.appspot.com/9070043/diff/15001/test/test_fakebackend.js#ne...
test/test_fakebackend.js:263: describe('FakeBackend.exportEnvironment',
function(done) {
Interesting, I didn't know you could do nested describes

https://codereview.appspot.com/9070043/diff/15001/test/test_fakebackend.js#ne...
test/test_fakebackend.js:290: this.timeout(300);
What is this here for?

https://codereview.appspot.com/9070043/diff/15001/test/test_sandbox.js
File test/test_sandbox.js (right):

https://codereview.appspot.com/9070043/diff/15001/test/test_sandbox.js#newcod...
test/test_sandbox.js:1383: * @method P
promise?

https://codereview.appspot.com/9070043/diff/15001/test/test_sandbox.js#newcod...
test/test_sandbox.js:1414: this.timeout(250);
what's this for?

https://codereview.appspot.com/9070043/diff/15001/test/test_sandbox.js#newcod...
test/test_sandbox.js:1417: // jshint is insisting on these 'new' prefixes.
possible old comment?

https://codereview.appspot.com/9070043/diff/15001/test/test_sandbox.js#newcod...
test/test_sandbox.js:1432: this.timeout(300);
another one

https://codereview.appspot.com/9070043/diff/15001/test/test_sandbox.js#newcod...
test/test_sandbox.js:1440: // Verify that we can now an find expected entry
find an

[bcsaller, 2013-05-02 20:28:55]

Thanks for the review, pushed up changes, if you want me to repropose let me
know

https://codereview.appspot.com/9070043/diff/15001/app/config-debug.js
File app/config-debug.js (right):

https://codereview.appspot.com/9070043/diff/15001/app/config-debug.js#newcode27
app/config-debug.js:27: sandbox: true,
On 2013/05/02 19:42:31, jeff.pihach wrote:
> You probably didn't mean to commit this change.

I have a card to fix how this works in the future, I need this set to test it,
hard to remember to change it on commit.

https://codereview.appspot.com/9070043/diff/15001/app/store/env/fakebackend.js
File app/store/env/fakebackend.js (right):

https://codereview.appspot.com/9070043/diff/15001/app/store/env/fakebackend.j...
app/store/env/fakebackend.js:1111: _promiseCharmForService:
function(serviceData) {
On 2013/05/02 19:42:31, jeff.pihach wrote:
> the model_controller code already has a method for a promise for charm data.
> Maybe you could use that one instead?

This is using the fakebackend version internally so it might not be quite the
same thing (as fakebackend can have its own charm store mapped). This really is
just a call wrapper, not so much a util method, I could even put it back in the
import method (its only consumer) but we might want it down the road in which
case maybe model controller would be right.

https://codereview.appspot.com/9070043/diff/15001/app/store/env/fakebackend.j...
app/store/env/fakebackend.js:1134: * Export environment state
On 2013/05/02 19:42:31, jeff.pihach wrote:
> These comment blocks seem to have incorrect indentation

make prep did that :(

https://codereview.appspot.com/9070043/diff/15001/app/store/env/python.js
File app/store/env/python.js (right):

https://codereview.appspot.com/9070043/diff/15001/app/store/env/python.js#new...
app/store/env/python.js:269: * @return {String} Sends a message to the server
only.
On 2013/05/02 19:42:31, jeff.pihach wrote:
> This will return undefined

Done.

https://codereview.appspot.com/9070043/diff/15001/app/store/env/python.js#new...
app/store/env/python.js:281: * @return {String} Sends a message to the server
only.
On 2013/05/02 19:42:31, jeff.pihach wrote:
> will return undefined

Done.

https://codereview.appspot.com/9070043/diff/15001/app/views/environment.js
File app/views/environment.js (right):

https://codereview.appspot.com/9070043/diff/15001/app/views/environment.js#ne...
app/views/environment.js:83: topo.addModule(views.ImportExportModule);
On 2013/05/02 19:42:31, jeff.pihach wrote:
> Isn't this going to add it to the environment for all backend types?

It currently does, yes. I can make it conditional for now, but all backends will
hopefully grow an import story soon. However unless the GUI implements turning
imports into proper backend commands (which is something backends *should* do)
its not possible to use this everywhere.

https://codereview.appspot.com/9070043/diff/15001/app/views/topology/importex...
File app/views/topology/importexport.js (right):

https://codereview.appspot.com/9070043/diff/15001/app/views/topology/importex...
app/views/topology/importexport.js:37: var evt = d3.event;
On 2013/05/02 19:42:31, jeff.pihach wrote:
> This seems kind of odd, shouldn't the event be passed into the handler?

d3 uses d3.event and that is the context of most of the topo code so I honored
their convention.

https://codereview.appspot.com/9070043/diff/15001/app/views/topology/importex...
app/views/topology/importexport.js:47: handleFileDrop: function(box, self) {
On 2013/05/02 19:42:31, jeff.pihach wrote:
> should probably be private _handleFileDrop because it's a callback

I'll change it, but everything in the module (all all the modules) is module
specific except for the methods triggered by the topo on all its mixins.

Modules are only created by the topology and only accessible in their own
runtime (or by poking at already private structs).

https://codereview.appspot.com/9070043/diff/15001/test/test_fakebackend.js
File test/test_fakebackend.js (right):

https://codereview.appspot.com/9070043/diff/15001/test/test_fakebackend.js#ne...
test/test_fakebackend.js:290: this.timeout(300);
On 2013/05/02 19:42:31, jeff.pihach wrote:
> What is this here for?

The default 10s timeout is a pain in development, when a test fails it can be a
long wait.

https://codereview.appspot.com/9070043/diff/15001/test/test_sandbox.js
File test/test_sandbox.js (right):

https://codereview.appspot.com/9070043/diff/15001/test/test_sandbox.js#newcod...
test/test_sandbox.js:1417: // jshint is insisting on these 'new' prefixes.
On 2013/05/02 19:42:31, jeff.pihach wrote:
> possible old comment?

yeah, it was

[gary.poster, 2013-05-03 00:23:21]

Assuming you clean up review notes, LGTM!  Yay, thanks!

[bcsaller, 2013-05-03 00:33:28]

*** Submitted:

Sandbox import/export w/File DnD support

Very basic import/export (doesn't use all the fakebackend method
directly as it should)

File Drag and Drop support. To test open a blank sandbox and a nautilus 
window (with test/data opened) and drag one of the samples to the canvas, 
sample-improv.json is taken from the rapi-delta branch and can be used
directly.

R=gary.poster, jeff.pihach
CC=
https://codereview.appspot.com/9070043