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: catch a possible exception in beta course configuration #36172

Merged
merged 2 commits into from
Jan 27, 2025
Merged

fix: catch a possible exception in beta course configuration #36172

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2626afc
fix: catch a possible exception in beta course configuration
deborahgu Jan 24, 2025
6b6de9d
fix: unused import
deborahgu Jan 24, 2025
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
45 changes: 24 additions & 21 deletions lms/djangoapps/courseware/access_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
It allows us to share code between access.py and block transformers.
"""


from datetime import datetime, timedelta
from logging import getLogger

Expand All @@ -21,7 +20,7 @@
EnrollmentRequiredAccessError,
IncorrectActiveEnterpriseAccessError,
StartDateEnterpriseLearnerError,
StartDateError
StartDateError,
)
from lms.djangoapps.courseware.masquerade import get_course_masquerade, is_masquerading_as_student
from openedx.features.course_experience import COURSE_ENABLE_UNENROLLED_ACCESS_FLAG, COURSE_PRE_START_ACCESS_FLAG
Expand Down Expand Up @@ -58,9 +57,12 @@ def adjust_start_date(user, days_early_for_beta, start, course_key):

if CourseBetaTesterRole(course_key).has_user(user):
debug("Adjust start time: user in beta role for %s", course_key)
delta = timedelta(days_early_for_beta)
effective = start - delta
return effective
# timedelta.max days from now is in the year 2739931, so that's probably pretty safe
delta = timedelta(min(days_early_for_beta, timedelta.max.days))
try:
return start - delta
except OverflowError:
return start

return start

Expand Down Expand Up @@ -93,7 +95,7 @@ def enterprise_learner_enrolled(request, user, course_key):
# enterprise_customer_data is either None (if learner is not linked to any customer) or a serialized
# EnterpriseCustomer representing the learner's active linked customer.
enterprise_customer_data = enterprise_customer_from_session_or_learner_data(request)
learner_portal_enabled = enterprise_customer_data and enterprise_customer_data['enable_learner_portal']
learner_portal_enabled = enterprise_customer_data and enterprise_customer_data["enable_learner_portal"]
Copy link
Member Author

@deborahgu deborahgu Jan 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

all of the changes in this file from this line down are just because I let my IDE autolint the file to our current standards. Usually I wouldn't commit an autoformat at the same time as an actual code change, but because the code change is just that one tiny block above, and the autoformat isn't extreme, it seemed like a legitimate choice. I can undo if reviewers prefer.

if not learner_portal_enabled:
return False

Expand All @@ -102,18 +104,18 @@ def enterprise_learner_enrolled(request, user, course_key):
enterprise_enrollments = EnterpriseCourseEnrollment.objects.filter(
course_id=course_key,
enterprise_customer_user__user_id=user.id,
enterprise_customer_user__enterprise_customer__uuid=enterprise_customer_data['uuid'],
enterprise_customer_user__enterprise_customer__uuid=enterprise_customer_data["uuid"],
)
enterprise_enrollment_exists = enterprise_enrollments.exists()
log.info(
(
'[enterprise_learner_enrolled] Checking for an enterprise enrollment for '
'lms_user_id=%s in course_key=%s via enterprise_customer_uuid=%s. '
'Exists: %s'
"[enterprise_learner_enrolled] Checking for an enterprise enrollment for "
"lms_user_id=%s in course_key=%s via enterprise_customer_uuid=%s. "
"Exists: %s"
),
user.id,
course_key,
enterprise_customer_data['uuid'],
enterprise_customer_data["uuid"],
enterprise_enrollment_exists,
)
return enterprise_enrollment_exists
Expand All @@ -130,7 +132,7 @@ def check_start_date(user, days_early_for_beta, start, course_key, display_error
Returns:
AccessResponse: Either ACCESS_GRANTED or StartDateError.
"""
start_dates_disabled = settings.FEATURES['DISABLE_START_DATES']
start_dates_disabled = settings.FEATURES["DISABLE_START_DATES"]
masquerading_as_student = is_masquerading_as_student(user, course_key)

