Fix settings emptied out on save.

Fix settings emptied out on save.

Only the new values were stored in the database
by the set_config callback.

Also fixed a confusing object passing (using Y.bind)
in inspector.js: at the moment newVals was defined
it included all the configs. A side effect of env.set_config
mutated the object so that the callback received just the 
new values. Now env.set_config no longer mutates its params.

QA
- bootstrap, deploy cs:~juju-gui/precise/juju-gui
  and expose the GUI;
- switch the GUI to this branch:
    juju set juju-gui \
        juju-gui-source=lp:~frankban/juju-gui/inspector-settings-emptied-out
  and wait 10 minutes for the change to be applied;
- deploy another service (e.g. haproxy);
- change the settings on haproxy and click save;
- all the other options should no longer be emptied
  out. The changed options should display their new
  value, even after refreshing the page.

https://code.launchpad.net/~frankban/juju-gui/inspector-settings-emptied-out/+merge/188018

(do not edit description out of merge proposal)

[frankban, 2013-09-27 11:03:26]

Please take a look.

[gary.poster, 2013-09-27 13:15:01]

LGTM with trivials and QA ok.  I was able to dupe in sandbox (the problematic
period is much smaller, because the sandbox delta timing is 1 second, not 5, but
it is still there) so I QA'd there instead.  If you'd prefer a full Go QA, let
me know and I'll fire up ec2.

Thank you!

Gary

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

https://codereview.appspot.com/14033043/diff/1/app/store/env/go.js#newcode1044
app/store/env/go.js:1044: newValues = utils.getUnchangedConfigOptions(config,
serviceConfig);
I like the positive aspect of the "get" rather than the "remove", but I think
the name should be "getChangedConfigOptions".  "getUnchangedConfigOptions" does
not accurately describe the behavior of the function.

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

https://codereview.appspot.com/14033043/diff/1/app/store/env/python.js#newcod...
app/store/env/python.js:413: config = utils.getUnchangedConfigOptions(config,
serviceConfig);
(getChanged...)

https://codereview.appspot.com/14033043/diff/1/app/views/ghost-inspector.js
File app/views/ghost-inspector.js (right):

https://codereview.appspot.com/14033043/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:120: config = utils.getUnchangedConfigOptions(
(getChanged...)

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

https://codereview.appspot.com/14033043/diff/1/app/views/utils.js#newcode662
app/views/utils.js:662: Find unchanged config options from a collection of
config values and return
Find changed...

https://codereview.appspot.com/14033043/diff/1/app/views/utils.js#newcode666
app/views/utils.js:666: @method getUnchangedConfigOptions
getChanged...

https://codereview.appspot.com/14033043/diff/1/app/views/utils.js#newcode672
app/views/utils.js:672: utils.getUnchangedConfigOptions = function(config,
options) {
getChanged...

https://codereview.appspot.com/14033043/diff/1/app/views/utils.js#newcode678
app/views/utils.js:678: var newValues = Object.create(null);
Nice fix to not mutate the original object.

Why did you prefer this spelling over "var newValues = {};" ?

https://codereview.appspot.com/14033043/diff/1/test/test_utils.js
File test/test_utils.js (right):

https://codereview.appspot.com/14033043/diff/1/test/test_utils.js#newcode288
test/test_utils.js:288: describe('getUnchangedConfigOptions', function() {
getChanged... here and below

[frankban, 2013-09-27 13:49:18]

On 2013/09/27 13:15:01, gary.poster wrote:
> LGTM with trivials and QA ok.  I was able to dupe in sandbox (the problematic
> period is much smaller, because the sandbox delta timing is 1 second, not 5,
but
> it is still there) so I QA'd there instead.  If you'd prefer a full Go QA, let
> me know and I'll fire up ec2.

It's ok thanks. I'll do an additional QA after the changes you suggested.
Renamed getUnchangedConfigOptions everywhere.
Thanks for the review!

> https://codereview.appspot.com/14033043/diff/1/app/views/utils.js#newcode678
> app/views/utils.js:678: var newValues = Object.create(null);
> Nice fix to not mutate the original object.
> 
> Why did you prefer this spelling over "var newValues = {};" ?

Object.create(null) doesn't inherit from the Object prototype, 
so it returns an object with no properties at all: it seems to me
more clean and lightweight in the cases you will use the result
only as a "dict" data structure.

[frankban, 2013-09-27 14:22:13]

*** Submitted:

Fix settings emptied out on save.

Only the new values were stored in the database
by the set_config callback.

Also fixed a confusing object passing (using Y.bind)
in inspector.js: at the moment newVals was defined
it included all the configs. A side effect of env.set_config
mutated the object so that the callback received just the 
new values. Now env.set_config no longer mutates its params.

QA
- bootstrap, deploy cs:~juju-gui/precise/juju-gui
  and expose the GUI;
- switch the GUI to this branch:
    juju set juju-gui \
        juju-gui-source=lp:~frankban/juju-gui/inspector-settings-emptied-out
  and wait 10 minutes for the change to be applied;
- deploy another service (e.g. haproxy);
- change the settings on haproxy and click save;
- all the other options should no longer be emptied
  out. The changed options should display their new
  value, even after refreshing the page.

R=gary.poster
CC=
https://codereview.appspot.com/14033043