feat: add audit gate to CourseChatView #134

ilee2u · 2024-11-18T18:30:18Z

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

Add functionality get or create an audit trial if the user is an audit learner.

If the user’s course enrollment mode (CourseMode) is audit:
- Get (or create if none exists*) the user’s LearningAssistantAuditTrial (using python API logic created here: https://2u-internal.atlassian.net/browse/COSMO-567 )
- Get remaining days remaining in their Xpert LA audit trial, using endpoint created above
- If remaining days is 0, user should not have access and should return 403
When an audit learner first sends a message in Xpert LA, we’ll need to create a LearningAssistantAuditTrial to save the start date for the audit trial

ilee2u · 2024-11-18T18:33:02Z

Reference for modes for audit & verified learners, per the course_modes app in edx-platform/common/djangoapps:

AUDIT_MODES = [
AUDIT,
HONOR,
UNPAID_EXECUTIVE_EDUCATION,
UNPAID_BOOTCAMP
]

VERIFIED_MODES = [
VERIFIED,
PROFESSIONAL,
MASTERS,
EXECUTIVE_EDUCATION,
PAID_EXECUTIVE_EDUCATION,
PAID_BOOTCAMP
]

github-actions · 2024-11-18T19:36:29Z

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
learning_assistant
api.py
constants.py
views.py					111-112
Project Total

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

varshamenon4

Looks good so far! Added some comments.

varshamenon4 · 2024-11-18T19:42:40Z

learning_assistant/api.py

+    )
+
+    # If the trial was just created, then it definitely isn't expired, so return False
+    if created:


Is it possible that a Trial could be created with 0 days left? For example, if the upgrade deadline has already passed?

If the upgrade deadline has passed, should we prevent them from creating a trial? I think that's what you're suggesting below, but just wanted to echo that here.

Also, in theory, this would depend on the value of AUDIT_TRIAL_MAX_DAYS. That value could be 0, in which case it's not correct for this to return True. Because that value is configurable, I don't know that we can say that always returning True in this case is the right thing to do. I think it would be better to rely on the code that uses the value of AUDIT_TRIAL_MAX_DAYS. It's a simple computation, so I don't think we lose much by using it in all cases.

varshamenon4 · 2024-11-18T19:45:25Z

learning_assistant/views.py

+            # NOTE: Will there ever be a case where the user has a course mod that's
+            # in neither VERIFIED_MODES nor AUDIT_MODES that we need to worry about?
+            enrollment_mode not in CourseMode.VERIFIED_MODES
+            and enrollment_mode in CourseMode.AUDIT_MODES


I'm not positive, but the logic here seems to match: https://github.com/openedx/frontend-app-learning/blob/6813872dd3b6737a1a6e13764feb9fa7596cbe3d/src/courseware/course/chat/Chat.jsx#L43. So I think that this should cover it?

If we want to display the learning assistant to audit learners, doesn't that mean we'll need to change that logic in the frontend as well?

The list of VERIFIED_MODES in the learning MFE is slightly different than the list in the LMS.

I think what is important is that a learner can upgrade from any of the audit modes, right? We'd only want to shower learners than can upgrade the trial. I'm not entirely sure what that exact list is. We may need to figure that out.

It seems like the VERIFIED_MODES in the learning MFE includes all the modes that are not part of the AUDIT_MODES in the LMS, so I'm wondering we should do one of two things:

Only check is the user's enrollment mode is in AUDIT_MODES,

or check that the mode is in frontend-app-learning's version of VERIFIED_MODES.

Kind of leaning towards the first option, but let me know your thoughts.

I tried to find where we allow learners to upgrade and what modes we allow a learner to be in to upgrade. I can't find a singular place. There is the is_mode_upgradeable method in the LMS, but it's not that widely used.

There is serialize_upgrade_info, which is used by several of the endpoints the learning MFE uses. can_show_verified_upgrade uses is_mode_upsellable, which refers to CourseMode.UPSELL_TO_VERIFIED_MODES, as does can_show_verified_upgrade later in teh function. CourseMode.UPSELL_TO_VERIFIED_MODES
is [HONOR, AUDIT].

