SmartThingsEdgeDrivers icon indicating copy to clipboard operation
SmartThingsEdgeDrivers copied to clipboard

HCS5008 - Remove AudioNotification Capability for samsung-audio driver

Open satyajitanand opened this issue 1 year ago • 10 comments

https://smartthings.atlassian.net/browse/HCS-5008: PM wants to exclude audioNotification capability.

Tested this locally. Now, Routine is not allowing options to play message on speaker. Hope this is the desired result as per HCS-5008. Kindly review.

satyajitanand avatar Jul 02 '24 11:07 satyajitanand

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

:white_check_mark: lelandblue
:x: satyajitanand
You have signed the CLA already but the status is still pending? Let us recheck it.

CLAassistant avatar Jul 02 '24 11:07 CLAassistant

Duplicate profile check: Passed - no duplicate profiles detected.

github-actions[bot] avatar Jul 02 '24 11:07 github-actions[bot]

Channel deleted.

github-actions[bot] avatar Jul 02 '24 11:07 github-actions[bot]

Test Results

   61 files    375 suites   0s :stopwatch: 1 821 tests 1 821 :white_check_mark: 0 :zzz: 0 :x: 3 170 runs  3 170 :white_check_mark: 0 :zzz: 0 :x:

Results for commit d55a9a21.

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar Jul 02 '24 11:07 github-actions[bot]

Minimum allowed coverage is 90%

Generated by :monkey: cobertura-action against d55a9a21a823b81e10d2b4d5253ce3d484b6fbf8

github-actions[bot] avatar Jul 02 '24 11:07 github-actions[bot]

@greens @cjswedes @inasail Can you pls review.

satyajitanand avatar Jul 03 '24 05:07 satyajitanand

@cjswedes that's actually a way better plan.

greens avatar Jul 03 '24 17:07 greens

@cjswedes - This is the data that the cloud team analyzed http used cases. https://smartthings.atlassian.net/wiki/spaces/AU/pages/3274802589/Audio+notification+HTTP

Korean : HTTP 호출 중인 스피커는 모두 동일한 samsung-audio edge driver를 사용 중에 있음 All speakers making a HTTP request are using samsung-audio edge driver.

And in case of samsung-audio edge driver, it replaces https with http when sending the audio uri to all speakers that use this edge driver. samsung-audio profile is only used by samsung-audio edge driver so I don't think that it will affect to other edge drivers. If I'm wrong, please correct me.

The concern case where it updates profile only when error happen is that, if the user creates automation using samsung-audio speaker, the user can see audioNotification capability on that device. The automation will be executed later and then, the profile will be updated with new one (without audioNotification) and the user cannot hear the notification. That will make the user to confuse.

I think that it would be better to update a profile before user tries to use audioNotification capability instead of error handling. Still it would be possible to replace current samsung-audio profile with new samsung-audio-notification-not-support profile when driver is updated if that is the preferred way.

@cjswedes , @greens - Could you please share your opinion how to update samsung-audio profile not to support audioNotification capability ?

CC : @inasail

sy39ju avatar Jul 04 '24 08:07 sy39ju

@cjswedes Thank for your opinion. @sy39ju Thanks for detailed explanation.

All the speakers(16,000) that using 'samsung-audio' driver can't use https because certificate was already expired. Among them, only 20 devices are using 'audioNotificaton' functionality for 10 days.(6/7~6/17) Security team ordered not to use http anymore because of security issue. So PM decided not to use this functionality without notice to the users. If customer make a complain about this, then CS team will handle it.

So, I think we don't need to make another profile and error handling.

inasail avatar Jul 04 '24 09:07 inasail

All the speakers(16,000) that using 'samsung-audio' driver can't use https because certificate was already expired. Among them, only 20 devices are using 'audioNotificaton' functionality for 10 days.(6/7~6/17) Security team ordered not to use http anymore because of security issue. So PM decided not to use this functionality without notice to the users. If customer make a complain about this, then CS team will handle it.

Thank you for the clarity here @inasail. Given this I am fine with the change as is. Refusing http connections on the server side will also work to just stop this functionality, and IMO we could just do that as soon as possible, and then get the profile updated over the next couple weeks with the normal release cycle.

Im going to defer my approval to @varzac or @lelandblue so that they have visibility into us moving forward in this way.

cjswedes avatar Jul 05 '24 13:07 cjswedes