SmartThingsCommunity / SmartThingsEdgeDrivers

Apache License 2.0
251 stars 442 forks source link

HCS5008 - Remove AudioNotification Capability for samsung-audio driver #1490

Open satyajitanand opened 1 week ago

satyajitanand commented 1 week ago

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.

CLAassistant commented 1 week ago

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

github-actions[bot] commented 1 week ago

Duplicate profile check: Passed - no duplicate profiles detected.

github-actions[bot] commented 1 week ago

Invitation URL: https://bestow-regional.api.smartthings.com/invite/gpjGgzYLWj4w

github-actions[bot] commented 1 week ago

Test Results

   60 files    377 suites   0s :stopwatch: 1 821 tests 1 821 :white_check_mark: 0 :zzz: 0 :x: 3 159 runs  3 159 :white_check_mark: 0 :zzz: 0 :x:

Results for commit b50e3871.

github-actions[bot] commented 1 week ago

Minimum allowed coverage is 90%

Generated by :monkey: cobertura-action against b50e38710201988589254b36de8f9cb231d095df

satyajitanand commented 1 week ago

@greens @cjswedes @inasail Can you pls review.

greens commented 1 week ago

@cjswedes that's actually a way better plan.

sy39ju commented 1 week ago

@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

inasail commented 1 week ago

@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.

cjswedes commented 1 week ago

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.