fix: Updated Learning Assistant History payload to return in ascending order #129
Conversation
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||
rijuma
force-pushed
the
rijuma/fix-message-history-order
branch
2 times, most recently
from
da175b6 to
a430ccb
Compare
rijuma
force-pushed
the
rijuma/fix-message-history-order
branch
from
a430ccb to
cc51613
Compare
learning_assistant/views.py
Outdated
| @@ -236,6 +236,6 @@ def get(self, request, course_run_id): | |||
| user = request.user | |||
|
|
|||
| message_count = int(request.GET.get('message_count', 50)) | |||
| message_history = get_message_history(courserun_key, user, message_count) | |||
| message_history = reversed(list(get_message_history(courserun_key, user, message_count))) # Reversing order | |||
There was a problem hiding this comment.
instead of reversing it here, can we just change the ordering here?
learning-assistant/learning_assistant/api.py
Line 221 in 9153aa1
There was a problem hiding this comment.
We can. It was one or the other. I'll make the change.
There was a problem hiding this comment.
Moved the reverse logic to the API call and updated corresponding tests.
rijuma
force-pushed
the
rijuma/fix-message-history-order
branch
2 times, most recently
from
2d310cd to
bc5ac84
Compare
rijuma
force-pushed
the
rijuma/fix-message-history-order
branch
from
bc5ac84 to
2f93c7e
Compare
| message_history = LearningAssistantMessage.objects.filter( | ||
| course_id=courserun_key, user=user).order_by('-created')[:message_count] | ||
| message_history = list(LearningAssistantMessage.objects.filter( | ||
| course_id=courserun_key, user=user).order_by('-created')[:message_count])[::-1] |
There was a problem hiding this comment.
You can get the same affect by doing order_by('created'). Right now you're still sorting twice.
There was a problem hiding this comment.
Discussed offline and the alternative would be less performant. I've added a comment with a justification on this.
rijuma
force-pushed
the
rijuma/fix-message-history-order
branch
2 times, most recently
from
ecec467 to
20b1a74
Compare
learning_assistant/api.py
Outdated
| # The result is the last chat messages for that user and course but in inversed order, so in order to flip them | ||
| # its first turn into a list and then reversed. | ||
| # The alternative of getting the last message_count ordering the query in ascending created order is not possible | ||
| # since negative indexing is not supported. |
There was a problem hiding this comment.
Sorry to nit pick, but I still don't think this explains why we are limiting the number of messages prior to re-ordering them. It would be helpful to explain that the indexing is equivalent to the sql limit, which is more efficient than retrieving all messages and then processing that query set.
There was a problem hiding this comment.
Just updated it. Let me know what you think.
rijuma
force-pushed
the
rijuma/fix-message-history-order
branch
from
20b1a74 to
4e1ac79
Compare
| @@ -2,6 +2,6 @@ | |||
| Plugin for a learning assistant backend, intended for use within edx-platform. | |||
| """ | |||
|
|
|||
| __version__ = '4.4.4' | |||
| __version__ = '4.4.5' | |||
There was a problem hiding this comment.
Can you include a changelog entry?
There was a problem hiding this comment.
We want the endpoint to return the chat history in ascending order by
timestamp(creation datetime).