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

Polls #179

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open

Polls #179

wants to merge 8 commits into from

Conversation

vishnuku
Copy link

Add poll functionality for meeting and webinars

Copy link
Owner

@prschmid prschmid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this PR and our work to expand this library! I've added in a couple of comments for minor fixes. Looking forward to merging this in.

Thanks!

responses.add(
responses.POST, "http://foo.com/webinar/ID/panelists",
)
response = self.component.add_panelists(panelists=[{"name": "Mary", "email": "[email protected]"}])
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make this a more human readable email? Maybe[email protected]?

)
response = self.component.add_panelists(panelists=[{"name": "Mary", "email": "[email protected]"}])
self.assertEqual(
response.request.body, '{panelists: [{"name": "Mary", "email": "[email protected]"}]}'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above



if __name__ == "__main__":
unittest.main()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you run black to check for formatting? Looks like some files are missing a newline at the end.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

)
response = self.component.delete_panelists(panelists=[{"name": "Mary", "email": "[email protected]"}])
self.assertEqual(
response.request.body, '{panelists: [{"name": "Mary", "email": "[email protected]"}]}'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See previous comment regarding email

@@ -7,4 +7,4 @@


__all__ = ["API_VERSION_1", "API_VERSION_2", "ZoomClient"]
__version__ = "1.1.3"
__version__ = "1.1.4"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert this change and leave it as 1.1.3 -- I'll update it to the appropriate version when I cut the next release (which likely will be 1.1.6).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure Did it

@@ -2,4 +2,4 @@

from __future__ import absolute_import

from . import meeting, metric, past_meeting, phone, recording, report, user, webinar
from . import meeting, metric, past_meeting, phone, recording, report, user, webinar, poll
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add the poll in the correct alphabetical order? I.e. after phone. Thanks!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! Did the same

def list(self, **kwargs):
util.require_keys(kwargs, "id")
return self.get_request(
f"/{self.type}/{kwargs.get('id')}/polls"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like in some cases you are using f"...." and in others "...".format(). Since the rest of the library uses the "...".format() syntax, can you please switch it to that for consistency's sake?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I have changed the f"...."and in this "...".format()

@prschmid
Copy link
Owner

@vishnuku Thank you for this PR! We merged in some code that it appears you based your work off of. Can I trouble you to resolve the conflicts so that we can get this merged? Thank you for your time and effort!

@prschmid prschmid added this to the 1.1.7 milestone Mar 17, 2021
@prschmid prschmid requested a review from mfldavidson April 28, 2021 13:36
@mfldavidson
Copy link
Contributor

@prschmid looks like @vishnuku hasn't responded to all of your suggested changes. I recommend we wait a week or so and if we haven't heard from them I will make the changes, resolve the conflicts, and then start a review.

@vishnuku
Copy link
Author

@prschmid looks like @vishnuku hasn't responded to all of your suggested changes. I recommend we wait a week or so and if we haven't heard from them I will make the changes, resolve the conflicts, and then start a review.

Hi @mfldavidson , I will complete these on this weekend.

Thanks for reminding

@mfldavidson
Copy link
Contributor

Hi @mfldavidson , I will complete these on this weekend.

Thanks for reminding

@vishnuku no problem, we really appreciate your work on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants