Make sure ValueNotification includes the latest event data

[raman325]

In #154 the order of operations for creating the ValueNotification was wrong and led to the value that came in the notification being returned as the value that we was previously in the state (see home-assistant/core#47432). This fixes the order of operations.

I could use some help writing some good test cases for this one, I don't think we have enough given all the issues that have occurred as we try to fix this

[raman325]

@kpine can you take a look at this and see if it makes sense since you spotted this immediately?

[kpine]

This is working fine for me. I took a slightly different approach, same result.

        # append metadata if value metadata is available
        value = self.values.get(_get_value_id_from_dict(self, event.data["args"]))
        if value:
            value.update(event.data["args"])
            value_notification = ValueNotification(self, value.data)
        else:
            value_notification = ValueNotification(self, event.data["args"])
        event.data["value_notification"] = value_notification

[kpine]

I was wondering, why is value modified at all? Isn't it needed just to get the metadata? These notifications are supposed to be events or stateless values, not value updates.

[MartinHjelmare]

To test this I'd create a Value instance on the node and then create an event coming in with new data, that should trigger a ValueNotification event. The ValueNotification data should then be created with the Value instance data as base but be updated with the new data from the event. So we need to make sure that we can assert that difference when creating the initial Value and event data

[MartinHjelmare]

I was wondering, why is value modified at all? Isn't it needed just to get the metadata? These notifications are supposed to be events or stateless values, not value updates.

Yes, it doesn't sound like the Value state should be updated when reading the docs about ValueNotification event:
https://zwave-js.github.io/node-zwave-js/#/api/node?id=quotvalue-notificationquot

[jcam]

Is there any pre-existing method to do this?
value_notification.update(value.metadata)
I imagine value.metadata doesn't exist yet...

[jcam]

ah, perhaps it does...
so maybe this value_notification.metadata.update(value.metadata)

[MartinHjelmare]

value_notification.metadata.update(value.metadata.data)

[jcam]

Just tested on my prod setup, this works, and resolves home-assistant/core#47432

value_notification.metadata.update(value.metadata.data)

[raman325]

To test this I'd create a Value instance on the node and then create an event coming in with new data, that should trigger a ValueNotification event. The ValueNotification data should then be created with the Value instance data as base but be updated with the new data from the event. So we need to make sure that we can assert that difference when creating the initial Value and event data

So it looks like my tests are missing a value in the args part of the value notification event, and I don't test the value explicitly. If I were to add it with a different value from the Values value, and then check it, would that be sufficient?

[raman325]

To test this I'd create a Value instance on the node and then create an event coming in with new data, that should trigger a ValueNotification event. The ValueNotification data should then be created with the Value instance data as base but be updated with the new data from the event. So we need to make sure that we can assert that difference when creating the initial Value and event data

So it looks like my tests are missing a value in the args part of the value notification event, and I don't test the value explicitly. If I were to add it with a different value from the Values value, and then check it, would that be sufficient?

actually, the Value in my test data doesn't even have a value, so if I add a value to the notification, we can guarantee it's coming from the notification event if the ValueNotification has a value

[raman325]

value_notification.metadata.update(value.metadata.data)

The problem with this approach is that ValueNotification.endpoint returns None with no other changes (this is why the tests in my latest commit fail). There are two ways to solve this:

1. Update the entire value_notification.data. This may have unintended side effects but not sure.
2. Update the endpoint property to return 0 by default. I think this is safe because every value has to be on an endpoint, doesn't it?

[jcam]

I think the endpoint is not included from the ValueNotification, so the endpoint shouldn't be included in the HA event?

[raman325]

I think the endpoint is not included from the ValueNotification, so the endpoint shouldn't be included in the HA event?

The notification is for a value that does have an endpoint. There's a third option, see my upcoming commit

[kpine]

Looks good!

FYI, my scene activation and basic set events do include endpoint from the server.