feat: GET endpoint to retrieve message history #119

ilee2u · 2024-10-25T20:30:48Z

Ticket: https://2u-internal.atlassian.net/browse/COSMO-434

Add GET endpoint to learning-assistant to retrieve message history for a user/course combination.

Should get messages by user/course combination
Should add query parameter to limit the amount of messages we receive

learning_assistant/views.py

learning_assistant/urls.py

schenedx · 2024-10-28T12:58:32Z

learning_assistant/views.py

+        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)


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?

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

learning_assistant/api.py

github-actions · 2024-10-29T20:31:47Z

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
learning_assistant
__init__.py
api.py
text_utils.py
urls.py
views.py					191-192
Project Total

_{This report was generated by python-coverage-comment-action}

varshamenon4

Looks good so far! A couple of questions/comments!

varshamenon4 · 2024-10-29T20:49:29Z

learning_assistant/api.py

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


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

varshamenon4 · 2024-10-29T20:50:07Z

learning_assistant/api.py

+    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]


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

varshamenon4 · 2024-10-29T20:52:20Z

learning_assistant/views.py

+            )
+
+        # 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?


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.

alangsto · 2024-10-30T12:32:03Z

learning_assistant/views.py

+    authentication_classes = (SessionAuthentication, JwtAuthentication,)
+    permission_classes = (IsAuthenticated,)
+
+    def get(self, request, course_run_id, message_count):


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.

alangsto · 2024-10-30T12:33:34Z

learning_assistant/views.py

+
+        course_id = get_course_id(course_run_id)
+        user = request.user
+        data = get_message_history(course_id, user, message_count)


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.

- some docstring nits - logic to ensure no index error w/ message_count in api - serialize data before returning

alangsto · 2024-10-30T20:04:54Z

learning_assistant/api.py

