feat: Call LearningAssistantSummary endpoint #68

ilee2u · 2024-11-26T19:14:52Z

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

varshamenon4

Looks good so far! A couple of questions!

src/data/slice.js

varshamenon4 · 2024-11-27T13:58:22Z

src/components/Sidebar/index.jsx

@@ -98,7 +99,10 @@ const Sidebar = ({
          </div>
        )
      }
-      {getMessageForm()}
+      {


Is this only happening in the case where the user is an audit learner? In other words, where are we handling the logic to check if the learner is audit or verified (since the experience will be different).

Following up from a standup discussion we had, I removed the gating here as that functionality will be implemented in another ticket.

Sounds great!

ilee2u · 2024-12-03T16:10:05Z

Note for self, this should be the shape of the data returned by the GET endpoint be worked on in edx/learning-assistant#140.

message_history will return an empty list if no access
audit_trial will return an empty dict if one doesn't exist

{
    "enabled": true,
    "message_history": [
        {
            "role": "user",
            "content": "test message from user",
            "timestamp": "2024-12-02T15:04:17.495928Z"
        },
        {
            "role": "assistant",
            "content": "test message from assistant",
            "timestamp": "2024-12-02T15:04:40.084584Z"
        }
    ],
    "trial": {
        "start_date": "2024-12-02T14:59:16.148236Z",
        "expiration_date": "2024-12-16T14:59:16.148236Z"
    }
}

- also refactored tests accordingly

ilee2u · 2024-12-03T20:27:54Z

src/data/api.js

-async function fetchLearningAssistantMessageHistory(courseId) {
-  const url = new URL(`${getConfig().CHAT_RESPONSE_URL}/${courseId}/history`);
+async function fetchLearningAssistantSummary(courseId) {
+  const url = new URL(`${getConfig().CHAT_RESPONSE_URL}/${courseId}/summary`);


NOTE: This Ticket/PR has been modified such that we're now calling the "summary" endpoint that Michael is working on in his PR: edx/learning-assistant#140.

This endpoint gets all the data for enabled, message_history, and audit_trial all in one GET request.

ilee2u · 2024-12-03T20:30:37Z

src/data/thunks.js

@@ -124,13 +101,46 @@ export function updateSidebarIsOpen(isOpen) {
  };
 }

-export function getIsEnabled(courseId) {
+export function getLearningAssistantSummary(courseId) {


I replaced the getLearningAssistantMessageHistory, getIsEnabled , and getAuditTrial thunks with a singular getLearningAssistantSummary thunk.

Nit: Could you rename this to getLearningAssistantChatSummary? Marcos had some concerns about confusion between that summary and the unit summary, which we may integrate into Learning Assistant.

ilee2u · 2024-12-03T20:31:27Z

src/hooks/index.js

Let me know if I need to revert this, but I removed the message-history hook because I was able to call the summary hook from Xpert.jsx

varshamenon4

Looks good! Just a couple of nits/comments.

varshamenon4 · 2024-12-04T14:01:56Z

src/data/api.test.js


-    const fakeGet = jest.fn(async () => ({
+    const mockGet = jest.fn(async () => ({


Nice renaming!

varshamenon4 · 2024-12-04T14:03:32Z

src/data/thunks.js

+        const messageList = rawMessageList
+          .map(({ timestamp, ...msg }) => ({
+            ...msg,
+            // NOTE to self: can't store Date() objects in store: https://github.com/reduxjs/redux-toolkit/issues/456


Question: would you just store as a string then and parse when needed?

Oh wait, that's what you're doing! My bad. So you can probably remove this comment?

varshamenon4 · 2024-12-04T14:09:29Z

src/widgets/Xpert.jsx

  }, [dispatch, courseId]);

+  // NOTE: This value can be used later on if/when we pass the enrollment mode to this componentn


nit:

Suggested change

// NOTE: This value can be used later on if/when we pass the enrollment mode to this componentn

// NOTE: This value can be used later on if/when we pass the enrollment mode to this component

MichaelRoytman

We shouldn't merge this pull request until the corresponding backend changes are released, because your changes will go to stage automatically and cause errors. And there is the risk someone will deploy them to production.

MichaelRoytman · 2024-12-04T18:21:13Z

src/data/slice.js

@@ -11,6 +11,10 @@ export const initialState = {
  disclosureAcknowledged: false,
  sidebarIsOpen: false,
  isEnabled: false,
+  auditTrial: {


Shouldn't the initial state be {}? The default would be no trial, which would be represented by the empty object.

MichaelRoytman · 2024-12-04T18:22:18Z

src/data/thunks.js

@@ -124,13 +101,46 @@ export function updateSidebarIsOpen(isOpen) {
  };
 }

-export function getIsEnabled(courseId) {
+export function getLearningAssistantSummary(courseId) {


Nit: Could you rename this to getLearningAssistantChatSummary? Marcos had some concerns about confusion between that summary and the unit summary, which we may integrate into Learning Assistant.

MichaelRoytman · 2024-12-04T18:24:31Z

src/data/thunks.js

+      const auditTrial = data.audit_trial;
+
+      // If returned audit trial data is not empty
+      if (Object.keys(auditTrial).length !== 0) { // eslint-disable-line no-undef


What was the issue with no-undef here? I don't see what would be undefined here. Do we need this eslint-disable declaration?

If auditTrial data is empty, is there a reason we shouldn't call setAuditTrial. If it's empty, it would just set auditTrial to the empty object. Is there something wrong with that behavior? I think that's a reasonable behavior, as the empty object signifies the absence of a trial.

Oh that's an artifact from when I was using lodash, I'll get rid of this

MichaelRoytman · 2024-12-04T18:27:01Z

src/data/thunks.js

    } catch (error) {
+      // NOTE: When used to not show if fetching the messages failed


Can you clarify what this comment means? I'm not following.

I meant to use "we" instead of "when", but also I don't think this comment is necessary so I'm removing it.

MichaelRoytman · 2024-12-04T18:28:01Z

src/widgets/Xpert.jsx

 import ToggleXpert from '../components/ToggleXpertButton';
 import Sidebar from '../components/Sidebar';
 import { ExperimentsProvider } from '../experiments';
-import { useMessageHistory } from '../hooks';
+// import { getLearningAssistantData } from '../hooks';


Please remove the commented out lines.

MichaelRoytman · 2024-12-04T18:31:44Z

src/widgets/Xpert.jsx

  }, [dispatch, courseId]);

+  // NOTE: This value can be used later on if/when we pass the enrollment mode to this component
+  const isAuditTrialNotExpired = () => { // eslint-disable-line no-unused-vars
+    const auditTrialExpired = (Date.now() - auditTrial.expirationDate) > 0;


auditTrial.expirationDate is a string. How is it represented in Redux? If it's just the string representation of the date returned by the backend, which it looks like it is, then it's going to cause a problem. Date.now() - '2024-12-02T14:59:16.148236Z', for example, is going to return NaN.

Good point, that value is stored in Redux as a string, so I'll create a date object from the string here.

MichaelRoytman

Thanks for addressing my feedback.

I left one comment about the conditional used for determining whether a trial has expired, but since the function is not used yet, I'm not blocking merging the PR.

Also, please squash your commits before/when merging.

I'd like to wait for the backend changes to be deployed and tested before we merge this since deployment to stage will be automatic. Could we check in tomorrow about a good time to merge? Thanks.

MichaelRoytman · 2024-12-04T21:22:25Z

src/widgets/Xpert.jsx

+  const isAuditTrialNotExpired = () => { // eslint-disable-line no-unused-vars
+    const auditTrialExpirationDate = new Date(auditTrial.expirationDate);
+
+    if ((Date.now() - auditTrialExpirationDate) >= 0) {


I think that if the current time is exactly the expiration date (i.e. Date.now() - auditTrialExpirationDate === 0), then the trial should be expired, so this should be >. What do you think?

ilee2u added 2 commits November 25, 2024 13:54

feat: Call LearningAssistantAuditTrial endpoint

c702ca1

fix: correct state + add gating for message form

0e65fc3

varshamenon4 reviewed Nov 27, 2024

View reviewed changes

ilee2u added 2 commits December 2, 2024 13:30

temp: fixed slice, attempting to implment tests

f238e09

temp: debug stdout for testing

75dfdb7

ilee2u added 2 commits December 3, 2024 13:41

temp: rollback point before summary endpt refactor

4cdc186

feat: refactored to fit with summary endpt

5c3b5cf

- also refactored tests accordingly

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

ilee2u changed the title ~~feat: Call LearningAssistantAuditTrial endpoint~~ feat: Call LearningAssistantSummary endpoint Dec 3, 2024

ilee2u commented Dec 3, 2024

View reviewed changes

varshamenon4 reviewed Dec 4, 2024

View reviewed changes

chore: some nits

6a4a2aa

MichaelRoytman requested changes Dec 4, 2024

View reviewed changes

fix: convert stored datestring back to date + nits

3625ed2

MichaelRoytman approved these changes Dec 4, 2024

View reviewed changes

chore: use > not >=

9199c92

ilee2u merged commit f1f690c into main Dec 5, 2024
4 checks passed

ilee2u deleted the ilee2u/audit-trial-get branch December 5, 2024 17:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Call LearningAssistantSummary endpoint #68

feat: Call LearningAssistantSummary endpoint #68

ilee2u commented Nov 26, 2024 •

edited

Loading

varshamenon4 left a comment

varshamenon4 Nov 27, 2024

ilee2u Dec 2, 2024

varshamenon4 Dec 3, 2024

ilee2u commented Dec 3, 2024

ilee2u Dec 3, 2024

ilee2u Dec 3, 2024

MichaelRoytman Dec 4, 2024

ilee2u Dec 3, 2024

varshamenon4 left a comment

varshamenon4 Dec 4, 2024

varshamenon4 Dec 4, 2024

varshamenon4 Dec 4, 2024

varshamenon4 Dec 4, 2024

MichaelRoytman left a comment

MichaelRoytman Dec 4, 2024

MichaelRoytman Dec 4, 2024

MichaelRoytman Dec 4, 2024

MichaelRoytman Dec 4, 2024

ilee2u Dec 4, 2024

MichaelRoytman Dec 4, 2024

ilee2u Dec 4, 2024

MichaelRoytman Dec 4, 2024

MichaelRoytman Dec 4, 2024

ilee2u Dec 4, 2024

MichaelRoytman left a comment

MichaelRoytman Dec 4, 2024


		const fakeGet = jest.fn(async () => ({
		const mockGet = jest.fn(async () => ({

		}, [dispatch, courseId]);

		// NOTE: This value can be used later on if/when we pass the enrollment mode to this componentn

		} catch (error) {
		// NOTE: When used to not show if fetching the messages failed

feat: Call LearningAssistantSummary endpoint #68

feat: Call LearningAssistantSummary endpoint #68

Conversation

ilee2u commented Nov 26, 2024 • edited Loading

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

ilee2u commented Dec 3, 2024

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

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

MichaelRoytman 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

MichaelRoytman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilee2u commented Nov 26, 2024 •

edited

Loading