From 54d5f7f394f244f90c3caf0fbf36a4045412dc87 Mon Sep 17 00:00:00 2001 From: Kyle McCormick Date: Fri, 18 Dec 2020 11:26:29 -0500 Subject: [PATCH] Course Blocks API: Fix handling of incorrectly-cased course keys (#25911) Commit 7f59688 attempted to solve this using `CourseOverview.course_exists`, but that check is case- insensitive. This commit instead does a small refactor to `lms.djangoapps.courseware.get_course` so that we can handle a failed course lookup without broadly catching a `ValueError`. --- .../bulk_email/tests/test_err_handling.py | 4 ++-- lms/djangoapps/course_api/blocks/permissions.py | 6 ++++-- lms/djangoapps/courseware/courses.py | 5 +++-- lms/djangoapps/courseware/exceptions.py | 16 ++++++++++++++++ 4 files changed, 25 insertions(+), 6 deletions(-) diff --git a/lms/djangoapps/bulk_email/tests/test_err_handling.py b/lms/djangoapps/bulk_email/tests/test_err_handling.py index 9de1cb4dcf..e79e77fb1e 100644 --- a/lms/djangoapps/bulk_email/tests/test_err_handling.py +++ b/lms/djangoapps/bulk_email/tests/test_err_handling.py @@ -21,6 +21,7 @@ from six.moves import range from lms.djangoapps.bulk_email.models import SEND_TO_MYSELF, BulkEmailFlag, CourseEmail from lms.djangoapps.bulk_email.tasks import perform_delegate_email_batches, send_course_email +from lms.djangoapps.courseware.exceptions import CourseRunNotFound from lms.djangoapps.instructor_task.exceptions import DuplicateTaskException from lms.djangoapps.instructor_task.models import InstructorTask from lms.djangoapps.instructor_task.subtasks import ( @@ -200,8 +201,7 @@ class TestEmailErrors(ModuleStoreTestCase): email.save() entry = InstructorTask.create(course_id, "task_type", "task_key", "task_input", self.instructor) task_input = {"email_id": email.id} - # (?i) is a regex for ignore case - with self.assertRaisesRegex(ValueError, r"(?i)course not found"): + with self.assertRaises(CourseRunNotFound): perform_delegate_email_batches(entry.id, course_id, task_input, "action_name") def test_nonexistent_to_option(self): diff --git a/lms/djangoapps/course_api/blocks/permissions.py b/lms/djangoapps/course_api/blocks/permissions.py index 6e32403fc2..971438adbe 100644 --- a/lms/djangoapps/course_api/blocks/permissions.py +++ b/lms/djangoapps/course_api/blocks/permissions.py @@ -7,6 +7,7 @@ from opaque_keys.edx.keys import CourseKey from lms.djangoapps.courseware.access import has_access from lms.djangoapps.courseware.access_response import AccessResponse from lms.djangoapps.courseware.access_utils import ACCESS_DENIED, ACCESS_GRANTED, check_public_access +from lms.djangoapps.courseware.exceptions import CourseRunNotFound from lms.djangoapps.courseware.courses import get_course from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from common.djangoapps.student.models import CourseEnrollment @@ -47,7 +48,8 @@ def is_course_public(course_key: CourseKey) -> AccessResponse: """ This checks if a course is publicly accessible or not. """ - if not CourseOverview.course_exists(course_key): + try: + course = get_course(course_key, depth=0) + except CourseRunNotFound: return ACCESS_DENIED - course = get_course(course_key, depth=0) return check_public_access(course, [COURSE_VISIBILITY_PUBLIC]) diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index 19eef9f3df..00f966f480 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -41,6 +41,7 @@ from lms.djangoapps.courseware.date_summary import ( VerificationDeadlineDate, VerifiedUpgradeDeadlineDate ) +from lms.djangoapps.courseware.exceptions import CourseRunNotFound from lms.djangoapps.courseware.masquerade import check_content_start_date_for_masquerade_user from lms.djangoapps.courseware.model_data import FieldDataCache from lms.djangoapps.courseware.module_render import get_module @@ -80,7 +81,7 @@ def get_course(course_id, depth=0): """ Given a course id, return the corresponding course descriptor. - If the course does not exist, raises a ValueError. This is appropriate + If the course does not exist, raises a CourseRunNotFound. This is appropriate for internal use. depth: The number of levels of children for the modulestore to cache. @@ -88,7 +89,7 @@ def get_course(course_id, depth=0): """ course = modulestore().get_course(course_id, depth=depth) if course is None: - raise ValueError(u"Course not found: {0}".format(course_id)) + raise CourseRunNotFound(course_key=course_id) return course diff --git a/lms/djangoapps/courseware/exceptions.py b/lms/djangoapps/courseware/exceptions.py index 449b0105a3..ca9ec73c0f 100644 --- a/lms/djangoapps/courseware/exceptions.py +++ b/lms/djangoapps/courseware/exceptions.py @@ -25,3 +25,19 @@ class CourseAccessRedirect(Redirect): def __init__(self, url, access_error=None): super(CourseAccessRedirect, self).__init__(url) self.access_error = access_error + + +class CourseRunNotFound(ValueError): + """ + Indicate that a supplied course run key does not map to a course run in the system. + """ + + def __init__(self, course_key): + """ + Initialize CourseRunNotFound exception. + + Arguments: + course_key (CourseKey|str): + course run key or stringified version thereof. + """ + super().__init__(f"Course run not found: {course_key}")