Feature/rooms resource

[vesuvian]

Got the List Rooms and List Locations API endpoints working

[Jericho]

Thank you for your contribution. I'll review and comment but in the mean time please raise an issue to describe the problem(s) your PR is attempting to solve. This is particularly important since I use a tool to automatically generate release notes and this tool relies on issues to determine the content of the notes.

[vesuvian]

Hey Jericho, I'm attempting to get the PR merged into your existing rooms feature branch, I believe this addresses issue https://github.com/Jericho/ZoomNet/issues/28

[Jericho]

Ok perfect. I'll link the issue so it gets automatically closed when we merge this PR.

There's a surprising number of modified files though. I was expecting a few new models and modifications to the Rooms resource and it's corresponding interface. I'm really surprised you had to modify files like the OAuthTokenHandler, the webhook parser, etc.

Let me dig into your PR, I'll probably figure out why you made these changes.

[vesuvian]

I got a little heavy handed with my change to the response parsing. The rooms response is paginated with token, but it's missing a total count. The parser was expecting a total count, defaulting to 0, but throwing an exception if it couldn't find the property.

I elected to make calls to GetPropertyValue with a default pass false for throwIfMissing.

[Jericho]

I got a little heavy handed with my change to the response parsing. The rooms response is paginated with token, but it's missing a total count. The parser was expecting a total count, defaulting to 0, but throwing an exception if it couldn't find the property.

where is it throwing an exception? I was careful to prevent exceptions when certain values are missing:

var totalRecords = jObject.GetPropertyValue<int?>("total_records", null);

but maybe I missed something?

[Jericho]

Please go through the comments in my review and either mark them as "Resolved" if you have addressed them or respond to my comments so we can continue the discussion.

[Jericho]

I rebased and resolved the conflicts so you can continue working on this PR.

[vesuvian]

@Jericho the number of features in room settings are becoming an organizational problem. Lots of deeply nested objects and the endpoint serves two different response types. I'm working away at adding everything for GetSettingsAsync, which you should be able to see for yourself later today.

How would you like to see UpdateSettingsAsync implemented? Having 50 optional parameters is probably not smart.

https://marketplace.zoom.us/docs/api-reference/zoom-api/rooms/updatezrsettings

[Jericho]

How would you like to see UpdateSettingsAsync implemented? Having 50 optional parameters is probably not smart.

Completely agree that 50 optional parameters would be overwhelming and confusing.

If I read the documentation correctly, it sounds like room settings are grouped into 5 categories: general room settings, security settings, client alert settings, notification settings and digital signage settings. And also, it sounds like you can update settings in one of these categories without updating settings in another category. So why don't we offer methods to update settings in each of these categories in separate methods. I propose that we add 5 methods like:

• UpdateGeneralSettings
• UpdateSecuritySettings
• UpdateAlertSettings
• UpdateNotificationSettings
• UpdateSignageSettings

Each of these methods would have a much more manageable number of optional parameters. Again, this is assuming I read the documentation correctly. I do not have a Zoom room license so I cannot run tests to validate any of my assumptions, so feel free to correct me if your testing contradicts anything I said.

FYI: I faced a similar situation when designing the method to update a Meeting. The number of parameters was overwhelming and, more importantly, some of these parameters did not apply to some meetings. For example, it didn't make sense to have optional parameters related to a meeting occurrence if a meeting is not recurring. So, I created three methods (see https://github.com/Jericho/ZoomNet/blob/develop/Source/ZoomNet/Resources/Meetings.cs)

• UpdateScheduledMeetingAsync
• UpdateRecurringMeetingAsync
• UpdateMeetingOccurrenceAsync

and I carefully chose the appropriate optional parameters for each of these three scenarios. Conceptually, very similar to the situation you are facing.