-
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: GET endpoint to retrieve message history #119
Changes from 5 commits
77b1f78
bd4102c
eca0b80
7ac52f4
8df9f11
43a106a
5816c33
d1157de
2d9fe12
a7982d8
2b27837
fb79e58
a506d86
4041df4
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 |
---|---|---|
|
@@ -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, | ||
|
@@ -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. | ||
""" | ||
message_history = LearningAssistantMessage.objects.filter( | ||
course_id=course_id, user=user).order_by('-created')[:message_count] | ||
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. 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 | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,3 +14,5 @@ | |
"html": "TEXT", | ||
"video": "VIDEO", | ||
} | ||
|
||
MESSAGE_COUNT = r'^[0-9]+$' |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
||
|
@@ -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} | ||
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. 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): | ||
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. 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? | ||
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. 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) | ||
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. Are you sure this logic condition is correct? 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. 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) | ||
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. 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) | ||
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.
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).