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/bundle-import-helpers.js > File app/assets/javascripts/bundle-import-helpers.js (right): https://codereview.appspot.com/26290043/diff/60001/app/assets/javascripts/bundle-import-helpers.js#newcode55 > 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/bundle-import-helpers.js#newcode105 > 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/bundle-import-helpers.js#newcode188 > 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#newcode528 > 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.js#newcode332 > 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_helpers.js > File test/test_bundle_import_helpers.js (right): https://codereview.appspot.com/26290043/diff/90001/test/test_bundle_import_helpers.js#newcode82 > 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. https://codereview.appspot.com/26290043/