Open JeremyTubongbanua opened 2 months ago
Could not get to this in the previous sprint, will be taking this up first in PR88.
Hi @srieteja I've been looking at this as it came up in another context. I'll pick it up this sprint.
Summary of the current behaviour for 'update' notifications
NotifyVerbHandler._getIsEncrypted
ignores whatever it has been sent and always sets isEncrypted to 'true'NotificationServiceImpl.notify
always encrypts the value but does not set isEncrypted in the metadata. Thus, usually (unless app code explicitly sets metadata.isEncrypted to true), metadata.isEncrypted is apparently false (default value) but the value has in fact been encryptedSummary of the changes I have in mind:
encryptValue
default it to trueencryptValue
In the distant future, once all of the client-side sdks are doing the right thing with their generated notify commands, we can default isEncrypted to 'false' if it is not set in the command but for now we have to continue to default it to 'true' so we don't break the world.
I need to think a little more about the sequencing of the at_commons and at_client changes; what we want to ensure is that we only ever send a notify command with isEncrypted:false when that is genuinely the case. We will likely need to do an at_commons major version change, although we may be able to do something fiddly in at_client to ensure the right behaviour
In the above image, I sent notification with
isEncrypted:false
Then in the receiving notification has
isEncrypted:true
Intended behaviour:
Running
notify:isEncrypted:false:<...>
Should have a receiving notification of:
notification:{..., "isEncrypted":false, ...}