+    Returns a number of messages equal to the message_count value.
+    """
+    # If the received message count exceeds the number of messages present, we return all the messages queried.
+    actual_message_count = LearningAssistantMessage.objects.count()


I actually don't know if this is strictly necessary. Have you tested the code without this, and just indexing using the message_count value? Only asking because the same syntax is used here: https://github.com/openedx/edx-platform/blob/b20498cc4eec4ed1b85508967bef6c915372dd5b/openedx/core/djangoapps/notifications/tasks.py#L105

Oh that actually works. I'll get rid of this code.

alangsto · 2024-10-30T20:05:27Z

learning_assistant/views.py

+    authentication_classes = (SessionAuthentication, JwtAuthentication,)
+    permission_classes = (IsAuthenticated,)
+
+    def get(self, request, course_run_id, message_count=50):


I still think this should be a query parameter, not a path parameter.

I keep mixing these up- I think I've got it right now, gonna use `request.GET.get('message_count', 50) instead.

Side note, what do you think the default value should be? Is 50 good?

Yes, that should be the right syntax! And 50 seems good.

alangsto · 2024-10-30T20:08:04Z

tests/test_views.py

+    @patch('learning_assistant.views.CourseMode')
+    @patch('learning_assistant.views.get_course_id')
+    def test_learning_message_history_view_get(self,
+                                               mock_get_course_id,


NIT: This spacing looks a little strange to me, can you remove the indents?

this was throwing a "line too long" quality error, I might just ignore it instead.

Actually, I a better way by just moving the "self" param to the next line, then moving the spacing of all the params back to one tab past the function declaration.

You can also keep them on the same line, they don't each need to be on their own!

alangsto · 2024-10-30T20:10:35Z

learning_assistant/urls.py

@@ -19,4 +19,9 @@
        LearningAssistantEnabledView.as_view(),
        name='enabled',
    ),
+    re_path(
+        fr'learning_assistant/v1/course_id/{COURSE_ID_PATTERN}/history/<int:message_count>',


the message count shouldn't be specified in the path, in my opinion. Happy to be convinced otherwise, but it feels more appropriate, as the message count param is used to filter on the resource being specified.

Just realizing query parameters don't need to be in the URL... I'll get rid of that now

schenedx

👍

alangsto

A few nits but otherwise looks good!

alangsto · 2024-10-31T12:58:32Z

learning_assistant/views.py

+
+    Accepts: [GET]
+
+    Path: /learning_assistant/v1/course_id/{course_key}/history/{message_count}


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

alangsto · 2024-10-31T12:59:41Z

learning_assistant/views.py

+            )
+
+        # If user does not have an enrollment record, or is not staff, they should not have access
+        # NOTE: This will likely be removed once work is done to allow audit learners to access xpert


NIT: Should this note be left here?

alangsto · 2024-10-31T13:38:15Z

Forgot one other NIT, but could you add an entry to the changelog for this? Thank you!

feat: GET endpoint to retrieve message history

77b1f78

alangsto reviewed Oct 28, 2024

View reviewed changes

learning_assistant/views.py Outdated Show resolved Hide resolved

learning_assistant/views.py Outdated Show resolved Hide resolved

learning_assistant/urls.py Outdated Show resolved Hide resolved

schenedx reviewed Oct 28, 2024

View reviewed changes

learning_assistant/api.py Outdated Show resolved Hide resolved

ilee2u added 2 commits October 29, 2024 16:29

fix: added message count as query parameter

bd4102c

chore: rename to MESSAGE_COUNT

eca0b80

ilee2u added 2 commits October 29, 2024 16:35

chore: lint

7ac52f4

chore: isort

8df9f11

varshamenon4 reviewed Oct 29, 2024

View reviewed changes

alangsto reviewed Oct 30, 2024

View reviewed changes

ilee2u added 6 commits October 30, 2024 15:07

fix: corrected query parameter

43a106a

- some docstring nits - logic to ensure no index error w/ message_count in api - serialize data before returning

test: add tests

5816c33

test: fix view tests

d1157de

chore: lint

2d9fe12

chore: lint

a7982d8

chore: LINT

2b27837

alangsto reviewed Oct 30, 2024

View reviewed changes

ilee2u added 2 commits October 30, 2024 16:29

fix: actually correct query param

fb79e58

chore: function param lint

a506d86

schenedx approved these changes Oct 30, 2024

View reviewed changes

alangsto approved these changes Oct 31, 2024

View reviewed changes

chore: nits

4041df4

ilee2u merged commit ecfb6e8 into main Oct 31, 2024
4 checks passed

ilee2u deleted the ilee2u/get-message-history branch October 31, 2024 15:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: GET endpoint to retrieve message history #119

feat: GET endpoint to retrieve message history #119

ilee2u commented Oct 25, 2024

schenedx Oct 28, 2024

ilee2u Oct 29, 2024

github-actions bot commented Oct 29, 2024 •

edited

Loading

varshamenon4 left a comment

varshamenon4 Oct 29, 2024

varshamenon4 Oct 29, 2024

varshamenon4 Oct 29, 2024

alangsto Oct 30, 2024

alangsto Oct 30, 2024

alangsto Oct 30, 2024

ilee2u Oct 30, 2024

alangsto Oct 30, 2024

ilee2u Oct 30, 2024

alangsto Oct 30, 2024

alangsto Oct 30, 2024

ilee2u Oct 30, 2024

ilee2u Oct 30, 2024

alangsto Oct 30, 2024

alangsto Oct 30, 2024

ilee2u Oct 30, 2024

schenedx left a comment

alangsto left a comment

alangsto Oct 31, 2024

alangsto Oct 31, 2024

alangsto commented Oct 31, 2024


		Accepts: [GET]

		Path: /learning_assistant/v1/course_id/{course_key}/history/{message_count}

feat: GET endpoint to retrieve message history #119

feat: GET endpoint to retrieve message history #119

Conversation

ilee2u commented Oct 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 29, 2024 • edited Loading

Coverage report

varshamenon4 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schenedx left a comment

Choose a reason for hiding this comment

alangsto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alangsto commented Oct 31, 2024

github-actions bot commented Oct 29, 2024 •

edited

Loading