feat: add trial days remaining banner #72

ilee2u · 2024-12-10T15:59:13Z

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

varshamenon4

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/components/Sidebar/index.jsx

src/data/thunks.js

MichaelRoytman · 2024-12-10T21:58:42Z

src/data/thunks.js

@@ -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) {


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.

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.

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.

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.

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.

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.

Demo video for that feature:

Screen.Recording.2024-12-12.at.1.10.24.PM.mov

MichaelRoytman · 2024-12-10T22:05:38Z

src/components/Sidebar/index.jsx

+      const oneDay = 24 * 60 * 60 * 1000; // hours*minutes*seconds*milliseconds
+      const daysRemaining = Math.ceil((auditTrialExpirationDate - Date.now()) / oneDay);
+
+      if (daysRemaining > 1) {


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.

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

MichaelRoytman · 2024-12-10T22:15:16Z

src/components/Sidebar/index.jsx

+  // Get this to work
+  const getDaysRemainingMessage = () => {
+    // if enrollment mode is NOT upgrade eligible, there's no audit trial data
+    if (!isUpgradeEligible) {


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.

Note: This is being addressed in https://2u-internal.atlassian.net/browse/COSMO-612

src/widgets/Xpert.jsx

src/components/Sidebar/Sidebar.scss

ilee2u · 2024-12-12T17:00:31Z

Demo of the now-operational upgrade URL:

Screen.Recording.2024-12-12.at.11.59.12.AM.mov

src/components/Sidebar/index.jsx

src/data/thunks.js

ilee2u · 2024-12-16T20:13:32Z

Demo of how this looks after rebasing onto Marcos' PR with the Disclosure: #73 (review)

Screen.Recording.2024-12-16.at.3.12.06.PM.mov

MichaelRoytman

I won't have time to review this today, but I wanted to leave this comment. If the PR still needs a review tomorrow, I'll take a look then.

MichaelRoytman · 2024-12-12T18:55:18Z

src/data/thunks.js

@@ -68,6 +110,12 @@ export function getChatResponse(courseId, unitId, promptExperimentVariationKey =
      const customQueryParams = promptExperimentVariationKey ? { responseVariation: promptExperimentVariationKey } : {};
      const message = await fetchChatResponse(courseId, messageList, unitId, customQueryParams);

+      // Refresh chat summary only on the first message so we can tell if the user has initiated an audit trial
+      // NOTE: This is a bit of a hacky solution that may be refined later
+      if (messageList.length === 1) {


I think this is fine to have as a POC, but I don't think we should merge this. This is going to make an API call for everyone after they send their first message, which is a lot of extraneous API calls. At a minimum, we should only make this call when the learner is eligible for an audit trial.

Noted, added a condition to only show this to upgrade eligible learners as well.

alangsto

A few nits, but otherwise this looks good!

src/hooks/use-course-upgrade.js

src/setupTest.js

alangsto · 2024-12-16T21:17:07Z

src/utils/optimizelyExperiment.js

@@ -3,7 +3,7 @@ import { getOptimizely } from '../data/optimizely';
 const trackChatBotMessageOptimizely = (userId, userAttributes = {}) => {
  const optimizelyInstance = getOptimizely();

-  if (!optimizelyInstance) { return; }
+  if (!optimizelyInstance || Object.keys(optimizelyInstance).length === 0) { return; }


Was this line causing issues?

This line was causing issues on my local setup, not sure if it was due to something missing on my end. I can put this back if that's preferred.

I think you probably need to add { moduleName: '@optimizely/react-sdk', dir: '../src/frontend-plugin-advertisements/src/mocks/optimizely', dist: '.' }, to your frontend-app-learning module.config.js file

That line is in my config file so I'm not sure what's up there...

module.exports = { /* Modules you want to use from local source code. Adding a module here means that when this app runs its build, it'll resolve the source from peer directories of this app. moduleName: the name you use to import code from the module. dir: The relative path to the module's source code. dist: The sub-directory of the source code where it puts its build artifact. Often "dist". */ localModules: [ { moduleName: '@edx/frontend-lib-special-exams', dir: '../src/frontend-lib-special-exams', dist: 'src' }, { moduleName: '@edx/frontend-plugin-advertisements', dir: '../src/frontend-plugin-advertisements', dist: 'src' }, { moduleName: '@optimizely/react-sdk', dir: '../src/frontend-plugin-advertisements/src/mocks/optimizely', dist: '.' }, { moduleName: '@edx/frontend-lib-learning-assistant', dir: '../src/frontend-lib-learning-assistant', dist: 'src' }, ], };

When I debugged this I found that the optomizelyInstance had a value of {} so that's why I inserted this condition.

What's the issue you're running into? I was not running into issues locally with optimizely so just trying to help narrow down what could be going wrong

varshamenon4

Just reviewed the sidebar, but let me know if other areas need review! Happy to take another look. Just had a few questions/nits so far, but looking good!

src/components/Sidebar/index.jsx

src/components/Sidebar/index.test.jsx

src/components/Sidebar/index.jsx

rijuma

LGTM, but please before merging check my nit comment on the tracking for the upgrade link and remove the "Your trial has expired" message since that would never be shown, it will render the paywall instead. (as suggested by Alison and Varsha)

varshamenon4

lgtm! I think there are a couple of outstanding comments to be addressed but otherwise looks great.

- temp commit as I set up the code

- getting a strange error where hooks aren't being picked up...

varshamenon4 reviewed Dec 10, 2024

View reviewed changes

src/components/Sidebar/index.jsx Outdated Show resolved Hide resolved

src/components/Sidebar/index.jsx Outdated Show resolved Hide resolved

MichaelRoytman reviewed Dec 10, 2024

View reviewed changes

ilee2u force-pushed the ilee2u/days-remaining-banner branch from bf41221 to 27d4b79 Compare December 12, 2024 17:39

schenedx reviewed Dec 12, 2024

View reviewed changes

src/components/Sidebar/index.jsx Outdated Show resolved Hide resolved

schenedx reviewed Dec 12, 2024

View reviewed changes

src/components/Sidebar/index.jsx Outdated Show resolved Hide resolved

schenedx reviewed Dec 12, 2024

View reviewed changes

src/components/Sidebar/index.jsx Outdated Show resolved Hide resolved

schenedx reviewed Dec 12, 2024

View reviewed changes

src/data/thunks.js Outdated Show resolved Hide resolved

ilee2u mentioned this pull request Dec 13, 2024

feat: Updated Disclaimer screen with trial option #73

Merged

ilee2u marked this pull request as ready for review December 13, 2024 18:40

ilee2u force-pushed the ilee2u/days-remaining-banner branch 4 times, most recently from 22dfc95 to d366bc0 Compare December 16, 2024 20:11

MichaelRoytman reviewed Dec 16, 2024

View reviewed changes

ilee2u force-pushed the ilee2u/days-remaining-banner branch from 045ba42 to 27104c2 Compare December 16, 2024 21:05

alangsto reviewed Dec 16, 2024

View reviewed changes

varshamenon4 reviewed Dec 16, 2024

View reviewed changes

src/components/Sidebar/index.jsx Outdated Show resolved Hide resolved

src/components/Sidebar/index.jsx Outdated Show resolved Hide resolved

src/components/Sidebar/index.test.jsx Outdated Show resolved Hide resolved

src/components/Sidebar/index.test.jsx Show resolved Hide resolved

ilee2u force-pushed the ilee2u/days-remaining-banner branch from 27104c2 to d0a8ae5 Compare December 16, 2024 21:47

rijuma reviewed Dec 16, 2024

View reviewed changes

src/components/Sidebar/index.jsx Outdated Show resolved Hide resolved

rijuma approved these changes Dec 16, 2024

View reviewed changes

varshamenon4 approved these changes Dec 17, 2024

View reviewed changes

ilee2u added 7 commits December 17, 2024 13:30

feat: add trial days remaining banner

794cab2

- temp commit as I set up the code

temp: working version w/ debug logs

717a0a4

feat: better functionality for days remaining

4d196f2

fix: set auditTrialLengthDays + nits

43f2c78

feat: add upgrade url to days remaining banner

5fa9f47

feat: simplify refresh chat summary call

078db13

temp: rollback point for tests

9890f24

ilee2u added 17 commits December 17, 2024 13:30

feat: only refresh chat-summary on 1st msg

77231f7

temp: attempting to mock useModel + factories

0947732

temp: some nits

11c0562

fix: re-syncing: mock useModel + thunk work

8ef838f

temp: progressing on using new audit trial hooks

13b9e27

temp: removing useModel mock

2a3d1d7

temp: trying to get hook to work

f3496eb

chore: lint

41bd3cd

temp: attempting to test Sidebar

b49f15f

- getting a strange error where hooks aren't being picked up...

chore: lint

f1ccd04

chore: trying to get around this weird lint error

9a48fd8

test: completed working unit tests

a8fa79d

fix: add upgrade eligibility gate for refresh

48e16b8

chore: lint

4d9f530

chore: more nits :D

3d2d0c1

chore: make intl name more descriptive

7c97e5f

fix: remove paywall placeholder + rebase main pt 2

1ac0c54

ilee2u force-pushed the ilee2u/days-remaining-banner branch from e7b9f01 to 1ac0c54 Compare December 17, 2024 18:45

feat: track event on upgrade link click

0352c85

ilee2u force-pushed the ilee2u/days-remaining-banner branch from 5547942 to 0352c85 Compare December 17, 2024 19:22

ilee2u merged commit daaa1cb into main Dec 17, 2024
4 checks passed

ilee2u deleted the ilee2u/days-remaining-banner branch December 17, 2024 19:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add trial days remaining banner #72

feat: add trial days remaining banner #72

ilee2u commented Dec 10, 2024 •

edited

Loading

varshamenon4 left a comment

MichaelRoytman Dec 10, 2024

MichaelRoytman Dec 10, 2024

ilee2u Dec 11, 2024

ilee2u Dec 12, 2024

ilee2u Dec 12, 2024 •

edited

Loading

ilee2u Dec 12, 2024 •

edited

Loading

ilee2u Dec 12, 2024

MichaelRoytman Dec 10, 2024

ilee2u Dec 12, 2024

MichaelRoytman Dec 10, 2024

ilee2u Dec 12, 2024

ilee2u commented Dec 12, 2024

ilee2u commented Dec 16, 2024

MichaelRoytman left a comment •

edited

Loading

MichaelRoytman Dec 12, 2024

ilee2u Dec 16, 2024

alangsto left a comment

alangsto Dec 16, 2024

ilee2u Dec 16, 2024

alangsto Dec 16, 2024

ilee2u Dec 17, 2024

alangsto Dec 17, 2024

varshamenon4 left a comment

rijuma left a comment •

edited

Loading

varshamenon4 left a comment

feat: add trial days remaining banner #72

feat: add trial days remaining banner #72

Conversation

ilee2u commented Dec 10, 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

Choose a reason for hiding this comment

ilee2u Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

ilee2u Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

NOTE: Ignore my replies above to this thread, this is what I'm doing for now:

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

ilee2u commented Dec 12, 2024

ilee2u commented Dec 16, 2024

MichaelRoytman left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this 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

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

rijuma left a comment • edited Loading

Choose a reason for hiding this comment

varshamenon4 left a comment

Choose a reason for hiding this comment

ilee2u commented Dec 10, 2024 •

edited

Loading

ilee2u Dec 12, 2024 •

edited

Loading

ilee2u Dec 12, 2024 •

edited

Loading

MichaelRoytman left a comment •

edited

Loading

rijuma left a comment •

edited

Loading