feat: add canvas integration plugin #8

arslanashraf7 · 2021-11-04T12:44:12Z

Related Tickets:

What's this PR does?

Migrates the canvas_integration app from edx-platform into a separate plugin

How to test?

To test this PR you will need a counterpart in the edx-platform and that part is specifically done in mitodl/edx-platform#274

Setup your edx-platform on the associated branch feat: Added Canvas support edx-platform#274 from here
Install this plugin (e.g. generate a build for this plugin with ./pants package :: and then install that into the edx-platform)
Restart the lms (make lms-restart)
Make sure to set the values for CANVAS_ACCESS_TOKEN=?, CANVAS_BASE_URL=https://mit.test.instructure.com` (You can ask me for the Token value)
(More instruction to be added once the PR is finalized)
Now open a course as admin and click Instructor tab and make sure you see a Canvas tab under it
Click the canvas tab and test all the functionality of canvas integration (e.g listing enrollments, syncing enrollments, pushing grades)

Background context:

We had made several changes inside our for of the edx-platform where we added a Django application to support canvas integration. In the process, we made many changes in open edX's Instructor django application since we wanted to run canvas_integration inside the Instrutor tab. (More details to come)

Screenshots:

Landing page and tasks details

List Canvas Enrollments

List Canvas Assignments

ziafazal · 2021-11-10T10:16:59Z

src/ol_openedx_canvas_integration/api.py

+    return results
+
+
+def get_subsection_user_grades(course):


@arslanashraf7 I think we can remove a lot of code around getting user's grate by assignment and use builtin- method of PersistentGrades

As discussed keeping the scope of this in mind, I've created a ticket to keep track of this optimization #10.

ziafazal

@arslanashraf7 few improvements I suggested which are not necessarily needed to be done right now but would be useful in future.

ziafazal · 2021-11-10T10:20:14Z

src/ol_openedx_canvas_integration/client.py

