diff --git a/common/lib/xmodule/xmodule/contentstore/utils.py b/common/lib/xmodule/xmodule/contentstore/utils.py index 002e83969d..f018d2d09e 100644 --- a/common/lib/xmodule/xmodule/contentstore/utils.py +++ b/common/lib/xmodule/xmodule/contentstore/utils.py @@ -1,5 +1,6 @@ # lint-amnesty, pylint: disable=missing-module-docstring +from opaque_keys.edx.keys import CourseKey, UsageKey from xmodule.contentstore.content import StaticContent from .django import contentstore @@ -45,3 +46,12 @@ def restore_asset_from_trashcan(location): store.save(thumbnail_content) except Exception: # lint-amnesty, pylint: disable=broad-except pass # OK if this is left dangling + + +def course_location_from_key(course_key: CourseKey) -> UsageKey: + """Creates a usage key for the toplevel course item, handling differences between mongo and newer keys""" + if getattr(course_key, 'deprecated', False): + block_id = course_key.run + else: + block_id = 'course' + return course_key.make_usage_key('course', block_id) diff --git a/common/test/acceptance/fixtures/course.py b/common/test/acceptance/fixtures/course.py index 2e43ff1a38..3aeef76ac8 100644 --- a/common/test/acceptance/fixtures/course.py +++ b/common/test/acceptance/fixtures/course.py @@ -14,6 +14,7 @@ from path import Path from common.test.acceptance.fixtures import STUDIO_BASE_URL from common.test.acceptance.fixtures.base import FixtureError, XBlockContainerFixture +from xmodule.contentstore.utils import course_location_from_key class XBlockFixtureDesc: @@ -252,11 +253,7 @@ class CourseFixture(XBlockContainerFixture): Return the locator string for the course. """ course_key = CourseKey.from_string(self._course_key) - if getattr(course_key, 'deprecated', False): - block_id = self._course_dict['run'] - else: - block_id = 'course' - return str(course_key.make_usage_key('course', block_id)) + return str(course_location_from_key(course_key)) @property def _assets_url(self): diff --git a/lms/djangoapps/course_home_api/outline/v1/views.py b/lms/djangoapps/course_home_api/outline/v1/views.py index 09b88ecac1..d35d5e4f02 100644 --- a/lms/djangoapps/course_home_api/outline/v1/views.py +++ b/lms/djangoapps/course_home_api/outline/v1/views.py @@ -310,7 +310,7 @@ class OutlineTabView(RetrieveAPIView): # another type, just skip it (don't filter it out). seq_data['type'] != 'sequential' ) - ] + ] if 'children' in chapter_data else [] data = { 'access_expiration': access_expiration, diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index d717fbd375..7ba4e3ef5c 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -186,15 +186,22 @@ class TestJumpTo(ModuleStoreTestCase): ) @ddt.unpack def test_jump_to_invalid_location(self, activate_mfe, store_type): + """Confirm that invalid locations redirect back to a general course URL""" with self.store.default_store(store_type): course = CourseFactory.create() location = course.id.make_usage_key(None, 'NoSuchPlace') + expected_redirect_url = ( + f'http://learning-mfe/course/{course.id}' + ) if activate_mfe else ( + f'/courses/{course.id}/courseware?' + urlencode({'activate_block_id': str(course.location)}) + ) # This is fragile, but unfortunately the problem is that within the LMS we # can't use the reverse calls from the CMS jumpto_url = f'/courses/{course.id}/jump_to/{location}' with _set_mfe_flag(activate_mfe): response = self.client.get(jumpto_url) - assert response.status_code == 404 + assert response.status_code == 302 + assert response.url == expected_redirect_url @_set_mfe_flag(activate_mfe=False) @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) diff --git a/lms/djangoapps/courseware/views/views.py b/lms/djangoapps/courseware/views/views.py index 11dd35f8a0..f37d3c13fb 100644 --- a/lms/djangoapps/courseware/views/views.py +++ b/lms/djangoapps/courseware/views/views.py @@ -129,6 +129,7 @@ from common.djangoapps.util.cache import cache, cache_if_anonymous from common.djangoapps.util.db import outer_atomic from common.djangoapps.util.milestones_helpers import get_prerequisite_courses_display from common.djangoapps.util.views import ensure_valid_course_key, ensure_valid_usage_key +from xmodule.contentstore.utils import course_location_from_key from xmodule.course_module import COURSE_VISIBILITY_PUBLIC, COURSE_VISIBILITY_PUBLIC_OUTLINE from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import ItemNotFoundError, NoPathToItem @@ -413,8 +414,8 @@ def jump_to(request, course_id, location): try: course_key = CourseKey.from_string(course_id) usage_key = UsageKey.from_string(location).replace(course_key=course_key) - except InvalidKeyError: - raise Http404("Invalid course_key or usage_key") # lint-amnesty, pylint: disable=raise-missing-from + except InvalidKeyError as exc: + raise Http404("Invalid course_key or usage_key") from exc experience_param = request.GET.get("experience", "").lower() if experience_param == "new": @@ -430,10 +431,16 @@ def jump_to(request, course_id, location): request=request, experience=experience, ) - except ItemNotFoundError: - raise Http404(f"No data at this location: {usage_key}") # lint-amnesty, pylint: disable=raise-missing-from - except NoPathToItem: - raise Http404(f"This location is not in any class: {usage_key}") # lint-amnesty, pylint: disable=raise-missing-from + except (ItemNotFoundError, NoPathToItem): + # We used to 404 here, but that's ultimately a bad experience. There are real world use cases where a user + # hits a no-longer-valid URL (for example, "resume" buttons that link to no-longer-existing block IDs if the + # course changed out from under the user). So instead, let's just redirect to the beginning of the course, + # as it is at least a valid page the user can interact with... + redirect_url = get_courseware_url( + usage_key=course_location_from_key(course_key), + request=request, + experience=experience, + ) return redirect(redirect_url) diff --git a/openedx/core/djangoapps/courseware_api/tests/test_views.py b/openedx/core/djangoapps/courseware_api/tests/test_views.py index deed1941d1..7492b3602f 100644 --- a/openedx/core/djangoapps/courseware_api/tests/test_views.py +++ b/openedx/core/djangoapps/courseware_api/tests/test_views.py @@ -427,6 +427,16 @@ class ResumeApiTestViews(BaseCoursewareTests, CompletionWaffleTestMixin): assert response.data['unit_id'] == str(self.unit.location) assert response.data['section_id'] == str(self.sequence.location) + def test_resume_invalid_key(self): + """A resume key that does not exist should return null IDs (i.e. "redirect to first section")""" + self.override_waffle_switch(True) + submit_completions_for_testing(self.user, [self.course.id.make_usage_key('html', 'doesnotexist')]) + response = self.client.get(self.url) + assert response.status_code == 200 + assert response.data['block_id'] is None + assert response.data['unit_id'] is None + assert response.data['section_id'] is None + @ddt.ddt class CelebrationApiTestViews(BaseCoursewareTests, MasqueradeMixin): diff --git a/openedx/core/djangoapps/courseware_api/views.py b/openedx/core/djangoapps/courseware_api/views.py index c53122668c..1b104b830b 100644 --- a/openedx/core/djangoapps/courseware_api/views.py +++ b/openedx/core/djangoapps/courseware_api/views.py @@ -2,8 +2,6 @@ Course API Views """ -import json # lint-amnesty, pylint: disable=unused-import - from completion.exceptions import UnavailableCompletionData from completion.utilities import get_key_to_last_completed_block from django.conf import settings @@ -62,6 +60,7 @@ from common.djangoapps.student.models import ( LinkedInAddToProfileConfiguration ) from xmodule.modulestore.django import modulestore +from xmodule.modulestore.exceptions import ItemNotFoundError, NoPathToItem from xmodule.modulestore.search import path_to_location from xmodule.x_module import PUBLIC_VIEW, STUDENT_VIEW @@ -623,8 +622,8 @@ class Resume(DeveloperErrorViewMixin, APIView): resp['unit_id'] = str(path[3]) resp['block_id'] = str(block_key) - except UnavailableCompletionData: - pass + except (ItemNotFoundError, NoPathToItem, UnavailableCompletionData): + pass # leaving all the IDs as None indicates a redirect to the first unit in the course, as a backup return Response(resp)