Created a model controller with promises

Created a model controller with promises

This branch takes the work from a number of previous branches
and combined the work under a single ModelController class which
will be used from now on to interact with the models and db
to avoid having to pass the env and db into them. 

This also implements a sandbox fix from Gary.

Individual unit tests will be coming in a follow-up but in the
mean time all of this code is tested indirectly by the endpoints
tests.

https://code.launchpad.net/~hatch/juju-gui/model-control/+merge/161454

(do not edit description out of merge proposal)

[jeff.pihach, 2013-04-29 16:37:57]

Please take a look.

[gary.poster, 2013-04-29 18:22:03]

LGTM with trivials.  Thank you!

Gary

https://codereview.appspot.com/8686047/diff/1/app/models/model-controller.js
File app/models/model-controller.js (right):

https://codereview.appspot.com/8686047/diff/1/app/models/model-controller.js#...
app/models/model-controller.js:6: models to avoid needing to pass the
environments and other instances arounc
Trivial but suggest: "Provides a collection of utility methods to interact with
the db and its models."  Leave out the last bit. :-)

https://codereview.appspot.com/8686047/diff/1/app/models/model-controller.js#...
app/models/model-controller.js:36: db.fire('update');
Yeah, why do we have to do this?  Can we do it somewhere else instead?

