refactor: move more functionality to Canvas plugin #387

arslanashraf7 · 2024-10-29T12:42:53Z

What are the relevant tickets?

https://github.com/mitodl/hq/issues/5862

Description (What does it do?)

Migrates more code from our custom commit
Mostly, It migrates the task filtering part
Migrates the course setting part

How can this be tested?

Install the canvas plugin from this branch in while you have set up edx-platform on feat: add canvas integration support (#300) edx-platform#319
Test that the canvas works

asadali145 · 2024-11-15T09:25:22Z

src/ol_openedx_canvas_integration/utils.py

+
+    if course:
+        return course.other_course_settings.get("canvas_id")
+    return None


Suggested change

if course:

return course.other_course_settings.get("canvas_id")

return None

return course.other_course_settings.get("canvas_id") if course else None

asadali145 · 2024-11-15T09:30:02Z

src/ol_openedx_canvas_integration/README.rst

+**For "Quince" or more recent release of edX platform, you should cherry-pick below commit:**
+
+https://github.com/mitodl/edx-platform/commit/7a2edd5d29ead6845cb33d2001746207cf696383
+
 **For "Nutmeg" or more recent release of edX platform, you should cherry-pick below commit:**


Suggested change

**For "Nutmeg" or more recent release of edX platform, you should cherry-pick below commit:**

**For "Nutmeg" release of edX platform, you should cherry-pick below commit:**

These were in incremental order so a person reading from top to bottom would always go from Sumac to downward. When the go there they would see the next release and it's predecessors message. Eventually they will get the right version out of the list.

In any case, we can't specify one release name, it's always going to be a range. I went there and that made things more complex to read than using the same more recent format. If we still need to change, I'd go for a release range and not just one specific release.

The is for all the other comments on things like these below.

I commented because Nutmeg and prior means Nutmeg, Quince, and Sumac. These releases have specific commits/versions specified, so I think we can mention a single release in this.

The idea was that the person would read from top to bottom. So they will read Sumac message first. I'm changing it into a range in next commit anyway

asadali145 · 2024-11-15T09:30:13Z

src/ol_openedx_canvas_integration/README.rst

+
+TBA
+
+**For "Quince" or more recent release of edX platform, you should cherry-pick below commit:**


Suggested change

**For "Quince" or more recent release of edX platform, you should cherry-pick below commit:**

**For "Quince" release of edX platform, you should cherry-pick below commit:**

asadali145 · 2024-11-15T09:30:42Z

src/ol_openedx_canvas_integration/README.rst

+
+Use ``0.3.0`` or a above version of this plugin
+
+
 **For "Nutmeg" or more recent release of edX platform**


Suggested change

**For "Nutmeg" or more recent release of edX platform**

**For "Nutmeg" release of edX platform**

asadali145 · 2024-11-15T09:30:51Z

src/ol_openedx_canvas_integration/README.rst

+
+Use ``0.4.0`` or a above version of this plugin
+
+**For "Quince" or more recent release of edX platform**


Suggested change

**For "Quince" or more recent release of edX platform**

**For "Quince" release of edX platform**

asadali145 · 2024-11-15T09:34:03Z

src/ol_openedx_canvas_integration/views.py

-    client = CanvasClient(canvas_course_id=course.canvas_course_id)
-    if not course.canvas_course_id:
+    canvas_course_id = get_canvas_course_id(course)
+    client = CanvasClient(canvas_course_id=canvas_course_id)


We are initializing client before checking canvas_course_id.

Been there for a long time becasue and it didn't break because the canvas course Id check below that would return anyway, I'll move the client initialization a couple of lines below.

Yeah, I checked that. I just mentioned it because of the validation. We can either initialize it after the validation or remove the validation if it is not needed.

asadali145 · 2024-11-15T09:34:19Z

src/ol_openedx_canvas_integration/views.py

-    client = CanvasClient(canvas_course_id=course.canvas_course_id)
-    if not course.canvas_course_id:
+    canvas_course_id = get_canvas_course_id(course)
+    client = CanvasClient(canvas_course_id=canvas_course_id)


Same, Initialization before validation

asadali145

LGTM!

arslanashraf7 · 2024-11-15T10:57:42Z

I've incorporated all the changes in 34efad7. @asadali145 Could you take a look?

*Edit: I have added additional validations where applicable.

asadali145 · 2024-11-15T11:21:44Z

src/ol_openedx_canvas_integration/api.py

+    canvas_course_id = get_canvas_course_id(course)
+    if not course:
+        raise Exception(COURSE_KEY_ID_EMPTY)  # noqa: TRY002


We are validating course values after getting canvas id.

Fixed in e028503

arslanashraf7 force-pushed the arslan/5862-enhance-canvas-plugin branch from 3563b20 to 4e4b32c Compare October 30, 2024 10:57

arslanashraf7 marked this pull request as ready for review November 6, 2024 14:12

arslanashraf7 mentioned this pull request Nov 14, 2024

feat: add canvas integration support (#300) mitodl/edx-platform#319

Merged

arslanashraf7 force-pushed the arslan/5862-enhance-canvas-plugin branch 3 times, most recently from 02a4875 to 4f4bf17 Compare November 14, 2024 12:50

arslanashraf7 added the Needs Review label Nov 14, 2024

arslanashraf7 requested a review from asadali145 November 15, 2024 08:23

asadali145 reviewed Nov 15, 2024

View reviewed changes

asadali145 self-assigned this Nov 15, 2024

asadali145 added Waiting on Author and removed Needs Review labels Nov 15, 2024

asadali145 approved these changes Nov 15, 2024

View reviewed changes

arslanashraf7 force-pushed the arslan/5862-enhance-canvas-plugin branch 2 times, most recently from 91dc185 to 34efad7 Compare November 15, 2024 10:56

arslanashraf7 requested a review from asadali145 November 15, 2024 10:57

asadali145 approved these changes Nov 15, 2024

View reviewed changes

arslanashraf7 added 5 commits November 18, 2024 15:35

refactor: move more functionality to Canvas plugin

b2f0315

update readme

c96f928

docs: udpate readme

6072f10

feedback

6e10166

feedback 2

88f1500

arslanashraf7 force-pushed the arslan/5862-enhance-canvas-plugin branch from e028503 to 88f1500 Compare November 18, 2024 10:35

arslanashraf7 merged commit 5cdd913 into main Nov 18, 2024
5 of 11 checks passed

arslanashraf7 deleted the arslan/5862-enhance-canvas-plugin branch November 18, 2024 10:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: move more functionality to Canvas plugin #387

refactor: move more functionality to Canvas plugin #387

arslanashraf7 commented Oct 29, 2024 •

edited

Loading

asadali145 Nov 15, 2024

asadali145 Nov 15, 2024

arslanashraf7 Nov 15, 2024

asadali145 Nov 15, 2024

arslanashraf7 Nov 15, 2024

asadali145 Nov 15, 2024

asadali145 Nov 15, 2024

asadali145 Nov 15, 2024

asadali145 Nov 15, 2024

arslanashraf7 Nov 15, 2024

asadali145 Nov 15, 2024

asadali145 Nov 15, 2024

asadali145 left a comment

arslanashraf7 commented Nov 15, 2024 •

edited

Loading

asadali145 Nov 15, 2024

arslanashraf7 Nov 15, 2024

	For "Nutmeg" or more recent release of edX platform, you should cherry-pick below commit:
	For "Nutmeg" release of edX platform, you should cherry-pick below commit:


		TBA

		For "Quince" or more recent release of edX platform, you should cherry-pick below commit:

	For "Quince" or more recent release of edX platform, you should cherry-pick below commit:
	For "Quince" release of edX platform, you should cherry-pick below commit:


		Use ``0.3.0`` or a above version of this plugin


		For "Nutmeg" or more recent release of edX platform

	For "Nutmeg" or more recent release of edX platform
	For "Nutmeg" release of edX platform


		Use ``0.4.0`` or a above version of this plugin

		For "Quince" or more recent release of edX platform

	For "Quince" or more recent release of edX platform
	For "Quince" release of edX platform

refactor: move more functionality to Canvas plugin #387

refactor: move more functionality to Canvas plugin #387

Conversation

arslanashraf7 commented Oct 29, 2024 • edited Loading

What are the relevant tickets?

Description (What does it do?)

How can this be tested?

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asadali145 left a comment

Choose a reason for hiding this comment

arslanashraf7 commented Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arslanashraf7 commented Oct 29, 2024 •

edited

Loading

arslanashraf7 commented Nov 15, 2024 •

edited

Loading