if start_dates_disabled and not masquerading_as_student:
Expand Down Expand Up @@ -161,8 +163,8 @@ def in_preview_mode():
Returns whether the user is in preview mode or not.
"""
hostname = get_current_request_hostname()
preview_lms_base = settings.FEATURES.get('PREVIEW_LMS_BASE', None)
return bool(preview_lms_base and hostname and hostname.split(':')[0] == preview_lms_base.split(':')[0])
preview_lms_base = settings.FEATURES.get("PREVIEW_LMS_BASE", None)
return bool(preview_lms_base and hostname and hostname.split(":")[0] == preview_lms_base.split(":")[0])


def check_course_open_for_learner(user, course):
Expand Down Expand Up @@ -233,18 +235,19 @@ def check_public_access(course, visibilities):

def check_data_sharing_consent(course_id):
"""
Grants access if the user is do not need DataSharing consent, otherwise returns data sharing link.
Grants access if the user is do not need DataSharing consent, otherwise returns data sharing link.

Returns:
AccessResponse: Either ACCESS_GRANTED or DataSharingConsentRequiredAccessError
"""
Returns:
AccessResponse: Either ACCESS_GRANTED or DataSharingConsentRequiredAccessError
"""
from openedx.features.enterprise_support.api import get_enterprise_consent_url

consent_url = get_enterprise_consent_url(
request=get_current_request(),
course_id=str(course_id),
return_to='courseware',
return_to="courseware",
enrollment_exists=True,
source='CoursewareAccess'
source="CoursewareAccess",
)
if consent_url:
return DataSharingConsentRequiredAccessError(consent_url=consent_url)
Expand Down Expand Up @@ -274,7 +277,7 @@ def check_correct_active_enterprise_customer(user, course_id):
except (EnterpriseCustomerUser.DoesNotExist, EnterpriseCustomerUser.MultipleObjectsReturned):
# Ideally this should not happen. As there should be only 1 active enterprise customer in our system
log.error("Multiple or No Active Enterprise found for the user %s.", user.id)
active_enterprise_name = 'Incorrect'
active_enterprise_name = "Incorrect"

enrollment_enterprise_name = enterprise_enrollments.first().enterprise_customer_user.enterprise_customer.name
return IncorrectActiveEnterpriseAccessError(enrollment_enterprise_name, active_enterprise_name)
Expand Down
103 changes: 64 additions & 39 deletions lms/djangoapps/courseware/tests/test_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
"""
Unit test for various Utility functions
"""

import json
from datetime import date, timedelta
from unittest.mock import patch

import ddt
Expand All @@ -14,13 +16,44 @@
from common.djangoapps.student.tests.factories import GlobalStaffFactory, UserFactory
from lms.djangoapps.courseware.constants import UNEXPECTED_ERROR_IS_ELIGIBLE
from lms.djangoapps.courseware.tests.factories import FinancialAssistanceConfigurationFactory
from lms.djangoapps.courseware.access_utils import adjust_start_date
from lms.djangoapps.courseware.utils import (
create_financial_assistance_application,
get_financial_assistance_application_status,
is_eligible_for_financial_aid
is_eligible_for_financial_aid,
)


@ddt.ddt
class TestAccessUtils(TestCase):
"""Tests for the access_utils functions."""

@ddt.data(
# days_early_for_beta, is_beta_user, expected_date
(None, True, "2025-01-03"),
(2, True, "2025-01-01"),
(timedelta.max.days + 10, True, "2025-01-03"),
(None, False, "2025-01-03"),
(2, False, "2025-01-03"),
(timedelta.max.days + 10, False, "2025-01-03"),
)
@ddt.unpack
def test_adjust_start_date(self, days_early_for_beta, is_beta_user, expected_date):
"""Tests adjust_start_date

Should only modify the date if the user is beta for the course,
and `days_early_for_beta` is sensible number."""
start = date(2025, 1, 3)
expected = date.fromisoformat(expected_date)
user = "princessofpower"
course_key = "edx1+8675"
with patch("lms.djangoapps.courseware.access_utils.CourseBetaTesterRole") as role_mock:
instance = role_mock.return_value
instance.has_user.return_value = is_beta_user
adjusted_date = adjust_start_date(user, days_early_for_beta, start, course_key)
self.assertEqual(expected, adjusted_date)


