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: GET endpoint to retrieve message history #119

Merged
merged 14 commits into from
Oct 31, 2024
13 changes: 12 additions & 1 deletion learning_assistant/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

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.models import LearningAssistantCourseEnabled, LearningAssistantMessage
from learning_assistant.platform_imports import (
block_get_children,
block_leaf_filter,
Expand Down Expand Up @@ -187,3 +187,14 @@
course_data = get_cache_course_run_data(course_run_id, ['course'])
course_key = course_data['course']
return course_key


def get_message_history(course_id, user, message_count):
"""
Given a course run id (str), user (User), and message count (int), return the associated message history.

Returns a number of messages equal to the message_count value.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: If the message count exceeds the number of messages, we could return messages that are less than this value (in other words, the number of messages cannot exceed this value but could be less than this value).

"""
message_history = LearningAssistantMessage.objects.filter(
course_id=course_id, user=user).order_by('-created')[:message_count]
Copy link
Member

Choose a reason for hiding this comment

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

Would we need the handle the case here if the message count exceeds the number of messages? (i.e. to prevent and index error)

return message_history.content

Check failure on line 200 in learning_assistant/api.py

View workflow job for this annotation

GitHub Actions / tests (ubuntu-20.04, 3.11, django42)

Missing coverage

Missing coverage on lines 198-200
2 changes: 2 additions & 0 deletions learning_assistant/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,5 @@
"html": "TEXT",
"video": "VIDEO",
}

MESSAGE_COUNT = r'^[0-9]+$'
9 changes: 7 additions & 2 deletions learning_assistant/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
"""
from django.urls import re_path

from learning_assistant.constants import COURSE_ID_PATTERN
from learning_assistant.views import CourseChatView, LearningAssistantEnabledView
from learning_assistant.constants import COURSE_ID_PATTERN, MESSAGE_COUNT
from learning_assistant.views import CourseChatView, LearningAssistantEnabledView, LearningAssistantMessageHistoryView

app_name = 'learning_assistant'

Expand All @@ -19,4 +19,9 @@
LearningAssistantEnabledView.as_view(),
name='enabled',
),
re_path(
fr'learning_assistant/v1/course_id/{COURSE_ID_PATTERN}/history/{MESSAGE_COUNT}',
LearningAssistantMessageHistoryView.as_view(),
name='message-history',
),
]
79 changes: 78 additions & 1 deletion learning_assistant/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,12 @@
except ImportError:
pass

from learning_assistant.api import get_course_id, learning_assistant_enabled, render_prompt_template
from learning_assistant.api import (
get_course_id,
get_message_history,
learning_assistant_enabled,
render_prompt_template,
)
from learning_assistant.serializers import MessageSerializer
from learning_assistant.utils import get_chat_response, user_role_is_staff

Expand Down Expand Up @@ -149,3 +154,75 @@
}

return Response(status=http_status.HTTP_200_OK, data=data)


class LearningAssistantMessageHistoryView(APIView):
"""
View to retrieve the message history for user in a course.

This endpoint returns the message history stored in the LearningAssistantMessage table in a course
represented by the course_key, which is provided in the URL.

Accepts: [GET]

Path: /learning_assistant/v1/course_id/{course_key}/history/{message_count}
Copy link
Member

Choose a reason for hiding this comment

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

NIT: Could you update the path to reflect that message_count is no longer a path parameter?


Parameters:
* course_key: the ID of the course

Responses:
* 200: OK
* 400: Malformed Request - Course ID is not a valid course ID.
"""

authentication_classes = (SessionAuthentication, JwtAuthentication,)
permission_classes = (IsAuthenticated,)

def get(self, request, course_run_id, message_count):
Copy link
Member

Choose a reason for hiding this comment

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

I think the message count should be a query parameter, not a path parameter. If no message count is provided as a query parameter, we should have a default value.

"""
Given a course run ID, retrieve the message history for the corresponding user.

The response will be in the following format.

{
'status': <str>,
'data: {
'detail': <str>,
OR
'message_history': <str object>,
},
}
"""
try:
courserun_key = CourseKey.from_string(course_run_id)
except InvalidKeyError:
return Response(
status=http_status.HTTP_400_BAD_REQUEST,
data={'detail': 'Course ID is not a valid course ID.'}
)

if not learning_assistant_enabled(courserun_key):
return Response(
status=http_status.HTTP_403_FORBIDDEN,
data={'detail': 'Learning assistant not enabled for course.'}
)

# If user does not have an enrollment record, or is not staff, they should not have access
# NOTE: Do we need this? Do we currently not have any plans to show message history to audit mode learners?
Copy link
Member

Choose a reason for hiding this comment

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

I think we do want to have this logic for now and then will adjust once we add the audit experience work. We'll need to have logic to limit access to audit learners.

user_role = get_user_role(request.user, courserun_key)
enrollment_object = CourseEnrollment.get_enrollment(request.user, courserun_key)
enrollment_mode = enrollment_object.mode if enrollment_object else None
if (
(enrollment_mode not in CourseMode.VERIFIED_MODES)
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure this logic condition is correct?
We should return if user_role_is_staff, no?
Are you sure we don't want to show message history to audit mode learners?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not quite sure, I copy/pasted this from another view so this could be wrong. @alangsto Do you have an opinion on this?

and not user_role_is_staff(user_role)
):
return Response(
status=http_status.HTTP_403_FORBIDDEN,
data={'detail': 'Must be staff or have valid enrollment.'}
)

course_id = get_course_id(course_run_id)
user = request.user
data = get_message_history(course_id, user, message_count)
Copy link
Member

Choose a reason for hiding this comment

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

Before being returned, this should be serialized (like https://github.com/edx/edx-exams/blob/d8cf60a98bd86be4cab31f16ae5969a7c52ef396/edx_exams/apps/api/v1/views.py#L227). This gives us better control over what we return.


return Response(status=http_status.HTTP_200_OK, data=data)

Check failure on line 228 in learning_assistant/views.py

View workflow job for this annotation

GitHub Actions / tests (ubuntu-20.04, 3.11, django42)

Missing coverage

Missing coverage on lines 196-228
Loading