Annotations support for sandbox/fakebackend

Annotations support for sandbox/fakebackend

getAnnotations/updateAnnotations/removeAnnotations support including 
for environment.

https://code.launchpad.net/~bcsaller/juju-gui/inmem-anno/+merge/157581

(do not edit description out of merge proposal)

[bcsaller, 2013-04-08 05:28:49]

Please take a look.

[bcsaller, 2013-04-08 05:51:55]

Please take a look.

[gary.poster, 2013-04-08 14:11:34]

LGTM after addressing comments, one way or another.

Thank you!

Gary

https://codereview.appspot.com/8315046/diff/3001/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/8315046/diff/3001/app/models/models.js#newcode536
app/models/models.js:536: * Resolve from an id to a Database entity. The look
pattern
I'm not clear on what you mean by "look pattern."  Maybe "The pattern of the id
is such that..."?

https://codereview.appspot.com/8315046/diff/3001/app/models/models.js#newcode537
app/models/models.js:537: * is such that
I thought I saw that the new preferred docstring format is to not use preceding
asterisks in the interior lines.

https://codereview.appspot.com/8315046/diff/3001/app/models/models.js#newcode548
app/models/models.js:548: resolveModelByName: function(entityName) {
Is this really "resolveModelByAnnotationName"?  Or
"resolveModelByPyAnnotationName"? I think it would be good to clarify the scope
of the method, in the docs and possibly in the name.

https://codereview.appspot.com/8315046/diff/3001/app/models/models.js#newcode556
app/models/models.js:556: if (Y.Lang.isNumber(nameAsInt)) {
trivial in this context AFAICT, but parseInt's easygoing behavior
(parseInt('4ever') -> 4) makes me want this to be better defined. Also, I
suspect that the ids in the db are actually strings.  If I'm right about all
that...

if (/^\d+$/.test(entityName)) {
  return this.machines.getById(entityName)
}

https://codereview.appspot.com/8315046/diff/3001/app/models/models.js#newcode560
app/models/models.js:560: if (/\S+\/\d+/.test(entityName)) {
Again, probably trivial in this context, but I've found in the past that being
specific when I know specifics does help out in edge cases that you didn't
consider.  So, I'd prefer this regex:

/^\S+\/\d+$/

https://codereview.appspot.com/8315046/diff/3001/app/models/models.js#newcode564
app/models/models.js:564: return this.services.getById(entityName);
But this fall-through is why most or all of what I said is probably unimportant.
 Still cleaner, though.

https://codereview.appspot.com/8315046/diff/3001/app/store/env/fakebackend.js
File app/store/env/fakebackend.js (right):

https://codereview.appspot.com/8315046/diff/3001/app/store/env/fakebackend.js...
app/store/env/fakebackend.js:108: Return all of the recently anotated objects.
docstrings are supposed to be indented two lines now IIRC?

https://codereview.appspot.com/8315046/diff/3001/app/store/env/fakebackend.js...
app/store/env/fakebackend.js:111: @return {Object} A hash of the keys
'services', 'machines', 'units' and
docstring needs to be updated

https://codereview.appspot.com/8315046/diff/3001/app/store/env/fakebackend.js...
app/store/env/fakebackend.js:533: if (!this.get('authenticated')) {
thanks for adding this

https://codereview.appspot.com/8315046/diff/3001/app/store/env/fakebackend.js...
app/store/env/fakebackend.js:533: if (!this.get('authenticated')) {
I don't think we have any tests for expose or unexpose in the fakebackend at
all. :-/ It would be really cool if you could at least start a suite just with
the authentication tests.  Not required for this branch though.  I'll file a bug
for no tests of expose/unexpose once I verify my understanding.

https://codereview.appspot.com/8315046/diff/3001/app/store/env/fakebackend.js...
app/store/env/fakebackend.js:540: this.changes.services[service.get('id')] =
[service, true];
yes, thanks again.  and for repeats below.

https://codereview.appspot.com/8315046/diff/3001/app/store/env/fakebackend.js...
app/store/env/fakebackend.js:593: * @return {undefined} side-effects only.
returns hash with "error" string if there is an error, or undefined on success.

https://codereview.appspot.com/8315046/diff/3001/app/store/env/fakebackend.js...
app/store/env/fakebackend.js:605: if (entity.name === 'serviceUnit') {
machines are this way too, and so should be included as part of this condition
(entity.name === 'serviceUnit' || entity.name === 'whateverMachinesAreCalled').

https://codereview.appspot.com/8315046/diff/3001/app/store/env/fakebackend.js...
app/store/env/fakebackend.js:631: // Note the 's' for compatibility with other
This is only for the internal names in this.annotations, right?  If you'd prefer
to change this.annotations that would be AOK with me.

https://codereview.appspot.com/8315046/diff/3001/app/store/env/fakebackend.js...
app/store/env/fakebackend.js:660: if (entity.name === 'serviceUnit') {
or machines

https://codereview.appspot.com/8315046/diff/3001/app/store/env/fakebackend.js...
app/store/env/fakebackend.js:712: // Arrange delta stream updates.
The below is duplicated code from updateAnnotations.  Not necessary, but might
be nice to have a helper instead.  My rule is three duplications requires a
refactoring, and two suggests one. :-)

https://codereview.appspot.com/8315046/diff/3001/app/store/env/sandbox.js
File app/store/env/sandbox.js (right):

https://codereview.appspot.com/8315046/diff/3001/app/store/env/sandbox.js#new...
app/store/env/sandbox.js:300: // changeset.
In the go api, they are separate deltas but within the same changeset.  in the
Python API IIRC, they are combined into the same deltas but within the same
changeset.  Since we treat changesets effectively as transactions, the
separation is not important AFAIK.  Therefore, until proven otherwise, I think
what we have works, and I don't think we need this TODO.

https://codereview.appspot.com/8315046/diff/3001/app/store/env/sandbox.js#new...
app/store/env/sandbox.js:491: OP(this, 'updateAnnotations', ['entity', 'data'],
data);
Oh, cool, I hadn't seen your helper functions before.  Neat.

https://codereview.appspot.com/8315046/diff/3001/test/index.html
File test/index.html (right):

https://codereview.appspot.com/8315046/diff/3001/test/index.html#newcode17
test/index.html:17: mocha.setup({ignoreLeaks: false, timeout: 5000})
Any such reduction looks good to me, assuming it works on CI as well, but I am
curious about the story behind it.

https://codereview.appspot.com/8315046/diff/3001/test/test_fakebackend.js
File test/test_fakebackend.js (right):

https://codereview.appspot.com/8315046/diff/3001/test/test_fakebackend.js#new...
test/test_fakebackend.js:349: describe('FakeBackend.Annotations', function() {
I like tests verifying the fact the unauthenticated check is in place too.  I
won't make a bug fuss, but I think they would be nice.

https://codereview.appspot.com/8315046/diff/3001/test/test_fakebackend.js#new...
test/test_fakebackend.js:349: describe('FakeBackend.Annotations', function() {
Please add tests for nextAnnotations, and particularly for the fact that the
"nextAnnotations" data reflects the changes that we make below.

https://codereview.appspot.com/8315046/diff/3001/test/test_fakebackend.js#new...
test/test_fakebackend.js:363: {'foo': 'bar',
trivial: if you add a space after the curly brace ("{ 'foo': 'bar',", subsequent
lines line up better.

https://codereview.appspot.com/8315046/diff/3001/test/test_model.js
File test/test_model.js (right):

https://codereview.appspot.com/8315046/diff/3001/test/test_model.js#newcode205
test/test_model.js:205: it('must be able to resolve models by their name',
function() {
Add a test for machines?

https://codereview.appspot.com/8315046/diff/3001/test/test_sandbox.js
File test/test_sandbox.js (right):

https://codereview.appspot.com/8315046/diff/3001/test/test_sandbox.js#newcode...
test/test_sandbox.js:1041: delta = Y.JSON.parse(delta.data);
I figured you'd have delta tests here.  I'd really prefer having the core
functionality tested in the fakebackend, and only the api-specific behavior
tested in the py api.  However, my reasoning is based on cleanliness and on the
future Go API sandbox work, which is not a requirement now; and I'd like to see
this land asap.  So, maybe put a low-priority card in the kanban board, and we
can get to this if we have time later?  If not, we'll deal with it when and if
we write the go side of the sandbox.

[bcsaller, 2013-04-08 19:14:11]

Thanks for the review, good points.

https://codereview.appspot.com/8315046/diff/3001/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/8315046/diff/3001/app/models/models.js#newcode536
app/models/models.js:536: * Resolve from an id to a Database entity. The look
pattern
On 2013/04/08 14:11:34, gary.poster wrote:
> I'm not clear on what you mean by "look pattern."  Maybe "The pattern of the
id
> is such that..."?

Should have read 'lookup' and it refers to this pattern of resolving from name
to model without regard to type.

https://codereview.appspot.com/8315046/diff/3001/app/models/models.js#newcode537
app/models/models.js:537: * is such that
On 2013/04/08 14:11:34, gary.poster wrote:
> I thought I saw that the new preferred docstring format is to not use
preceding
> asterisks in the interior lines.

A number of us have editors that automatically put this these in. I'd suggest
that it doesn't matter much as it neither greatly impacts readability or output.
The YUI codebase isn't consistent in this respect either and that has been w/o
impact. Still happy to discuss it more.

https://codereview.appspot.com/8315046/diff/3001/app/models/models.js#newcode548
app/models/models.js:548: resolveModelByName: function(entityName) {
On 2013/04/08 14:11:34, gary.poster wrote:
> Is this really "resolveModelByAnnotationName"?  Or
> "resolveModelByPyAnnotationName"? I think it would be good to clarify the
scope
> of the method, in the docs and possibly in the name.

Its more like getModelByEntityName and not limited to annotations.

https://codereview.appspot.com/8315046/diff/3001/app/models/models.js#newcode556
app/models/models.js:556: if (Y.Lang.isNumber(nameAsInt)) {
On 2013/04/08 14:11:34, gary.poster wrote:
> trivial in this context AFAICT, but parseInt's easygoing behavior
> (parseInt('4ever') -> 4) makes me want this to be better defined. Also, I
> suspect that the ids in the db are actually strings.  If I'm right about all
> that...
> 
> if (/^\d+$/.test(entityName)) {
>   return this.machines.getById(entityName)
> }

Done.

https://codereview.appspot.com/8315046/diff/3001/app/models/models.js#newcode560
app/models/models.js:560: if (/\S+\/\d+/.test(entityName)) {
On 2013/04/08 14:11:34, gary.poster wrote:
> Again, probably trivial in this context, but I've found in the past that being
> specific when I know specifics does help out in edge cases that you didn't
> consider.  So, I'd prefer this regex:
> 
> /^\S+\/\d+$/

Done.

https://codereview.appspot.com/8315046/diff/3001/app/store/env/fakebackend.js
File app/store/env/fakebackend.js (right):

https://codereview.appspot.com/8315046/diff/3001/app/store/env/fakebackend.js...
app/store/env/fakebackend.js:108: Return all of the recently anotated objects.
On 2013/04/08 14:11:34, gary.poster wrote:
> docstrings are supposed to be indented two lines now IIRC?

Done.

https://codereview.appspot.com/8315046/diff/3001/app/store/env/fakebackend.js...
app/store/env/fakebackend.js:111: @return {Object} A hash of the keys
'services', 'machines', 'units' and
On 2013/04/08 14:11:34, gary.poster wrote:
> docstring needs to be updated

Done.

https://codereview.appspot.com/8315046/diff/3001/app/store/env/fakebackend.js...
app/store/env/fakebackend.js:593: * @return {undefined} side-effects only.
On 2013/04/08 14:11:34, gary.poster wrote:
> returns hash with "error" string if there is an error, or undefined on
success.

Done.

https://codereview.appspot.com/8315046/diff/3001/app/store/env/fakebackend.js...
app/store/env/fakebackend.js:605: if (entity.name === 'serviceUnit') {
On 2013/04/08 14:11:34, gary.poster wrote:
> machines are this way too, and so should be included as part of this condition
> (entity.name === 'serviceUnit' || entity.name ===
'whateverMachinesAreCalled').

Ahh, good catch, it appears it didn't even have a name property, added one.

https://codereview.appspot.com/8315046/diff/3001/app/store/env/fakebackend.js...
app/store/env/fakebackend.js:631: // Note the 's' for compatibility with other
On 2013/04/08 14:11:34, gary.poster wrote:
> This is only for the internal names in this.annotations, right?  If you'd
prefer
> to change this.annotations that would be AOK with me.
Changed to the more sensible 'annotations' throughout

https://codereview.appspot.com/8315046/diff/3001/app/store/env/fakebackend.js...
app/store/env/fakebackend.js:712: // Arrange delta stream updates.
On 2013/04/08 14:11:34, gary.poster wrote:
> The below is duplicated code from updateAnnotations.  Not necessary, but might
> be nice to have a helper instead.  My rule is three duplications requires a
> refactoring, and two suggests one. :-)

Added wrappers around models.getAnnotation/setAnnotation and _getAnnotationGroup
locally. It is cleaner, thanks.

https://codereview.appspot.com/8315046/diff/3001/app/store/env/sandbox.js
File app/store/env/sandbox.js (right):

https://codereview.appspot.com/8315046/diff/3001/app/store/env/sandbox.js#new...
app/store/env/sandbox.js:300: // changeset.
On 2013/04/08 14:11:34, gary.poster wrote:
> In the go api, they are separate deltas but within the same changeset.  in the
> Python API IIRC, they are combined into the same deltas but within the same
> changeset.  Since we treat changesets effectively as transactions, the
> separation is not important AFAIK.  Therefore, until proven otherwise, I think
> what we have works, and I don't think we need this TODO.

Yeah, It was written as WIP and not removed. The getAnnotations call should be
able to handle what we need moving forward.

https://codereview.appspot.com/8315046/diff/3001/app/store/env/sandbox.js#new...
app/store/env/sandbox.js:491: OP(this, 'updateAnnotations', ['entity', 'data'],
data);
On 2013/04/08 14:11:34, gary.poster wrote:
> Oh, cool, I hadn't seen your helper functions before.  Neat.

I like the way those turned out as well. It is however the reason that some
backend method that said 'side-effects only' needed to be updated. This expects
a uniform interface.

https://codereview.appspot.com/8315046/diff/3001/test/index.html
File test/index.html (right):

https://codereview.appspot.com/8315046/diff/3001/test/index.html#newcode17
test/index.html:17: mocha.setup({ignoreLeaks: false, timeout: 5000})
On 2013/04/08 14:11:34, gary.poster wrote:
> Any such reduction looks good to me, assuming it works on CI as well, but I am
> curious about the story behind it.

During development async tests often fail due to timeout as they are not
properly triggered, 5 seconds seems generous for most things and tests that
actually take 20 seconds to run most likely are integration and not unit tests
and belong in Selenium.

https://codereview.appspot.com/8315046/diff/3001/test/test_fakebackend.js
File test/test_fakebackend.js (right):

https://codereview.appspot.com/8315046/diff/3001/test/test_fakebackend.js#new...
test/test_fakebackend.js:349: describe('FakeBackend.Annotations', function() {
On 2013/04/08 14:11:34, gary.poster wrote:
> I like tests verifying the fact the unauthenticated check is in place too.  I
> won't make a bug fuss, but I think they would be nice.

Done.

https://codereview.appspot.com/8315046/diff/3001/test/test_fakebackend.js#new...
test/test_fakebackend.js:349: describe('FakeBackend.Annotations', function() {
On 2013/04/08 14:11:34, gary.poster wrote:
> Please add tests for nextAnnotations, and particularly for the fact that the
> "nextAnnotations" data reflects the changes that we make below.

Done.

https://codereview.appspot.com/8315046/diff/3001/test/test_model.js
File test/test_model.js (right):

https://codereview.appspot.com/8315046/diff/3001/test/test_model.js#newcode205
test/test_model.js:205: it('must be able to resolve models by their name',
function() {
On 2013/04/08 14:11:34, gary.poster wrote:
> Add a test for machines?

Done.

https://codereview.appspot.com/8315046/diff/3001/test/test_sandbox.js
File test/test_sandbox.js (right):

https://codereview.appspot.com/8315046/diff/3001/test/test_sandbox.js#newcode...
test/test_sandbox.js:1041: delta = Y.JSON.parse(delta.data);
On 2013/04/08 14:11:34, gary.poster wrote:
> I figured you'd have delta tests here.  I'd really prefer having the core
> functionality tested in the fakebackend, and only the api-specific behavior
> tested in the py api.  However, my reasoning is based on cleanliness and on
the
> future Go API sandbox work, which is not a requirement now; and I'd like to
see
> this land asap.  So, maybe put a low-priority card in the kanban board, and we
> can get to this if we have time later?  If not, we'll deal with it when and if
> we write the go side of the sandbox.
My thinking here was that with the name mapping and the special casing around
env it was worth making sure things mapped out as expected. I can add a card
though.

[bcsaller, 2013-04-08 19:15:51]

Please take a look.

[jeff.pihach, 2013-04-08 23:41:21]

LGTM Thanks for doing all of that :)

[bcsaller, 2013-04-08 23:46:19]

*** Submitted:

Annotations support for sandbox/fakebackend

getAnnotations/updateAnnotations/removeAnnotations support including 
for environment.

R=gary.poster, jeff.pihach
CC=
https://codereview.appspot.com/8315046