-
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: Adding chat messages to the DB #121
Conversation
1f63bf7
to
c98c8b2
Compare
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
2e8ddd7
to
4f2529d
Compare
@@ -76,6 +79,15 @@ def post(self, request, course_run_id): | |||
unit_id = request.query_params.get('unit_id') | |||
|
|||
message_list = request.data | |||
|
|||
# Check that the last message in the list corresponds to a user | |||
new_user_message = message_list[-1] |
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 don't really understand the code block and the comment. Can you explain more to help someone like to understand the logic better?
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 if the message_list array is empty?
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 assumed the endpoint was to process a new user message. Maybe I misunderstood it but if it is, then it should include at least one user message.
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.
Just to point out: We discussed this offline and came to the conclusion that the approach is correct.
{'role': 'assistant', 'content': 'It is 4'}, | ||
{'role': 'user', 'content': 'And what else?'}, |
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've added a check that validates the last message from the list to be from a user.
course_id = CourseKeyField(max_length=255, db_index=True) | ||
user = models.ForeignKey(USER_MODEL, db_index=True, on_delete=models.CASCADE) | ||
role = models.CharField(max_length=64) | ||
role = models.CharField(choices=Roles, max_length=64) |
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 think this doesn't affect the database, so no migration needed.
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.
Are you sure? Did you run makemigrations
?
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 didn't. I'm sorry for that.
Just added the migration file.
learning_assistant/views.py
Outdated
@@ -103,6 +115,22 @@ def post(self, request, course_run_id): | |||
) | |||
status_code, message = get_chat_response(prompt_template, message_list) | |||
|
|||
user = User.objects.get(id=request.user.id) # Based on the previous code, user exists. |
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 feel the code block below should be wrapped into a new function
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.
Moved to the API module.
learning_assistant/views.py
Outdated
@@ -35,6 +38,27 @@ class CourseChatView(APIView): | |||
authentication_classes = (SessionAuthentication, JwtAuthentication,) | |||
permission_classes = (IsAuthenticated,) | |||
|
|||
def __save_user_interaction(self, user_id, user_message, assistant_message): |
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.
can this be moved to the api.py
file? I also think it might make more sense for it to look like save_user_message(user_id, role, message)
, and call it twice from within the view.
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.
Moved to the API module and added two calls in the view.
learning_assistant/views.py
Outdated
@@ -103,6 +136,12 @@ def post(self, request, course_run_id): | |||
) | |||
status_code, message = get_chat_response(prompt_template, message_list) | |||
|
|||
self.__save_user_interaction( |
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.
This needs to be gated by a waffle flag, which also needs to be created.
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.
Oh shoot. I totally forgot about it. Thanks for calling this out!
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.
Added waffle flag and gated the functionality behind it.
84e2983
to
e1c3471
Compare
User = get_user_model() | ||
|
||
|
||
class TestClient(Client): | ||
class FakeClient(Client): |
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.
This class was renamed to remove the following test warning:
tests/test_views.py:22
/Users/(ueser)/edx/src/learning-assistant/tests/test_views.py:22: PytestCollectionWarning: cannot collect test class 'TestClient' because it has a __init__ constructor (from: tests/test_views.py)
class TestClient(Client):
956d19d
to
e28918a
Compare
learning_assistant/views.py
Outdated
course_id = get_course_id(course_run_id) | ||
user_id = request.user.id | ||
|
||
if chat_history_enabled(course_id): |
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.
Should this be the course_id or a course key?
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.
Good catch. According to the code to check the flag we check for Learning Assistant enabled state is a CourseKey based on the course run. I'll update it quickly.
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.
Updated.
9fa2e46
to
6106dc8
Compare
8bc73aa
to
7cdc763
Compare
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.
👍
course_id = CourseKeyField(max_length=255, db_index=True) | ||
user = models.ForeignKey(USER_MODEL, db_index=True, on_delete=models.CASCADE) | ||
role = models.CharField(max_length=64) | ||
role = models.CharField(choices=Roles, max_length=64) |
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.
Are you sure? Did you run makemigrations
?
learning_assistant/api.py
Outdated
@@ -189,6 +191,24 @@ def get_course_id(course_run_id): | |||
return course_key | |||
|
|||
|
|||
def save_chat_message(user_id, chat_role, message): |
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.
This function should also take in a course_id, and use that as a field in the message being saved
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.
Nice catch. I wonder why this wasn't detected on the unit tests, maybe there's a constraint missing?
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.
Just updated save_chat_message()
to take the Courserun Key and save it to the model.
c20a920
to
2fe0ceb
Compare
2fe0ceb
to
4fa9bf5
Compare
Just to confirm, I've validated locally that no message is saved without enabling the feature flag and this is the result after enabling it:
|
This update adds the last user message to the course chat view.
I've also added some constants to
LearningAssistantMessage
model for consistency.Warning
I've added an extra validation on the endpoint requiring the last message from the chat to be from the user.