Jericho / ZoomNet

.NET client library for the Zoom.us REST API v2
MIT License
69 stars 45 forks source link

Added endpoint to get list of Zoom Phone users #344

Closed snega1003 closed 2 months ago

snega1003 commented 2 months ago

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:

A few notes about the PhoneCallUserProfile model:

Additionally, I've added some unit tests.

Jericho commented 2 months ago

Please review the PR. It'd be nice if you could add this feature to the next release:)

I agree, this would be a nice enhancement.

  • I've extended the IPhone resource by adding a new method to send a request to this API with all available query arguments.

Excellent.

  • I've also added a new model PhoneCallUserProfilesPaginationObject that represents paginated results.

I wonder why this was necessary. ZoomNet already has a class called PaginatedResponseWithToken<T> for this purpose and it's used throughout the ZoomNet library whenever an endpoint returns paginated data. Is there a reason you had to create another class?

The type of users array object in this new model is the existing PhoneCallUserProfile model, as it's the most similar one to the actual model from the docs.

Seems reasonable at first glance.

  • I've added the Name property to the model as we may need it.

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 name property (maybe it's a mistake in the documentation?). For the time being, I think you made a reasonable decision by adding the 'Name' property. However, if we find too many other discrepancies between the data you are trying to model and the existing PhoneCallUserProfile class, we'll have to consider creating a distinct class.

  • The model contains SiteId and SiteAdmin properties, but the actual endpoint response model contains a site object: "site": { "id": "...", "name": "..." }

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.

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! :)

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?

Additionally, I've added some unit tests.

This is awesome! I always appreciate when collaborators take the time to add unit tests.

snega1003 commented 2 months ago

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 PhoneCallUserProfile model...

Let me show you real responses from these two endpoints in question, which I got through Postman requesting Zoom APIs (real data was changed):

  1. GET phone/users: TL;DR: the response is identical to that in the docs:

    "next_page_token": "F2qwertyg5eIqRRgC2YMauur8ZHUaJqtS3i",
    "page_size": 1,
    "total_records": 1,
    "users": [
        {
            "id": "NL3cEpSdRc-c2t8aLoZqiw",
            "phone_user_id": "u7pnC468TaS46OuNoEw6GA",
            "email": "test_phone_user@testapi.com",
            "name": "test phone user",
            "extension_id": "CcrEGgmeQem1uyJsuIRKwA",
            "extension_number": 123,
            "status": "activate",
            "calling_plans": [
                {
                    "type": 600,
                    "name": "Delhi billing",
                    "billing_account_id": "3WWAEiEjTj2IQuyDiKMd_A",
                    "billing_account_name": "Delhi billing"
                }
            ],
            "phone_numbers": [
                {
                    "id": "---M1padRvSUtw7YihN7sA",
                    "number": "14232058798"
                }
            ],
            "site": {
                "id": "8f71O6rWT8KFUGQmJIFAdQ",
                "name": "Test Site"
            },
            "department": "Test",
            "cost_center": "Cost Test Center"
        }
    ]
    }
  2. GET phone/users/NL3cEpSdRc-c2t8aLoZqiw: it's too big so I put it to the file - get user profile.json TL;DR: Comparing to the real user profile response, PhoneCallUserProfile model lacks of policy object only. And the response indeed has 'siteId' and 'siteAdmins' as plain properties. Also the response indeed doesn't have 'name' property.

But overall the current PhoneCallUserProfile model is similar to what "List phone users" endpoint returns. With the following exceptions:

For our purposes, we won't use 'siteId' and 'siteAdmins' and can ignore 'EmergencyAddress', but anyway it might be confusing for other users..

snega1003 commented 2 months ago

I've added new model PhoneUser with new model for site object, and used it in "List phone users" endpoint response. Changes in PhoneCallUserProfile have been reverted. Please take a look :)

Perhaps then it'd make sense to create a base class containing the common properties of these two models, and then have PhoneUser and PhoneCallUserProfile inherit from this base class. WDYT? Or is it not necessary at the moment? :)

Jericho commented 2 months ago

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:

   Shouldly.ShouldAssertException : exception.Message
       should be
   "Records per page must be between 1 and 100 (Parameter 'pageSize')"
       but was
   "Records per page must be between 1 and 100
   Parameter name: pageSize"

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.

Jericho commented 2 months ago

:tada: This issue has been resolved in version 0.76.0 :tada:

The release is available on:

Your GitReleaseManager bot :package::rocket: