feat: add CourseApp for Learning Assistant feature and register as entrypoint #52

MichaelRoytman · 2024-01-10T17:16:04Z

Description

NOTE: This pull request should not be merged until the configuration modal is added to the Course Authoring MFE for this feature. Please see COSMO-136 and COSMO-137.

This commit adds a concrete implementation of the CourseApp ABC from the CourseApps edx-platform Django app. This enables the Learning Assistant to be represented by a card on the Studio Pages & Resources pages. Please see the associated ADR for more details.

This commit also registers this CourseApp class as an entrypoint to the CourseApps edx-platfrom Django app. Because the CourseApps REST API is a part of the CMS, the Learning Assistant plugin was also added as entrypoint to the CMS.

MichaelRoytman · 2024-01-10T17:24:28Z

learning_assistant/plugins.py

+
+    app_id = 'learning_assistant'
+    name = 'Learning Assistant'
+    description = 'TBD'


TBD: We need to provide a privacy-approved description. DO NOT merge without filling this in.

MichaelRoytman · 2024-01-10T17:26:22Z

setup.py

@@ -131,6 +131,12 @@ def is_requirement(line):
    entry_points={
        'lms.djangoapp': [
            "learning_assistant = learning_assistant.apps:LearningAssistantConfig"
+        ],
+        'cms.djangoapp': [


Because the CourseApps API is registered under the CMS application, the Learning Assistant needs to be registered as a plugin there as well. Otherwise, the plugin is not included in the CMS's INSTALLED_APPS list, which causes a runtime error, because the LearningAssistantCourseApp plugin refers to the Learning Assistant models, which are not available in the CMS.

Zacharis278 · 2024-01-11T15:26:32Z

learning_assistant/plugins_api.py

+Because the LearningAssistantCourseApp plugin inherits from the CourseApp class, which is imported from the
+edx-platform, we cannot test that plugin directly, because pytest will run outside the platform context.
+Instead, the CourseApp abstract methods are implemented here and
+imported into and used by the LearningAssistantCourseApp. This way, these implementations can be tested.


ahh good idea

learning_assistant/utils.py

Zacharis278 · 2024-01-11T15:33:40Z

learning_assistant/data.py

+
+
+@frozen
+class LearningAssistantCourseEnabledData:


what's the data class used for?

I was following our Django App Patterns OEP, so I don't expose a modal instance via the API. I need to return something from set_learning_assistant_enabled, because the CourseApp ABC requires that the set_enabled function implementation returns the value of the toggle.

On second look, I could return a boolean from set_learning_assistant_enabled and avoid needing the data class entirely. I think I was treating set_learning_assistant_enabled as a create/update operation and was thinking about returning the created/update object, which necessitated the data class. For now, this is a simple model/object, so that could be overkill.

If it feels heavy handed/confusing, let me know. I can return a boolean.

makes sense to me 👍

Zacharis278 · 2024-01-11T15:37:58Z

learning_assistant/platform_imports.py

+        * bool: whether the Learning Assistant feature is available
+    """
+    # pylint: disable=import-error, import-outside-toplevel
+    from lms.djangoapps.courseware.toggles import learning_assistant_is_active


Thinking out loud. Does this create a cms -> learning -> lms import loop and is that a problem in any way?

That's a good question. I don't think so. Where do you think the loop would be? I've tried tracing it out, but I can't think of where there might be a loop currently.

I haven't seen any issues when testing the CourseApps API in the CMS or when testing the Pages & Resources page.

Some other plugins are defined in the lms directory (e.g. ProctoringCourseApp), so I'd imagine they would have a similar problem, if I'm understanding you correctly.

ah I think a 'loop' might be the wrong term here. It's definitely weird to me we're importing from the LMS into the CMS tho. I'd expect shared code to exist in /openedx or /common. I guess if we're already following an established pattern for CourseApps best not to break it however.

alangsto

One nit, but otherwise this looks good!

alangsto · 2024-01-12T14:27:29Z

learning_assistant/platform_imports.py

+        * course_key (CourseKey): the key of the course in which to get the user's role
+
+    Returns:
+        * bool: whether the Learning Assistant feature is available


NIT: return type is the same as above

Zacharis278 · 2024-01-22T18:10:55Z

learning_assistant/platform_imports.py

+        * bool: whether the Learning Assistant feature is available
+    """
+    # pylint: disable=import-error, import-outside-toplevel
+    from lms.djangoapps.courseware.toggles import learning_assistant_is_active


ah I think a 'loop' might be the wrong term here. It's definitely weird to me we're importing from the LMS into the CMS tho. I'd expect shared code to exist in /openedx or /common. I guess if we're already following an established pattern for CourseApps best not to break it however.

Zacharis278 · 2024-01-22T18:15:14Z

learning_assistant/data.py

+
+
+@frozen
+class LearningAssistantCourseEnabledData:


makes sense to me 👍

…trypoint This commit adds a concrete implementation of the CourseApp ABC from the CourseApps edx-platform Django app. This enables the Learning Assistant to be represented by a card on the Studio Pages & Resources pages. Please see the associated ADR for more details. This commit also registers this CourseApp class as an entrypoint to the CourseApps edx-platfrom Django app. Because the CourseApps REST API is a part of the CMS, the Learning Assistant plugin was also added as entrypoint to the CMS.

MichaelRoytman force-pushed the michaelroytman/COSMO-135-CourseApp-plugin branch from 61a8e66 to 03ccf9a Compare January 10, 2024 17:21

MichaelRoytman commented Jan 10, 2024

View reviewed changes

MichaelRoytman force-pushed the michaelroytman/COSMO-135-CourseApp-plugin branch 5 times, most recently from d77cadf to 440c711 Compare January 10, 2024 22:17

Zacharis278 reviewed Jan 11, 2024

View reviewed changes

MichaelRoytman force-pushed the michaelroytman/COSMO-135-CourseApp-plugin branch from 440c711 to 39b5ab4 Compare January 11, 2024 20:39

alangsto approved these changes Jan 12, 2024

View reviewed changes

MichaelRoytman mentioned this pull request Jan 12, 2024

Add settings modal for Xpert Learning Assistant feature. openedx/frontend-app-authoring#794

Merged

Zacharis278 approved these changes Jan 22, 2024

View reviewed changes

MichaelRoytman force-pushed the michaelroytman/COSMO-135-CourseApp-plugin branch 3 times, most recently from deb03c4 to 35d74e6 Compare January 23, 2024 19:57

MichaelRoytman mentioned this pull request Jan 29, 2024

feat: return 403 if Learning Assistant feature toggle or instructor toggle disabled #60

Merged

MichaelRoytman force-pushed the michaelroytman/COSMO-135-CourseApp-plugin branch 2 times, most recently from 5bf4885 to ef032a0 Compare February 6, 2024 19:36

MichaelRoytman force-pushed the michaelroytman/COSMO-135-CourseApp-plugin branch from ef032a0 to 705b526 Compare February 6, 2024 19:56

MichaelRoytman merged commit b64608f into main Feb 7, 2024
8 checks passed

MichaelRoytman deleted the michaelroytman/COSMO-135-CourseApp-plugin branch February 7, 2024 13:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add CourseApp for Learning Assistant feature and register as entrypoint #52

feat: add CourseApp for Learning Assistant feature and register as entrypoint #52

MichaelRoytman commented Jan 10, 2024 •

edited

Loading

MichaelRoytman Jan 10, 2024

MichaelRoytman Jan 10, 2024 •

edited

Loading

Zacharis278 Jan 11, 2024

Zacharis278 Jan 11, 2024

MichaelRoytman Jan 11, 2024

Zacharis278 Jan 22, 2024

Zacharis278 Jan 11, 2024

MichaelRoytman Jan 11, 2024

Zacharis278 Jan 22, 2024

alangsto left a comment

alangsto Jan 12, 2024

Zacharis278 Jan 22, 2024

Zacharis278 Jan 22, 2024

feat: add CourseApp for Learning Assistant feature and register as entrypoint #52

feat: add CourseApp for Learning Assistant feature and register as entrypoint #52

Conversation

MichaelRoytman commented Jan 10, 2024 • edited Loading

Description

Choose a reason for hiding this comment

MichaelRoytman Jan 10, 2024 • edited Loading

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

alangsto 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

MichaelRoytman commented Jan 10, 2024 •

edited

Loading

MichaelRoytman Jan 10, 2024 •

edited

Loading