This kind of makes sense to me. I don't know if we enroll can enroll learners in ExecEd and Bootcamps via the usual upgrade process, so it's possible learners in the UNPAID_EXECUTIVE_EDUCATION, and UNPAID_BOOTCAMP modes do not see any upgrade messaging.

If that's the case, we may have three states.

learners in verified modes and staff - full access

learners in the audit and honor modes - ability to start trial

learners in other modes - no access

I'm harping on this because I wouldn't want to advertise a feature that a learner cannot access via upgrade. I think this is worth a conversation at tomorrow's stand up. What do you think?

P.S. I would also want to confirm that we want all "verified" modes to have access to Xpert. Do we want Masters users to have access to Xpert, for example?

I did some digging through the code you linked. Long story short, this seems like what we need (this is a slight modification of what you posted):

Any learners in verified modes (per the list in the frontend) or staff will have full access.

Any learners with course modes in CourseMode.UPSELL_TO_VERIFIED_MODES will start or continue their trial (unless it's expired, in which case return 403).

Learners in all other course modes (captured in an else statement) will get returned 403.

I've just committed changes reflecting this.

varshamenon4 · 2024-11-18T19:47:04Z

learning_assistant/api.py

+    if created:
+        return False
+
+    # If the user's trial is expired, return True. Else, return False


Related to above, but we may need to include logic for checking if the course upgrade deadline has passed (unless that's somewhere else, in which case, disregard!)

MichaelRoytman · 2024-11-18T19:56:28Z

learning_assistant/api.py

+
+    # If the user's trial is expired, return True. Else, return False
+    DAYS_SINCE_TRIAL_START_DATE = datetime.now() - audit_trial.start_date
+    if DAYS_SINCE_TRIAL_START_DATE > timedelta(days=AUDIT_TRIAL_MAX_DAYS):


I think this should be >=, right?

MichaelRoytman · 2024-11-18T19:57:12Z

learning_assistant/api.py

+    )
+
+    # If the trial was just created, then it definitely isn't expired, so return False
+    if created:


If the upgrade deadline has passed, should we prevent them from creating a trial? I think that's what you're suggesting below, but just wanted to echo that here.

MichaelRoytman · 2024-11-18T19:58:55Z

learning_assistant/api.py

+    )
+
+    # If the trial was just created, then it definitely isn't expired, so return False
+    if created:


Also, in theory, this would depend on the value of AUDIT_TRIAL_MAX_DAYS. That value could be 0, in which case it's not correct for this to return True. Because that value is configurable, I don't know that we can say that always returning True in this case is the right thing to do. I think it would be better to rely on the code that uses the value of AUDIT_TRIAL_MAX_DAYS. It's a simple computation, so I don't think we lose much by using it in all cases.

MichaelRoytman · 2024-11-18T20:14:57Z

learning_assistant/views.py

+            # NOTE: Will there ever be a case where the user has a course mod that's
+            # in neither VERIFIED_MODES nor AUDIT_MODES that we need to worry about?
+            enrollment_mode not in CourseMode.VERIFIED_MODES
+            and enrollment_mode in CourseMode.AUDIT_MODES


The list of VERIFIED_MODES in the learning MFE is slightly different than the list in the LMS.

I think what is important is that a learner can upgrade from any of the audit modes, right? We'd only want to shower learners than can upgrade the trial. I'm not entirely sure what that exact list is. We may need to figure that out.

schenedx

See my inline comments

schenedx · 2024-11-19T17:51:36Z

learning_assistant/api.py

@@ -224,3 +229,26 @@ def get_message_history(courserun_key, user, message_count):
    message_history = list(LearningAssistantMessage.objects.filter(
        course_id=courserun_key, user=user).order_by('-created')[:message_count])[::-1]
    return message_history
+
+
+def check_if_audit_trial_is_expired(user, upgrade_deadline):


