Support read-only mode in the GUI

Support read-only mode in the GUI

The GUI can be configured so that it denies the 
execution of write operations. This branch 
introduces a check at Juju environment level:
if the user tries to change the environment and
the GUI is in read-only mode, the environment
object refuses to send the message to the server,
fires a permissionDenied event, logs the exception
and executes the provided callback (if any) 
passing an error event.
Subsequently, an error notification is created by
the app. In a pre-implementation call with Gary 
we decided that controls and widget can still be
displayed, they are just ineffective.

Other changes:

Added documentation for all modified 
objects/methods/functions.

Fixed a bug: the environment view was not 
propagating the getModelURL from the app to
the topology instance.
Also added a test for this bug.

Updated the destroy service callback to make
the destroy dialog disappear also when
an error occurs.

Fixed an intermittent failure when 
test_environment_view.js is run in isolation.

https://code.launchpad.net/~frankban/juju-gui/bug-1083928-read-only-mode/+merge/143289

(do not edit description out of merge proposal)

[frankban, 2013-01-15 12:35:48]

Please take a look.

[gary.poster, 2013-01-15 14:57:20]

Hi Francesco.  This is a code-only review, which I'm posting so you can look and
respond.  I'm going to do QA now, and make another review comment after that.

This looks very good.  Thank you.

I am afraid that blocking the annotation sets/removes is important.  I wonder if
it will require more work than simply changing the env, keeping the environment
from allowing drags to happen at all.  Alternatively, I suppose you could have
services return to their original positions on error.

Gary

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

https://codereview.appspot.com/7095058/diff/1/app/store/env.js#newcode143
app/store/env.js:143: * Send a message to the server using the websocket
connection.
Thank you for adding all this documentation.  Nice to have, and nicely done.

