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

fix: use course enrollment instead of course mode to calculate audit trial is expired #146

Merged
merged 1 commit into from
Jan 6, 2025
Merged

fix: use course enrollment instead of course mode to calculate audit trial is expired #146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions CHANGELOG.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,19 +13,23 @@ Change Log

Unreleased
**********

4.6.3 - 2025-01-06
******************
* Uses CourseEnrollment instead of CourseMode to get the upgrade deadline required to calculate if a learner's audit trial is expired.
* Updated setup docs

4.6.2 - 2025-12-18
4.6.2 - 2024-12-18
******************
* Fixed the params for expiration_date in the admin table for audit trial.
* Add ENABLE_XPERT_AUDIT instructions.

4.6.1 - 2025-12-17
4.6.1 - 2024-12-17
******************
* Added an admin table for the LearningAssistantAuditTrial model. This table includes an expiration_date valued that is
calculated based on the start_date.

4.6.0 - 2025-12-10
4.6.0 - 2024-12-10
******************
* Add an audit_trial_length_days attribute to the response returned by the ChatSummaryView, representing the
number of days in an audit trial as currently configured. It does not necessarily represent the number of days in the
Expand Down
2 changes: 1 addition & 1 deletion learning_assistant/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,6 @@
Plugin for a learning assistant backend, intended for use within edx-platform.
"""

__version__ = '4.6.2'
__version__ = '4.6.3'

default_app_config = 'learning_assistant.apps.LearningAssistantConfig' # pylint: disable=invalid-name
29 changes: 15 additions & 14 deletions learning_assistant/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"""
import datetime
import logging
from datetime import datetime, timedelta
from datetime import datetime, timedelta, timezone

from django.conf import settings
from django.contrib.auth import get_user_model
Expand All @@ -12,11 +12,6 @@
from jinja2 import BaseLoader, Environment
from opaque_keys import InvalidKeyError

try:
from common.djangoapps.course_modes.models import CourseMode
except ImportError:
CourseMode = None

from learning_assistant.constants import ACCEPTED_CATEGORY_TYPES, CATEGORY_TYPE_MAP
from learning_assistant.data import LearningAssistantAuditTrialData, LearningAssistantCourseEnabledData
from learning_assistant.models import (
Expand Down Expand Up @@ -307,19 +302,25 @@ def get_or_create_audit_trial(user):
)


def audit_trial_is_expired(audit_trial_data, courserun_key):
"""
Given a user (User), get or create the corresponding LearningAssistantAuditTrial trial object.
def audit_trial_is_expired(enrollment, audit_trial_data):
"""
course_mode = CourseMode.objects.get(course=courserun_key)
Given an enrollment and audit_trial_data, return whether the audit trial is expired as a boolean.

Arguments:
* enrollment (CourseEnrollment): the user course enrollment
* audit_trial_data (LearningAssistantAuditTrialData): the data related to the audit trial

