feat: add trial days remaining banner #72
Conversation
There was a problem hiding this comment.
Looks great so far! Just a couple suggestions. Also this may be an unrelated bug (in which case we could create a separate ticket), but I noticed that, in the demo, the "bubble" that contains the message seems to be missing for the response message. Is that just the way you're testing/mocking out the xpert chatgpt prompt logic?
src/data/thunks.js
Outdated
| @@ -70,6 +70,9 @@ export function getChatResponse(courseId, unitId, promptExperimentVariationKey = | |||
|
|
|||
| dispatch(setApiIsLoading(false)); | |||
| dispatch(addChatMessage(message.role, message.content, courseId, promptExperimentVariationKey)); | |||
| if (message.audit_trial_created) { | |||
There was a problem hiding this comment.
Where is audit_trial_created coming from? Are you planning to add it to the backend API? Do we have a ticket for that? If so, make a note of that here, please.
There was a problem hiding this comment.
I think it would make sense to sync up with Marcos to see how he plans to incorporate getChatResponse into this work, since his work may affect how this function is called.
As an alternative to message.audit_trial_created, it may also make sense to pass a variable to this thunk (e.g. rehydrateChatSummary) so that the frontend can control when to re-fetch this data. For example, we may know when a learner is posting a message from the audit trial CTA, depending on Marcos's approach. This approach would allow us to avoid also needing to change the backend API.
There was a problem hiding this comment.
Ah, I was experimenting with sending a new value from the backend, and forgot to put this back to its original state. I'm going to change this such that the chat-summary endpoint is called every time a message is sent so that this works while I decide on a more efficient solution.
There was a problem hiding this comment.
To follow up on this, I added this code to the backend only on my local setup to make this work:
response = self._get_next_message(request, courserun_key, course_run_id)
# Temp for testing
audit_trial_created = True
if audit_trial_created:
# TODO: Determine if this is something we should send to gate the extra calls to chat summary
response.data['audit_trial_created'] = True
return response
I'll leave this as is for now until we're all on the same page on how/when to rehydrate the chat summary related state variables in the frontend.
There was a problem hiding this comment.
Ok, reverting back to not having a audit_trial_created value in the frontend or backend to keep things simple as agreed upon. We will just call chat-summary with every message received. We can add this feature later if we have performance issues.
There was a problem hiding this comment.
NOTE: Ignore my replies above to this thread, this is what I'm doing for now:
Per our Slack discussion, I decided on an approach where we will only refresh the chat-summary after the first message is sent. This isn't "optimal", but it at least prevents us from calling the chat-summary endpoint w/ every message.
There was a problem hiding this comment.
Demo video for that feature:
Screen.Recording.2024-12-12.at.1.10.24.PM.mov
src/components/Sidebar/index.jsx
Outdated
| const oneDay = 24 * 60 * 60 * 1000; // hours*minutes*seconds*milliseconds | ||
| const daysRemaining = Math.ceil((auditTrialExpirationDate - Date.now()) / oneDay); | ||
|
|
||
| if (daysRemaining > 1) { |
There was a problem hiding this comment.
Although I'm not familiar with it, we should consider Marcos's suggestion of Intl.RelativeTimeFormat to better internationalize these strings. Although, we don't internationalize this feature anyway.
There was a problem hiding this comment.
Tried this out, seems like using this means we'd have to change the banner text for this as, at least in English, the RelativeTimeFormat spits out "in X days", with no way to change the exact wording. Wondering if that's worth doing given that we aren't intl'ing right now.
src/components/Sidebar/index.jsx
Outdated
| // Get this to work | ||
| const getDaysRemainingMessage = () => { | ||
| // if enrollment mode is NOT upgrade eligible, there's no audit trial data | ||
| if (!isUpgradeEligible) { |
There was a problem hiding this comment.
I started a thread in Slack, but I don't think isUpgradeEligible is computed the same way on the backend, which will lead to issues. I don't think that's an issue for your code changes, but I wanted to flag that. We can chat in Slack.
There was a problem hiding this comment.
Note: This is being addressed in https://2u-internal.atlassian.net/browse/COSMO-612
|
Demo of the now-operational upgrade URL: Screen.Recording.2024-12-12.at.11.59.12.AM.mov |
ilee2u
force-pushed
the
ilee2u/days-remaining-banner
branch
from
bf41221 to
27d4b79
Compare
* feat: Added trial url and days to the chat intro * feat: Updated some logic for the useCourseUpgrade() hook
Co-authored-by: Alison Langston <46360176+alangsto@users.noreply.github.com>
ilee2u
force-pushed
the
ilee2u/days-remaining-banner
branch
from
ba3aa91 to
20464ea
Compare
- temp commit as I set up the code
ilee2u
force-pushed
the
ilee2u/days-remaining-banner
branch
from
412da04 to
2877a4a
Compare
ilee2u
force-pushed
the
ilee2u/days-remaining-banner
branch
from
2877a4a to
f9809f1
Compare
- getting a strange error where hooks aren't being picked up...
https://2u-internal.atlassian.net/browse/COSMO-572
Video Demo, showing varying days remaining (fyi the admin page for audit trials is something I did locally, would need to make a PR for it):
Screen.Recording.2024-12-10.at.4.23.55.PM.2.mov