Copy link
Member Author

Choose a reason for hiding this comment

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

same as above – from here on down it is just autoformatting. Again, I can undo it and just put in the individual changes the reviewers think it is too excessive.

@ddt.ddt
class TestFinancialAssistanceViews(TestCase):
"""
Expand All @@ -29,17 +62,17 @@ class TestFinancialAssistanceViews(TestCase):

def setUp(self) -> None:
super().setUp()
self.test_course_id = 'course-v1:edX+Test+1'
self.test_course_id = "course-v1:edX+Test+1"
self.user = UserFactory()
self.global_staff = GlobalStaffFactory.create()
_ = FinancialAssistanceConfigurationFactory(
api_base_url='http://financial.assistance.test:1234',
api_base_url="http://financial.assistance.test:1234",
service_username=self.global_staff.username,
fa_backend_enabled_courses_percentage=100,
enabled=True
enabled=True,
)
_ = Application.objects.create(
name='Test Application',
name="Test Application",
user=self.global_staff,
client_type=Application.CLIENT_PUBLIC,
authorization_grant_type=Application.GRANT_CLIENT_CREDENTIALS,
Expand All @@ -51,33 +84,31 @@ def _mock_response(self, status_code, content=None):
"""
mock_response = Response()
mock_response.status_code = status_code
mock_response._content = json.dumps(content).encode('utf-8') # pylint: disable=protected-access
mock_response._content = json.dumps(content).encode("utf-8") # pylint: disable=protected-access
return mock_response

@ddt.data(
{'is_eligible': True, 'reason': None},
{'is_eligible': False, 'reason': 'This course is not eligible for financial aid'}
{"is_eligible": True, "reason": None},
{"is_eligible": False, "reason": "This course is not eligible for financial aid"},
)
def test_is_eligible_for_financial_aid(self, response_data):
"""
Tests the functionality of is_eligible_for_financial_aid which calls edx-financial-assistance backend
to return eligibility status for financial assistance for a given course.
"""
with patch.object(OAuthAPIClient, 'request') as oauth_mock:
with patch.object(OAuthAPIClient, "request") as oauth_mock:
oauth_mock.return_value = self._mock_response(status.HTTP_200_OK, response_data)
is_eligible, reason = is_eligible_for_financial_aid(self.test_course_id)
assert is_eligible is response_data.get('is_eligible')
assert reason == response_data.get('reason')
assert is_eligible is response_data.get("is_eligible")
assert reason == response_data.get("reason")