upgrade_deadline = course_mode.expiration_datetime()
Returns:
* audit_trial_is_expired (boolean): whether the audit trial is expired
"""
upgrade_deadline = enrollment.upgrade_deadline
Copy link
Member

Choose a reason for hiding this comment

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

This is great! 🎉

today = datetime.now(tz=timezone.utc)

# If the upgrade deadline has passed, return True for expired. Upgrade deadline is an optional attribute of a
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: I've changed the way that this is calculated to avoid issues with timezone naive/aware timezones. This now converts all of the datetimes into dates so that we're just calculating the difference in whole days (I believe this is more consistent with the calculations of days remaining in the frontend as well).

Copy link
Member

Choose a reason for hiding this comment

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

I don't see where in the JavaScript we're treating an expiration date as a whole day. I'm looking at the useCourseUpgrade hook. The Date represents the date as milliseconds since the epoch, so the comparison is done on the order of milliseconds and not days. I can see that we're computing the number of days left by dividing by the number of milliseconds in a day and taking the ceiling, but the actual computation of the number of time left is on the order of milliseconds.

Can we resolve the timezone issues by constructing the datetime the same way (i.e. timezone-aware or timezone-naive datetimes) between the two places that have issues - sounds like the code and the tests?

Can you help me understand why we're calculating this by date? My concern is that we're using time data in the calculation on the frontend but not on the backend. Wouldn't a learner's audit trials be considered expired at midnight on the last day of the trial?

The impact of this is that the frontend determines that the trial is not-expired, but the new Python code considers it expired. This means that the chat is going to show for the learner on the frontend, but the backend is going to return a 403 when the learner sends a message.

Here's an example I worked through.

Python
This shows that removing the time component results in two different values between the old and new code for calculating if the upgrade deadline has passed and if the expiration date has passed.

// Set up.
>>> from datetime import date, datetime, timedelta
>>> today_with_time = today_with_time = datetime(2024, 12, 19, 15, 29, 37, 901850)
>>> today_with_time
datetime.datetime(2024, 12, 19, 15, 29, 37, 901850)
>>> today_without_time = date(2024, 12, 19)
>>> upgrade_deadline = datetime(2024, 12, 19, 16, 29, 37, 901850) // 1 hour from now

// Old way of computing whether a trial is expired, with time.
// Note that the return value is False.
>>> days_until_upgrade_with_time = today_with_time - upgrade_deadline
>>> days_until_upgrade_with_time
datetime.timedelta(days=-1, seconds=82800)
>>> past_deadline_with_time = days_until_upgrade_with_time >= timedelta(days=0)
>>> past_deadline_with_time
False

// New way of computing whether a trial is expired, without time.
// Note that the return value is True.
>>> days_until_upgrade_without_time = today_without_time - upgrade_deadline.date()
>>> past_deadline_without_time = days_until_upgrade_without_time.days >= 0
>>> past_deadline_without_time
True

// I know the variable name is upgrade_deadline here, but just imagine it's audit_trial_data.expiration_date instead.
expiration_date = upgrade_deadline
>>> expiration_date <= today_with_time
False
>>> expiration_date.date() <= today_without_time
True

JavaScript

// Set up.
let auditTrialExpirationDate = new Date('2024-12-19T16:29:37.901850');
const millisecondsInOneDay = 24 * 60 * 60 * 1000;

auditTrialExpirationDate
Thu Dec 19 2024 16:29:37 GMT-0500 (Eastern Standard Time)

// To simulate the expiration date being one hour from now, use a variable instead of Date.now().
now = new Date('2024-12-19T15:29:37.901850');

now
Thu Dec 19 2024 15:29:37 GMT-0500 (Eastern Standard Time)

// The number of audit trial days remaining is 1. I substituted Date.now() with now.
let auditTrialDaysRemaining = Math.ceil((auditTrialExpirationDate - now) / millisecondsInOneDay);

auditTrialDaysRemaining
1

// The audit trial is not yet expired.
let auditTrialExpired = auditTrialDaysRemaining < 0

auditTrialExpired
false

Copy link
Member

@rijuma rijuma Jan 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think forcing the timezone as tz=None might cause issues, since the dates should be treated in the same timezone.

In the case of the frontend, the date received is in UTC but when a date is created like in the hook as new Date(auditTrial.expirationDate), the resulting date is converted to the local timezone, that's why it can be compared with Date.now() since in the frontend, both dates are in the same timezone.

I'm no expert on Python, but I think it should be similar, so parsing a UTC time should (IMHO) be converted to the server timezone, so it should be able to be compared with datetime.now().

I don't think truncating the dates would be a viable solution.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call! I've updated to have the timezone set when creating datetime.now() to have it be timezone aware so it can be compared with the other times.

Copy link
Member

Choose a reason for hiding this comment

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

Do we know anything about the upgrade_deadline in terms of the timezone? Is it always in UTC? Or is it in the learner's local server time? Is it an aware datetime? I assume so given the need to keep today aware.

I'm concerned about the use of timezone.utc here, because we may be comparing against another timezone in upgrade_deadline, and Python isn't going to automatically convert today to the learner's local server time or to the same timezone that upgrade_deadline is in.

Copy link
Member Author

@varshamenon4 varshamenon4 Jan 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I can do some more research on this but the reason I did this was because my understanding is that when comparing two aware datetimes, the timezone is considered. So you can compare two aware datetimes and the timezone is taken into account. See here: https://stackabuse.com/comparing-datetimes-in-python-with-and-without-timezones/.

If it's better I could convert all the timezones to UTC but I didn't think this was necessary.

Copy link
Member

Choose a reason for hiding this comment

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

Didn't know that. Good call.

Copy link
Member

Choose a reason for hiding this comment

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

I think you're right that Python compares them correctly under-the-hood. I just couldn't find that state explicitly in the documentation. Looks good to me!

Copy link
Member Author

Choose a reason for hiding this comment

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

So yes, I confirmed by testing this:

@freeze_time('2024-01-01 00:00:01')
    def test_timezone(self):
        today_utc = datetime.now(tz=timezone.utc)
        london = datetime.now(zoneinfo.ZoneInfo("Europe/London"))
        nyc = datetime.now(zoneinfo.ZoneInfo("America/New_York"))
        logging.info(today_utc)
        logging.info(london)
        logging.info(nyc)

        self.assertEqual(today_utc, london)
        self.assertNotEqual(today_utc, nyc)
        self.assertNotEqual(london, nyc)

And these are the outputs of each:

FakeDatetime(2024, 1, 1, 0, 0, 1, tzinfo=datetime.timezone.utc)
FakeDatetime(2024, 1, 1, 0, 0, 1, tzinfo=zoneinfo.ZoneInfo(key='Europe/London'))
FakeDatetime(2023, 12, 31, 19, 0, 1, tzinfo=zoneinfo.ZoneInfo(key='America/New_York'))

So it looks like the comparison of datetimes takes the timezones into account!

# CourseMode, so if it's None, then do not return True.
days_until_upgrade_deadline = datetime.now() - upgrade_deadline if upgrade_deadline else None
# CourseEnrollment, so if it's None, then do not return True.
days_until_upgrade_deadline = today - upgrade_deadline if upgrade_deadline else None
if days_until_upgrade_deadline is not None and days_until_upgrade_deadline >= timedelta(days=0):
return True

# If the user's trial is past its expiry date, return True for expired. Else, return False.
return audit_trial_data is None or audit_trial_data.expiration_date <= datetime.now()
return audit_trial_data is None or audit_trial_data.expiration_date <= today
4 changes: 2 additions & 2 deletions learning_assistant/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ def post(self, request, course_run_id):
# next message. Otherwise, return 403
elif enrollment_mode in CourseMode.UPSELL_TO_VERIFIED_MODES: # AUDIT, HONOR
audit_trial = get_or_create_audit_trial(request.user)
is_user_audit_trial_expired = audit_trial_is_expired(audit_trial, courserun_key)
is_user_audit_trial_expired = audit_trial_is_expired(enrollment_object, audit_trial)
if is_user_audit_trial_expired:
return Response(
status=http_status.HTTP_403_FORBIDDEN,
Expand Down Expand Up @@ -383,7 +383,7 @@ def get(self, request, course_run_id):
has_trial_access = (
enrollment_mode in valid_trial_access_modes
and audit_trial
and not audit_trial_is_expired(audit_trial, courserun_key)
and not audit_trial_is_expired(enrollment_object, audit_trial)
)

if (
Expand Down
79 changes: 38 additions & 41 deletions tests/test_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
Test cases for the learning-assistant api module.
"""
import itertools
from datetime import datetime, timedelta
from datetime import datetime, timedelta, timezone
from unittest.mock import MagicMock, patch

