-
Notifications
You must be signed in to change notification settings - Fork 10
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
I469 canvas oauth flow #501
I469 canvas oauth flow #501
Conversation
CANVAS_OAUTH_CANVAS_DOMAIN = os.getenv('CANVAS_OAUTH_CANVAS_DOMAIN', 'canvas.test.instructure.com') | ||
# Scopes environment variable provides a way to recover if Canvas changes scope identifiers. | ||
if isinstance((env_canvas_scopes := os.getenv('CANVAS_OAUTH_SCOPES')), str): |
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.
This could be simplified to
CANVAS_OAUTH_SCOPES = os.getenv('CANVAS_OAUTH_SCOPES', '').split(',') or DEFAULT_CANVAS_SCOPES
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.
This logic won't work since if os.getenv('CANVAS_OAUTH_SCOPES'))
not added in .env
it returns a None, we can't really escape a if/else logic with a one-liner
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.
Right, but if you have a empty string as a default as I suggested it should work as expected. I'm fine leaving it as-is though.
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.
May be i will look into it later.
config/.env.sample
Outdated
|
||
#(optional) The Canvas API scopes needed by the application | ||
# (This should only be used if Canvas changes the scopes from what is in the source code in backend/canvas_scopes.py.) | ||
CANVAS_OAUTH_SCOPES= |
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.
This should be commented out by default, as if it's an empty string it looks like it won't work.
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.
@pushyamig this is what was causing my issues locally testing. commenting it out allowed me to successfully pass the canvas authorize page.
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.
Sorry for not give clear direction when testing, In my local .env
I did not add the scopes variable since I don't want a long list of scope in env. So I let it as optional. I commented out the the value in env.sample
now as per direction
README.md
Outdated
15. Enable the "Enforce Scopes" option, "Allow Include Parameters", then add all scopes needed by the application (i.e., those listed in `backend/ccm/canvas_scopes.py`). | ||
16. Click the "Save" button. | ||
17. Copy the ID number of the API key created in Canvas. The ID is the long number shown in the "Details" column of the "Developer Keys" page. It usually looks like "`17700000000000nnn`". | ||
18. Click the "Show Key" button underneath the ID located in step 16, and copy the secret that appears in the dialog. |
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.
for both steps 17 and 18 it may help to mention where the API key ID and secret should get pasted to in the .env file
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.
Done. I add more instructions for clarity
08ceeed
to
ff84556
Compare
I made changes as requested and update to readme for clarity |
@jonespm If you don't have any comment then please approve. |
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.
Reviewed through this and talked about it with @jaydonkrooss , looks fine to me.
Fixes #469
.env.sample
Courses