-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: add CourseApp for Learning Assistant feature and register as entrypoint #52
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,3 +8,4 @@ omit = | |
*admin.py | ||
*static* | ||
*templates* | ||
learning_assistant/plugins.py |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
""" | ||
Data classes for the Learning Assistant application. | ||
""" | ||
from attrs import field, frozen, validators | ||
from opaque_keys.edx.keys import CourseKey | ||
|
||
|
||
@frozen | ||
class LearningAssistantCourseEnabledData: | ||
""" | ||
Data class representing whether Learning Assistant is enabled in a course. | ||
""" | ||
|
||
course_key: CourseKey = field(validator=validators.instance_of(CourseKey)) | ||
enabled: bool = field(validator=validators.instance_of(bool)) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -77,3 +77,19 @@ def learning_assistant_available(course_key): | |
# pylint: disable=import-error, import-outside-toplevel | ||
from lms.djangoapps.courseware.toggles import learning_assistant_is_active | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thinking out loud. Does this create a cms -> learning -> lms import loop and is that a problem in any way? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Some other plugins are defined in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
return learning_assistant_is_active(course_key) | ||
|
||
|
||
def get_user_role(user, course_key): | ||
""" | ||
Return the role of the user on the edX platform. | ||
|
||
Arguments: | ||
* user (User): the user who's role to get | ||
* course_key (CourseKey): the key of the course in which to get the user's role | ||
|
||
Returns: | ||
* str: the user's role | ||
""" | ||
# pylint: disable=import-error, import-outside-toplevel | ||
from lms.djangoapps.courseware.access import get_user_role as platform_get_user_role | ||
return platform_get_user_role(user, course_key) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,90 @@ | ||
""" | ||
Plugins for the Learning Assistant application. | ||
""" | ||
# pylint: disable=import-error | ||
from openedx.core.djangoapps.course_apps.plugins import CourseApp | ||
|
||
from learning_assistant import plugins_api | ||
|
||
|
||
class LearningAssistantCourseApp(CourseApp): | ||
""" | ||
A CourseApp plugin representing the Learning Assistant feature. | ||
|
||
Please see the associated ADR for more details. | ||
""" | ||
|
||
app_id = 'learning_assistant' | ||
name = 'Learning Assistant' | ||
description = 'Use generative AI to power a Learning Assistant using course content.' | ||
documentation_links = { | ||
'learn_more_openai': 'https://openai.com/', | ||
'learn_more_openai_data_privacy': 'https://openai.com/api-data-privacy', | ||
} | ||
|
||
@classmethod | ||
def is_available(cls, course_key): | ||
""" | ||
Return a boolean indicating this course app's availability for a given course. | ||
|
||
If an app is not available, it will not show up in the UI at all for that course, | ||
and it will not be possible to enable/disable/configure it. | ||
|
||
Args: | ||
course_key (CourseKey): Course key for course whose availability is being checked. | ||
|
||
Returns: | ||
bool: Availability status of app. | ||
""" | ||
return plugins_api.is_available(course_key) | ||
|
||
@classmethod | ||
def is_enabled(cls, course_key): | ||
""" | ||
Return if this course app is enabled for the provided course. | ||
|
||
Args: | ||
course_key (CourseKey): The course key for the course you | ||
want to check the status of. | ||
|
||
Returns: | ||
bool: The status of the course app for the specified course. | ||
""" | ||
return plugins_api.is_enabled(course_key) | ||
|
||
@classmethod | ||
def set_enabled(cls, course_key, enabled, user): | ||
""" | ||
Update the status of this app for the provided course and return the new status. | ||
|
||
Args: | ||
course_key (CourseKey): The course key for the course for which the app should be enabled. | ||
enabled (bool): The new status of the app. | ||
user (User): The user performing this operation. | ||
|
||
Returns: | ||
bool: The new status of the course app. | ||
""" | ||
return plugins_api.set_enabled(course_key, enabled, user) | ||
|
||
@classmethod | ||
def get_allowed_operations(cls, course_key, user=None): | ||
""" | ||
Return a dictionary of available operations for this app. | ||
|
||
Not all apps will support being configured, and some may support | ||
other operations via the UI. This will list, the minimum whether | ||
the app can be enabled/disabled and whether it can be configured. | ||
|
||
Args: | ||
course_key (CourseKey): The course key for a course. | ||
user (User): The user for which the operation is to be tested. | ||
|
||
Returns: | ||
A dictionary that has keys like 'enable', 'configure' etc | ||
with values indicating whether those operations are allowed. | ||
|
||
get_allowed_operations: function that returns a dictionary of the form | ||
{'enable': <bool>, 'configure': <bool>}. | ||
""" | ||
return plugins_api.get_allowed_operations(course_key, user) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,88 @@ | ||
""" | ||
Concrete implementations of abstract methods of the CourseApp plugin ABC, for use by the LearningAssistantCourseApp. | ||
|
||
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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ahh good idea |
||
""" | ||
|
||
from learning_assistant.api import learning_assistant_enabled, set_learning_assistant_enabled | ||
from learning_assistant.platform_imports import get_user_role, learning_assistant_available | ||
from learning_assistant.utils import user_role_is_staff | ||
|
||
|
||
def is_available(course_key): | ||
""" | ||
Return a boolean indicating this course app's availability for a given course. | ||
|
||
If an app is not available, it will not show up in the UI at all for that course, | ||
and it will not be possible to enable/disable/configure it. | ||
|
||
Args: | ||
course_key (CourseKey): Course key for course whose availability is being checked. | ||
|
||
Returns: | ||
bool: Availability status of app. | ||
""" | ||
return learning_assistant_available(course_key) | ||
|
||
|
||
def is_enabled(course_key): | ||
""" | ||
Return if this course app is enabled for the provided course. | ||
|
||
Args: | ||
course_key (CourseKey): The course key for the course you | ||
want to check the status of. | ||
|
||
Returns: | ||
bool: The status of the course app for the specified course. | ||
""" | ||
return learning_assistant_enabled(course_key) | ||
|
||
|
||
# pylint: disable=unused-argument | ||
def set_enabled(course_key, enabled, user): | ||
""" | ||
Update the status of this app for the provided course and return the new status. | ||
|
||
Args: | ||
course_key (CourseKey): The course key for the course for which the app should be enabled. | ||
enabled (bool): The new status of the app. | ||
user (User): The user performing this operation. | ||
|
||
Returns: | ||
bool: The new status of the course app. | ||
""" | ||
obj = set_learning_assistant_enabled(course_key, enabled) | ||
|
||
return obj.enabled | ||
|
||
|
||
def get_allowed_operations(course_key, user=None): | ||
""" | ||
Return a dictionary of available operations for this app. | ||
|
||
Not all apps will support being configured, and some may support | ||
other operations via the UI. This will list, the minimum whether | ||
the app can be enabled/disabled and whether it can be configured. | ||
|
||
Args: | ||
course_key (CourseKey): The course key for a course. | ||
user (User): The user for which the operation is to be tested. | ||
|
||
Returns: | ||
A dictionary that has keys like 'enable', 'configure' etc | ||
with values indicating whether those operations are allowed. | ||
|
||
get_allowed_operations: function that returns a dictionary of the form | ||
{'enable': <bool>, 'configure': <bool>}. | ||
""" | ||
if not user: | ||
return {'configure': False, 'enable': False} | ||
else: | ||
user_role = get_user_role(user, course_key) | ||
is_staff = user_role_is_staff(user_role) | ||
|
||
return {'configure': False, 'enable': is_staff} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -131,6 +131,12 @@ def is_requirement(line): | |
entry_points={ | ||
'lms.djangoapp': [ | ||
"learning_assistant = learning_assistant.apps:LearningAssistantConfig" | ||
], | ||
'cms.djangoapp': [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because the |
||
"learning_assistant = learning_assistant.apps:LearningAssistantConfig" | ||
], | ||
'openedx.course_app': [ | ||
"learning_assistant = learning_assistant.plugins:LearningAssistantCourseApp", | ||
] | ||
} | ||
) |
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.
what's the data class used for?
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.
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 theCourseApp
ABC requires that theset_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 treatingset_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.
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.
makes sense to me 👍