-
Notifications
You must be signed in to change notification settings - Fork 71
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
[GH-167][GH-215] Removed "Beta" label on Account-Level app, and updated documentation #328
Conversation
1. Added latest screenshot 2. Completed the documentation for plugin configuration.
README.md
Outdated
* Connect your users to Zoom using OAuth. | ||
* Use the Client ID and Client Secret generated when configuring Zoom to fill in the fields **Zoom OAuth Client ID** and **Zoom OAuth Client Secret**. | ||
* Use the Client ID and Client Secret generated when configuring Zoom to fill in the fields **Zoom OAuth Client ID** and **Zoom OAuth Client Secret**. (Make sure these credentials are according to the app type (user/account level) you are using.) |
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.
How might someone know the difference between the necessary credentials on the different app types?
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.
@mickmister By this line I meant that if the user has selected the app type as "Account level app". He should make sure that he is using a account level app on Zoom side as well.
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.
Maybe we should just say the sentence that you used above to explain the point. If the "credentials" don't change between the two app types, then the current sentence could be misleading
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.
@mickmister Fixed
LGTM, just a few suggestions |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #328 +/- ##
=======================================
Coverage 19.33% 19.33%
=======================================
Files 9 9
Lines 1479 1479
=======================================
Hits 286 286
Misses 1138 1138
Partials 55 55 ☔ View full report in Codecov by Sentry. |
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.
LGTM, just one grammatical suggestion
Co-authored-by: Michael Kochell <[email protected]>
@mickmister Fixed |
Summary
Screenshot:
Ticket Link
Fixes #167
Fixes #215