+        """
+        session = requests.Session()
+        session.headers.update({
+            "Authorization": "Bearer {token}".format(token=settings.CANVAS_ACCESS_TOKEN)


I think we can persist canvas credentials in encrypted field of django model instead of using them from settings

@arslanashraf7 can you check with @mitodl/devops for their preference for managing the secrets?

Sure, Currently we are injecting them from the vault https://github.com/search?q=org%3Amitodl+CANVAS_ACCESS_TOKEN&type=code. I'll talk to the ops guys if they want to go with this approach or the existing vault approach is better.

It's fine to keep it as a settings attribute because we can take the same approach of managing the value through the configuration template.

ziafazal · 2021-11-10T10:25:59Z

src/ol_openedx_canvas_integration/client.py

+            "name": subsection_block.display_name,
+            "integration_id": str(subsection_block.location),
+            "grading_type": "percent",
+            "points_possible": DEFAULT_ASSIGNMENT_POINTS,


possible grade of actual subsection instead of default value.

This would be done as part of #10 since it will be easier to directly get the max weightage/score for a subsection.

ziafazal · 2021-11-10T10:27:57Z

src/ol_openedx_canvas_integration/settings/common.py

+    """Settings for the canvas integration plugin."""
+    settings.CANVAS_ACCESS_TOKEN = None
+    settings.CANVAS_BASE_URL = None
+    settings.FEATURES['ENABLE_CANVAS_INTEGRATION'] = False


By default this feature should be enabled.

I agree with the default, but do we even need a feature flag? If the plugin is installed, shouldn't that be a clear signal that it should be turned on?

@pdpinch , We've been running the canvas behind a feature flag from when we first implemented that & I'm not sure what was the reason to keep it behind a flag but we can remove it and assume if the plugin is installed, the canvas is enabled.
The flag is used only once in the edx counterpart and we can make that check settings.INSTALLED_APPS instead of a flag. I'll remove the feature flag now unless you say otherwise.

I think it would be better to test on INSTALLED_APPS, but if you want to leave that for another issue & PR, I would be OK with that.

I felt it would be better to do this in current PR and make it available at the earliest stage possible of the plugin, Updated.

arslanashraf7 · 2021-11-12T10:07:44Z

@ziafazal I've addressed feedback and responded to some comments. This is ready for review again, Could you please take a look. In addition, I've also addressed a bug regarding grades/assignments update counts that were 0 in all the cases. This might have been a regression of mitodl/edx-platform#189.

ziafazal

👍

gsidebo

👍

This PR has been sitting for a while since it has taken so long for me to address other issues, get pants to actually build the plugin, and get the plugin itself working in devstack. It seems that my Canvas token is outdated, so I didn't directly test the functionality, but I finally managed to get the Canvas tab to show up and work correctly. Given the timing I think it's best to merge this as it is, and if there are any lingering bugs, those can be addressed in future PRs. I have a couple concerns though:

The instructions on this PR are not complete. I had to dig around past issues/PRs to figure out that I needed to add a canvas_course_id setting to my course, which I ended up doing in Studio. That leads me to the next concern...
Why is the documentation not included in this PR? Why would we add it in a future PR? This is the PR that adds the feature, so why not merge it with instructions to use the feature?

Anyway, feel free to merge this as it is and please keep those notes in mind for future PRs

arslanashraf7 · 2021-11-23T11:17:06Z

@gsidebo I'm sorry I somehow missed some details in the PR description but thanks for reviewing this PR anyway. I agree with your idea of adding the documentation along with the feature which should be followed in the oncoming PRs.
For the documentation of this plugin, I'll create a follow-up PR.

arslanashraf7 added Do Not Merge Work in progress labels Nov 4, 2021

arslanashraf7 force-pushed the arslan/canvas-integration-plugin branch from 95617a6 to 649d64f Compare November 5, 2021 08:52

arslanashraf7 mentioned this pull request Nov 5, 2021

feat: Added Canvas support mitodl/edx-platform#274

Merged

arslanashraf7 force-pushed the arslan/canvas-integration-plugin branch from 300bc95 to d76d51d Compare November 8, 2021 13:54

feat: Add cnavas_integration plugin

ffa1c41

arslanashraf7 force-pushed the arslan/canvas-integration-plugin branch from 123176d to ffa1c41 Compare November 10, 2021 09:14

ziafazal reviewed Nov 10, 2021

View reviewed changes

arslanashraf7 mentioned this pull request Nov 11, 2021

Refactor the usage of subsection block grades #10

Open

2 tasks

Address feedback and fix grades count fix

fc79f50

arslanashraf7 added Needs Review and removed Work in progress labels Nov 12, 2021

ziafazal approved these changes Nov 12, 2021

View reviewed changes

gsidebo self-assigned this Nov 15, 2021

This was referenced Nov 15, 2021

Canvas Integration: Serve HTML/JS/CSS from within the plugin #13

Closed

Identify pluggable & un-pluggable parts of the canvas integration #7

Closed

Add tests for Canvas Integration plugin #16

Open

gsidebo approved these changes Nov 22, 2021

View reviewed changes

gsidebo added Waiting on Author and removed Needs Review labels Nov 22, 2021

arslanashraf7 merged commit b3d7226 into main Nov 23, 2021

arslanashraf7 deleted the arslan/canvas-integration-plugin branch November 23, 2021 11:18

arslanashraf7 removed the Do Not Merge label Nov 23, 2021

arslanashraf7 mentioned this pull request Nov 24, 2021

docs: Add documentation for Canvas Integration plugin #19

Merged

This was referenced Dec 13, 2021

Port canvas_integration django app from mitodl/edx-platform #4

Closed

Create a plugin for Canvas integration #3

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add canvas integration plugin #8

feat: add canvas integration plugin #8

arslanashraf7 commented Nov 4, 2021 •

edited

Loading

ziafazal Nov 10, 2021

arslanashraf7 Nov 12, 2021

ziafazal left a comment

ziafazal Nov 10, 2021

pdpinch Nov 10, 2021

arslanashraf7 Nov 10, 2021

blarghmatey Nov 10, 2021

ziafazal Nov 10, 2021

arslanashraf7 Nov 12, 2021

ziafazal Nov 10, 2021

pdpinch Nov 10, 2021

arslanashraf7 Nov 10, 2021 •

edited

Loading

pdpinch Nov 10, 2021

arslanashraf7 Nov 12, 2021 •

edited

Loading

arslanashraf7 commented Nov 12, 2021 •

edited

Loading

ziafazal left a comment

gsidebo left a comment

arslanashraf7 commented Nov 23, 2021

feat: add canvas integration plugin #8

feat: add canvas integration plugin #8

Conversation

arslanashraf7 commented Nov 4, 2021 • edited Loading

Related Tickets:

What's this PR does?

How to test?

Background context:

Screenshots:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ziafazal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arslanashraf7 Nov 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arslanashraf7 Nov 12, 2021 • edited Loading

Choose a reason for hiding this comment

arslanashraf7 commented Nov 12, 2021 • edited Loading

ziafazal left a comment

Choose a reason for hiding this comment

gsidebo left a comment

Choose a reason for hiding this comment

arslanashraf7 commented Nov 23, 2021

arslanashraf7 commented Nov 4, 2021 •

edited

Loading

arslanashraf7 Nov 10, 2021 •

edited

Loading

arslanashraf7 Nov 12, 2021 •

edited

Loading

arslanashraf7 commented Nov 12, 2021 •

edited

Loading