-
Notifications
You must be signed in to change notification settings - Fork 48
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
Added endpoint to get list of Zoom Phone users #344
Added endpoint to get list of Zoom Phone users #344
Conversation
… value to be in available range; added unit test
I agree, this would be a nice enhancement.
Excellent.
I wonder why this was necessary. ZoomNet already has a class called
Seems reasonable at first glance.
The user profile class was created to model the response when retrieving a user's profile. As surprising as it may seem, the response to this "Get a user's profile" endpoint does not have a documented
I have a suspicion that, at the time PhoneCallUserProfile was created, the site id and site name were json nodes in the response of the "Get a user profile" endpoint as evidenced by the fact that the sample JSON in our unit test includes a site_id node. This was probably changed at some point in the Zoom API (and documentation) but our library was not modified.
I'll raise a separate issue to track the investigation into this puzzling situation. Maybe you can help with investigating this situation: is it a mistake in the documentation? Is my suspicion correct that this situation was changed at some point in time?
This is awesome! I always appreciate when collaborators take the time to add unit tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this.
As for the response model for "List phone users" endpoint, I began to think that it might be better to have a separate model after all, rather than using the Let me show you real responses from these two endpoints in question, which I got through Postman requesting Zoom APIs (real data was changed):
But overall the current
For our purposes, we won't use 'siteId' and 'siteAdmins' and can ignore 'EmergencyAddress', but anyway it might be confusing for other users.. |
…ource instead of 'PhoneCallUserProfile' model
I've added new model Perhaps then it'd make sense to create a base class containing the common properties of these two models, and then have |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
Thank you for your contribution.
Surprisingly, the following assertion succeeds when running unit tests under .NET 6, 7 and 8 but it fails under .NET 4.8: exception.Message.ShouldBe($"Records per page must be between 1 and 100 (Parameter '{nameof(pageSize)}')"); with the following message:
Looks like the error message of an ArgumentOutOfRange exception was slightly different under .NET 4.8. I replace the assertion with exception.Message.ShouldStartWith("Records per page must be between 1 and 100"); and it succeeds no matter which .NET framework is used. |
🎉 This issue has been resolved in version 0.76.0 🎉 The release is available on: Your GitReleaseManager bot 📦🚀 |
Hi!
We need an endpoint to get all users of Zoom Phone.
The endpoint API can be found here: https://developers.zoom.us/docs/api/rest/reference/phone/methods/#operation/listPhoneUsers
Please review the PR. It'd be nice if you could add this feature to the next release:)
What is done:
IPhone
resource by adding a new method to send a request to this API with all available query arguments.PhoneCallUserProfilesPaginationObject
that represents paginated results.The type of
users
array object in this new model is the existingPhoneCallUserProfile
model, as it's the most similar one to the actual model from the docs.A few notes about the
PhoneCallUserProfile
model:Name
property to the model as we may need it.SiteId
andSiteAdmin
properties, but the actual endpoint response model contains asite
object:"site": { "id": "...", "name": "..." }
We won't need these properties, but if you think it may be a little confusing and it's better to add a new model for the list of users instead of
PhoneCallUserProfile
, please let me know - I'll add! :)Additionally, I've added some unit tests.