Add a BFFE endpoint for Learning Assistant (LA), including information about LA is enabled, LA message history, and LA audit trial. #140

MichaelRoytman · 2024-12-02T21:16:50Z

This pull request adds a back-end-for-frontend (BFFE) endpoint for the Learning Assistant to get all the necessary data it needs to function.

The response from this endpoint includes the following information.

whether the Learning Assistant is enabled
message history information, if the learner is eligible to use the Learning Assistant
audit trial information

github-actions · 2024-12-03T20:19:04Z

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
data.py
urls.py
views.py					21-23
Project Total

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

ilee2u · 2024-12-03T20:34:58Z

learning_assistant/views.py

+            trial_data['start_date'] = trial.start_date
+            trial_data['expiration_date'] = trial.expiration_date
+
+        data['trial'] = trial_data


I currently have the frontend extracting 'audit_trial' and not 'trial' from the GET request data. Wondering if we should name this attribute 'audit_trial', or extract the 'trial' attribute in the frontend. Let me know your thoughts!

Agree on the question. My vote is on audit_trial for consistency.

Sure, no problem. I chose trial because the trial is available to more than just learners in the audit track, but it's not an issue to change.

rijuma · 2024-12-03T21:37:33Z

learning_assistant/urls.py

+    re_path(
+        fr'learning_assistant/v1/course_id/{COURSE_ID_PATTERN}/summary',
+        LearningAssistantSummaryView.as_view(),
+        name='summary',
+    ),


I think we have plans to enable (and integrate) the Unit Summary to Xpert and this /summary reference could be confusing. Maybe we could use something like /chat instead?

To clarify, are you suggesting that I change the URL/name to chat? I think that would conflict with the chat endpoint, so I would prefer not to do that. I also don't think chat represents the purpose of this endpoint.

Personally, I think summary and unit_summary would be reasonable URLs to coexist, and I wouldn't find them confusing. I don't think we should name the URL for unit summaries summary.

What about one of these? overview, details, or info?

I totally forgot about the existing /chat one. I think info fits better, but it's your call.

varshamenon4

Looks great! A few comments and some nits (feel free to disregard those though!).

varshamenon4 · 2024-12-03T20:40:05Z

learning_assistant/api.py

-    DAYS_SINCE_UPGRADE_DEADLINE = datetime.now() - upgrade_deadline
-    if DAYS_SINCE_UPGRADE_DEADLINE >= timedelta(days=0):
-        return True
+    default_trial_length_days = 14


Good idea to have a default!

varshamenon4 · 2024-12-03T21:27:12Z

learning_assistant/api.py

+
+    # If the upgrade deadline has passed, return True for expired. Upgrade deadline is an optional attribute of a
+    # CourseMode, so if it's None, then do not return True.
+    days_since_upgrade_deadline = datetime.now() - upgrade_deadline if upgrade_deadline else None


nit:

Suggested change

days_since_upgrade_deadline = datetime.now() - upgrade_deadline if upgrade_deadline else None

days_until_upgrade_deadline = datetime.now() - upgrade_deadline if upgrade_deadline else None

Thanks for the suggestion. I'll make that change.

varshamenon4 · 2024-12-03T21:28:00Z

learning_assistant/data.py

@@ -13,3 +15,14 @@ class LearningAssistantCourseEnabledData:

    course_key: CourseKey = field(validator=validators.instance_of(CourseKey))
    enabled: bool = field(validator=validators.instance_of(bool))
+
+
+@frozen


question: does this just mean that this cannot be changed?

Yes. The frozen decorator makes LearningAssistantAuditTrialData class immutable, so once an instance of this class is created, its attributes cannot be changed.

... just let it go.

varshamenon4 · 2024-12-03T21:29:59Z

learning_assistant/views.py

+        * 200: OK
+        * 400: Malformed Request - Course ID is not a valid course ID.
+        * 403: Forbidden - Learning assistant not enabled for course or learner does not have a valid enrollment or is
+                           not staff.


nit: I'm not sure if it's just how it's rendered here, but is this spacing intended?

I'm not sure if you're seeing something different than what I'm seeing, but I tend to align subsequent lines of paragraphs to the beginning character of the paragraph, because I find it easier to read. Would you prefer a different indentation?

varshamenon4 · 2024-12-03T21:39:46Z

learning_assistant/views.py

