Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: Call LearningAssistantSummary endpoint #68

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

feat: Call LearningAssistantSummary endpoint #68

wants to merge 6 commits into from
+287 −252

Conversation

ilee2u
Copy link
Member

@ilee2u ilee2u commented Nov 26, 2024 •
edited
Loading

varshamenon4
varshamenon4 reviewed Nov 27, 2024
View reviewed changes
Copy link
Member

@varshamenon4 varshamenon4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far! A couple of questions!

src/data/slice.js Outdated
@@ -11,6 +11,10 @@ export const initialState = {
disclosureAcknowledged: false,
sidebarIsOpen: false,
isEnabled: false,
auditTrial: {
startDate: 0,
expirationDate: null, // TODO: what do we use for a datetime value here?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify, are you looking to set the default value? Could it be the same (either 0 or null?)?

src/components/Sidebar/index.jsx Outdated
@@ -98,7 +99,10 @@ const Sidebar = ({
</div>
)
}
{getMessageForm()}
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ilee2u
Copy link
Member Author

ilee2u commented Dec 3, 2024

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"
    }
}

@ilee2u ilee2u marked this pull request as ready for review December 3, 2024 20:23
@ilee2u ilee2u changed the title feat: Call LearningAssistantAuditTrial endpoint feat: Call LearningAssistantSummary endpoint Dec 3, 2024
ilee2u
ilee2u commented Dec 3, 2024
View reviewed changes
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`);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/data/thunks.js
@@ -124,13 +101,46 @@ export function updateSidebarIsOpen(isOpen) {
};
}

export function getIsEnabled(courseId) {
export function getLearningAssistantSummary(courseId) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/hooks/index.js
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@varshamenon4 varshamenon4 varshamenon4 left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ilee2u @varshamenon4