https://codereview.appspot.com/7095058/diff/1/app/store/env.js#newcode154
app/store/env.js:154: _send_rpc: function(op, callback, writePermissionRequired)
{
I wondered whether we ought to have a separate env for the readonly behavior,
but I like what you did.  Seems pleasantly simple to me.

https://codereview.appspot.com/7095058/diff/1/app/store/env.js#newcode426
app/store/env.js:426: update_annotations: function(entity, data, callback) {
We need to "protect" this too.

https://codereview.appspot.com/7095058/diff/1/app/store/env.js#newcode462
app/store/env.js:462: remove_annotations: function(entity, keys, callback) {
We need to "protect" this too.

https://codereview.appspot.com/7095058/diff/1/test/test_env.js
File test/test_env.js (right):

https://codereview.appspot.com/7095058/diff/1/test/test_env.js#newcode219
test/test_env.js:219: Y.each(writeOperations, function(args, operation) {
My first inclination would have been to have a test for each write operation,
with a test helper to make each test extremely small.  On reflection, that still
seems preferable to me.  Did you consider that option?  What do you think?

https://codereview.appspot.com/7095058/diff/1/test/test_env.js#newcode250
test/test_env.js:250: Y.each(readOperations, function(args, operation) {
Same comment as above.

https://codereview.appspot.com/7095058/diff/1/undocumented
File undocumented (left):

https://codereview.appspot.com/7095058/diff/1/undocumented#oldcode25
undocumented:25: app/store/env.js:240 "set_config"
The People from the Future thank you, 13 times.

[frankban, 2013-01-15 15:27:13]

On 2013/01/15 14:57:20, gary.poster wrote:
> Hi Francesco.  This is a code-only review, which I'm posting so you can look
and
> respond.  I'm going to do QA now, and make another review comment after that.
> 
> This looks very good.  Thank you.
> 
> I am afraid that blocking the annotation sets/removes is important.  I wonder
if
> it will require more work than simply changing the env, keeping the
environment
> from allowing drags to happen at all.  Alternatively, I suppose you could have
> services return to their original positions on error.

As you already know, it's easy to mark two more methods as 
"needs to change the env". I need to do some investigation about
the idea of disallowing drags. Is that what we want? AFAIK, without
the possibility to update annotation, since it will not affect
other users, we could just let the read-only user move the services 
around. This seems also consistent with the goal of this branch:
in read-only mode, the user can still interact with forms and widgets,
but he will not produce server side changes.
What do you think?

> https://codereview.appspot.com/7095058/diff/1/app/store/env.js#newcode154
> app/store/env.js:154: _send_rpc: function(op, callback,
writePermissionRequired)
> {
> I wondered whether we ought to have a separate env for the readonly behavior,
> but I like what you did.  Seems pleasantly simple to me.

Cool!

> https://codereview.appspot.com/7095058/diff/1/app/store/env.js#newcode426
> app/store/env.js:426: update_annotations: function(entity, data, callback) {
> We need to "protect" this too.

Agreed.

> 
> https://codereview.appspot.com/7095058/diff/1/app/store/env.js#newcode462
> app/store/env.js:462: remove_annotations: function(entity, keys, callback) {
> We need to "protect" this too.

Agreed.


> https://codereview.appspot.com/7095058/diff/1/test/test_env.js#newcode219
> test/test_env.js:219: Y.each(writeOperations, function(args, operation) {
> My first inclination would have been to have a test for each write operation,
> with a test helper to make each test extremely small.  On reflection, that
still
> seems preferable to me.  Did you consider that option?  What do you think?

I am not sure.
With a for loop we achieve:
- less builerplate code;
- faster test runs.
But with separate tests:
- isolation (a failure in a test does not prevent others to be executed);
- possibility to customize a test case if required in the future.
- any other?
I am inclined to accept your suggestion, given that it should be quite 
easy to make this change.

Thanks for the review!

[gary.poster, 2013-01-15 15:45:53]

On 2013/01/15 15:27:13, frankban wrote:
> On 2013/01/15 14:57:20, gary.poster wrote:
> > Hi Francesco.  This is a code-only review, which I'm posting so you can look
> and
> > respond.  I'm going to do QA now, and make another review comment after
that.
> > 
> > This looks very good.  Thank you.
> > 
> > I am afraid that blocking the annotation sets/removes is important.  I
wonder
> if
> > it will require more work than simply changing the env, keeping the
> environment
> > from allowing drags to happen at all.  Alternatively, I suppose you could
have
> > services return to their original positions on error.
> 
> As you already know, it's easy to mark two more methods as 
> "needs to change the env". I need to do some investigation about
> the idea of disallowing drags. Is that what we want? AFAIK, without
> the possibility to update annotation, since it will not affect
> other users, we could just let the read-only user move the services 
> around. This seems also consistent with the goal of this branch:
> in read-only mode, the user can still interact with forms and widgets,
> but he will not produce server side changes.
> What do you think?

I like it.  Thank you.

> 
> > https://codereview.appspot.com/7095058/diff/1/app/store/env.js#newcode154
> > app/store/env.js:154: _send_rpc: function(op, callback,
> writePermissionRequired)
> > {
> > I wondered whether we ought to have a separate env for the readonly
behavior,
> > but I like what you did.  Seems pleasantly simple to me.
> 
> Cool!
> 
> > https://codereview.appspot.com/7095058/diff/1/app/store/env.js#newcode426
> > app/store/env.js:426: update_annotations: function(entity, data, callback) {
> > We need to "protect" this too.
> 
> Agreed.
> 
> > 
> > https://codereview.appspot.com/7095058/diff/1/app/store/env.js#newcode462
> > app/store/env.js:462: remove_annotations: function(entity, keys, callback) {
> > We need to "protect" this too.
> 
> Agreed.
> 
> 
> > https://codereview.appspot.com/7095058/diff/1/test/test_env.js#newcode219
> > test/test_env.js:219: Y.each(writeOperations, function(args, operation) {
> > My first inclination would have been to have a test for each write
operation,
> > with a test helper to make each test extremely small.  On reflection, that
> still
> > seems preferable to me.  Did you consider that option?  What do you think?
> 
> I am not sure.
> With a for loop we achieve:
> - less builerplate code;
> - faster test runs.
> But with separate tests:
> - isolation (a failure in a test does not prevent others to be executed);
> - possibility to customize a test case if required in the future.
> - any other?
> I am inclined to accept your suggestion, given that it should be quite 
> easy to make this change.

Cool.

As I mentioned, I found some problems on trunk so I am filing bugs and then will
return to doing a qa of the branch.  Tests passed for me, fwiw.

Gary

[teknico, 2013-01-15 16:45:43]

Land with changes.

Special thanks for all those method docs, and sorry for peppering the diff with
all those "No camel case needed " comments. :-)

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

https://codereview.appspot.com/7095058/diff/1/app/app.js#newcode223
app/app.js:223: readOnly: this.get('readOnly')});
No need for camel case here, "readonly" is an established technical word
nowadays. Almost 10M hits on Google <http://www.google.com/search?q="readonly">.

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

https://codereview.appspot.com/7095058/diff/1/app/config-debug.js#newcode13
app/config-debug.js:13: readOnly: false,
No camel case needed here either.

https://codereview.appspot.com/7095058/diff/1/app/config-debug.js#newcode14
app/config-debug.js:14: login_help: 'For this demonstration, use the password
"admin" to connect.'
[Unrelated] Uhm, we should have used camel case here, though.

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

https://codereview.appspot.com/7095058/diff/1/app/config-prod.js#newcode13
app/config-prod.js:13: readOnly: false,
No camel case needed here either.

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

https://codereview.appspot.com/7095058/diff/1/app/store/env.js#newcode25
app/store/env.js:25: 'readOnly': {value: false}
No camel case needed here either.

https://codereview.appspot.com/7095058/diff/1/app/store/env.js#newcode157
app/store/env.js:157: if (writePermissionRequired && this.get('readOnly')) {
No camel case needed here either.

https://codereview.appspot.com/7095058/diff/1/app/store/env.js#newcode159
app/store/env.js:159: var message = ('GUI is in read only mode and this
operation ' +
"...read-only..."?

https://codereview.appspot.com/7095058/diff/1/test/test_app.js
File test/test_app.js (right):

https://codereview.appspot.com/7095058/diff/1/test/test_app.js#newcode86
test/test_app.js:86: it('propagates the readOnly option from the configuration',
function() {
No camel case needed here either.

https://codereview.appspot.com/7095058/diff/1/test/test_app.js#newcode90
test/test_app.js:90: readOnly: true,
No camel case needed here either.

https://codereview.appspot.com/7095058/diff/1/test/test_app.js#newcode92
test/test_app.js:92: assert.isTrue(app.env.get('readOnly'));
No camel case needed here either.

https://codereview.appspot.com/7095058/diff/1/test/test_env.js
File test/test_env.js (right):

https://codereview.appspot.com/7095058/diff/1/test/test_env.js#newcode210
test/test_env.js:210: env.set('readOnly', true);
No camel case needed here either.

https://codereview.appspot.com/7095058/diff/1/test/test_env.js#newcode249
test/test_env.js:249: env.set('readOnly', true);
No camel case needed here either.

https://codereview.appspot.com/7095058/diff/1/undocumented
File undocumented (left):

https://codereview.appspot.com/7095058/diff/1/undocumented#oldcode25
undocumented:25: app/store/env.js:240 "set_config"
gary.poster wrote:
> The People from the Future thank you, 13 times.

+13 :-)

[gary.poster, 2013-01-15 18:06:25]

Land with changes.  QA and test runs went well other than issues already on
trunk.  Thank you!

Gary

[gary.poster, 2013-01-15 18:06:48]

Land with changes.  QA and test runs went well other than issues already on
trunk.  Thank you!

Gary

[frankban, 2013-01-15 18:09:59]

Please take a look.

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

https://codereview.appspot.com/7095058/diff/1/app/store/env.js#newcode159
app/store/env.js:159: var message = ('GUI is in read only mode and this
operation ' +
On 2013/01/15 16:45:43, teknico wrote:
> "...read-only..."?

Done.

https://codereview.appspot.com/7095058/diff/1/app/store/env.js#newcode426
app/store/env.js:426: update_annotations: function(entity, data, callback) {
On 2013/01/15 14:57:20, gary.poster wrote:
> We need to "protect" this too.

Done.

https://codereview.appspot.com/7095058/diff/1/app/store/env.js#newcode462
app/store/env.js:462: remove_annotations: function(entity, keys, callback) {
On 2013/01/15 14:57:20, gary.poster wrote:
> We need to "protect" this too.

Done.

https://codereview.appspot.com/7095058/diff/1/test/test_env.js
File test/test_env.js (right):

https://codereview.appspot.com/7095058/diff/1/test/test_env.js#newcode219
test/test_env.js:219: Y.each(writeOperations, function(args, operation) {
On 2013/01/15 14:57:20, gary.poster wrote:
> My first inclination would have been to have a test for each write operation,
> with a test helper to make each test extremely small.  On reflection, that
still
> seems preferable to me.  Did you consider that option?  What do you think?

Done.

https://codereview.appspot.com/7095058/diff/1/test/test_env.js#newcode250
test/test_env.js:250: Y.each(readOperations, function(args, operation) {
On 2013/01/15 14:57:20, gary.poster wrote:
> Same comment as above.

Done.

[frankban, 2013-01-15 18:30:01]

*** Submitted:

Support read-only mode in the GUI

The GUI can be configured so that it denies the 
execution of write operations. This branch 
introduces a check at Juju environment level:
if the user tries to change the environment and
the GUI is in read-only mode, the environment
object refuses to send the message to the server,
fires a permissionDenied event, logs the exception
and executes the provided callback (if any) 
passing an error event.
Subsequently, an error notification is created by
the app. In a pre-implementation call with Gary 
we decided that controls and widget can still be
displayed, they are just ineffective.

Other changes:

Added documentation for all modified 
objects/methods/functions.

Fixed a bug: the environment view was not 
propagating the getModelURL from the app to
the topology instance.
Also added a test for this bug.

Updated the destroy service callback to make
the destroy dialog disappear also when
an error occurs.

Fixed an intermittent failure when 
test_environment_view.js is run in isolation.

R=gary.poster, teknico
CC=
https://codereview.appspot.com/7095058