https://codereview.appspot.com/8686047/diff/1/app/models/model-controller.js#...
app/models/model-controller.js:90: populateService: function(serviceId) {
I feel like the endpoints event handler that causes a new service to be
populated ought to be in this file as well.  It explains how it is used, and it
is really bigger than endpoints.

https://codereview.appspot.com/8686047/diff/1/app/models/model-controller.js#...
app/models/model-controller.js:99:
mController.getCharm(service.get('charm')).then(function(charm) {
I wonder if we ought to make a 'populated' attribute, like the 'loaded' one. 
Might be nice for event handlers.  I can imagine code working like this:
controller.getService('cs:precise/wordpress').then(function(service)
{service.once('populated', function(ev) {"I can do stuff here with a fully
populated service!"})})

Maybe we don't need that yet, in which case never mind. :-)

https://codereview.appspot.com/8686047/diff/1/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/8686047/diff/1/app/models/models.js#newcode756
app/models/models.js:756: 'promise'
We don't need this any more, now that you have mved stuff out.

https://codereview.appspot.com/8686047/diff/1/app/store/endpoints.js
File app/store/endpoints.js (right):

https://codereview.appspot.com/8686047/diff/1/app/store/endpoints.js#newcode227
app/store/endpoints.js:227: env: {},
We don't use this anymore, do we?  Or if we do, can't we get it off the
modelController?

https://codereview.appspot.com/8686047/diff/1/app/store/endpoints.js#newcode236
app/store/endpoints.js:236: db: {},
ditto

https://codereview.appspot.com/8686047/diff/1/test/test_endpoints.js
File test/test_endpoints.js (right):

https://codereview.appspot.com/8686047/diff/1/test/test_endpoints.js#newcode345
test/test_endpoints.js:345: // a response from a promise so in 100ms if it's not
there, it won't be.
Maybe talk a bit about why you used a timeout as opposed to another approach, so
someone coming along knows the rationale, and what to look out for if they try
to switch to another approach.

[jeff.pihach, 2013-04-29 20:47:27]

Thanks for the review!

https://codereview.appspot.com/8686047/diff/1/app/models/model-controller.js
File app/models/model-controller.js (right):

https://codereview.appspot.com/8686047/diff/1/app/models/model-controller.js#...
app/models/model-controller.js:6: models to avoid needing to pass the
environments and other instances arounc
On 2013/04/29 18:22:04, gary.poster wrote:
> Trivial but suggest: "Provides a collection of utility methods to interact
with
> the db and its models."  Leave out the last bit. :-)

Done.

https://codereview.appspot.com/8686047/diff/1/app/models/model-controller.js#...
app/models/model-controller.js:36: db.fire('update');
I think this is here because the views listen for db:update to update their UI's
instead of being bound to their respective models.

https://codereview.appspot.com/8686047/diff/1/app/models/model-controller.js#...
app/models/model-controller.js:90: populateService: function(serviceId) {
I agree however I have ran into some issues prototyping it and would like to
leave this change to a follow-up branch

https://codereview.appspot.com/8686047/diff/1/app/models/model-controller.js#...
app/models/model-controller.js:99:
mController.getCharm(service.get('charm')).then(function(charm) {
I'd like to do a followup branch for this to work on a better approach as there
have been some issues in prototyping this.

https://codereview.appspot.com/8686047/diff/1/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/8686047/diff/1/app/models/models.js#newcode756
app/models/models.js:756: 'promise'
On 2013/04/29 18:22:04, gary.poster wrote:
> We don't need this any more, now that you have mved stuff out.

Done.

https://codereview.appspot.com/8686047/diff/1/app/store/endpoints.js
File app/store/endpoints.js (right):

https://codereview.appspot.com/8686047/diff/1/app/store/endpoints.js#newcode227
app/store/endpoints.js:227: env: {},
I added it here because it was being passed in but you are right it's not being
used so I will remove it here and remove it from being passed in.

https://codereview.appspot.com/8686047/diff/1/app/store/endpoints.js#newcode236
app/store/endpoints.js:236: db: {},
This is used in the bind methods I just added it here to document it.

https://codereview.appspot.com/8686047/diff/1/test/test_endpoints.js
File test/test_endpoints.js (right):

https://codereview.appspot.com/8686047/diff/1/test/test_endpoints.js#newcode345
test/test_endpoints.js:345: // a response from a promise so in 100ms if it's not
there, it won't be.
On 2013/04/29 18:22:04, gary.poster wrote:
> Maybe talk a bit about why you used a timeout as opposed to another approach,
so
> someone coming along knows the rationale, and what to look out for if they try
> to switch to another approach.

Done.

[rharding, 2013-04-30 12:36:59]

LGTM with a couple of notes on confusion and take it or leave it clarity
suggestions.

https://codereview.appspot.com/8686047/diff/1/app/models/model-controller.js
File app/models/model-controller.js (right):

https://codereview.appspot.com/8686047/diff/1/app/models/model-controller.js#...
app/models/model-controller.js:11: var ModelController =
Y.Base.create('model-controller', Y.Base, [], {
should this be juju-model-controller preserved in the namespace.

https://codereview.appspot.com/8686047/diff/1/app/models/model-controller.js#...
app/models/model-controller.js:27: // resolving here on purpose because this is
a side effect.
This comment is confusing to me. We're not resolving here, but just below we do?
Is the fact that resolve is dependent on other lookups what you're noting out?

https://codereview.appspot.com/8686047/diff/1/app/models/model-controller.js#...
app/models/model-controller.js:36: db.fire('update');
On 2013/04/29 20:47:28, jeff.pihach wrote:
> I think this is here because the views listen for db:update to update their
UI's
> instead of being bound to their respective models.

Can you verify that and update the comment to be more specific vs the 'even
mildly harmful' that's very unclear.

https://codereview.appspot.com/8686047/diff/1/app/models/model-controller.js#...
app/models/model-controller.js:65: env.get_service(serviceId, function(result) {
what about s/result/found? found.err, found.result.config, etc?

https://codereview.appspot.com/8686047/diff/1/test/test_endpoints.js
File test/test_endpoints.js (right):

https://codereview.appspot.com/8686047/diff/1/test/test_endpoints.js#newcode323
test/test_endpoints.js:323: app = new Y.juju.App({env: env, consoleEnabled:
true});
did you mean to leave this set? I think this does help prevent polluting a giant
console of stuff when run with all tests.

[jeff.pihach, 2013-04-30 15:49:38]

Thanks for the reviews!

https://codereview.appspot.com/8686047/diff/1/app/models/model-controller.js
File app/models/model-controller.js (right):

https://codereview.appspot.com/8686047/diff/1/app/models/model-controller.js#...
app/models/model-controller.js:11: var ModelController =
Y.Base.create('model-controller', Y.Base, [], {
On 2013/04/30 12:36:59, rharding wrote:
> should this be juju-model-controller preserved in the namespace.

Done.

https://codereview.appspot.com/8686047/diff/1/app/models/model-controller.js#...
app/models/model-controller.js:27: // resolving here on purpose because this is
a side effect.
On 2013/04/30 12:36:59, rharding wrote:
> This comment is confusing to me. We're not resolving here, but just below we
do?
> Is the fact that resolve is dependent on other lookups what you're noting out?

Done.

https://codereview.appspot.com/8686047/diff/1/app/models/model-controller.js#...
app/models/model-controller.js:36: db.fire('update');
I didn't write the original comment so I'm not sure why it would be harmful so I
just left the comment in there. I'll add additional documentation as to why it's
there.

[jeff.pihach, 2013-04-30 15:57:25]

*** Submitted:

Created a model controller with promises

This branch takes the work from a number of previous branches
and combined the work under a single ModelController class which
will be used from now on to interact with the models and db
to avoid having to pass the env and db into them. 

This also implements a sandbox fix from Gary.

Individual unit tests will be coming in a follow-up but in the
mean time all of this code is tested indirectly by the endpoints
tests.

R=gary.poster, rharding
CC=
https://codereview.appspot.com/8686047