@@ -271,3 +288,125 @@ def get(self, request, course_run_id):
        message_history = get_message_history(courserun_key, user, message_count)
        data = MessageSerializer(message_history, many=True).data
        return Response(status=http_status.HTTP_200_OK, data=data)
+
+
+class LearningAssistantSummaryView(APIView):


varshamenon4 · 2024-12-03T21:42:03Z

learning_assistant/views.py

+        """
+        Given a course run ID, return all the data necessary for the Learning Assistant to fuction.
+
+        The response will be in the following format.


Nice, I like this format!

varshamenon4 · 2024-12-03T21:45:01Z

tests/test_api.py

+    """
+    Test suite for get_audit_trial_expiration_date.
+    """
+    @ddt.data(


These cases look pretty complete to me!

MichaelRoytman · 2024-12-02T21:17:57Z

learning_assistant/api.py

+        trial_length_days = default_trial_length_days
+
+    # If LEARNING_ASSISTANT_AUDIT_TRIAL_LENGTH_DAYS is set to a negative number, assume it should be 0.
+    # pylint: disable=consider-using-max-builtin


pylint told me to "consider" doing trial_length_days = max(trial_length_days, 0) instead, which I thought was less readable, so I didn't. Interesting that it forces me to address something it tells me to consider 😄.

"I'm ~~asking~~ telling you to consider, and I won't take a NO for an answer.." 🤌

MichaelRoytman · 2024-12-02T21:20:09Z

tests/test_views.py

@@ -95,13 +95,6 @@ def setUp(self):
        )
        self.patcher.start()

-    @patch('learning_assistant.views.learning_assistant_enabled')


I'm not sure why this test is here. It tests the enabled endpoint. Copypasta? I removed it.

MichaelRoytman · 2024-12-02T21:22:21Z

tests/test_views.py

@@ -264,13 +261,6 @@ def setUp(self):
        super().setUp()
        self.course_id = 'course-v1:edx+test+23'

-    @patch('learning_assistant.views.learning_assistant_enabled')


I'm not sure why this test is here. It tests the enabled endpoint. Copypasta? I removed it.

MichaelRoytman · 2024-12-03T20:02:15Z

learning_assistant/api.py

+    return expiration_datetime
+
+
+def get_audit_trial(user):


I had to create a get and a get_or_create method because when we make a POST call to the /chat endpoint, we want to create a trial if one doesn't exist, but when we make a GET call to the /summary endpoint, we do not.

MichaelRoytman · 2024-12-03T20:03:03Z

learning_assistant/api.py

+
+    # If the upgrade deadline has passed, return True for expired. Upgrade deadline is an optional attribute of a
+    # CourseMode, so if it's None, then do not return True.
+    days_since_upgrade_deadline = datetime.now() - upgrade_deadline if upgrade_deadline else None


I had to rewrite Isaac's original function a bit, so I don't want this change to get lost in the diff. The expiration_datetime field of the CourseMode model may be None, so I've added handling for that here.

MichaelRoytman · 2024-12-03T20:03:46Z

learning_assistant/views.py

@@ -38,51 +43,28 @@
 class CourseChatView(APIView):
    """
    View to retrieve chat response.
+


I added some missing documentation.

MichaelRoytman · 2024-12-03T21:48:31Z

learning_assistant/urls.py

+    re_path(
+        fr'learning_assistant/v1/course_id/{COURSE_ID_PATTERN}/summary',
+        LearningAssistantSummaryView.as_view(),
+        name='summary',
+    ),


To clarify, are you suggesting that I change the URL/name to chat? I think that would conflict with the chat endpoint, so I would prefer not to do that. I also don't think chat represents the purpose of this endpoint.

Personally, I think summary and unit_summary would be reasonable URLs to coexist, and I wouldn't find them confusing. I don't think we should name the URL for unit summaries summary.

What about one of these? overview, details, or info?

MichaelRoytman · 2024-12-03T21:49:15Z

learning_assistant/views.py

+            trial_data['start_date'] = trial.start_date
+            trial_data['expiration_date'] = trial.expiration_date
+
+        data['trial'] = trial_data


Sure, no problem. I chose trial because the trial is available to more than just learners in the audit track, but it's not an issue to change.

rijuma · 2024-12-04T12:29:48Z

learning_assistant/views.py

+
+    Accepts: [GET]
+
+    Path: /learning_assistant/v1/course_id/{course_run_id}/


nit: Shouldn't this path include the /summary part (or whatever variant you choose for it)?

Yes! Thank you for noticing that - very observant 😀.