import ddt
Expand Down Expand Up @@ -491,6 +491,7 @@ class GetAuditTrialExpirationDateTests(TestCase):
"""
Test suite for get_audit_trial_expiration_date.
"""

@ddt.data(
(datetime(2024, 1, 1, 0, 0, 0), datetime(2024, 1, 2, 0, 0, 0), 1),
(datetime(2024, 1, 18, 0, 0, 0), datetime(2024, 1, 19, 0, 0, 0), 1),
Expand All @@ -516,6 +517,7 @@ class GetAuditTrialTests(TestCase):
"""
Test suite for get_audit_trial.
"""

@freeze_time('2024-01-01')
def setUp(self):
super().setUp()
Expand Down Expand Up @@ -548,6 +550,7 @@ class GetOrCreateAuditTrialTests(TestCase):
"""
Test suite for get_or_create_audit_trial.
"""

def setUp(self):
super().setUp()
self.user = User(username='tester', email='[email protected]')
Expand Down Expand Up @@ -596,86 +599,80 @@ def setUp(self):
self.user = User(username='tester', email='[email protected]')
self.user.save()

self.upgrade_deadline = datetime.now() + timedelta(days=1) # 1 day from now

@freeze_time('2024-01-01')
@patch('learning_assistant.api.CourseMode')
def test_upgrade_deadline_expired(self, mock_course_mode):

mock_mode = MagicMock()
mock_mode.expiration_datetime.return_value = datetime.now() - timedelta(days=1) # yesterday
mock_course_mode.objects.get.return_value = mock_mode
@freeze_time('2024-01-01 00:00:01 UTC')
def test_upgrade_deadline_expired(self):
today = datetime.now(tz=timezone.utc)
mock_enrollment = MagicMock()
mock_enrollment.upgrade_deadline = today - timedelta(days=1) # yesterday

start_date = datetime.now()
start_date = today
audit_trial_data = LearningAssistantAuditTrialData(
user_id=self.user.id,
start_date=start_date,
expiration_date=start_date + timedelta(days=settings.LEARNING_ASSISTANT_AUDIT_TRIAL_LENGTH_DAYS),
)

self.assertEqual(audit_trial_is_expired(audit_trial_data, self.course_key), True)

@freeze_time('2024-01-01')
@patch('learning_assistant.api.CourseMode')
def test_upgrade_deadline_none(self, mock_course_mode):
self.assertEqual(audit_trial_is_expired(mock_enrollment, audit_trial_data), True)

mock_mode = MagicMock()
mock_mode.expiration_datetime.return_value = None
mock_course_mode.objects.get.return_value = mock_mode
@freeze_time('2024-01-01 00:00:01 UTC')
def test_upgrade_deadline_none(self):
today = datetime.now(tz=timezone.utc)
mock_enrollment = MagicMock()
mock_enrollment.upgrade_deadline = None

# Verify that the audit trial data is considered when determing whether an audit trial is expired and not the
# Verify that the audit trial data is considered when determining whether an audit trial is expired and not the
# upgrade deadline.
start_date = datetime.now()
start_date = today
audit_trial_data = LearningAssistantAuditTrialData(
user_id=self.user.id,
start_date=start_date,
expiration_date=start_date + timedelta(days=settings.LEARNING_ASSISTANT_AUDIT_TRIAL_LENGTH_DAYS),
)

self.assertEqual(audit_trial_is_expired(audit_trial_data, self.course_key), False)
self.assertEqual(audit_trial_is_expired(mock_enrollment, audit_trial_data), False)

start_date = datetime.now() - timedelta(days=settings.LEARNING_ASSISTANT_AUDIT_TRIAL_LENGTH_DAYS + 1)
start_date = today - timedelta(days=settings.LEARNING_ASSISTANT_AUDIT_TRIAL_LENGTH_DAYS + 1)
audit_trial_data = LearningAssistantAuditTrialData(
user_id=self.user.id,
start_date=start_date,
expiration_date=start_date + timedelta(days=settings.LEARNING_ASSISTANT_AUDIT_TRIAL_LENGTH_DAYS),
)

self.assertEqual(audit_trial_is_expired(audit_trial_data, self.course_key), True)
self.assertEqual(audit_trial_is_expired(mock_enrollment, audit_trial_data), True)

@ddt.data(
# exactly the trial deadline
datetime(year=2024, month=1, day=1) - timedelta(days=settings.LEARNING_ASSISTANT_AUDIT_TRIAL_LENGTH_DAYS),
datetime(year=2024, month=1, day=1, tzinfo=timezone.utc) -
timedelta(days=settings.LEARNING_ASSISTANT_AUDIT_TRIAL_LENGTH_DAYS),
# 1 day more than trial deadline
datetime(year=2024, month=1, day=1) - timedelta(days=settings.LEARNING_ASSISTANT_AUDIT_TRIAL_LENGTH_DAYS + 1),
datetime(year=2024, month=1, day=1, tzinfo=timezone.utc) -
timedelta(days=settings.LEARNING_ASSISTANT_AUDIT_TRIAL_LENGTH_DAYS + 1),
)
@freeze_time('2024-01-01')
@patch('learning_assistant.api.CourseMode')
def test_audit_trial_expired(self, start_date, mock_course_mode):
mock_mode = MagicMock()
mock_mode.expiration_datetime.return_value = datetime.now() + timedelta(days=1) # tomorrow
mock_course_mode.objects.get.return_value = mock_mode
@freeze_time('2024-01-01 00:00:01 UTC')
def test_audit_trial_expired(self, start_date):
today = datetime.now(tz=timezone.utc)
mock_enrollment = MagicMock()
mock_enrollment.upgrade_deadline = today + timedelta(days=1) # tomorrow

audit_trial_data = LearningAssistantAuditTrialData(
user_id=self.user.id,
start_date=start_date,
expiration_date=get_audit_trial_expiration_date(start_date),
)

self.assertEqual(audit_trial_is_expired(audit_trial_data, self.upgrade_deadline), True)
self.assertEqual(audit_trial_is_expired(mock_enrollment, audit_trial_data), True)

@freeze_time('2024-01-01')
@patch('learning_assistant.api.CourseMode')
def test_audit_trial_unexpired(self, mock_course_mode):
mock_mode = MagicMock()
mock_mode.expiration_datetime.return_value = datetime.now() + timedelta(days=1) # tomorrow
mock_course_mode.objects.get.return_value = mock_mode
@freeze_time('2024-01-01 00:00:01 UTC')
def test_audit_trial_unexpired(self):
today = datetime.now(tz=timezone.utc)
mock_enrollment = MagicMock()
mock_enrollment.upgrade_deadline = today + timedelta(days=1) # tomorrow

start_date = datetime.now() - timedelta(days=settings.LEARNING_ASSISTANT_AUDIT_TRIAL_LENGTH_DAYS - 1)
start_date = today - timedelta(days=settings.LEARNING_ASSISTANT_AUDIT_TRIAL_LENGTH_DAYS - 1)
audit_trial_data = LearningAssistantAuditTrialData(
user_id=self.user.id,
start_date=start_date,
expiration_date=get_audit_trial_expiration_date(start_date),
)

self.assertEqual(audit_trial_is_expired(audit_trial_data, self.upgrade_deadline), False)
self.assertEqual(audit_trial_is_expired(mock_enrollment, audit_trial_data), False)
Loading