Implement service constraints edit feature

Implement service constraints edit feature



https://code.launchpad.net/~tveronezi/juju-ui/srv-constraints-edit-2/+merge/125590

(do not edit description out of merge proposal)

[thiago, 2012-09-20 22:06:58]

Please take a look.

[benjamin.saller, 2012-09-21 13:49:12]

Some minors and a question below. After thats changed and answered let me know
and we can try to land this. 

Thanks

https://codereview.appspot.com/6551048/diff/1/app/templates/service-constrain...
File app/templates/service-constraints.handlebars (right):

https://codereview.appspot.com/6551048/diff/1/app/templates/service-constrain...
app/templates/service-constraints.handlebars:7: {{#readOnly}}
might want to rename readOnly to readOnlyConstraints here and in the js, its not
quite clear what you're iterating on here.

https://codereview.appspot.com/6551048/diff/1/lib/views/stylesheet.less
File lib/views/stylesheet.less (right):

https://codereview.appspot.com/6551048/diff/1/lib/views/stylesheet.less#newco...
lib/views/stylesheet.less:166: .controls {
controls is far too generic a name unless you scope to the services container
class

https://codereview.appspot.com/6551048/diff/1/test/test_service_config_view.js
File test/test_service_config_view.js (right):

https://codereview.appspot.com/6551048/diff/1/test/test_service_config_view.j...
test/test_service_config_view.js:139: assertButtonState(false);
If I'm reading this right the saveConfig internally will use the overridden
set_config testing that the button is true and then when the call ends the
button should be false again. 

If this reading is ok I'd at least explain it in comment and if my understanding
is wrong this should be made more clear and explained.

[hazmat, 2012-09-21 14:05:45]

a few items.

https://codereview.appspot.com/6551048/diff/1/app/views/service.js
File app/views/service.js (right):

https://codereview.appspot.com/6551048/diff/1/app/views/service.js#newcode49
app/views/service.js:49: var getElementsValuesMap = function(container, cls,
originalMap) {
mutation of params on a getter method is something to be avoided. this behavior
is also unused.

https://codereview.appspot.com/6551048/diff/1/app/views/service.js#newcode58
app/views/service.js:58: var buildServerCallbackHandler = function(config) {
this name is a bit overly verbose.. i'd suggest rpcHandler

https://codereview.appspot.com/6551048/diff/1/app/views/service.js#newcode62
app/views/service.js:62: initHandler = config.initHandler,
these could drop Handler suffixes.

https://codereview.appspot.com/6551048/diff/1/app/views/service.js#newcode62
app/views/service.js:62: initHandler = config.initHandler,
initHandler is not used, nor is it clear that its needed, the caller already has
the init context prior to invocation.

https://codereview.appspot.com/6551048/diff/1/app/views/service.js#newcode103
app/views/service.js:103:
Y.namespace('juju.views.utils').buildServerCallbackHandler =
buildServerCallbackHandler;
items in this namespace should be defined in views/utils.js else we're making a
rabbit chase when looking for definitions.

https://codereview.appspot.com/6551048/diff/1/app/views/service.js#newcode104
app/views/service.js:104: Y.namespace('juju.views.utils')._serverErrorMessage =
'An error ocurred.';
since this is referenced also by tests its not purely a private variable, just a
global one.. s/_/ALL_CAPS

https://codereview.appspot.com/6551048/diff/1/app/views/service.js#newcode282
app/views/service.js:282: getElementsValuesMap(container, '.config-field'),
why is this being called, its not passing in a value map or using the return

https://codereview.appspot.com/6551048/diff/1/test/test_service_config_view.js
File test/test_service_config_view.js (right):

https://codereview.appspot.com/6551048/diff/1/test/test_service_config_view.j...
test/test_service_config_view.js:126: assertButtonState(true);
shouldn't button state be false here?

https://codereview.appspot.com/6551048/diff/1/test/test_service_config_view.j...
test/test_service_config_view.js:161:
Y.namespace('juju.views.utils').buildServerCallbackHandler({
populate this at test def scope instead of 'importing' at local scope multiple
times.

[thiago, 2012-09-21 15:51:55]

In order to merge the white-spaces fixes coming from the trunk, we needed to
create another branch with diff whitespaces changes.

https://codereview.appspot.com/6551048/diff/1/app/templates/service-constrain...
File app/templates/service-constraints.handlebars (right):

https://codereview.appspot.com/6551048/diff/1/app/templates/service-constrain...
app/templates/service-constraints.handlebars:7: {{#readOnly}}
On 2012/09/21 13:49:12, benjamin.saller wrote:
> might want to rename readOnly to readOnlyConstraints here and in the js, its
not
> quite clear what you're iterating on here.

Done.

https://codereview.appspot.com/6551048/diff/1/app/views/service.js
File app/views/service.js (right):

https://codereview.appspot.com/6551048/diff/1/app/views/service.js#newcode49
app/views/service.js:49: var getElementsValuesMap = function(container, cls,
originalMap) {
On 2012/09/21 14:05:45, hazmat wrote:
> mutation of params on a getter method is something to be avoided. this
behavior
> is also unused.

Done.

https://codereview.appspot.com/6551048/diff/1/app/views/service.js#newcode58
app/views/service.js:58: var buildServerCallbackHandler = function(config) {
On 2012/09/21 14:05:45, hazmat wrote:
> this name is a bit overly verbose.. i'd suggest rpcHandler

Done.

https://codereview.appspot.com/6551048/diff/1/app/views/service.js#newcode62
app/views/service.js:62: initHandler = config.initHandler,
On 2012/09/21 14:05:45, hazmat wrote:
> these could drop Handler suffixes.

Done.

https://codereview.appspot.com/6551048/diff/1/app/views/service.js#newcode62
app/views/service.js:62: initHandler = config.initHandler,
On 2012/09/21 14:05:45, hazmat wrote:
> initHandler is not used, nor is it clear that its needed, the caller already
has
> the init context prior to invocation.

Done.

https://codereview.appspot.com/6551048/diff/1/app/views/service.js#newcode62
app/views/service.js:62: initHandler = config.initHandler,
On 2012/09/21 14:05:45, hazmat wrote:
> these could drop Handler suffixes.

We (or Benji at least) like it this way, so unless you feel strongly about it,
we would prefer to keep it this way because the names are a bit more
descriptive.

https://codereview.appspot.com/6551048/diff/1/app/views/service.js#newcode103
app/views/service.js:103:
Y.namespace('juju.views.utils').buildServerCallbackHandler =
buildServerCallbackHandler;
On 2012/09/21 14:05:45, hazmat wrote:
> items in this namespace should be defined in views/utils.js else we're making
a
> rabbit chase when looking for definitions.

Done.

https://codereview.appspot.com/6551048/diff/1/app/views/service.js#newcode104
app/views/service.js:104: Y.namespace('juju.views.utils')._serverErrorMessage =
'An error ocurred.';
On 2012/09/21 14:05:45, hazmat wrote:
> since this is referenced also by tests its not purely a private variable, just
a
> global one.. s/_/ALL_CAPS

Done.

https://codereview.appspot.com/6551048/diff/1/lib/views/stylesheet.less
File lib/views/stylesheet.less (right):

https://codereview.appspot.com/6551048/diff/1/lib/views/stylesheet.less#newco...
lib/views/stylesheet.less:166: .controls {
On 2012/09/21 13:49:12, benjamin.saller wrote:
> controls is far too generic a name unless you scope to the services container
> class

The class name "controls" is from bootstrap, we just needed read-only text that
is aligned properly on the form.

https://codereview.appspot.com/6551048/diff/1/test/test_service_config_view.js
File test/test_service_config_view.js (right):

https://codereview.appspot.com/6551048/diff/1/test/test_service_config_view.j...
test/test_service_config_view.js:126: assertButtonState(true);
This should be true (disabled=true). 
The view.saveConfig method disables the button before calling the this rpc
method. We are renaming the method to "assertButtonDisabled"

On 2012/09/21 14:05:45, hazmat wrote:
> shouldn't button state be false here?

https://codereview.appspot.com/6551048/diff/1/test/test_service_config_view.j...
test/test_service_config_view.js:139: assertButtonState(false);
You are right. We are adding the comment.

On 2012/09/21 13:49:12, benjamin.saller wrote:
> If I'm reading this right the saveConfig internally will use the overridden
> set_config testing that the button is true and then when the call ends the
> button should be false again. 
> 
> If this reading is ok I'd at least explain it in comment and if my
understanding
> is wrong this should be made more clear and explained.

https://codereview.appspot.com/6551048/diff/1/test/test_service_config_view.j...
test/test_service_config_view.js:161:
Y.namespace('juju.views.utils').buildServerCallbackHandler({
On 2012/09/21 14:05:45, hazmat wrote:
> populate this at test def scope instead of 'importing' at local scope multiple
> times.

Done.

[hazmat, 2012-09-21 16:38:58]

On Fri, Sep 21, 2012 at 11:51 AM, <thiago@veronezi.org> wrote:

> In order to merge the white-spaces fixes coming from the trunk, we
> needed to create another branch with diff whitespaces changes.
>
>
understood that's why i suggested it for future branches, as i've noticed
this naming convention in other  branches submitted as well.



>
> https://codereview.appspot.**com/6551048/diff/1/app/views/**
>
service.js#newcode62<https://codereview.appspot.com/6551048/diff/1/app/views/service.js#newcode62>
> app/views/service.js:62: initHandler = config.initHandler,
> On 2012/09/21 14:05:45, hazmat wrote:
>
>> these could drop Handler suffixes.
>>
>
> We (or Benji at least) like it this way, so unless you feel strongly
> about it, we would prefer to keep it this way because the names are a
> bit more descriptive.


if its internal to the rpcHandler implementation okay, but if the caller
needs to use rpcHandler({'successHandler':}) that's silly to force api
consumers to spell handler multiple times when they've already specified
their creating a handler, and al of its non context/scope params are
handlers.