Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Add missing advanced machine detection mode #947

Conversation

d-vanhees-micoworks
Copy link
Contributor

Description

First of all, since this is my first PR in this repo, please correct and help by guiding me if I am wrong, thank you 🙇

Background of this PR is to add missing default enum for AdvancedMachineDetectionMode

Motivation and Context

According to the docs in here, there are 3 modes for Advanced Machine detection:

  • default
  • detect
  • detect_beep

However, the first one is missing from AdvancedMachineDetectionMode which means we are unable to use that mode using the provided SDK as-is. Please let me know if this is intentional because advanced machine detection is still in beta.
Also I was unsure what to put as comment. According to documentation, the default value is connect (sync), and not default (async), however when testing out without specifying mode, it sounded as if machine detection was async. So I am unsure whether default is actually default mode or not in the end.

Testing Details

Sorry I was unable to add unit test as I am lacking knowledge of expected request and responses 🙇

Existing test are still passing.

Example Output or Screenshots (if appropriate)

image

https://developer.vonage.com/en/api/voice#createCall

Types of changes

  • 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

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

@d-vanhees-micoworks
Copy link
Contributor Author

I am honestly not sure why WhiteSource security has issues accessing my forked repo.
Any insights and suggestions are appreciated 🙇

@manchuck
Copy link
Contributor

@d-vanhees-micoworks WhiteSource will not run for you since your repo cannot access our secrets with all the access keys. Other than that, it looks good. I'm running the actions for the PR then I will cut a patch for you

@d-vanhees-micoworks
Copy link
Contributor Author

d-vanhees-micoworks commented Jul 23, 2024

@manchuck Awesome, thank you very much for your review and cutting a patch. 🙇

@manchuck manchuck merged commit f2cc9c1 into Vonage:3.x Jul 23, 2024
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants