Skip to content
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

Merged
merged 1 commit into from
Feb 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .coveragerc
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,4 @@ omit =
*admin.py
*static*
*templates*
learning_assistant/plugins.py
4 changes: 4 additions & 0 deletions docs/decisions/0003-courseapp-use.rst
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ Consequences
Assistant plugin is installed into ``edx-platform``.
* It will become slightly more difficult to know which ``CourseApps`` are available simply by reading the code, because
this ``CourseApp`` plugin is not stored in the `edx-platform repository`_.
* Because the `CourseApps` API is registered under the CMS application, the Learning Assistant needs to be registered as
a plugin to the CMS as well. Otherwise, the plugin is not included in the CMS application's ``INSTALLED_APPS`` list.
This causes a runtime error, because the Learning Assistant CourseApp plugin will refer to the Learning Assistant's
models, and this are not available in the CMS if the Learning Assistant plugin is not installed.

Rejected Alternatives
*********************
Expand Down
2 changes: 1 addition & 1 deletion learning_assistant/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,6 @@
Plugin for a learning assistant backend, intended for use within edx-platform.
"""

__version__ = '3.4.0'
__version__ = '3.5.0'

default_app_config = 'learning_assistant.apps.LearningAssistantConfig' # pylint: disable=invalid-name
23 changes: 23 additions & 0 deletions learning_assistant/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from opaque_keys.edx.keys import CourseKey

from learning_assistant.constants import ACCEPTED_CATEGORY_TYPES, CATEGORY_TYPE_MAP
from learning_assistant.data import LearningAssistantCourseEnabledData
from learning_assistant.models import LearningAssistantCourseEnabled
from learning_assistant.platform_imports import (
block_get_children,
Expand Down Expand Up @@ -139,3 +140,25 @@ def learning_assistant_enabled(course_key):
enabled = True

return learning_assistant_available(course_key) and enabled


def set_learning_assistant_enabled(course_key, enabled):
"""
Set whether the Learning Assistant is enabled and return a representation of the created data.
Arguments:
* course_key: (CourseKey): the course's key
* enabled (bool): whether the Learning Assistant should be enabled
Returns:
* bool: whether the Learning Assistant is enabled
"""
obj, _ = LearningAssistantCourseEnabled.objects.update_or_create(
course_id=course_key,
defaults={'enabled': enabled}
)

return LearningAssistantCourseEnabledData(
course_key=obj.course_id,
enabled=obj.enabled
)
15 changes: 15 additions & 0 deletions learning_assistant/data.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:
Copy link
Member

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?

Copy link
Member Author

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 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense to me 👍

"""
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))
16 changes: 16 additions & 0 deletions learning_assistant/platform_imports.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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 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.

Copy link
Member

Choose a reason for hiding this comment

The 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)
90 changes: 90 additions & 0 deletions learning_assistant/plugins.py
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)
88 changes: 88 additions & 0 deletions learning_assistant/plugins_api.py
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.
Copy link
Member

Choose a reason for hiding this comment

The 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}
13 changes: 13 additions & 0 deletions learning_assistant/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -123,3 +123,16 @@ def get_chat_response(prompt_template, message_list, courserun_id):
chat = 'Completion endpoint is not defined.'

return response_status, chat


def user_role_is_staff(role):
Zacharis278 marked this conversation as resolved.
Show resolved Hide resolved
"""
Return whether the user role parameter represents that of a staff member.

Arguments:
* role (str): the user's role

Returns:
* bool: whether the user's role is that of a staff member
"""
return role in ('staff', 'instructor')
4 changes: 2 additions & 2 deletions learning_assistant/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@

from learning_assistant.api import learning_assistant_enabled, render_prompt_template
from learning_assistant.serializers import MessageSerializer
from learning_assistant.utils import get_chat_response
from learning_assistant.utils import get_chat_response, user_role_is_staff

log = logging.getLogger(__name__)

Expand Down Expand Up @@ -65,7 +65,7 @@ def post(self, request, course_id):
enrollment_mode = enrollment_object.mode if enrollment_object else None
if (
(enrollment_mode not in CourseMode.VERIFIED_MODES)
and user_role not in ('staff', 'instructor')
and not user_role_is_staff(user_role)
):
return Response(
status=http_status.HTTP_403_FORBIDDEN,
Expand Down
1 change: 1 addition & 0 deletions requirements/base.in
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# Core requirements for using this application
-c constraints.txt

attrs
Django # Web application framework
django-model-utils
djangorestframework
Expand Down
2 changes: 2 additions & 0 deletions requirements/base.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
#
asgiref==3.7.2
# via django
attrs==23.2.0
# via -r requirements/base.in
certifi==2023.11.17
# via requests
cffi==1.16.0
Expand Down
2 changes: 2 additions & 0 deletions requirements/dev.txt
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ astroid==3.0.2
# -r requirements/quality.txt
# pylint
# pylint-celery
attrs==23.2.0
# via -r requirements/quality.txt
build==1.0.3
# via
# -r requirements/pip-tools.txt
Expand Down
2 changes: 2 additions & 0 deletions requirements/doc.txt
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ asgiref==3.7.2
# via
# -r requirements/test.txt
# django
attrs==23.2.0
# via -r requirements/test.txt
babel==2.14.0
# via sphinx
build==1.0.3
Expand Down
2 changes: 2 additions & 0 deletions requirements/quality.txt
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ astroid==3.0.2
# via
# pylint
# pylint-celery
attrs==23.2.0
# via -r requirements/test.txt
certifi==2023.11.17
# via
# -r requirements/test.txt
Expand Down
2 changes: 2 additions & 0 deletions requirements/test.txt
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ asgiref==3.7.2
# via
# -r requirements/base.txt
# django
attrs==23.2.0
# via -r requirements/base.txt
certifi==2023.11.17
# via
# -r requirements/base.txt
Expand Down
6 changes: 6 additions & 0 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,12 @@ def is_requirement(line):
entry_points={
'lms.djangoapp': [
"learning_assistant = learning_assistant.apps:LearningAssistantConfig"
],
'cms.djangoapp': [
Copy link
Member Author

@MichaelRoytman MichaelRoytman Jan 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"learning_assistant = learning_assistant.apps:LearningAssistantConfig"
],
'openedx.course_app': [
"learning_assistant = learning_assistant.plugins:LearningAssistantCourseApp",
]
}
)
Loading
Loading