feat: Added Canvas support #274
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
arslanashraf7
force-pushed
the
arslan/only-canvas
branch
2 times, most recently
from
fb120f9 to
a6e73aa
Compare
arslanashraf7
force-pushed
the
arslan/only-canvas
branch
from
ea7c10d to
7a68882
Compare
arslanashraf7
force-pushed
the
arslan/only-canvas
branch
3 times, most recently
from
3a1c1af to
66c92ec
Compare
arslanashraf7
force-pushed
the
arslan/only-canvas
branch
from
53023a0 to
5b8acfc
Compare
|
@gsidebo This also requires your review if possible, This is the corresponding PR for |
gsidebo
approved these changes
Dec 2, 2021
There was a problem hiding this comment.
One comment in addition to the docstring code comment: this PR does not appear to be "removing Remote gradebook & canvas_integration Django apps". Given the branch you're merging into, it's not removing anything. It's just adding Canvas support. If that sounds right to you, can you change your commit message and the title of this PR to something like "Added Canvas plugin support"?
Once that's done, feel free to merge! 👍
| @@ -68,6 +69,34 @@ def get_running_instructor_tasks(course_id): | |||
| return instructor_tasks.order_by('-id') | |||
|
|
|||
|
|
|||
| def _get_filtered_instructor_tasks(course_id, user, task_types): | |||
| """ | |||
| Returns a filtered query of InstructorTask to use for listing canvas tasks | |||
There was a problem hiding this comment.
This was probably moved over directly, but this docstring isn't really accurate
Suggested change
| Returns a filtered query of InstructorTask to use for listing canvas tasks | |
| Returns a filtered query of InstructorTasks based on the course, user, and desired task types |
arslanashraf7
changed the title
arslanashraf7
force-pushed
the
arslan/only-canvas
branch
from
97f4138 to
4df12f4
Compare
arslanashraf7
force-pushed
the
arslan/only-canvas
branch
from
4df12f4 to
97a51d2
Compare
This was referenced Dec 13, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related Ticket:
mitodl/open-edx-plugins#6
Side Note 1: Once this PR is merged we'll be adding only that code which would be part of this PR(and probably future cherry-picks) instead of this whole chunk
What's this PR do?
remote_gradebookand its dependenciescanvas_integrationinto a separate plugincanvas_integrationplugin supporting counterpart(Mostly instructor functionality that the canvas depended upon)How to test?
This might need a counterpart a plugin to completely test. The counterpart to test this PR with is currently a WIP (mitodl/open-edx-plugins#8). Once that PR is finalized we can test this one:
/srcdir & then move tomake lms-shell& thenpip install /edx/src/dist/<plugin>) OR any other way suitablemake dev.provision.lms/make lms-restartCanvastab under the instructor tabCanvastab should enable you to test the canvas functionality