Don't update jabberId when renaming a host.

Don't update jabberId when renaming a host.

Previous the webapp specified jabberId in the update request when a host
is rename. To the directory this looks as updated jabberId, and so the
status is set to ONLINE. Another problem was that update request was
also setting the status field, but it shouldn't.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111469

[Jamie, 2011-11-23 20:53:23]

http://codereview.chromium.org/8679017/diff/1/remoting/webapp/me2mom/host_lis...
File remoting/webapp/me2mom/host_list.js (left):

http://codereview.chromium.org/8679017/diff/1/remoting/webapp/me2mom/host_lis...
remoting/webapp/me2mom/host_list.js:125: if (host.hostName && host.hostId &&
host.status && host.jabberId &&
I think we still need to check for jabberId. The purpose of this check is to
make sure that the data we get back from Apiary really does conform to the type
we're expecting. Since we refer to jabberId elsewhere, we need to make sure that
the field exists.

Ideally, this should be a static method on remoting.Host, to keep the check
close to the declaration, but that doesn't really belong in this CL.

[Sergey Ulanov, 2011-11-23 21:10:47]

http://codereview.chromium.org/8679017/diff/1/remoting/webapp/me2mom/host_lis...
File remoting/webapp/me2mom/host_list.js (left):

http://codereview.chromium.org/8679017/diff/1/remoting/webapp/me2mom/host_lis...
remoting/webapp/me2mom/host_list.js:125: if (host.hostName && host.hostId &&
host.status && host.jabberId &&
On 2011/11/23 20:53:23, Jamie wrote:
> I think we still need to check for jabberId. The purpose of this check is to
> make sure that the data we get back from Apiary really does conform to the
type
> we're expecting. Since we refer to jabberId elsewhere, we need to make sure
that
> the field exists.

JabberId may not be set if the host was registered, but never started. Maybe we
should set it to an empty string in this case?

> 
> Ideally, this should be a static method on remoting.Host, to keep the check
> close to the declaration, but that doesn't really belong in this CL.

[Sergey Ulanov, 2011-11-23 21:16:50]

http://codereview.chromium.org/8679017/diff/1/remoting/webapp/me2mom/host_lis...
File remoting/webapp/me2mom/host_list.js (left):

http://codereview.chromium.org/8679017/diff/1/remoting/webapp/me2mom/host_lis...
remoting/webapp/me2mom/host_list.js:125: if (host.hostName && host.hostId &&
host.status && host.jabberId &&
On 2011/11/23 21:10:47, sergeyu wrote:
> On 2011/11/23 20:53:23, Jamie wrote:
> > I think we still need to check for jabberId. The purpose of this check is to
> > make sure that the data we get back from Apiary really does conform to the
> type
> > we're expecting. Since we refer to jabberId elsewhere, we need to make sure
> that
> > the field exists.
> 
> JabberId may not be set if the host was registered, but never started. Maybe
we
> should set it to an empty string in this case?
> 
> > 
> > Ideally, this should be a static method on remoting.Host, to keep the check
> > close to the declaration, but that doesn't really belong in this CL.
> 


Reverted this change for now. Will address in a separate CL

[Jamie, 2011-11-23 21:17:49]

On 2011/11/23 21:10:47, sergeyu wrote:
>
http://codereview.chromium.org/8679017/diff/1/remoting/webapp/me2mom/host_lis...
> File remoting/webapp/me2mom/host_list.js (left):
> 
>
http://codereview.chromium.org/8679017/diff/1/remoting/webapp/me2mom/host_lis...
> remoting/webapp/me2mom/host_list.js:125: if (host.hostName && host.hostId &&
> host.status && host.jabberId &&
> On 2011/11/23 20:53:23, Jamie wrote:
> > I think we still need to check for jabberId. The purpose of this check is to
> > make sure that the data we get back from Apiary really does conform to the
> type
> > we're expecting. Since we refer to jabberId elsewhere, we need to make sure
> that
> > the field exists.
> 
> JabberId may not be set if the host was registered, but never started. Maybe
we
> should set it to an empty string in this case?

Should we be displaying the host in that case? I'd say not because you can't
connect to not, but neither is it off-line in the same sense that a computer
that's stopped heartbeating is. Do you have a work-flow in mind that would
feature unstarted, registered hosts?

[Sergey Ulanov, 2011-11-23 21:28:29]

On Wed, Nov 23, 2011 at 1:17 PM, <jamiewalch@chromium.org> wrote:

> On 2011/11/23 21:10:47, sergeyu wrote:
>
> http://codereview.chromium.**org/8679017/diff/1/remoting/**
>
webapp/me2mom/host_list.js<http://codereview.chromium.org/8679017/diff/1/remoting/webapp/me2mom/host_list.js>
>
>> File remoting/webapp/me2mom/host_**list.js (left):
>>
>
>
> http://codereview.chromium.**org/8679017/diff/1/remoting/**
>
webapp/me2mom/host_list.js#**oldcode125<http://codereview.chromium.org/8679017/diff/1/remoting/webapp/me2mom/host_list.js#oldcode125>
>
>> remoting/webapp/me2mom/host_**list.js:125: if (host.hostName &&
>> host.hostId &&
>> host.status && host.jabberId &&
>> On 2011/11/23 20:53:23, Jamie wrote:
>> > I think we still need to check for jabberId. The purpose of this check
>> is to
>> > make sure that the data we get back from Apiary really does conform to
>> the
>> type
>> > we're expecting. Since we refer to jabberId elsewhere, we need to make
>> sure
>> that
>> > the field exists.
>>
>
>  JabberId may not be set if the host was registered, but never started.
>> Maybe
>>
> we
>
>> should set it to an empty string in this case?
>>
>
> Should we be displaying the host in that case? I'd say not because you
> can't
> connect to not, but neither is it off-line in the same sense that a
> computer
> that's stopped heartbeating is. Do you have a work-flow in mind that would
> feature unstarted, registered hosts?
>

One example that comes to mind is when user's internet connection allows
HTTP/HTTPS connections only - in this case the host will fail to make XMPP
connection to talk after it is registered. I believe it is better to show
it as Offline - user can always delete it if it is not needed.

In any case, I removed this change from this CL.


>
http://codereview.chromium.**org/8679017/<http://codereview.chromium.org/8679...
>

[Jamie, 2011-11-23 21:34:18]

lgtm

http://codereview.chromium.org/8679017/diff/3002/remoting/webapp/me2mom/host_...
File remoting/webapp/me2mom/host_list.js (right):

http://codereview.chromium.org/8679017/diff/3002/remoting/webapp/me2mom/host_...
remoting/webapp/me2mom/host_list.js:126: && host.publicKey) {
nit: This change now looks a little out-of-place.

[Sergey Ulanov, 2011-11-24 00:15:29]

http://codereview.chromium.org/8679017/diff/3002/remoting/webapp/me2mom/host_...
File remoting/webapp/me2mom/host_list.js (right):

http://codereview.chromium.org/8679017/diff/3002/remoting/webapp/me2mom/host_...
remoting/webapp/me2mom/host_list.js:126: && host.publicKey) {
On 2011/11/23 21:34:19, Jamie wrote:
> nit: This change now looks a little out-of-place.

Done.

[Jamie, 2011-11-24 00:19:46]

lgtm