def test_is_eligible_for_financial_aid_invalid_course_id(self):
"""
Tests the functionality of is_eligible_for_financial_aid for an invalid course id.
"""
error_message = f"Invalid course id {self.test_course_id} provided"
with patch.object(OAuthAPIClient, 'request') as oauth_mock:
oauth_mock.return_value = self._mock_response(
status.HTTP_400_BAD_REQUEST, {"message": error_message}
)
with patch.object(OAuthAPIClient, "request") as oauth_mock:
oauth_mock.return_value = self._mock_response(status.HTTP_400_BAD_REQUEST, {"message": error_message})
is_eligible, reason = is_eligible_for_financial_aid(self.test_course_id)
assert is_eligible is False
assert reason == error_message
Expand All @@ -86,9 +117,9 @@ def test_is_eligible_for_financial_aid_invalid_unexpected_error(self):
"""
Tests the functionality of is_eligible_for_financial_aid for an unexpected error
"""
with patch.object(OAuthAPIClient, 'request') as oauth_mock:
with patch.object(OAuthAPIClient, "request") as oauth_mock:
oauth_mock.return_value = self._mock_response(
status.HTTP_500_INTERNAL_SERVER_ERROR, {'error': 'unexpected error occurred'}
status.HTTP_500_INTERNAL_SERVER_ERROR, {"error": "unexpected error occurred"}
)
is_eligible, reason = is_eligible_for_financial_aid(self.test_course_id)
assert is_eligible is False
Expand All @@ -99,45 +130,39 @@ def test_get_financial_assistance_application_status(self):
Tests the functionality of get_financial_assistance_application_status against a user id and a course id
edx-financial-assistance backend to return status of a financial assistance application.
"""
test_response = {'id': 123, 'status': 'ACCEPTED', 'coupon_code': 'ABCD..'}
with patch.object(OAuthAPIClient, 'request') as oauth_mock:
test_response = {"id": 123, "status": "ACCEPTED", "coupon_code": "ABCD.."}
with patch.object(OAuthAPIClient, "request") as oauth_mock:
oauth_mock.return_value = self._mock_response(status.HTTP_200_OK, test_response)
has_application, reason = get_financial_assistance_application_status(self.user.id, self.test_course_id)
assert has_application is True
assert reason == test_response

@ddt.data(
{
'status': status.HTTP_400_BAD_REQUEST,
'content': {'message': 'Invalid course id provided'}
},
{
'status': status.HTTP_404_NOT_FOUND,
'content': {'message': 'Application details not found'}
}
{"status": status.HTTP_400_BAD_REQUEST, "content": {"message": "Invalid course id provided"}},
{"status": status.HTTP_404_NOT_FOUND, "content": {"message": "Application details not found"}},
)
def test_get_financial_assistance_application_status_unsuccessful(self, response_data):
"""
Tests unsuccessful scenarios of get_financial_assistance_application_status
against a user id and a course id edx-financial-assistance backend.
"""
with patch.object(OAuthAPIClient, 'request') as oauth_mock:
oauth_mock.return_value = self._mock_response(response_data.get('status'), response_data.get('content'))
with patch.object(OAuthAPIClient, "request") as oauth_mock:
oauth_mock.return_value = self._mock_response(response_data.get("status"), response_data.get("content"))
has_application, reason = get_financial_assistance_application_status(self.user.id, self.test_course_id)
assert has_application is False
assert reason == response_data.get('content').get('message')
assert reason == response_data.get("content").get("message")

def test_create_financial_assistance_application(self):
"""
Tests the functionality of create_financial_assistance_application which calls edx-financial-assistance backend
to create a new financial assistance application given a form data.
"""
test_form_data = {
'lms_user_id': self.user.id,
'course_id': self.test_course_id,
"lms_user_id": self.user.id,
"course_id": self.test_course_id,
}
with patch.object(OAuthAPIClient, 'request') as oauth_mock:
oauth_mock.return_value = self._mock_response(status.HTTP_200_OK, {'success': True})
with patch.object(OAuthAPIClient, "request") as oauth_mock:
oauth_mock.return_value = self._mock_response(status.HTTP_200_OK, {"success": True})
response = create_financial_assistance_application(form_data=test_form_data)
assert response.status_code == status.HTTP_204_NO_CONTENT

Expand All @@ -147,12 +172,12 @@ def test_create_financial_assistance_application_bad_request(self):
to create a new financial assistance application given a form data.
"""
test_form_data = {
'lms_user_id': self.user.id,
'course_id': 'invalid_course_id',
"lms_user_id": self.user.id,
"course_id": "invalid_course_id",
}
error_response = {'message': 'Invalid course id provided'}
with patch.object(OAuthAPIClient, 'request') as oauth_mock:
error_response = {"message": "Invalid course id provided"}
with patch.object(OAuthAPIClient, "request") as oauth_mock:
oauth_mock.return_value = self._mock_response(status.HTTP_400_BAD_REQUEST, error_response)
response = create_financial_assistance_application(form_data=test_form_data)
assert response.status_code == status.HTTP_400_BAD_REQUEST
assert json.loads(response.content.decode('utf-8')) == error_response
assert json.loads(response.content.decode("utf-8")) == error_response
Loading