/new_things improvements #103
Conversation
models/things.js
Outdated
| @@ -116,7 +116,7 @@ var Things = { | |||
|
|
|||
| // Notify each open websocket of the new Thing | |||
| this.websockets.forEach(function(socket) { | |||
| socket.send(JSON.stringify(thing.getDescription())); | |||
| socket.send(JSON.stringify(thing)); | |||
There was a problem hiding this comment.
This seems like a bad idea, you are serializing the whole Thing object which will include methods as well as properties. Currently the two are quite similar, but that will not continue to be the case.
The idea of getDescription() is to get a description of a Thing suitable for serialization, so if you want to send the ID as part of that description you should add it in getDescription(). However, the omission of the ID is intentional because at the web API level we try to represent Things by their Thing URL rather than their internal ID.
Can you explain more fully the problem you are trying to solve here? With ZigBee and Z-Wave Things are already "paired" once they get to this stage and a Thing URL can be generated for them. Is this different in the adapter(s) you are implementing?
There was a problem hiding this comment.
Both NewThingsController.get('/') and NewThingsController.ws('/') send directly JSON-ed things (https://github.com/moziot/gateway/blob/master/controllers/new_things_controller.js#L24 and https://github.com/moziot/gateway/blob/master/controllers/new_things_controller.js#L42). I didn't realize this was not the intended behavior and can modify the PR accordingly.
The problem I'm trying to solve here is that the Add New Thing UI needs to have some id for the Thing it is trying to save for the POST /things to work. This works in the normal cases of NewThingsController.get('/') and NewThingsController.ws('/') because they send the full Thing object for all Things currently in the database. Where it doesn't work is when a Thing is added with the Add New Thing UI open. In this case, Things.handleNewThing will be invoked and will call getDescription before sending the Thing object. Neither of the other instances of sending Thing objects calls getDescription.
This matters because if the Philips Hue is paired while the Add New Thing UI is open it will add a Thing for each bulb. The adding of these Things is communicated to the UI through handleNewThing which invokes getDescription.
There was a problem hiding this comment.
I was a bit wrong about the behavior of Things.getNewThings(). The Thing descriptions returned by it are actually from device.asThing, not true Thing objects.
I'm not sure what the best way to fix this is, but I don't think that adding the id key to getDescription is correct. Manually adding back the id key also feels less-than-ideal.
There was a problem hiding this comment.
There's a difference between a "new thing" and a "thing". New things come from the adapter manager and get their thing description from Device.asThing(). Things come from the database and get their description from Thing.getDescription().
New things have an internal ID in their thing description until they are turned into things in the database which have a URL to identify them in the web API. Things using the Thing Description format we're aiming to standardise do not have an ID property because their URL is their ID.
What's happening here is the handleNewThing() is using the Thing constructor to validate the description of the new thing, which ends up stripping out the internal ID. I don't actually know how this is working because the client side needs the ID to generate the POST request https://github.com/moziot/gateway/blob/master/static/js/add-thing.js#L115 (it might be that you've found a bug).
I'd suggest that either you remove the call to the Thing() constructor in handleNewThing and pass the new thing description (including its id) directly to the websocket message as is done elsewhere, or you simply add back in the ID after it has been validated.
It's good to be getting extra eyes on this code to find these inconsistencies :)
There was a problem hiding this comment.
Thanks for the clarification! I think I'll go with the first approach. I do think I've found a bug :)
| @@ -12,6 +12,7 @@ | |||
| /* globals assert */ | |||
|
|
|||
| process.env.NODE_ENV = 'test'; | |||
| process.env.NODE_TLS_REJECT_UNAUTHORIZED = '0'; | |||
There was a problem hiding this comment.
The WebSocket test needs to connect to the original https server because express-ws is bound to it. All the other tests are okay because chai-http creates a new http server by default.
| @@ -145,4 +145,8 @@ var Things = { | |||
| } | |||
| }; | |||
|
|
|||
| AdapterManager.on('thing-added', function(thing) { | |||
| Things.handleNewThing(thing); | |||
| }); | |||
There was a problem hiding this comment.
See my comments in #99 regarding pairing multiple devices at a time. In principle this makes sense, but we need to think about the potential implications for other adapters.
There was a problem hiding this comment.
Something I want to note here because I just confirmed it for myself: the emitted thing-added event is sending over the correct device.asThing() format, not a raw Thing object.
The Actions model uses a one-shot promise to wait for AdapterManager to add new things. What this means is that if an Adapter adds more than one new Device in response to startPairing, the user will only be able to see the first Device in the Add New Things menu. This is because the Actions model only calls Things.handleNewThing on the first Device the AdapterManager finds (which resolves the Promise returned by addNewThing). Instead of this one-shot promise approach, I attach a listener for the 'thing-added' event to the AdapterManager which properly sends all new things over the websocket connection to the UI.
hobinjk
force-pushed
the
new_things-improvements
branch
from
f1abb13
to
2e0a3f3
Compare
|
Superseded by #108 |
This fixes several issues with the behavior of /new_things. First, it updates
handleNewThingto send the Thing instead of a description of the thing. This fixes a case where pairing a new device would be impossible due to the description's lack of its corresponding thing's id.Second, it replaces the one-shot AdapterManager promise with a listener on 'thing-added' events. This is explained in more detail at #99 (comment) and #99 in general.
It finally adds a test for the updated behavior which will fail if either fix is not present.