#NIT the function name is verbose. What about audit_trial_is_expired?

schenedx · 2024-11-19T17:52:16Z

learning_assistant/api.py

+
+    # If the user's trial is past its expiry date, return "True" for expired. Else, return False
+    DAYS_SINCE_TRIAL_START_DATE = datetime.now() - audit_trial.start_date
+    if DAYS_SINCE_TRIAL_START_DATE >= timedelta(days=AUDIT_TRIAL_MAX_DAYS):


What about just do return DAYS_SINCE_TRIAL_START_DATE >= timedelta(days=AUDIT_TRIAL_MAX_DAYS)?

schenedx · 2024-11-19T17:53:47Z

learning_assistant/views.py

-                status=http_status.HTTP_403_FORBIDDEN,
-                data={'detail': 'Must be staff or have valid enrollment.'}
-            )
+            # TODO: Add logic to make sure upgrade deadline has not passed.


The TODO comment should not be needed?

alangsto

One question about test coverage, but this looks good!

alangsto · 2024-11-21T14:39:06Z

learning_assistant/views.py

+                    data={'detail': 'The audit trial for this user has expired.'}
+                )
+            else:
+                return self._get_next_message(request, courserun_key, course_run_id)


This seems like a somewhat important case to cover, could a test be added to ensure that this code is still called?

schenedx

👍

feat: add audit gate to CourseChatView

95206e7

test: add tests

2466783

varshamenon4 reviewed Nov 18, 2024

View reviewed changes

MichaelRoytman reviewed Nov 18, 2024

View reviewed changes

ilee2u added 2 commits November 18, 2024 15:21

feat: add upgrade deadline gate

046d13f

fix: remove "created" test for trial expiration

04e0710

schenedx reviewed Nov 19, 2024

View reviewed changes

ilee2u added 3 commits November 20, 2024 15:22

chore: nits

0b01a7b

fix: distinguish between course modes correctly

4de25d8

chore: lint

07e446d

alangsto reviewed Nov 21, 2024

View reviewed changes

fix: correct test to cover unexpired audit trial

09d0c55

schenedx approved these changes Nov 21, 2024

View reviewed changes

ilee2u merged commit ada1330 into main Nov 21, 2024
4 checks passed

ilee2u deleted the ilee2u/add-audit-gate-to-CourseChatView branch November 21, 2024 18:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add audit gate to CourseChatView #134

feat: add audit gate to CourseChatView #134

ilee2u commented Nov 18, 2024 •

edited

Loading

ilee2u commented Nov 18, 2024

github-actions bot commented Nov 18, 2024 •

edited

Loading

varshamenon4 left a comment

varshamenon4 Nov 18, 2024

MichaelRoytman Nov 18, 2024

MichaelRoytman Nov 18, 2024

varshamenon4 Nov 18, 2024

ilee2u Nov 18, 2024

MichaelRoytman Nov 18, 2024

ilee2u Nov 18, 2024

MichaelRoytman Nov 18, 2024

ilee2u Nov 20, 2024

varshamenon4 Nov 18, 2024

MichaelRoytman Nov 18, 2024

MichaelRoytman Nov 18, 2024

MichaelRoytman Nov 18, 2024

MichaelRoytman Nov 18, 2024

schenedx left a comment

schenedx Nov 19, 2024

schenedx Nov 19, 2024

schenedx Nov 19, 2024

alangsto left a comment

alangsto Nov 21, 2024

schenedx left a comment

feat: add audit gate to CourseChatView #134

feat: add audit gate to CourseChatView #134

Conversation

ilee2u commented Nov 18, 2024 • edited Loading

ilee2u commented Nov 18, 2024

github-actions bot commented Nov 18, 2024 • edited Loading

Coverage report

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

schenedx 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

alangsto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schenedx left a comment

Choose a reason for hiding this comment

ilee2u commented Nov 18, 2024 •

edited

Loading

github-actions bot commented Nov 18, 2024 •

edited

Loading