Jericho
commented
3 months ago Thanks for this very detailed PR. You took the time to add unit tests. You documented every class, property and method. You even added the new model classes to the serialization context. I'm impressed!
Thinking out loud: what do you think about creating a new resource called Sms (along with it's corresponding ISms interface) where we would group all the sms related methods rather than adding them to the Phone resource?
Reviewing Zoom's documentation, I notice a few other methods related to sms (such as "get SMS by message id", "get user's SMS sessions", etc.) and also a few methods related to SMS campaigns (such as "assign phone number to SMS campaign", "get a SMS campaign", etc.). I'm not suggesting you should work on all these other methods (unless you want to!). I'm just trying to make sure we plan ahead and organize our methods in a way to makes them easily discoverable. Also, since the resource would clearly identify that methods are related to SMS functionality, we could slightly simplify their name. What I mean is:
- rather than
Phone.GetSmsSessionDetailsAsync, the new method you introduced in the PR would beSms.GetSessionDetailsAsync. - in the future, we would be able to add methods such as
Sms.GetMessageById,Sms.GetUserSessionsandSms.GetCampaignrather thanPhone.GetSmsMessageById,Phone.GetUserSmsSessionsandPhone.GetSmsCampaign.
pvgritsenko-ansible
We are happy to use your library. Now we need this endpoint to receive data about sent SMS from our users. Therefore, we would like to see these changes in the next ZoomNet release. Please review this PR=)
IPhone resource extended with new method to receive SMS message history via endpoint phone/sms/sessions/{SessionID}.