…ata to function This commit adds a back-end-for-frontend (BFFE) endpoint for the Learning Assistant to get all the necessary data it needs to function. The response from this endpoint includes the following information. * whether the Learning Assistant is enabled * message history information, if the learner is eligible to use the Learning Assistant * audit trial information

ilee2u mentioned this pull request Dec 3, 2024

feat: Call LearningAssistantSummary endpoint edx/frontend-lib-learning-assistant#68

Merged

MichaelRoytman force-pushed the michaelroytman/COSMO-567-GET-endpoint-BFFE branch 2 times, most recently from 06a3033 to 1420f66 Compare December 3, 2024 20:18

MichaelRoytman marked this pull request as ready for review December 3, 2024 20:20

MichaelRoytman force-pushed the michaelroytman/COSMO-567-GET-endpoint-BFFE branch from 1420f66 to e9652ac Compare December 3, 2024 20:26

ilee2u reviewed Dec 3, 2024

View reviewed changes

rijuma reviewed Dec 3, 2024

View reviewed changes

varshamenon4 reviewed Dec 3, 2024

View reviewed changes

MichaelRoytman commented Dec 3, 2024

View reviewed changes

rijuma reviewed Dec 4, 2024

View reviewed changes

MichaelRoytman force-pushed the michaelroytman/COSMO-567-GET-endpoint-BFFE branch from efc1002 to 45e0393 Compare December 4, 2024 18:44

ilee2u approved these changes Dec 4, 2024

View reviewed changes

MichaelRoytman force-pushed the michaelroytman/COSMO-567-GET-endpoint-BFFE branch from 45e0393 to 1fc3972 Compare December 4, 2024 19:21

MichaelRoytman merged commit ed0061e into main Dec 4, 2024
4 checks passed

MichaelRoytman deleted the michaelroytman/COSMO-567-GET-endpoint-BFFE branch December 4, 2024 19:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a BFFE endpoint for Learning Assistant (LA), including information about LA is enabled, LA message history, and LA audit trial. #140

Add a BFFE endpoint for Learning Assistant (LA), including information about LA is enabled, LA message history, and LA audit trial. #140

MichaelRoytman commented Dec 2, 2024 •

edited

Loading

github-actions bot commented Dec 3, 2024 •

edited

Loading

ilee2u Dec 3, 2024

rijuma Dec 3, 2024

MichaelRoytman Dec 3, 2024

rijuma Dec 3, 2024 •

edited

Loading

MichaelRoytman Dec 3, 2024

rijuma Dec 4, 2024

varshamenon4 left a comment

varshamenon4 Dec 3, 2024

varshamenon4 Dec 3, 2024

MichaelRoytman Dec 3, 2024

varshamenon4 Dec 3, 2024

MichaelRoytman Dec 3, 2024

rijuma Dec 4, 2024

varshamenon4 Dec 3, 2024

MichaelRoytman Dec 3, 2024

varshamenon4 Dec 3, 2024

varshamenon4 Dec 3, 2024

varshamenon4 Dec 3, 2024

MichaelRoytman Dec 2, 2024

rijuma Dec 4, 2024

MichaelRoytman Dec 2, 2024

MichaelRoytman Dec 2, 2024

MichaelRoytman Dec 3, 2024

MichaelRoytman Dec 3, 2024

MichaelRoytman Dec 3, 2024

MichaelRoytman Dec 3, 2024

MichaelRoytman Dec 3, 2024

rijuma Dec 4, 2024

MichaelRoytman Dec 4, 2024

	days_since_upgrade_deadline = datetime.now() - upgrade_deadline if upgrade_deadline else None
	days_until_upgrade_deadline = datetime.now() - upgrade_deadline if upgrade_deadline else None


		Accepts: [GET]

		Path: /learning_assistant/v1/course_id/{course_run_id}/

Add a BFFE endpoint for Learning Assistant (LA), including information about LA is enabled, LA message history, and LA audit trial. #140

Add a BFFE endpoint for Learning Assistant (LA), including information about LA is enabled, LA message history, and LA audit trial. #140

Conversation

MichaelRoytman commented Dec 2, 2024 • edited Loading

github-actions bot commented Dec 3, 2024 • edited Loading

Coverage report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rijuma Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

MichaelRoytman commented Dec 2, 2024 •

edited

Loading

github-actions bot commented Dec 3, 2024 •

edited

Loading

rijuma Dec 3, 2024 •

edited

Loading