Allows multiple ghosts with the same name

Allows multiple ghosts with the same name

This fixes bug #1226253

https://code.launchpad.net/~hatch/juju-gui/multi-ghost-1226253/+merge/186115

(do not edit description out of merge proposal)

[jeff.pihach, 2013-09-17 17:37:21]

Please take a look.

[gary.poster, 2013-09-17 18:10:00]

I like the approach but I think there's some things to tweak discuss.

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

https://codereview.appspot.com/13457052/diff/1/app/models/models.js#newcode252
app/models/models.js:252: return value;
Ah, clever. :-)

https://codereview.appspot.com/13457052/diff/1/app/models/models.js#newcode414
app/models/models.js:414: id: Math.floor(Math.random() * 100000000),
huh.  allow me my paranoia: how 'bout instead we generate the id separately just
above here and verify that there isn't one around?  Also, I suggest using a
disallowed character in this name, like "(" or something, so it will never clash
with something that comes from juju itself.

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

https://codereview.appspot.com/13457052/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:182: id: name,
wait a sec!  we don't need to do this, yeah?  and it causes the issue you
describe above, and reintroduces the problem you are trying to fix, right? 
Let's only copy over the id to the data when we save, and to the model when we
have successfully saved.

[jeff.pihach, 2013-09-17 18:28:29]

Thanks for the review - comments below

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

https://codereview.appspot.com/13457052/diff/1/app/models/models.js#newcode252
app/models/models.js:252: return value;
thanks

https://codereview.appspot.com/13457052/diff/1/app/models/models.js#newcode414
app/models/models.js:414: id: Math.floor(Math.random() * 100000000),
This code generates a random 8 digit integer - I figured that it would be pretty
unlikely that in the number of ghosts a user will ever have deployed at once
that it would generate an identical id :) This Id is only ever used until the
user clicks 'deploy' at which point it is then the serviceName as before.

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

https://codereview.appspot.com/13457052/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:182: id: name,
we 'must' set the id because that is what d3 indexes off of, so it needs to
change to have d3 update the element in the canvas. It then updates the
displayName because that's the name we want it to show.

[jeff.pihach, 2013-09-17 21:42:18]

Please take a look.

[gary.poster, 2013-09-17 23:37:43]

LGTM with small suggestion.  Thank you very much!

https://codereview.appspot.com/13457052/diff/8001/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/13457052/diff/8001/app/models/models.js#newcod...
app/models/models.js:414: do {
nice.  Thank you, and thanks for not including comments like "I can't believe
Gary made me do this" and "it's good enough for Git but not good enough for
Gary" and so on. :-)

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

https://codereview.appspot.com/13457052/diff/8001/app/views/ghost-inspector.j...
app/views/ghost-inspector.js:302: displayName: serviceName,
I'd suggest setting displayName to undefined here, in order for the usual
displayName code to be triggered once this is a real service.  Just a
suggestion.

[jeff.pihach, 2013-09-18 13:26:01]

Thanks for the review!

https://codereview.appspot.com/13457052/diff/8001/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/13457052/diff/8001/app/models/models.js#newcod...
app/models/models.js:414: do {
haha np!

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

https://codereview.appspot.com/13457052/diff/8001/app/views/ghost-inspector.j...
app/views/ghost-inspector.js:302: displayName: serviceName,
On 2013/09/17 23:37:43, gary.poster wrote:
> I'd suggest setting displayName to undefined here, in order for the usual
> displayName code to be triggered once this is a real service.  Just a
> suggestion.

Done.

[jeff.pihach, 2013-09-18 13:40:13]

*** Submitted:

Allows multiple ghosts with the same name

This fixes bug #1226253

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