observeForceMute attribute is not returned by the /call endpoint - Githubissues

opentok / OpenTok-PHP-SDK

OpenTok PHP Server SDK

https://tokbox.com/developer/sdks/php/

MIT License

139 stars 146 forks source link

observeForceMute attribute is not returned by the /call endpoint #309

Closed rGaillard closed 2 years ago

rGaillard commented 2 years ago

Description

The /call endpoint don't return anything about the observeForceMute parameter, eg of response:

array ( 'id' => '', 'projectId' => '', 'sessionId' => '', 'connectionId' => '', 'streamId' => '', 'createdAt' => 1656512547785, 'updatedAt' => 1656512547785, )

I also tried the /dial endpoint (the right one according to your documentation: https://tokbox.com/developer/rest/#sip_call) but the result is the same. But maybe the issue is on your API that don't expose this attribute yet.

Motivation and Context

We can't use the latest version of your SDK right now.

How Has This Been Tested?

Example Output or Screenshots (if appropriate):

Types of changes

[X] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

[X] My code follows the code style of this project.
[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
[X] I have read the CONTRIBUTING document.
[ ] I have added tests to cover my changes.
[X] All new and existing tests passed.

SecondeJK commented 2 years ago

This cannot be merged until the unit tests are fixed. Can we remove the observeForceMute key from the dialForceMute mock response, and remove the assertion in OpenTokTest.php:2169.? Then we should be good to go, thanks!

SecondeJK commented 2 years ago

Oh in addition to deleting the array key in dialForceMute, it needs to be removed in dial as well.

rGaillard commented 2 years ago

@SecondeJK sure, i just made a new commit to update the tests cases. Thank you.

SecondeJK commented 2 years ago

Sorry can you pull in the latest main into your fork and push? We need the test suite to fire, and it's out of date.

rGaillard commented 2 years ago

@SecondeJK sry for delay. I just rebased my PR.

rGaillard commented 2 years ago

I updated the mocks with valid json... Tests are passing now.

SecondeJK commented 2 years ago

I'm unable to merge because this introduces a slight increase in test coverage holes. If you're able to look into finding some testable code to keep codecov happy, that would be good

SecondeJK commented 2 years ago

I've fixed the required codecoverage in #322 which will be merged in. Thank you for your contribution!