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

feanil/api docs #32971

Merged
merged 10 commits into from
Aug 18, 2023
Merged

feanil/api docs #32971

merged 10 commits into from
Aug 18, 2023

Conversation

feanil
Copy link
Contributor

@feanil feanil commented Aug 9, 2023

  • docs: Add some basic docs on the Rest API.
  • docs: Add some docs for using the REST API

Copy link
Contributor

@pomegranited pomegranited 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 adding these docs @feanil !

Couple of nits and one request, if you have time.

docs/concepts/rest_apis.rst Outdated Show resolved Hide resolved
docs/concepts/rest_apis.rst Outdated Show resolved Hide resolved
OAuth2 Application (the application is associated with your user).

JWTs by default expire every hour so when they expire you'll have to get a new
one before you can call the API again.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mind to add a code sample to the how-to that shows how you detect that a JWT is expired, and how to refresh it?

Copy link
Contributor

Choose a reason for hiding this comment

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

@pomegranited: Refreshing tokens is a different idea which I am guessing Feanil was ignoring for the sake of this document. He is simply asking them to get a new one using the same original credentials, which I think is fine. I'm not sure if it will add confusion or not to mention that there is also a way to refresh using the refresh token, but that adds complexity, and would probably go in the other how to anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up adding a reference section of useful code that includes using refresh tokens if you are working with a public client (you don't get refresh tokens with your JWT for Confidential clients). Take a look at the new auth_code_samples.rst.

docs/how-tos/use_the_api.rst Outdated Show resolved Hide resolved
docs/how-tos/use_the_api.rst Show resolved Hide resolved
docs/how-tos/use_the_api.rst Show resolved Hide resolved
docs/how-tos/use_the_api.rst Outdated Show resolved Hide resolved
docs/how-tos/use_the_api.rst Show resolved Hide resolved
OAuth2 Application (the application is associated with your user).

JWTs by default expire every hour so when they expire you'll have to get a new
one before you can call the API again.
Copy link
Contributor

Choose a reason for hiding this comment

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

@pomegranited: Refreshing tokens is a different idea which I am guessing Feanil was ignoring for the sake of this document. He is simply asking them to get a new one using the same original credentials, which I think is fine. I'm not sure if it will add confusion or not to mention that there is also a way to refresh using the refresh token, but that adds complexity, and would probably go in the other how to anyway.

docs/how-tos/use_the_api.rst Outdated Show resolved Hide resolved
docs/concepts/rest_apis.rst Show resolved Hide resolved
docs/concepts/rest_apis.rst Outdated Show resolved Hide resolved
@robrap robrap added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Aug 14, 2023
There is probably a lot more to say but add a quick How-To and concept
docs that we can build on.
@robrap
Copy link
Contributor

robrap commented Aug 17, 2023

[inform] This somewhat related doc exists: https://github.com/openedx/edx-platform/blob/master/openedx/core/djangoapps/oauth_dispatch/docs/how_tos/testing_manually.rst.

I also created an issue in edx-platform, because this how-to is not in the published docs: #33047

@feanil
Copy link
Contributor Author

feanil commented Aug 17, 2023

This should be ready for re-review.

@robrap robrap added waiting for eng review PR is ready for review. Review and merge it, or suggest changes. and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Aug 17, 2023
Copy link
Contributor

@robrap robrap left a comment

Choose a reason for hiding this comment

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

Thanks @feanil.

docs/how-tos/use_the_api.rst Outdated Show resolved Hide resolved
docs/how-tos/use_the_api.rst Show resolved Hide resolved
docs/how-tos/use_the_api.rst Outdated Show resolved Hide resolved
docs/how-tos/use_the_api.rst Outdated Show resolved Hide resolved
docs/how-tos/use_the_api.rst Show resolved Hide resolved
docs/references/auth_code_samples.rst Outdated Show resolved Hide resolved
docs/references/auth_code_samples.rst Outdated Show resolved Hide resolved
docs/references/auth_code_samples.rst Show resolved Hide resolved
docs/references/auth_code_samples.rst Outdated Show resolved Hide resolved
Feanil Patel and others added 2 commits August 18, 2023 10:14
Typos and other small fixes.

Co-authored-by: Robert Raposa <rraposa@edx.org>
Copy link
Contributor

@robrap robrap left a comment

Choose a reason for hiding this comment

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

Thanks @feanil

@feanil feanil merged commit a76238c into master Aug 18, 2023
42 checks passed
@feanil feanil deleted the feanil/api-docs branch August 18, 2023 21:09
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for eng review PR is ready for review. Review and merge it, or suggest changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants