From 3334325ff9b8c338176cf59db5636fbf2736d604 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Thu, 22 May 2025 10:58:52 -0400 Subject: [PATCH 1/7] fix: Drop _get_legacy_courseware_url and related usage. We were running some tests using this function but it is not actually used in the running application anymore so drop those tests and remove the function in preparation for removing the legacy courseware itself. --- lms/djangoapps/courseware/tests/test_views.py | 76 ------------------- lms/djangoapps/courseware/testutils.py | 27 ------- .../features/course_experience/url_helpers.py | 41 ---------- 3 files changed, 144 deletions(-) diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index 2a27a3fc41..8cf7737f1b 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -108,7 +108,6 @@ from openedx.features.course_experience import ( ) from openedx.features.course_experience.tests.views.helpers import add_course_mode from openedx.features.course_experience.url_helpers import ( - _get_legacy_courseware_url, get_learning_mfe_home_url, make_learning_mfe_courseware_url ) @@ -232,31 +231,6 @@ class TestJumpTo(ModuleStoreTestCase): assert response.status_code == 302 assert response.url == expected_redirect_url - # The new courseware experience does not support this sort of course structure; - # it assumes a simple course->chapter->sequence->unit->component tree. - @patch.object(views, 'get_courseware_url', new=_get_legacy_courseware_url) - def test_jump_to_legacy_from_nested_block(self): - with self.store.default_store(ModuleStoreEnum.Type.split): - course = CourseFactory.create() - chapter = BlockFactory.create(category='chapter', parent_location=course.location) - sequence = BlockFactory.create(category='sequential', parent_location=chapter.location) - vertical = BlockFactory.create(category='vertical', parent_location=sequence.location) - nested_sequence = BlockFactory.create(category='sequential', parent_location=vertical.location) - nested_vertical1 = BlockFactory.create(category='vertical', parent_location=nested_sequence.location) - # put a block into nested_vertical1 for completeness - BlockFactory.create(category='html', parent_location=nested_vertical1.location) - nested_vertical2 = BlockFactory.create(category='vertical', parent_location=nested_sequence.location) - block2 = BlockFactory.create(category='html', parent_location=nested_vertical2.location) - - # internal position of block2 will be 1_2 (2nd item withing 1st item) - activate_block_id = urlencode({'activate_block_id': str(block2.location)}) - expected_redirect_url = ( - f'/courses/{course.id}/courseware/{chapter.url_name}/{sequence.url_name}/1?{activate_block_id}' - ) - jumpto_url = f'/courses/{course.id}/jump_to/{block2.location}' - response = self.client.get(jumpto_url) - self.assertRedirects(response, expected_redirect_url, status_code=302, target_status_code=302) - @ddt.data( (False, ModuleStoreEnum.Type.split), (True, ModuleStoreEnum.Type.split), @@ -273,44 +247,6 @@ class TestJumpTo(ModuleStoreTestCase): response = self.client.get(jumpto_url) assert response.status_code == 404 - @ddt.data( - (ModuleStoreEnum.Type.split, False, '1'), - (ModuleStoreEnum.Type.split, True, '2'), - ) - @ddt.unpack - def test_jump_to_legacy_for_learner_with_staff_only_content(self, store_type, is_staff_user, position): - """ - Test for checking correct position in redirect_url for learner when a course has staff-only units. - - (When the MFE is active, it handles this logic itself with the help of the - courseware blocks/metadata/outline APIs, so we don't test for it here.) - """ - with self.store.default_store(store_type): - course = CourseFactory.create() - request = RequestFactory().get('/') - request.user = UserFactory(is_staff=is_staff_user, username="staff") - request.session = {} - course_key = CourseKey.from_string(str(course.id)) - chapter = BlockFactory.create(category='chapter', parent_location=course.location) - sequence = BlockFactory.create(category='sequential', parent_location=chapter.location) - __ = BlockFactory.create(category='vertical', parent_location=sequence.location) - staff_only_vertical = BlockFactory.create(category='vertical', parent_location=sequence.location, - metadata=dict(visible_to_staff_only=True)) - __ = BlockFactory.create(category='vertical', parent_location=sequence.location) - - usage_key = UsageKey.from_string(str(staff_only_vertical.location)).replace(course_key=course_key) - expected_url = reverse( - 'courseware_position', - kwargs={ - 'course_id': str(course.id), - 'chapter': chapter.url_name, - 'section': sequence.url_name, - 'position': position, - } - ) - expected_url += "?{}".format(urlencode({'activate_block_id': str(staff_only_vertical.location)})) - assert expected_url == _get_legacy_courseware_url(usage_key, request) - class IndexQueryTestCase(ModuleStoreTestCase): """ @@ -474,18 +410,6 @@ class CoursewareIndexTestCase(BaseViewsTestCase): assert response.status_code == expected_response_code return response - def test_get_redirect_url(self): - # test the course location - assert '/courses/{course_key}/courseware?{activate_block_id}'.format( - course_key=str(self.course_key), - activate_block_id=urlencode({'activate_block_id': str(self.course.location)}) - ) == _get_legacy_courseware_url(self.course.location) - # test a section location - assert '/courses/{course_key}/courseware/Chapter_1/Sequential_1/?{activate_block_id}'.format( - course_key=str(self.course_key), - activate_block_id=urlencode({'activate_block_id': str(self.section.location)}) - ) == _get_legacy_courseware_url(self.section.location) - def test_index_invalid_position(self): request_url = '/'.join([ '/courses', diff --git a/lms/djangoapps/courseware/testutils.py b/lms/djangoapps/courseware/testutils.py index b0f444f288..8730e29e38 100644 --- a/lms/djangoapps/courseware/testutils.py +++ b/lms/djangoapps/courseware/testutils.py @@ -12,7 +12,6 @@ import ddt from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from lms.djangoapps.courseware.utils import is_mode_upsellable -from openedx.features.course_experience.url_helpers import _get_legacy_courseware_url from common.djangoapps.student.tests.factories import AdminFactory, CourseEnrollmentFactory, UserFactory from common.djangoapps.course_modes.models import CourseMode from common.djangoapps.course_modes.tests.factories import CourseModeFactory @@ -165,32 +164,6 @@ class RenderXBlockTestMixin(MasqueradeMixin, metaclass=ABCMeta): self.assertNotContains(response, self.html_block.data, status_code=expected_response_code) return response - @ddt.data( - ('vertical_block', 4), - ('html_block', 4), - ) - @ddt.unpack - @patch('lms.djangoapps.courseware.views.index.CoursewareIndex._redirect_to_learning_mfe', return_value=None) - def test_courseware_html(self, block_name, mongo_calls, mock_redirect): - """ - To verify that the removal of courseware chrome elements is working, - we include this test here to make sure the chrome elements that should - be removed actually exist in the full courseware page. - If this test fails, it's probably because the HTML template for courseware - has changed and COURSEWARE_CHROME_HTML_ELEMENTS needs to be updated. - """ - with self.store.default_store(ModuleStoreEnum.Type.split): - self.block_name_to_be_tested = block_name - self.setup_course(ModuleStoreEnum.Type.split) - self.setup_user(admin=True, enroll=True, login=True) - - with check_mongo_calls(mongo_calls): - url = _get_legacy_courseware_url(self.block_to_be_tested.location) - response = self.client.get(url) - expected_elements = self.block_specific_chrome_html_elements + self.COURSEWARE_CHROME_HTML_ELEMENTS - for chrome_element in expected_elements: - self.assertContains(response, chrome_element) - def test_success_enrolled_staff(self): self.setup_course() self.setup_user(admin=True, enroll=True, login=True) diff --git a/openedx/features/course_experience/url_helpers.py b/openedx/features/course_experience/url_helpers.py index be8c02d2bd..dca139d6f1 100644 --- a/openedx/features/course_experience/url_helpers.py +++ b/openedx/features/course_experience/url_helpers.py @@ -44,47 +44,6 @@ def get_courseware_url( return _get_new_courseware_url(usage_key=usage_key, request=request, is_staff=is_staff) -def _get_legacy_courseware_url( - usage_key: UsageKey, - request: Optional[HttpRequest] = None, - is_staff: bool = None -) -> str: - """ - Return the URL to Legacy (LMS-rendered) courseware content. - - Raises: - * ItemNotFoundError if no data at the usage_key. - * NoPathToItem if location not in any class. - """ - ( - course_key, chapter, section, vertical_unused, - position, final_target_id - ) = path_to_location(modulestore(), usage_key, request) - - # choose the appropriate view (and provide the necessary args) based on the - # args provided by the redirect. - # Rely on index to do all error handling and access control. - if chapter is None: - redirect_url = reverse('courseware', args=(str(course_key), )) - elif section is None: - redirect_url = reverse('courseware_chapter', args=(str(course_key), chapter)) - elif position is None: - redirect_url = reverse( - 'courseware_section', - args=(str(course_key), chapter, section) - ) - else: - # Here we use the navigation_index from the position returned from - # path_to_location - we can only navigate to the topmost vertical at the - # moment - redirect_url = reverse( - 'courseware_position', - args=(str(course_key), chapter, section, navigation_index(position)) - ) - redirect_url += "?{}".format(urlencode({'activate_block_id': str(final_target_id)})) - return redirect_url - - def _get_new_courseware_url( usage_key: UsageKey, request: Optional[HttpRequest] = None, From 0121a0ad9ecf88ba38a18c44c44a1bfc3de1a51a Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Thu, 22 May 2025 11:02:56 -0400 Subject: [PATCH 2/7] test: Drop a legacy test. This test tests whether or not we can load the legacy courseware page if we have not met prerequisites. We don't need this anymore because we are in the process of removing those pages and the default is now to load the MFE instead. The underlying checks still happens as a part of the `_has_access_course` function which calls `lms/djangoapps/courseware/access.py:_can_view_courseware_with_prerequisites` --- .../courseware/tests/test_access.py | 41 ------------------- 1 file changed, 41 deletions(-) diff --git a/lms/djangoapps/courseware/tests/test_access.py b/lms/djangoapps/courseware/tests/test_access.py index bb6d42025e..5b910565e4 100644 --- a/lms/djangoapps/courseware/tests/test_access.py +++ b/lms/djangoapps/courseware/tests/test_access.py @@ -681,47 +681,6 @@ class AccessTestCase(LoginEnrollmentTestCase, ModuleStoreTestCase, MilestonesTes assert bool(access._has_access_course(self.student, 'load_mobile', course)) == student_expected assert bool(access._has_access_course(self.staff, 'load_mobile', course)) == staff_expected - @patch.dict("django.conf.settings.FEATURES", {'ENABLE_PREREQUISITE_COURSES': True, 'MILESTONES_APP': True}) - def test_courseware_page_unfulfilled_prereqs(self): - """ - Test courseware access when a course has pre-requisite course yet to be completed - """ - pre_requisite_course = CourseFactory.create( - org='edX', - course='900', - run='test_run', - ) - - pre_requisite_courses = [str(pre_requisite_course.id)] - course = CourseFactory.create( - org='edX', - course='1000', - run='test_run', - pre_requisite_courses=pre_requisite_courses, - ) - set_prerequisite_courses(course.id, pre_requisite_courses) - - test_password = 't3stp4ss.!' - user = UserFactory.create() - user.set_password(test_password) - user.save() - self.login(user.email, test_password) - CourseEnrollmentFactory(user=user, course_id=course.id) - - url = reverse('courseware', args=[str(course.id)]) - response = self.client.get(url) - self.assertRedirects( - response, - reverse( - 'dashboard' - ) - ) - assert response.status_code == 302 - - fulfill_course_milestone(pre_requisite_course.id, user) - response = self.client.get(url) - assert response.status_code == 200 - class UserRoleTestCase(TestCase): """ From 3520b6b8e1a01db6343647c6e68352f1faa2e518 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Thu, 22 May 2025 11:09:48 -0400 Subject: [PATCH 3/7] test: Drop testing for legacy courseware UI This has all been replaced by the learning MFE and will be removed from the platform in subsequent commits. For masquerade testing, the page no longer renders content and so shouldn't be a part of this test. The render_xblock url is what is used by the MFE so we're still testing that the course-wide content is being loaded correctly for content served by the learning MFE. --- .../courseware/tests/test_masquerade.py | 45 -- .../courseware/tests/test_navigation.py | 285 --------- .../tests/test_view_authentication.py | 60 +- lms/djangoapps/courseware/tests/test_views.py | 541 +----------------- 4 files changed, 2 insertions(+), 929 deletions(-) delete mode 100644 lms/djangoapps/courseware/tests/test_navigation.py diff --git a/lms/djangoapps/courseware/tests/test_masquerade.py b/lms/djangoapps/courseware/tests/test_masquerade.py index 33d100c7a2..ed5947ffc6 100644 --- a/lms/djangoapps/courseware/tests/test_masquerade.py +++ b/lms/djangoapps/courseware/tests/test_masquerade.py @@ -244,51 +244,6 @@ class TestMasqueradeOptionsNoContentGroups(StaffMasqueradeTestCase): assert is_target_available == expected -# These tests are testing a capability of the old courseware page. We have to not -# force redirect to the new MFE in order to be able to load the old pages which are -# being tested by this page. -# -# This is a temporary change, until we can remove the old courseware pages -# all together. -@patch('lms.djangoapps.courseware.views.index.CoursewareIndex._redirect_to_learning_mfe', return_value=None) -class TestStaffMasqueradeAsStudent(StaffMasqueradeTestCase): - """ - Check for staff being able to masquerade as student. - """ - - @patch.dict('django.conf.settings.FEATURES', {'DISABLE_START_DATES': False}) - def test_staff_debug_with_masquerade(self, mock_redirect): - """ - Tests that staff debug control is not visible when masquerading as a student. - """ - # Verify staff initially can see staff debug - self.verify_staff_debug_present(True) - - # Toggle masquerade to student - self.update_masquerade(role='student') - self.verify_staff_debug_present(False) - - # Toggle masquerade back to staff - self.update_masquerade(role='staff') - self.verify_staff_debug_present(True) - - @patch.dict('django.conf.settings.FEATURES', {'DISABLE_START_DATES': False}) - def test_show_answer_for_staff(self, mock_redirect): - """ - Tests that "Show Answer" is not visible when masquerading as a student. - """ - # Verify that staff initially can see "Show Answer". - self.verify_show_answer_present(True) - - # Toggle masquerade to student - self.update_masquerade(role='student') - self.verify_show_answer_present(False) - - # Toggle masquerade back to staff - self.update_masquerade(role='staff') - self.verify_show_answer_present(True) - - @ddt.ddt class TestStaffMasqueradeAsSpecificStudent(StaffMasqueradeTestCase, ProblemSubmissionTestMixin): """ diff --git a/lms/djangoapps/courseware/tests/test_navigation.py b/lms/djangoapps/courseware/tests/test_navigation.py deleted file mode 100644 index 8c2144ee2a..0000000000 --- a/lms/djangoapps/courseware/tests/test_navigation.py +++ /dev/null @@ -1,285 +0,0 @@ -""" -This test file will run through some LMS test scenarios regarding access and navigation of the LMS -""" - - -import time - -from unittest.mock import patch -from django.conf import settings -from django.test.utils import override_settings -from django.urls import reverse -from edx_toggles.toggles.testutils import override_waffle_flag -from xmodule.modulestore.django import modulestore -from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase -from xmodule.modulestore.tests.factories import CourseFactory, BlockFactory - -from common.djangoapps.student.tests.factories import GlobalStaffFactory -from lms.djangoapps.courseware.tests.helpers import LoginEnrollmentTestCase -from openedx.features.course_experience import DISABLE_COURSE_OUTLINE_PAGE_FLAG -from openedx.features.course_experience.url_helpers import make_learning_mfe_courseware_url - - -class TestNavigation(SharedModuleStoreTestCase, LoginEnrollmentTestCase): - """ - Check that navigation state is saved properly. - """ - @classmethod - def setUpClass(cls): - # pylint: disable=super-method-not-called - with super().setUpClassAndTestData(): - cls.test_course = CourseFactory.create() - cls.test_course_proctored = CourseFactory.create() - cls.course = CourseFactory.create() - - @classmethod - def setUpTestData(cls): # lint-amnesty, pylint: disable=super-method-not-called - cls.chapter0 = BlockFactory.create(parent=cls.course, - display_name='Overview') - cls.chapter9 = BlockFactory.create(parent=cls.course, - display_name='factory_chapter') - cls.section0 = BlockFactory.create(parent=cls.chapter0, - display_name='Welcome') - cls.section9 = BlockFactory.create(parent=cls.chapter9, - display_name='factory_section') - cls.unit0 = BlockFactory.create(parent=cls.section0, - display_name='New Unit 0') - - cls.chapterchrome = BlockFactory.create(parent=cls.course, - display_name='Chrome') - cls.chromelesssection = BlockFactory.create(parent=cls.chapterchrome, - display_name='chromeless', - chrome='none') - cls.accordionsection = BlockFactory.create(parent=cls.chapterchrome, - display_name='accordion', - chrome='accordion') - cls.tabssection = BlockFactory.create(parent=cls.chapterchrome, - display_name='tabs', - chrome='tabs') - cls.defaultchromesection = BlockFactory.create( - parent=cls.chapterchrome, - display_name='defaultchrome', - ) - cls.fullchromesection = BlockFactory.create(parent=cls.chapterchrome, - display_name='fullchrome', - chrome='accordion,tabs') - cls.tabtest = BlockFactory.create(parent=cls.chapterchrome, - display_name='pdf_textbooks_tab', - default_tab='progress') - - cls.user = GlobalStaffFactory(password='test') - - def setUp(self): - super().setUp() - self.login(self.user.email, 'test') - self.patcher = patch( - 'lms.djangoapps.courseware.views.index.CoursewareIndex._redirect_to_learning_mfe', return_value=None) - self.mock_redirect = self.patcher.start() - - def tearDown(self): - self.patcher.stop() - super().tearDown() - - def assertTabActive(self, tabname, response): - ''' Check if the progress tab is active in the tab set ''' - for line in response.content.decode('utf-8').split('\n'): - if tabname in line and 'active' in line: - return - raise AssertionError(f"assertTabActive failed: {tabname} not active") - - def assertTabInactive(self, tabname, response): # lint-amnesty, pylint: disable=useless-return - ''' Check if the progress tab is active in the tab set ''' - for line in response.content.decode('utf-8').split('\n'): - if tabname in line and 'active' in line: - raise AssertionError("assertTabInactive failed: " + tabname + " active") - return - - # TODO: LEARNER-71: Do we need to adjust or remove this test? - @override_waffle_flag(DISABLE_COURSE_OUTLINE_PAGE_FLAG, active=True) - def test_chrome_settings(self): - ''' - Test settings for disabling and modifying navigation chrome in the courseware: - - Accordion enabled, or disabled - - Navigation tabs enabled, disabled, or redirected - ''' - test_data = ( - ('tabs', False, True), - ('none', False, False), - ('accordion', True, False), - ('fullchrome', True, True), - ) - for (displayname, accordion, tabs) in test_data: - response = self.client.get(reverse('courseware_section', kwargs={ - 'course_id': str(self.course.id), - 'chapter': 'Chrome', - 'section': displayname, - })) - assert ('course-tabs' in response.content.decode('utf-8')) == tabs - assert ('course-navigation' in response.content.decode('utf-8')) == accordion - - self.assertTabInactive('progress', response) - self.assertTabActive(make_learning_mfe_courseware_url(self.course.id), response) - - response = self.client.get(reverse('courseware_section', kwargs={ - 'course_id': str(self.course.id), - 'chapter': 'Chrome', - 'section': 'pdf_textbooks_tab', - })) - - self.assertTabActive('progress', response) - self.assertTabInactive(make_learning_mfe_courseware_url(self.course.id), response) - - @override_settings(SESSION_INACTIVITY_TIMEOUT_IN_SECONDS=1) - def test_inactive_session_timeout(self): - """ - Verify that an inactive session times out and redirects to the - login page - """ - # make sure we can access courseware immediately - resp = self.client.get(reverse('dashboard')) - assert resp.status_code == 200 - - # then wait a bit and see if we get timed out - time.sleep(2) - - resp = self.client.get(reverse('dashboard')) - - # re-request, and we should get a redirect to login page - self.assertRedirects(resp, settings.LOGIN_REDIRECT_URL + '?next=' + reverse('dashboard')) - - def test_redirects_first_time(self): - """ - Verify that the first time we click on the courseware tab we are - redirected to the 'Welcome' section. - """ - resp = self.client.get(reverse('courseware', - kwargs={'course_id': str(self.course.id)})) - self.assertRedirects(resp, reverse( - 'courseware_section', kwargs={'course_id': str(self.course.id), - 'chapter': 'Overview', - 'section': 'Welcome'})) - - def test_redirects_second_time(self): - """ - Verify the accordion remembers we've already visited the Welcome section - and redirects correspondingly. - """ - section_url = reverse( - 'courseware_section', - kwargs={ - 'course_id': str(self.course.id), - 'chapter': 'Overview', - 'section': 'Welcome', - }, - ) - self.client.get(section_url) - resp = self.client.get( - reverse('courseware', kwargs={'course_id': str(self.course.id)}), - ) - self.assertRedirects(resp, section_url) - - def test_accordion_state(self): - """ - Verify the accordion remembers which chapter you were last viewing. - """ - # Now we directly navigate to a section in a chapter other than 'Overview'. - section_url = reverse( - 'courseware_section', - kwargs={ - 'course_id': str(self.course.id), - 'chapter': 'factory_chapter', - 'section': 'factory_section', - } - ) - self.assert_request_status_code(200, section_url) - - # And now hitting the courseware tab should redirect to 'factory_chapter' - url = reverse( - 'courseware', - kwargs={'course_id': str(self.course.id)} - ) - resp = self.client.get(url) - self.assertRedirects(resp, section_url) - - # TODO: LEARNER-71: Do we need to adjust or remove this test? - @override_waffle_flag(DISABLE_COURSE_OUTLINE_PAGE_FLAG, active=True) - def test_incomplete_course(self): - test_course_id = str(self.test_course.id) - - url = reverse( - 'courseware', - kwargs={'course_id': test_course_id} - ) - response = self.assert_request_status_code(200, url) - self.assertContains(response, "No content has been added to this course") - - section = BlockFactory.create( - parent_location=self.test_course.location, - display_name='New Section' - ) - url = reverse( - 'courseware', - kwargs={'course_id': test_course_id} - ) - response = self.assert_request_status_code(200, url) - self.assertNotContains(response, "No content has been added to this course") - self.assertContains(response, "New Section") - - subsection = BlockFactory.create( - parent_location=section.location, - display_name='New Subsection', - ) - url = reverse( - 'courseware', - kwargs={'course_id': test_course_id} - ) - response = self.assert_request_status_code(200, url) - self.assertContains(response, "New Subsection") - self.assertNotContains(response, "sequence-nav") - - BlockFactory.create( - parent_location=subsection.location, - display_name='New Unit', - ) - url = reverse( - 'courseware', - kwargs={'course_id': test_course_id} - ) - self.assert_request_status_code(302, url) - - def test_proctoring_js_includes(self): - """ - Make sure that proctoring JS does not get included on - courseware pages if either the FEATURE flag is turned off - or the course is not proctored enabled - """ - test_course_id = str(self.test_course_proctored.id) - - with patch.dict(settings.FEATURES, {'ENABLE_SPECIAL_EXAMS': False}): - url = reverse( - 'courseware', - kwargs={'course_id': test_course_id} - ) - resp = self.client.get(url) - - self.assertNotContains(resp, '/static/js/lms-proctoring.js') - - with patch.dict(settings.FEATURES, {'ENABLE_SPECIAL_EXAMS': True}): - url = reverse( - 'courseware', - kwargs={'course_id': test_course_id} - ) - resp = self.client.get(url) - - self.assertNotContains(resp, '/static/js/lms-proctoring.js') - - # now set up a course which is proctored enabled - - self.test_course_proctored.enable_proctored_exams = True - self.test_course_proctored.save() - - modulestore().update_item(self.test_course_proctored, self.user.id) - - resp = self.client.get(url) - - self.assertContains(resp, '/static/js/lms-proctoring.js') diff --git a/lms/djangoapps/courseware/tests/test_view_authentication.py b/lms/djangoapps/courseware/tests/test_view_authentication.py index 67ada18e85..f9668853b2 100644 --- a/lms/djangoapps/courseware/tests/test_view_authentication.py +++ b/lms/djangoapps/courseware/tests/test_view_authentication.py @@ -63,7 +63,7 @@ class TestViewAuth(EnterpriseTestConsentRequired, ModuleStoreTestCase, LoginEnro Check that non-staff don't have access to dark urls. """ - names = ['courseware', 'progress'] + names = ['progress'] urls = self._reverse_urls(names, course) urls.extend([ reverse('book', kwargs={'course_id': str(course.id), @@ -106,10 +106,6 @@ class TestViewAuth(EnterpriseTestConsentRequired, ModuleStoreTestCase, LoginEnro ) self.assert_request_status_code(302, url) - # The courseware url should redirect, not 200 - url = self._reverse_urls(['courseware'], course)[0] - self.assert_request_status_code(302, url) - def login(self, user): # lint-amnesty, pylint: disable=arguments-differ return super().login(user.email, self.TEST_PASSWORD) @@ -164,60 +160,6 @@ class TestViewAuth(EnterpriseTestConsentRequired, ModuleStoreTestCase, LoginEnro self.org_staff_user = OrgStaffFactory(course_key=self.course.id) self.org_instructor_user = OrgInstructorFactory(course_key=self.course.id) - def test_redirection_unenrolled(self): - """ - Verify unenrolled student is redirected to the 'about' section of the chapter - instead of the 'Welcome' section after clicking on the courseware tab. - """ - self.login(self.unenrolled_user) - response = self.client.get(reverse('courseware', - kwargs={'course_id': str(self.course.id)})) - self.assertRedirects( - response, - reverse( - 'about_course', - args=[str(self.course.id)] - ) - ) - - def test_redirection_enrolled(self): - """ - Verify enrolled student is redirected to the 'Welcome' section of - the chapter after clicking on the courseware tab. - """ - self.login(self.enrolled_user) - - response = self.client.get( - reverse( - 'courseware', - kwargs={'course_id': str(self.course.id)} - ) - ) - - self.assertRedirects( - response, - reverse( - 'courseware_section', - kwargs={'course_id': str(self.course.id), - 'chapter': self.overview_chapter.url_name, - 'section': self.welcome_section.url_name} - ), - fetch_redirect_response=False, # just sends us on to MFE - ) - - def test_redirection_missing_enterprise_consent(self): - """ - Verify that enrolled students are redirected to the Enterprise consent - URL if a linked Enterprise Customer requires data sharing consent - and it has not yet been provided. - """ - self.login(self.enrolled_user) - url = reverse( - 'courseware', - kwargs={'course_id': str(self.course.id)} - ) - self.verify_consent_required(self.client, url, status_code=302) # lint-amnesty, pylint: disable=no-value-for-parameter - def test_instructor_page_access_nonstaff(self): """ Verify non-staff cannot load the instructor diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index 8cf7737f1b..8575b192a8 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -109,6 +109,7 @@ from openedx.features.course_experience import ( from openedx.features.course_experience.tests.views.helpers import add_course_mode from openedx.features.course_experience.url_helpers import ( get_learning_mfe_home_url, + get_courseware_url, make_learning_mfe_courseware_url ) from openedx.features.enterprise_support.tests.factories import ( @@ -248,42 +249,6 @@ class TestJumpTo(ModuleStoreTestCase): assert response.status_code == 404 -class IndexQueryTestCase(ModuleStoreTestCase): - """ - Tests for query count. - """ - NUM_PROBLEMS = 20 - - @patch('lms.djangoapps.courseware.views.index.CoursewareIndex._redirect_to_learning_mfe', return_value=None) - def test_index_query_counts(self, mock_redirect): - # TODO: decrease query count as part of REVO-28 - ContentTypeGatingConfig.objects.create(enabled=True, enabled_as_of=datetime(2018, 1, 1)) - with self.store.default_store(ModuleStoreEnum.Type.split): - course = CourseFactory.create() - with self.store.bulk_operations(course.id): - chapter = BlockFactory.create(category='chapter', parent_location=course.location) - section = BlockFactory.create(category='sequential', parent_location=chapter.location) - vertical = BlockFactory.create(category='vertical', parent_location=section.location) - for _ in range(self.NUM_PROBLEMS): - BlockFactory.create(category='problem', parent_location=vertical.location) - - self.client.login(username=self.user.username, password=self.user_password) - CourseEnrollment.enroll(self.user, course.id) - - with self.assertNumQueries(152, table_ignorelist=QUERY_COUNT_TABLE_IGNORELIST): - with check_mongo_calls(3): - url = reverse( - 'courseware_section', - kwargs={ - 'course_id': str(course.id), - 'chapter': str(chapter.location.block_id), - 'section': str(section.location.block_id), - } - ) - response = self.client.get(url) - assert response.status_code == 200 - - class BaseViewsTestCase(ModuleStoreTestCase, MasqueradeMixin): """Base class for courseware tests""" CREATE_USER = False @@ -357,108 +322,6 @@ class BaseViewsTestCase(ModuleStoreTestCase, MasqueradeMixin): assert self.client.login(username=self.global_staff.username, password=TEST_PASSWORD) -@ddt.ddt -class CoursewareIndexTestCase(BaseViewsTestCase): - """ - Tests for the courseware index view, used for instructor previews. - """ - def setUp(self): - super().setUp() - self._create_global_staff_user() # this view needs staff permission - - @patch('lms.djangoapps.courseware.views.index.CoursewareIndex._redirect_to_learning_mfe', return_value=None) - def test_index_success(self, mock_redirect): - response = self._verify_index_response() - self.assertContains(response, self.problem2.location.replace(branch=None, version_guid=None)) - - # re-access to the main course page redirects to last accessed view. - url = reverse('courseware', kwargs={'course_id': str(self.course_key)}) - response = self.client.get(url) - assert response.status_code == 302 - response = self.client.get(response.url) - self.assertNotContains(response, self.problem.location.replace(branch=None, version_guid=None)) - self.assertContains(response, self.problem2.location.replace(branch=None, version_guid=None)) - - def test_index_nonexistent_chapter(self): - self._verify_index_response(expected_response_code=404, chapter_name='non-existent') - - def test_index_nonexistent_chapter_masquerade(self): - self.update_masquerade(username=self.user.username) - self._verify_index_response(expected_response_code=302, chapter_name='non-existent') - - def test_index_nonexistent_section(self): - self._verify_index_response(expected_response_code=404, section_name='non-existent') - - def test_index_nonexistent_section_masquerade(self): - self.update_masquerade(username=self.user.username) - self._verify_index_response(expected_response_code=302, section_name='non-existent') - - def _verify_index_response(self, expected_response_code=200, chapter_name=None, section_name=None): - """ - Verifies the response when the courseware index page is accessed with - the given chapter and section names. - """ - url = reverse( - 'courseware_section', - kwargs={ - 'course_id': str(self.course_key), - 'chapter': str(self.chapter.location.block_id) if chapter_name is None else chapter_name, - 'section': str(self.section2.location.block_id) if section_name is None else section_name, - } - ) - response = self.client.get(url) - assert response.status_code == expected_response_code - return response - - def test_index_invalid_position(self): - request_url = '/'.join([ - '/courses', - str(self.course.id), - 'courseware', - self.chapter.location.block_id, - self.section.location.block_id, - 'f' - ]) - response = self.client.get(request_url) - assert response.status_code == 404 - - def test_unicode_handling_in_url(self): - url_parts = [ - '/courses', - str(self.course.id), - 'courseware', - self.chapter.location.block_id, - self.section.location.block_id, - '1' - ] - for idx, val in enumerate(url_parts): - url_parts_copy = url_parts[:] - url_parts_copy[idx] = val + 'χ' - request_url = '/'.join(url_parts_copy) - response = self.client.get(request_url) - assert response.status_code == 404 - - # TODO: TNL-6387: Remove test - @override_waffle_flag(DISABLE_COURSE_OUTLINE_PAGE_FLAG, active=True) - @patch('lms.djangoapps.courseware.views.index.CoursewareIndex._redirect_to_learning_mfe', return_value=None) - def test_accordion(self, mock_redirect): - """ - This needs a response_context, which is not included in the render_accordion's main method - returning a render_to_string, so we will render via the courseware URL in order to include - the needed context - """ - response = self.client.get( - reverse('courseware', args=[str(self.course.id)]), - follow=True - ) - test_responses = [ - '

Sequential 1 current section

', - '

Sequential 2

' - ] - for test in test_responses: - self.assertContains(response, test) - - @ddt.ddt class ViewsTestCase(BaseViewsTestCase): """ @@ -1046,56 +909,6 @@ class TestProgressDueDate(BaseDueDateTests): return self.client.get(reverse('progress', args=[str(course.id)])) -# TODO: LEARNER-71: Delete entire TestAccordionDueDate class -class TestAccordionDueDate(BaseDueDateTests): - """ - Test that the accordion page displays due dates correctly - """ - __test__ = True - - def setUp(self): - super().setUp() - self.patcher = patch( - 'lms.djangoapps.courseware.views.index.CoursewareIndex._redirect_to_learning_mfe', return_value=None) - self.mock_redirect = self.patcher.start() - - def tearDown(self): - self.patcher.stop() - super().tearDown() - - def get_response(self, course): - """ Returns the HTML for the accordion """ - return self.client.get( - reverse('courseware', args=[str(course.id)]), - follow=True - ) - - # TODO: LEARNER-71: Delete entire TestAccordionDueDate class - @override_waffle_flag(DISABLE_COURSE_OUTLINE_PAGE_FLAG, active=True) - def test_backwards_compatibility(self): - super().test_backwards_compatibility() - - # TODO: LEARNER-71: Delete entire TestAccordionDueDate class - @override_waffle_flag(DISABLE_COURSE_OUTLINE_PAGE_FLAG, active=True) - def test_defaults(self): - super().test_defaults() - - # TODO: LEARNER-71: Delete entire TestAccordionDueDate class - @override_waffle_flag(DISABLE_COURSE_OUTLINE_PAGE_FLAG, active=True) - def test_format_date(self): - super().test_format_date() - - # TODO: LEARNER-71: Delete entire TestAccordionDueDate class - @override_waffle_flag(DISABLE_COURSE_OUTLINE_PAGE_FLAG, active=True) - def test_format_invalid(self): - super().test_format_invalid() - - # TODO: LEARNER-71: Delete entire TestAccordionDueDate class - @override_waffle_flag(DISABLE_COURSE_OUTLINE_PAGE_FLAG, active=True) - def test_format_none(self): - super().test_format_none() - - class StartDateTests(ModuleStoreTestCase): """ Test that start dates are properly localized and displayed on the student @@ -2275,72 +2088,6 @@ class TestIndexView(ModuleStoreTestCase): """ Tests of the courseware.views.index view. """ - @XBlock.register_temp_plugin(ViewCheckerBlock, 'view_checker') - @patch('lms.djangoapps.courseware.views.index.CoursewareIndex._redirect_to_learning_mfe', return_value=None) - def test_student_state(self, mock_redirect): - """ - Verify that saved student state is loaded for xblocks rendered in the index view. - """ - with modulestore().default_store(ModuleStoreEnum.Type.split): - course = CourseFactory.create() - chapter = BlockFactory.create(parent_location=course.location, category='chapter') - section = BlockFactory.create(parent_location=chapter.location, category='view_checker', - display_name="Sequence Checker") - vertical = BlockFactory.create(parent_location=section.location, category='view_checker', - display_name="Vertical Checker") - block = BlockFactory.create(parent_location=vertical.location, category='view_checker', - display_name="Block Checker") - - for item in (section, vertical, block): - StudentModuleFactory.create( - student=self.user, - course_id=course.id, - module_state_key=item.scope_ids.usage_id, - state=json.dumps({'state': str(item.scope_ids.usage_id)}) - ) - - CourseOverview.load_from_module_store(course.id) - CourseEnrollmentFactory(user=self.user, course_id=course.id) - - assert self.client.login(username=self.user.username, password=self.user_password) - response = self.client.get( - reverse( - 'courseware_section', - kwargs={ - 'course_id': str(course.id), - 'chapter': chapter.url_name, - 'section': section.url_name, - } - ) - ) - # Trigger the assertions embedded in the ViewCheckerBlocks - self.assertContains(response, "ViewCheckerPassed", count=3) - - @XBlock.register_temp_plugin(ActivateIDCheckerBlock, 'id_checker') - @patch('lms.djangoapps.courseware.views.index.CoursewareIndex._redirect_to_learning_mfe', return_value=None) - def test_activate_block_id(self, mock_redirect): - course = CourseFactory.create() - with self.store.bulk_operations(course.id): - chapter = BlockFactory.create(parent=course, category='chapter') - section = BlockFactory.create(parent=chapter, category='sequential', display_name="Sequence") - vertical = BlockFactory.create(parent=section, category='vertical', display_name="Vertical") - BlockFactory.create(parent=vertical, category='id_checker', display_name="ID Checker") - - CourseOverview.load_from_module_store(course.id) - CourseEnrollmentFactory(user=self.user, course_id=course.id) - - assert self.client.login(username=self.user.username, password=self.user_password) - response = self.client.get( - reverse( - 'courseware_section', - kwargs={ - 'course_id': str(course.id), - 'chapter': chapter.url_name, - 'section': section.url_name, - } - ) + '?activate_block_id=test_block_id' - ) - self.assertContains(response, "Activate Block ID: test_block_id") @patch('lms.djangoapps.courseware.views.views.CourseTabView.course_open_for_learner_enrollment') @patch('openedx.core.djangoapps.util.user_messages.PageLevelMessages.register_warning_message') @@ -2425,207 +2172,6 @@ class TestIndexView(ModuleStoreTestCase): assert views.CourseTabView.course_open_for_learner_enrollment(course) == expected_should_show_enroll_button -@ddt.ddt -class TestIndexViewCompleteOnView(ModuleStoreTestCase, CompletionWaffleTestMixin): - """ - Tests CompleteOnView is set up correctly in CoursewareIndex. - """ - - def setup_course(self, default_store): - """ - Set up course content for modulestore. - """ - # pylint:disable=attribute-defined-outside-init - - self.request_factory = RequestFactoryNoCsrf() - - with modulestore().default_store(default_store): - self.course = CourseFactory.create() - - with self.store.bulk_operations(self.course.id): - - self.chapter = BlockFactory.create( - parent_location=self.course.location, category='chapter', display_name='Week 1' - ) - self.section_1 = BlockFactory.create( - parent_location=self.chapter.location, category='sequential', display_name='Lesson 1' - ) - self.vertical_1 = BlockFactory.create( - parent_location=self.section_1.location, category='vertical', display_name='Subsection 1' - ) - self.html_1_1 = BlockFactory.create( - parent_location=self.vertical_1.location, category='html', display_name="HTML 1_1" - ) - self.problem_1 = BlockFactory.create( - parent_location=self.vertical_1.location, category='problem', display_name="Problem 1" - ) - self.html_1_2 = BlockFactory.create( - parent_location=self.vertical_1.location, category='html', display_name="HTML 1_2" - ) - - self.section_2 = BlockFactory.create( - parent_location=self.chapter.location, category='sequential', display_name='Lesson 2' - ) - self.vertical_2 = BlockFactory.create( - parent_location=self.section_2.location, category='vertical', display_name='Subsection 2' - ) - self.video_2 = BlockFactory.create( - parent_location=self.vertical_2.location, category='video', display_name="Video 2" - ) - self.problem_2 = BlockFactory.create( - parent_location=self.vertical_2.location, category='problem', display_name="Problem 2" - ) - - self.section_1_url = reverse( - 'courseware_section', - kwargs={ - 'course_id': str(self.course.id), - 'chapter': self.chapter.url_name, - 'section': self.section_1.url_name, - } - ) - - self.section_2_url = reverse( - 'courseware_section', - kwargs={ - 'course_id': str(self.course.id), - 'chapter': self.chapter.url_name, - 'section': self.section_2.url_name, - } - ) - - CourseOverview.load_from_module_store(self.course.id) - CourseEnrollmentFactory(user=self.user, course_id=self.course.id) - assert self.client.login(username=self.user.username, password=self.user_password) - - @patch('lms.djangoapps.courseware.views.index.CoursewareIndex._redirect_to_learning_mfe', return_value=None) - def test_completion_service_disabled(self, mock_redirect): - - self.setup_course(ModuleStoreEnum.Type.split) - - response = self.client.get(self.section_1_url) - self.assertNotContains(response, 'data-mark-completed-on-view-after-delay') - - response = self.client.get(self.section_2_url) - self.assertNotContains(response, 'data-mark-completed-on-view-after-delay') - - @patch('lms.djangoapps.courseware.views.index.CoursewareIndex._redirect_to_learning_mfe', return_value=None) - def test_completion_service_enabled(self, mock_redirect): - - self.override_waffle_switch(True) - - self.setup_course(ModuleStoreEnum.Type.split) - - response = self.client.get(self.section_1_url) - self.assertContains(response, 'data-mark-completed-on-view-after-delay') - self.assertContains(response, 'data-mark-completed-on-view-after-delay', count=2) - - request = self.request_factory.post( - '/', - data=json.dumps({"completion": 1}), - content_type='application/json', - ) - request.user = self.user - request.session = {} - response = handle_xblock_callback( - request, - str(self.course.id), - quote_slashes(str(self.html_1_1.scope_ids.usage_id)), - 'publish_completion', - ) - assert json.loads(response.content.decode('utf-8')) == {'result': 'ok'} - - response = self.client.get(self.section_1_url) - self.assertContains(response, 'data-mark-completed-on-view-after-delay') - self.assertContains(response, 'data-mark-completed-on-view-after-delay', count=1) - - request = self.request_factory.post( - '/', - data=json.dumps({"completion": 1}), - content_type='application/json', - ) - request.user = self.user - request.session = {} - response = handle_xblock_callback( - request, - str(self.course.id), - quote_slashes(str(self.html_1_2.scope_ids.usage_id)), - 'publish_completion', - ) - assert json.loads(response.content.decode('utf-8')) == {'result': 'ok'} - - response = self.client.get(self.section_1_url) - self.assertNotContains(response, 'data-mark-completed-on-view-after-delay') - - response = self.client.get(self.section_2_url) - self.assertNotContains(response, 'data-mark-completed-on-view-after-delay') - - -@ddt.ddt -class TestIndexViewWithVerticalPositions(ModuleStoreTestCase): - """ - Test the index view to handle vertical positions. Confirms that first position is loaded - if input position is non-positive or greater than number of positions available. - """ - - def setUp(self): - """ - Set up initial test data - """ - super().setUp() - - # create course with 3 positions - self.course = CourseFactory.create() - with self.store.bulk_operations(self.course.id): - self.chapter = BlockFactory.create(parent_location=self.course.location, category='chapter') - self.section = BlockFactory.create(parent_location=self.chapter.location, category='sequential', - display_name="Sequence") - BlockFactory.create(parent_location=self.section.location, category='vertical', display_name="Vertical1") - BlockFactory.create(parent_location=self.section.location, category='vertical', display_name="Vertical2") - BlockFactory.create(parent_location=self.section.location, category='vertical', display_name="Vertical3") - - CourseOverview.load_from_module_store(self.course.id) - - self.client.login(username=self.user, password=self.user_password) - CourseEnrollmentFactory(user=self.user, course_id=self.course.id) - - def _get_course_vertical_by_position(self, input_position): - """ - Returns client response to input position. - """ - return self.client.get( - reverse( - 'courseware_position', - kwargs={ - 'course_id': str(self.course.id), - 'chapter': self.chapter.url_name, - 'section': self.section.url_name, - 'position': input_position, - } - ) - ) - - def _assert_correct_position(self, response, expected_position): - """ - Asserts that the expected position and the position in the response are the same - """ - self.assertContains(response, f'data-position="{expected_position}"') - - @ddt.data(("-1", 1), ("0", 1), ("-0", 1), ("2", 2), ("5", 1)) - @ddt.unpack - @patch('lms.djangoapps.courseware.views.index.CoursewareIndex._redirect_to_learning_mfe', return_value=None) - def test_vertical_positions(self, input_position, expected_position, mock_redirect): - """ - Tests the following cases: - * Load first position when negative position inputted. - * Load first position when 0/-0 position inputted. - * Load given position when 0 < input_position <= num_positions_available. - * Load first position when positive position > num_positions_available. - """ - resp = self._get_course_vertical_by_position(input_position) - self._assert_correct_position(resp, expected_position) - - @ddt.ddt class TestRenderXBlock(RenderXBlockTestMixin, ModuleStoreTestCase, CompletionWaffleTestMixin): """ @@ -3007,89 +2553,6 @@ class TestRenderXBlockSelfPaced(TestRenderXBlock): # lint-amnesty, pylint: disa return options -class TestIndexViewCrawlerStudentStateWrites(SharedModuleStoreTestCase): - """ - Ensure that courseware index requests do not trigger student state writes. - This is to prevent locking issues that have caused latency spikes in the - courseware_studentmodule table when concurrent requests each try to update - the same rows for sequence, section, and course positions. - """ - @classmethod - def setUpClass(cls): - """Set up the simplest course possible.""" - # setUpClassAndTestData() already calls setUpClass on SharedModuleStoreTestCase - # pylint: disable=super-method-not-called - with super().setUpClassAndTestData(): - cls.course = CourseFactory.create() - with cls.store.bulk_operations(cls.course.id): - cls.chapter = BlockFactory.create(category='chapter', parent_location=cls.course.location) - cls.section = BlockFactory.create(category='sequential', parent_location=cls.chapter.location) - cls.vertical = BlockFactory.create(category='vertical', parent_location=cls.section.location) - - @classmethod - def setUpTestData(cls): # lint-amnesty, pylint: disable=super-method-not-called - """Set up and enroll our fake user in the course.""" - cls.user = UserFactory(is_staff=True) - CourseEnrollment.enroll(cls.user, cls.course.id) - - def setUp(self): - """Do the client login.""" - super().setUp() - self.client.login(username=self.user.username, password=TEST_PASSWORD) - - @patch('lms.djangoapps.courseware.views.index.CoursewareIndex._redirect_to_learning_mfe', return_value=None) - def test_write_by_default(self, mock_redirect): - """By default, always write student state, regardless of user agent.""" - with patch('lms.djangoapps.courseware.model_data.UserStateCache.set_many') as patched_state_client_set_many: - # Simulate someone using Chrome - self._load_courseware('Mozilla/5.0 AppleWebKit/537.36') - assert patched_state_client_set_many.called - patched_state_client_set_many.reset_mock() - - # Common crawler user agent - self._load_courseware('edX-downloader/0.1') - assert patched_state_client_set_many.called - - @patch('lms.djangoapps.courseware.views.index.CoursewareIndex._redirect_to_learning_mfe', return_value=None) - def test_writes_with_config(self, mock_redirect): - """Test state writes (or lack thereof) based on config values.""" - CrawlersConfig.objects.create(known_user_agents='edX-downloader,crawler_foo', enabled=True) - with patch('lms.djangoapps.courseware.model_data.UserStateCache.set_many') as patched_state_client_set_many: - # Exact matching of crawler user agent - self._load_courseware('crawler_foo') - assert not patched_state_client_set_many.called - - # Partial matching of crawler user agent - self._load_courseware('edX-downloader/0.1') - assert not patched_state_client_set_many.called - - # Simulate an actual browser hitting it (we should write) - self._load_courseware('Mozilla/5.0 AppleWebKit/537.36') - assert patched_state_client_set_many.called - - # Disabling the crawlers config should revert us to default behavior - CrawlersConfig.objects.create(enabled=False) - - # Disabling the violation because pylint just can't see that we'll get the mock_redirect param passed in via the - # patch. - self.test_write_by_default() # pylint: disable=no-value-for-parameter - - def _load_courseware(self, user_agent): - """Helper to load the actual courseware page.""" - url = reverse( - 'courseware_section', - kwargs={ - 'course_id': str(self.course.id), - 'chapter': str(self.chapter.location.block_id), - 'section': str(self.section.location.block_id), - } - ) - response = self.client.get(url, HTTP_USER_AGENT=user_agent) - # Make sure we get back an actual 200, and aren't redirected because we - # messed up the setup somehow (e.g. didn't enroll properly) - assert response.status_code == 200 - - class EnterpriseConsentTestCase(EnterpriseTestConsentRequired, ModuleStoreTestCase): """ Ensure that the Enterprise Data Consent redirects are in place only when consent is required. @@ -3112,7 +2575,6 @@ class EnterpriseConsentTestCase(EnterpriseTestConsentRequired, ModuleStoreTestCa course_id = str(self.course.id) for url in ( - reverse("courseware", kwargs=dict(course_id=course_id)), reverse("progress", kwargs=dict(course_id=course_id)), reverse("student_progress", kwargs=dict(course_id=course_id, student_id=str(self.user.id))), ): @@ -3343,7 +2805,6 @@ class TestCourseWideResources(ModuleStoreTestCase): """ @ddt.data( - ('courseware', 'course_id', False, True), ('progress', 'course_id', False, False), ('instructor_dashboard', 'course_id', True, False), ('forum_form_discussion', 'course_id', False, False), From 2fc068220d62bd9125ad0bcc54e81ea3b93ce139 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Thu, 22 May 2025 13:44:59 -0400 Subject: [PATCH 4/7] style: Fix various linting issues. --- lms/djangoapps/courseware/tests/test_views.py | 6 ------ lms/djangoapps/courseware/testutils.py | 1 - openedx/features/course_experience/url_helpers.py | 5 ++--- 3 files changed, 2 insertions(+), 10 deletions(-) diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index 8575b192a8..da5b178136 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -73,7 +73,6 @@ from lms.djangoapps.commerce.utils import EcommerceService from lms.djangoapps.courseware.access_utils import check_course_open_for_learner from lms.djangoapps.courseware.model_data import FieldDataCache, set_score from lms.djangoapps.courseware.block_render import get_block, handle_xblock_callback -from lms.djangoapps.courseware.tests.factories import StudentModuleFactory from lms.djangoapps.courseware.tests.helpers import MasqueradeMixin, get_expiration_banner_text from lms.djangoapps.courseware.testutils import RenderXBlockTestMixin from lms.djangoapps.courseware.toggles import ( @@ -94,7 +93,6 @@ from lms.djangoapps.verify_student.services import IDVerificationService from openedx.core.djangoapps.catalog.tests.factories import CourseFactory as CatalogCourseFactory from openedx.core.djangoapps.catalog.tests.factories import CourseRunFactory, ProgramFactory from openedx.core.djangoapps.content.course_overviews.models import CourseOverview -from openedx.core.djangoapps.crawlers.models import CrawlersConfig from openedx.core.djangoapps.credit.api import set_credit_requirements from openedx.core.djangoapps.credit.models import CreditCourse, CreditProvider from openedx.core.djangoapps.waffle_utils.testutils import WAFFLE_TABLES @@ -103,13 +101,9 @@ from openedx.core.djangoapps.video_config.toggles import PUBLIC_VIDEO_SHARE from openedx.core.lib.url_utils import quote_slashes from openedx.features.content_type_gating.models import ContentTypeGatingConfig from openedx.features.course_duration_limits.models import CourseDurationLimitConfig -from openedx.features.course_experience import ( - DISABLE_COURSE_OUTLINE_PAGE_FLAG, -) from openedx.features.course_experience.tests.views.helpers import add_course_mode from openedx.features.course_experience.url_helpers import ( get_learning_mfe_home_url, - get_courseware_url, make_learning_mfe_courseware_url ) from openedx.features.enterprise_support.tests.factories import ( diff --git a/lms/djangoapps/courseware/testutils.py b/lms/djangoapps/courseware/testutils.py index 8730e29e38..5f47b258ae 100644 --- a/lms/djangoapps/courseware/testutils.py +++ b/lms/djangoapps/courseware/testutils.py @@ -16,7 +16,6 @@ from common.djangoapps.student.tests.factories import AdminFactory, CourseEnroll from common.djangoapps.course_modes.models import CourseMode from common.djangoapps.course_modes.tests.factories import CourseModeFactory -from xmodule.modulestore import ModuleStoreEnum # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.tests.factories import CourseFactory, BlockFactory, check_mongo_calls # lint-amnesty, pylint: disable=wrong-import-order diff --git a/openedx/features/course_experience/url_helpers.py b/openedx/features/course_experience/url_helpers.py index dca139d6f1..44cfdb4aa1 100644 --- a/openedx/features/course_experience/url_helpers.py +++ b/openedx/features/course_experience/url_helpers.py @@ -10,13 +10,12 @@ from django.conf import settings from django.contrib.auth import get_user_model from django.http import HttpRequest from django.http.request import QueryDict -from django.urls import reverse from opaque_keys.edx.keys import CourseKey, UsageKey -from six.moves.urllib.parse import urlencode, urlparse +from urllib.parse import urlparse from xmodule.modulestore import ModuleStoreEnum # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order -from xmodule.modulestore.search import navigation_index, path_to_location # lint-amnesty, pylint: disable=wrong-import-order +from xmodule.modulestore.search import path_to_location # lint-amnesty, pylint: disable=wrong-import-order User = get_user_model() From 562df7386cea74ba4c1f7072a8b63fd930311fcc Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Fri, 23 May 2025 16:26:05 -0400 Subject: [PATCH 5/7] feat: Make CoursewareIndex just a redirect. We don't need to load the old UI and so don't need all the logic related to it, just the logic that is expected to occur around other backend functionality like masquerading. --- .../courseware/tests/test_course_survey.py | 47 +- lms/djangoapps/courseware/tests/test_views.py | 52 ++ lms/djangoapps/courseware/views/index.py | 527 ++---------------- lms/templates/courseware/courseware.html | 318 ----------- 4 files changed, 136 insertions(+), 808 deletions(-) delete mode 100644 lms/templates/courseware/courseware.html diff --git a/lms/djangoapps/courseware/tests/test_course_survey.py b/lms/djangoapps/courseware/tests/test_course_survey.py index 98b5842cd2..25dc37724d 100644 --- a/lms/djangoapps/courseware/tests/test_course_survey.py +++ b/lms/djangoapps/courseware/tests/test_course_survey.py @@ -16,6 +16,7 @@ from common.test.utils import XssTestMixin from lms.djangoapps.courseware.tests.helpers import LoginEnrollmentTestCase from lms.djangoapps.survey.models import SurveyAnswer, SurveyForm from openedx.features.course_experience import course_home_url +from openedx.features.course_experience.url_helpers import make_learning_mfe_courseware_url class SurveyViewsTests(LoginEnrollmentTestCase, SharedModuleStoreTestCase, XssTestMixin): @@ -89,26 +90,44 @@ class SurveyViewsTests(LoginEnrollmentTestCase, SharedModuleStoreTestCase, XssTe reverse('course_survey', kwargs={'course_id': str(course.id)}) ) - def _assert_no_redirect(self, course): + def _assert_no_survey_redirect(self, course): """ - Helper method to asswer that all known conditionally redirect points do - not redirect as expected + Helper method to assert that all known conditionally redirecting endpoints do + not redirect to the survey as expected """ - for view_name in ['courseware', 'progress']: - resp = self.client.get( - reverse( - view_name, - kwargs={'course_id': str(course.id)} - ) + + # Make sure we get to the progress page. + resp = self.client.get( + reverse( + 'progress', + kwargs={'course_id': str(course.id)} ) - assert resp.status_code == 200 + ) + assert resp.status_code == 200 + + # Make sure we are redirected to the MFE for courseware + resp = self.client.get( + reverse( + 'courseware', + kwargs={'course_id': str(course.id)} + ) + ) + assert resp.status_code == 302 + expected_redirect_url = make_learning_mfe_courseware_url( + course.id, + None, + None, + params=None, + preview=False + ) + assert resp.url == expected_redirect_url def test_visiting_course_without_survey(self): """ Verifies that going to the courseware which does not have a survey does not redirect to a survey """ - self._assert_no_redirect(self.course_without_survey) + self._assert_no_survey_redirect(self.course_without_survey) def test_visiting_course_with_survey_redirects(self): """ @@ -143,7 +162,7 @@ class SurveyViewsTests(LoginEnrollmentTestCase, SharedModuleStoreTestCase, XssTe ) assert resp.status_code == 200 - self._assert_no_redirect(self.course) + self._assert_no_survey_redirect(self.course) def test_course_id_field(self): """ @@ -180,7 +199,7 @@ class SurveyViewsTests(LoginEnrollmentTestCase, SharedModuleStoreTestCase, XssTe ) assert resp.status_code == 200 - self._assert_no_redirect(self.course) + self._assert_no_survey_redirect(self.course) # however we want to make sure we persist the course_id answer_objs = SurveyAnswer.objects.filter( @@ -195,7 +214,7 @@ class SurveyViewsTests(LoginEnrollmentTestCase, SharedModuleStoreTestCase, XssTe """ Verifies that going to the courseware with a required, but non-existing survey, does not redirect """ - self._assert_no_redirect(self.course_with_bogus_survey) + self._assert_no_survey_redirect(self.course_with_bogus_survey) def test_visiting_survey_with_bogus_survey_name(self): """ diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index da5b178136..4b59bfac24 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -316,6 +316,58 @@ class BaseViewsTestCase(ModuleStoreTestCase, MasqueradeMixin): assert self.client.login(username=self.global_staff.username, password=TEST_PASSWORD) +@ddt.ddt +class CoursewareIndexTestCase(BaseViewsTestCase): + """ + Tests for the courseware index view, used for instructor previews. + """ + def setUp(self): + super().setUp() + self._create_global_staff_user() # this view needs staff permission + + def test_course_redirect(self): + lms_url = reverse( + 'courseware', + kwargs={ + 'course_id': str(self.course_key), + } + ) + + mfe_url = make_learning_mfe_courseware_url(self.course.id) + + response = self.client.get(lms_url) + assert response.url == mfe_url + + def test_section_redirect(self): + lms_url = reverse( + 'courseware_section', + kwargs={ + 'course_id': str(self.course_key), + 'section': str(self.chapter.location.block_id), + } + ) + + mfe_url = make_learning_mfe_courseware_url(self.course.id) + + response = self.client.get(lms_url) + assert response.url == mfe_url + + def test_subsection_redirect(self): + lms_url = reverse( + 'courseware_subsection', + kwargs={ + 'course_id': str(self.course_key), + 'section': str(self.chapter.location.block_id), + 'subsection': str(self.section2.location.block_id), + } + ) + + mfe_url = make_learning_mfe_courseware_url(self.course.id, self.section2.location) + + response = self.client.get(lms_url) + assert response.url == mfe_url + + @ddt.ddt class ViewsTestCase(BaseViewsTestCase): """ diff --git a/lms/djangoapps/courseware/views/index.py b/lms/djangoapps/courseware/views/index.py index 02f0ef1f7f..e36d43b75b 100644 --- a/lms/djangoapps/courseware/views/index.py +++ b/lms/djangoapps/courseware/views/index.py @@ -6,72 +6,32 @@ View for Courseware Index import logging -import urllib -from django.conf import settings from django.contrib.auth.views import redirect_to_login -from django.db import transaction -from django.http import Http404 -from django.template.context_processors import csrf -from django.urls import reverse from django.utils.decorators import method_decorator -from django.utils.functional import cached_property -from django.utils.translation import gettext as _ from django.views.decorators.cache import cache_control from django.views.decorators.csrf import ensure_csrf_cookie +from django.utils.functional import cached_property from django.views.generic import View -from edx_django_utils.monitoring import set_custom_attributes_for_course_key + from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey, UsageKey -from web_fragments.fragment import Fragment -from xmodule.course_block import COURSE_VISIBILITY_PUBLIC from xmodule.modulestore.django import modulestore -from xmodule.x_module import PUBLIC_VIEW, STUDENT_VIEW -from common.djangoapps.edxmako.shortcuts import render_to_response, render_to_string -from common.djangoapps.student.models import CourseEnrollment from common.djangoapps.util.views import ensure_valid_course_key -from lms.djangoapps.courseware.exceptions import CourseAccessRedirect, Redirect -from lms.djangoapps.experiments.utils import get_experiment_user_metadata_context -from lms.djangoapps.gating.api import get_entrance_exam_score, get_entrance_exam_usage_key -from lms.djangoapps.grades.api import CourseGradeFactory -from openedx.core.djangoapps.content.course_overviews.models import CourseOverview -from openedx.core.djangoapps.crawlers.models import CrawlersConfig -from openedx.core.djangoapps.lang_pref import LANGUAGE_KEY -from openedx.core.djangoapps.user_api.preferences.api import get_user_preference -from openedx.core.djangoapps.util.user_messages import PageLevelMessages -from openedx.core.djangolib.markup import HTML, Text -from openedx.features.course_experience import ( - COURSE_ENABLE_UNENROLLED_ACCESS_FLAG, - DISABLE_COURSE_OUTLINE_PAGE_FLAG, - default_course_url -) +from lms.djangoapps.courseware.exceptions import Redirect +from lms.djangoapps.courseware.masquerade import setup_masquerade from openedx.features.course_experience.url_helpers import make_learning_mfe_courseware_url +from openedx.features.course_experience import COURSE_ENABLE_UNENROLLED_ACCESS_FLAG from openedx.features.enterprise_support.api import data_sharing_consent_required -from ..access import has_access -from ..access_utils import check_public_access -from ..courses import get_course_with_access, get_current_child, get_studio_url -from ..entrance_exams import ( - course_has_entrance_exam, - get_entrance_exam_content, - user_can_skip_entrance_exam, - user_has_passed_entrance_exam -) -from ..masquerade import check_content_start_date_for_masquerade_user, setup_masquerade -from ..model_data import FieldDataCache -from ..block_render import get_block_for_descriptor, toc_for_course +from ..block_render import get_block_for_descriptor +from ..courses import get_course_with_access from ..permissions import MASQUERADE_AS_STUDENT -from ..toggles import ENABLE_OPTIMIZELY_IN_COURSEWARE -from .views import CourseTabView log = logging.getLogger("edx.courseware.views.index") -TEMPLATE_IMPORTS = {'urllib': urllib} -CONTENT_DEPTH = 2 - -@method_decorator(transaction.non_atomic_requests, name='dispatch') class CoursewareIndex(View): """ View class for the Courseware page. @@ -87,15 +47,9 @@ class CoursewareIndex(View): @method_decorator(data_sharing_consent_required) def get(self, request, course_id, chapter=None, section=None, position=None): """ - Displays courseware accordion and associated content. If course, chapter, - and section are all specified, renders the page, or returns an error if they - are invalid. - - If section is not specified, displays the accordion opened to the right - chapter. - - If neither chapter or section are specified, displays the user's most - recent chapter, or the first chapter if this is the user's first visit. + Instead of loading the legacy courseware sequences pages, load the equivalent URL + in the learning MFE. This view does not do any auth checks since they are done by + the MFE when attempting to load content. Arguments: request: HTTP request @@ -103,442 +57,63 @@ class CoursewareIndex(View): chapter (unicode): chapter url_name section (unicode): section url_name position (unicode): position in block, eg of block + """ + self.course_key = CourseKey.from_string(course_id) if not (request.user.is_authenticated or self.enable_unenrolled_access): return redirect_to_login(request.get_full_path()) - self.original_chapter_url_name = chapter - self.original_section_url_name = section - self.chapter_url_name = chapter - self.section_url_name = section - self.position = position - self.chapter, self.section = None, None - self.course = None - self.url = request.path + # Course load to resolve chapters/sections + with modulestore().bulk_operations(self.course_key): + course = get_course_with_access( + request.user, + "load", + self.course_key, + depth=2, + check_if_enrolled=True, + check_if_authenticated=True, + ) + + # Get the chapter, section and unit blocks so that we can redirect to the right content + # location in the MFE + section_location = None + if chapter and section: + chapter_block = course.get_child_by(lambda m: m.location.block_id == chapter) + if chapter_block: + section_block = chapter_block.get_child_by(lambda m: m.location.block_id == section) + if section_block: + section_location = section_block.location try: - set_custom_attributes_for_course_key(self.course_key) - self._clean_position() - with modulestore().bulk_operations(self.course_key): - - self.view = STUDENT_VIEW - - self.course = get_course_with_access( - request.user, 'load', self.course_key, - depth=CONTENT_DEPTH, - check_if_enrolled=True, - check_if_authenticated=True - ) - self.course_overview = CourseOverview.get_from_id(self.course.id) - self.is_staff = has_access(request.user, 'staff', self.course) - - # There's only one situation where we want to show the public view - if ( - not self.is_staff and - self.enable_unenrolled_access and - self.course.course_visibility == COURSE_VISIBILITY_PUBLIC and - not CourseEnrollment.is_enrolled(request.user, self.course_key) - ): - self.view = PUBLIC_VIEW - - self.can_masquerade = request.user.has_perm(MASQUERADE_AS_STUDENT, self.course) - self._setup_masquerade_for_effective_user() - - return self.render(request) - except Exception as exception: # pylint: disable=broad-except - return CourseTabView.handle_exceptions(request, self.course_key, self.course, exception) - - def _setup_masquerade_for_effective_user(self): - """ - Setup the masquerade information to allow the request to - be processed for the requested effective user. - """ - self.real_user = self.request.user - self.masquerade, self.effective_user = setup_masquerade( - self.request, - self.course_key, - self.can_masquerade, - reset_masquerade_data=True - ) - # Set the user in the request to the effective user. - self.request.user = self.effective_user - - def _redirect_to_learning_mfe(self): - """ - Can the user access this sequence in the courseware MFE? If so, redirect to MFE. - """ - # If the MFE is active, prefer that - raise Redirect(self.microfrontend_url) - - @property - def microfrontend_url(self): - """ - Return absolute URL to this section in the courseware micro-frontend. - """ - try: - unit_key = UsageKey.from_string(self.request.GET.get('activate_block_id', '')) - # `activate_block_id` is typically a Unit (a.k.a. Vertical), - # but it can technically be any block type. Do a check to - # make sure it's really a Unit before we use it for the MFE. + unit_key = UsageKey.from_string(request.GET.get('activate_block_id', '')) if unit_key.block_type != 'vertical': unit_key = None except InvalidKeyError: unit_key = None - is_preview = False - url = make_learning_mfe_courseware_url( + + # Setup masquerading if needed for this user. + # This in needed even though this view just does a redirect because + # the relevant cookies and session data is set for future requests + # when this function is called. + self.masquerade, self.effective_user = setup_masquerade( + self.request, self.course_key, - self.section.location if self.section else None, + request.user.has_perm(MASQUERADE_AS_STUDENT, course), + reset_masquerade_data=True + ) + # Set the user in the request to the effective user. + self.request.user = self.effective_user + mfe_url = make_learning_mfe_courseware_url( + self.course_key, + section_location, unit_key, - params=self.request.GET, - preview=is_preview, + params=request.GET, + preview=False ) - return url + raise Redirect(mfe_url) - def render(self, request): - """ - Render the index page. - """ - self._prefetch_and_bind_course(request) - - if self.course.has_children_at_depth(CONTENT_DEPTH): - self._reset_section_to_exam_if_required() - self.chapter = self._find_chapter() - self.section = self._find_section() - - if self.chapter and self.section: - self._redirect_if_not_requested_section() - self._save_positions() - self._prefetch_and_bind_section() - self._redirect_to_learning_mfe() - - check_content_start_date_for_masquerade_user(self.course_key, self.effective_user, request, - self.course.start, self.chapter.start, self.section.start) - - if not request.user.is_authenticated: - qs = urllib.parse.urlencode({ - 'course_id': self.course_key, - 'enrollment_action': 'enroll', - 'email_opt_in': False, - }) - - allow_anonymous = check_public_access(self.course, [COURSE_VISIBILITY_PUBLIC]) - - if not allow_anonymous: - PageLevelMessages.register_warning_message( - request, - Text(_("You are not signed in. To see additional course content, {sign_in_link} or " - "{register_link}, and enroll in this course.")).format( - sign_in_link=HTML('{sign_in_label}').format( - sign_in_label=_('sign in'), - url='{}?{}'.format(reverse('signin_user'), qs), - ), - register_link=HTML('{register_label}').format( - register_label=_('register'), - url='{}?{}'.format(reverse('register_user'), qs), - ), - ) - ) - - return render_to_response('courseware/courseware.html', self._create_courseware_context(request)) - - def _redirect_if_not_requested_section(self): - """ - If the resulting section and chapter are different from what was initially - requested, redirect back to the index page, but with an updated URL that includes - the correct section and chapter values. We do this so that our analytics events - and error logs have the appropriate URLs. - """ - if ( - self.chapter.url_name != self.original_chapter_url_name or - (self.original_section_url_name and self.section.url_name != self.original_section_url_name) - ): - raise CourseAccessRedirect( - reverse( - 'courseware_section', - kwargs={ - 'course_id': str(self.course_key), - 'chapter': self.chapter.url_name, - 'section': self.section.url_name, - }, - ) - ) - - def _clean_position(self): - """ - Verify that the given position is an integer. If it is not positive, set it to 1. - """ - if self.position is not None: - try: - self.position = max(int(self.position), 1) - except ValueError: - raise Http404(f"Position {self.position} is not an integer!") # lint-amnesty, pylint: disable=raise-missing-from - - def _reset_section_to_exam_if_required(self): - """ - Check to see if an Entrance Exam is required for the user. - """ - if not user_can_skip_entrance_exam(self.effective_user, self.course): - exam_chapter = get_entrance_exam_content(self.effective_user, self.course) - if exam_chapter and exam_chapter.get_children(): - exam_section = exam_chapter.get_children()[0] - if exam_section: - self.chapter_url_name = exam_chapter.url_name - self.section_url_name = exam_section.url_name - - def _get_language_preference(self): - """ - Returns the preferred language for the actual user making the request. - """ - language_preference = settings.LANGUAGE_CODE - - if self.request.user.is_authenticated: - language_preference = get_user_preference(self.real_user, LANGUAGE_KEY) - - return language_preference - - def _is_masquerading_as_student(self): - """ - Returns whether the current request is masquerading as a student. - """ - return self.masquerade and self.masquerade.role == 'student' - - def _is_masquerading_as_specific_student(self): - """ - Returns whether the current request is masqueurading as a specific student. - """ - return self._is_masquerading_as_student() and self.masquerade.user_name - - def _find_block(self, parent, url_name, block_type, min_depth=None): - """ - Finds the block in the parent with the specified url_name. - If not found, calls get_current_child on the parent. - """ - child = None - if url_name: - child = parent.get_child_by(lambda m: m.location.block_id == url_name) - if not child: - # User may be trying to access a child that isn't live yet - if not self._is_masquerading_as_student(): - raise Http404('No {block_type} found with name {url_name}'.format( - block_type=block_type, - url_name=url_name, - )) - elif min_depth and not child.has_children_at_depth(min_depth - 1): - child = None - if not child: - child = get_current_child(parent, min_depth=min_depth, requested_child=self.request.GET.get("child")) - return child - - def _find_chapter(self): - """ - Finds the requested chapter. - """ - return self._find_block(self.course, self.chapter_url_name, 'chapter', CONTENT_DEPTH - 1) - - def _find_section(self): - """ - Finds the requested section. - """ - if self.chapter: - return self._find_block(self.chapter, self.section_url_name, 'section') - - def _prefetch_and_bind_course(self, request): - """ - Prefetches all descendant data for the requested section and - sets up the runtime, which binds the request user to the section. - """ - self.field_data_cache = FieldDataCache.cache_for_block_descendents( - self.course_key, - self.effective_user, - self.course, - depth=CONTENT_DEPTH, - read_only=CrawlersConfig.is_crawler(request), - ) - - self.course = get_block_for_descriptor( - self.effective_user, - self.request, - self.course, - self.field_data_cache, - self.course_key, - course=self.course, - will_recheck_access=True, - ) - - def _prefetch_and_bind_section(self): - """ - Prefetches all descendant data for the requested section and - sets up the runtime, which binds the request user to the section. - """ - # Pre-fetch all descendant data - self.section = modulestore().get_item(self.section.location, depth=None, lazy=False) - self.field_data_cache.add_block_descendents(self.section, depth=None) - - # Bind section to user - self.section = get_block_for_descriptor( - self.effective_user, - self.request, - self.section, - self.field_data_cache, - self.course_key, - self.position, - course=self.course, - will_recheck_access=True, - ) - - def _save_positions(self): - """ - Save where we are in the course and chapter. - """ - save_child_position(self.course, self.chapter_url_name) - save_child_position(self.chapter, self.section_url_name) - - def _create_courseware_context(self, request): - """ - Returns and creates the rendering context for the courseware. - Also returns the table of contents for the courseware. - """ - - course_url = default_course_url(self.course.id) - show_search = ( - settings.FEATURES.get('ENABLE_COURSEWARE_SEARCH') or - (settings.FEATURES.get('ENABLE_COURSEWARE_SEARCH_FOR_COURSE_STAFF') and self.is_staff) - ) - staff_access = self.is_staff - - courseware_context = { - 'csrf': csrf(self.request)['csrf_token'], - 'course': self.course, - 'course_url': course_url, - 'chapter': self.chapter, - 'section': self.section, - 'init': '', - 'fragment': Fragment(), - 'staff_access': staff_access, - 'can_masquerade': self.can_masquerade, - 'masquerade': self.masquerade, - 'supports_preview_menu': True, - 'studio_url': get_studio_url(self.course, 'course'), - 'xqa_server': settings.FEATURES.get('XQA_SERVER', "http://your_xqa_server.com"), - 'bookmarks_api_url': reverse('bookmarks'), - 'language_preference': self._get_language_preference(), - 'disable_optimizely': not ENABLE_OPTIMIZELY_IN_COURSEWARE.is_enabled(), - 'section_title': None, - 'sequence_title': None, - 'disable_accordion': not DISABLE_COURSE_OUTLINE_PAGE_FLAG.is_enabled(self.course.id), - 'show_search': show_search, - 'render_course_wide_assets': True, - } - courseware_context.update( - get_experiment_user_metadata_context( - self.course, - self.effective_user, - ) - ) - table_of_contents = toc_for_course( - self.effective_user, - self.request, - self.course, - self.chapter_url_name, - self.section_url_name, - self.field_data_cache, - ) - courseware_context['accordion'] = render_accordion( - self.request, - self.course, - table_of_contents['chapters'], - ) - - # entrance exam data - self._add_entrance_exam_to_context(courseware_context) - - if self.section: - # chromeless data - if self.section.chrome: - chrome = [s.strip() for s in self.section.chrome.lower().split(",")] - if 'accordion' not in chrome: - courseware_context['disable_accordion'] = True - if 'tabs' not in chrome: - courseware_context['disable_tabs'] = True - - # default tab - if self.section.default_tab: - courseware_context['default_tab'] = self.section.default_tab - - # section data - courseware_context['section_title'] = self.section.display_name_with_default - section_context = self._create_section_context( - table_of_contents['previous_of_active_section'], - table_of_contents['next_of_active_section'], - ) - courseware_context['fragment'] = self.section.render(self.view, section_context) - - if self.section.position and self.section.has_children: - self._add_sequence_title_to_context(courseware_context) - - return courseware_context - - def _add_sequence_title_to_context(self, courseware_context): - """ - Adds sequence title to the given context. - - If we're rendering a section with some display items, but position - exceeds the length of the displayable items, default the position - to the first element. - """ - display_items = self.section.get_children() - if not display_items: - return - if self.section.position > len(display_items): - self.section.position = 1 - courseware_context['sequence_title'] = display_items[self.section.position - 1].display_name_with_default - - def _add_entrance_exam_to_context(self, courseware_context): - """ - Adds entrance exam related information to the given context. - """ - if course_has_entrance_exam(self.course) and getattr(self.chapter, 'is_entrance_exam', False): - courseware_context['entrance_exam_passed'] = user_has_passed_entrance_exam(self.effective_user, self.course) - courseware_context['entrance_exam_current_score'] = get_entrance_exam_score( - CourseGradeFactory().read(self.effective_user, self.course), - get_entrance_exam_usage_key(self.course), - ) - - def _create_section_context(self, previous_of_active_section, next_of_active_section): - """ - Returns and creates the rendering context for the section. - """ - def _compute_section_url(section_info, requested_child): - """ - Returns the section URL for the given section_info with the given child parameter. - """ - return "{url}?child={requested_child}".format( - url=reverse( - 'courseware_section', - args=[str(self.course_key), section_info['chapter_url_name'], section_info['url_name']], - ), - requested_child=requested_child, - ) - - # NOTE (CCB): Pull the position from the URL for un-authenticated users. Otherwise, pull the saved - # state from the data store. - position = None if self.request.user.is_authenticated else self.position - section_context = { - 'activate_block_id': self.request.GET.get('activate_block_id'), - 'requested_child': self.request.GET.get("child"), - 'progress_url': reverse('progress', kwargs={'course_id': str(self.course_key)}), - 'user_authenticated': self.request.user.is_authenticated, - 'position': position, - } - if previous_of_active_section: - section_context['prev_url'] = _compute_section_url(previous_of_active_section, 'last') - if next_of_active_section: - section_context['next_url'] = _compute_section_url(next_of_active_section, 'first') - # sections can hide data that masquerading staff should see when debugging issues with specific students - section_context['specific_masquerade'] = self._is_masquerading_as_specific_student() - return section_context def render_accordion(request, course, table_of_contents): diff --git a/lms/templates/courseware/courseware.html b/lms/templates/courseware/courseware.html deleted file mode 100644 index ce9dd19b29..0000000000 --- a/lms/templates/courseware/courseware.html +++ /dev/null @@ -1,318 +0,0 @@ -<%page expression_filter="h"/> -<%inherit file="/main.html" /> -<%namespace name='static' file='/static_content.html'/> -<%def name="online_help_token()"><% return "courseware" %> -<%! -import waffle - -from django.conf import settings -from django.urls import reverse -from django.utils.translation import gettext as _ - -from lms.djangoapps.edxnotes.helpers import is_feature_enabled as is_edxnotes_enabled -from openedx.core.djangolib.js_utils import js_escaped_string -from openedx.core.djangolib.markup import HTML -from openedx.features.course_experience import course_home_page_title, DISABLE_COURSE_OUTLINE_PAGE_FLAG -%> -<% - include_special_exams = ( - request.user.is_authenticated and - settings.FEATURES.get('ENABLE_SPECIAL_EXAMS', False) and - (course.enable_proctored_exams or course.enable_timed_exams) - ) - - completion_aggregator_url = getattr(settings, "COMPLETION_AGGREGATOR_URL", "") -%> - -<%def name="course_name()"> - <% return _("{course_number} Courseware").format(course_number=course.display_number_with_default) %> - - -<%block name="bodyclass">view-in-course view-courseware courseware ${course.css_class or ''} - -<%block name="title"> - - ${static.get_page_title_breadcrumbs(sequence_title, section_title, course_name())} - - - -<%block name="header_extras"> - -% for template_name in ["image-modal"]: - -% endfor - -% if include_special_exams is not UNDEFINED and include_special_exams: - % for template_name in ["proctored-exam-status"]: - - % endfor -% endif - - - -<%block name="headextra"> -<%static:css group='style-course-vendor'/> -<%static:css group='style-course'/> -## Utility: Notes -% if is_edxnotes_enabled(course, request.user): -<%static:css group='style-student-notes'/> -% endif - - - - - - ${HTML(fragment.head_html())} - - -<%block name="js_extra"> - - - - <%static:js group='courseware'/> - <%include file="/mathjax_include.html" args="disable_fast_preview=True"/> - - % if show_search: - <%static:require_module module_name="course_search/js/course_search_factory" class_name="CourseSearchFactory"> - var courseId = $('.courseware-results').data('courseId'); - CourseSearchFactory({ - courseId: courseId, - searchHeader: $('.search-bar') - }); - - % endif - - <%static:require_module module_name="js/courseware/courseware_factory" class_name="CoursewareFactory"> - CoursewareFactory(); - - - % if staff_access: - <%include file="xqa_interface.html"/> - % endif - - - - % if not request.user.is_authenticated: - - % endif - - - -${HTML(fragment.foot_html())} - - - -
- -% if default_tab: - <%include file="/courseware/course_navigation.html" /> -% else: - <%include file="/courseware/course_navigation.html" args="active_page='courseware'" /> -% endif - -
- -
- -% if course.show_calculator or is_edxnotes_enabled(course, request.user): - -% endif From d24f45f3ee718bfcb6a69551bf28da0028ec228a Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Fri, 23 May 2025 16:34:19 -0400 Subject: [PATCH 6/7] feat: Drop unused render_accordion function and template. This was referenced by the legacy courseware page so we don't need it anymore. --- lms/djangoapps/courseware/views/index.py | 18 ----- lms/templates/courseware/accordion.html | 94 ------------------------ 2 files changed, 112 deletions(-) delete mode 100644 lms/templates/courseware/accordion.html diff --git a/lms/djangoapps/courseware/views/index.py b/lms/djangoapps/courseware/views/index.py index e36d43b75b..6fa0850632 100644 --- a/lms/djangoapps/courseware/views/index.py +++ b/lms/djangoapps/courseware/views/index.py @@ -115,24 +115,6 @@ class CoursewareIndex(View): raise Redirect(mfe_url) - -def render_accordion(request, course, table_of_contents): - """ - Returns the HTML that renders the navigation for the given course. - Expects the table_of_contents to have data on each chapter and section, - including which ones are active. - """ - context = dict( - [ - ('toc', table_of_contents), - ('course_id', str(course.id)), - ('csrf', csrf(request)['csrf_token']), - ('due_date_display_format', course.due_date_display_format), - ] + list(TEMPLATE_IMPORTS.items()) - ) - return render_to_string('courseware/accordion.html', context) - - def save_child_position(seq_block, child_name): """ child_name: url_name of the child diff --git a/lms/templates/courseware/accordion.html b/lms/templates/courseware/accordion.html deleted file mode 100644 index bfbff20731..0000000000 --- a/lms/templates/courseware/accordion.html +++ /dev/null @@ -1,94 +0,0 @@ -<%page expression_filter="h"/> -<%namespace name='static' file='../static_content.html'/> -<%! - from django.urls import reverse - from django.utils.translation import gettext as _ - from django.conf import settings - from openedx.core.djangolib.markup import HTML, Text -%> - -<%def name="make_chapter(chapter)"> -<% -if chapter.get('active'): - aria_label = _('{chapter} current chapter').format(chapter=chapter['display_name']) - active_class = 'active' -else: - aria_label = chapter['display_name'] - active_class = '' -%> - - - - -% for chapter in toc: - ${make_chapter(chapter)} -% endfor - - -% if toc: - <%static:require_module_async module_name="js/courseware/accordion_events" class_name="AccordionEvents"> - AccordionEvents(); - - - <%static:require_module_async module_name="js/dateutil_factory" class_name="DateUtilFactory"> - DateUtilFactory.transform(iterationKey=".localized-datetime"); - -% endif From f491b97b2226a31c543554da8f3480092c4c1d68 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Wed, 23 Jul 2025 10:13:07 -0400 Subject: [PATCH 7/7] fix: Update naming for courseware section/subsection. These used to be named chapter and section before but we want to update the courseware index view to use the new names if it's gonna stick around. --- .../courseware/tests/test_masquerade.py | 6 +-- lms/djangoapps/courseware/views/index.py | 24 +++++----- lms/djangoapps/edxnotes/helpers.py | 38 ++++++++-------- lms/djangoapps/edxnotes/tests.py | 44 +++++++++---------- lms/templates/courseware/progress.html | 2 +- lms/urls.py | 18 ++++---- .../test_crowdsource_hinter.py | 6 +-- 7 files changed, 69 insertions(+), 69 deletions(-) diff --git a/lms/djangoapps/courseware/tests/test_masquerade.py b/lms/djangoapps/courseware/tests/test_masquerade.py index ed5947ffc6..0f0eca4dc2 100644 --- a/lms/djangoapps/courseware/tests/test_masquerade.py +++ b/lms/djangoapps/courseware/tests/test_masquerade.py @@ -96,11 +96,11 @@ class MasqueradeTestCase(SharedModuleStoreTestCase, LoginEnrollmentTestCase, Mas Returns the server response for the courseware page. """ url = reverse( - 'courseware_section', + 'courseware_subsection', kwargs={ 'course_id': str(self.course.id), - 'chapter': self.chapter.location.block_id, - 'section': self.sequential.location.block_id, + 'section': self.chapter.location.block_id, + 'subsection': self.sequential.location.block_id, } ) return self.client.get(url) diff --git a/lms/djangoapps/courseware/views/index.py b/lms/djangoapps/courseware/views/index.py index 6fa0850632..1034674b7b 100644 --- a/lms/djangoapps/courseware/views/index.py +++ b/lms/djangoapps/courseware/views/index.py @@ -45,7 +45,7 @@ class CoursewareIndex(View): @method_decorator(cache_control(no_cache=True, no_store=True, must_revalidate=True)) @method_decorator(ensure_valid_course_key) @method_decorator(data_sharing_consent_required) - def get(self, request, course_id, chapter=None, section=None, position=None): + def get(self, request, course_id, section=None, subsection=None, position=None): """ Instead of loading the legacy courseware sequences pages, load the equivalent URL in the learning MFE. This view does not do any auth checks since they are done by @@ -54,8 +54,8 @@ class CoursewareIndex(View): Arguments: request: HTTP request course_id (unicode): course id - chapter (unicode): chapter url_name section (unicode): section url_name + subsection (unicode): subsection url_name position (unicode): position in block, eg of block """ @@ -65,7 +65,7 @@ class CoursewareIndex(View): if not (request.user.is_authenticated or self.enable_unenrolled_access): return redirect_to_login(request.get_full_path()) - # Course load to resolve chapters/sections + # Course load to resolve sections/subsections with modulestore().bulk_operations(self.course_key): course = get_course_with_access( request.user, @@ -76,15 +76,15 @@ class CoursewareIndex(View): check_if_authenticated=True, ) - # Get the chapter, section and unit blocks so that we can redirect to the right content + # Get the section, subsection and unit blocks so that we can redirect to the right content # location in the MFE - section_location = None - if chapter and section: - chapter_block = course.get_child_by(lambda m: m.location.block_id == chapter) - if chapter_block: - section_block = chapter_block.get_child_by(lambda m: m.location.block_id == section) - if section_block: - section_location = section_block.location + subsection_location = None + if section and subsection: + section_block = course.get_child_by(lambda m: m.location.block_id == section) + if section_block: + subsection_block = section_block.get_child_by(lambda m: m.location.block_id == subsection) + if subsection_block: + subsection_location = subsection_block.location try: unit_key = UsageKey.from_string(request.GET.get('activate_block_id', '')) @@ -107,7 +107,7 @@ class CoursewareIndex(View): self.request.user = self.effective_user mfe_url = make_learning_mfe_courseware_url( self.course_key, - section_location, + subsection_location, unit_key, params=request.GET, preview=False diff --git a/lms/djangoapps/edxnotes/helpers.py b/lms/djangoapps/edxnotes/helpers.py index f81947ce1e..5e68ae4b20 100644 --- a/lms/djangoapps/edxnotes/helpers.py +++ b/lms/djangoapps/edxnotes/helpers.py @@ -405,35 +405,35 @@ def get_course_position(course_block): """ Return the user's current place in the course. - If this is the user's first time, leads to COURSE/CHAPTER/SECTION. - If this isn't the users's first time, leads to COURSE/CHAPTER. + If this is the user's first time, leads to COURSE/SECTION/SUBSECTION. + If this isn't the users's first time, leads to COURSE/SECTION. - If there is no current position in the course or chapter, then selects + If there is no current position in the course or subsection, then selects the first child. """ urlargs = {'course_id': str(course_block.id)} - chapter = get_current_child(course_block, min_depth=1) - if chapter is None: - log.debug("No chapter found when loading current position in course") - return None - - urlargs['chapter'] = chapter.url_name - if course_block.position is not None: - return { - 'display_name': Text(chapter.display_name_with_default), - 'url': reverse('courseware_chapter', kwargs=urlargs), - } - - # Relying on default of returning first child - section = get_current_child(chapter, min_depth=1) + section = get_current_child(course_block, min_depth=1) if section is None: log.debug("No section found when loading current position in course") return None urlargs['section'] = section.url_name + if course_block.position is not None: + return { + 'display_name': Text(section.display_name_with_default), + 'url': reverse('courseware_section', kwargs=urlargs), + } + + # Relying on default of returning first child + subsection = get_current_child(section, min_depth=1) + if subsection is None: + log.debug("No subsection found when loading current position in course") + return None + + urlargs['subsection'] = subsection.url_name return { - 'display_name': Text(section.display_name_with_default), - 'url': reverse('courseware_section', kwargs=urlargs) + 'display_name': Text(subsection.display_name_with_default), + 'url': reverse('courseware_subsection', kwargs=urlargs) } diff --git a/lms/djangoapps/edxnotes/tests.py b/lms/djangoapps/edxnotes/tests.py index 688cd54386..acc6140d1a 100644 --- a/lms/djangoapps/edxnotes/tests.py +++ b/lms/djangoapps/edxnotes/tests.py @@ -244,8 +244,8 @@ class EdxNotesHelpersTest(ModuleStoreTestCase): """ return reverse('courseware_position', kwargs={ 'course_id': course.id, - 'chapter': chapter.url_name, - 'section': section.url_name, + 'section': chapter.url_name, + 'subsection': section.url_name, 'position': position, }) @@ -813,34 +813,34 @@ class EdxNotesHelpersTest(ModuleStoreTestCase): timeout=(settings.EDXNOTES_CONNECT_TIMEOUT, settings.EDXNOTES_READ_TIMEOUT) ) - def test_get_course_position_no_chapter(self): + def test_get_course_position_no_section(self): """ - Returns `None` if no chapter found. + Returns `None` if no section found. """ mock_course_block = MagicMock() mock_course_block.position = 3 mock_course_block.get_children.return_value = [] assert helpers.get_course_position(mock_course_block) is None - def test_get_course_position_to_chapter(self): + def test_get_course_position_to_section(self): """ - Returns a position that leads to COURSE/CHAPTER if this isn't the users's + Returns a position that leads to COURSE/SECTION if this isn't the users's first time. """ mock_course_block = MagicMock(id=self.course.id, position=3) - mock_chapter = MagicMock() - mock_chapter.url_name = 'chapter_url_name' - mock_chapter.display_name_with_default = 'Test Chapter Display Name' + mock_section = MagicMock() + mock_section.url_name = 'section_url_name' + mock_section.display_name_with_default = 'Test Chapter Display Name' - mock_course_block.get_children.return_value = [mock_chapter] + mock_course_block.get_children.return_value = [mock_section] assert helpers.get_course_position(mock_course_block) == { 'display_name': 'Test Chapter Display Name', - 'url': f'/courses/{self.course.id}/courseware/chapter_url_name/', + 'url': f'/courses/{self.course.id}/courseware/section_url_name/', } - def test_get_course_position_no_section(self): + def test_get_course_position_no_subsection(self): """ Returns `None` if no section found. """ @@ -848,27 +848,27 @@ class EdxNotesHelpersTest(ModuleStoreTestCase): mock_course_block.get_children.return_value = [MagicMock()] assert helpers.get_course_position(mock_course_block) is None - def test_get_course_position_to_section(self): + def test_get_course_position_to_subsection(self): """ - Returns a position that leads to COURSE/CHAPTER/SECTION if this is the + Returns a position that leads to COURSE/SECTION/SUBSECTION if this is the user's first time. """ mock_course_block = MagicMock(id=self.course.id, position=None) - mock_chapter = MagicMock() - mock_chapter.url_name = 'chapter_url_name' - mock_course_block.get_children.return_value = [mock_chapter] - mock_section = MagicMock() mock_section.url_name = 'section_url_name' - mock_section.display_name_with_default = 'Test Section Display Name' + mock_course_block.get_children.return_value = [mock_section] - mock_chapter.get_children.return_value = [mock_section] - mock_section.get_children.return_value = [MagicMock()] + mock_subsection = MagicMock() + mock_subsection.url_name = 'subsection_url_name' + mock_subsection.display_name_with_default = 'Test Section Display Name' + + mock_section.get_children.return_value = [mock_subsection] + mock_subsection.get_children.return_value = [MagicMock()] assert helpers.get_course_position(mock_course_block) == { 'display_name': 'Test Section Display Name', - 'url': f'/courses/{self.course.id}/courseware/chapter_url_name/section_url_name/', + 'url': f'/courses/{self.course.id}/courseware/section_url_name/subsection_url_name/', } def test_get_index(self): diff --git a/lms/templates/courseware/progress.html b/lms/templates/courseware/progress.html index 26f1bb59cb..711fad8954 100644 --- a/lms/templates/courseware/progress.html +++ b/lms/templates/courseware/progress.html @@ -187,7 +187,7 @@ username = get_enterprise_learner_generic_name(request) or student.username %endif

%else: - + ${ section.display_name} %if (total > 0 or earned > 0) and section.show_grades(staff_access): diff --git a/lms/urls.py b/lms/urls.py index 6cd51e94cf..092dcb6be9 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -482,21 +482,21 @@ urlpatterns += [ name='courseware', ), re_path( - r'^courses/{}/courseware/(?P[^/]*)/$'.format( - settings.COURSE_ID_PATTERN, - ), - CoursewareIndex.as_view(), - name='courseware_chapter', - ), - re_path( - r'^courses/{}/courseware/(?P[^/]*)/(?P
[^/]*)/$'.format( + r'^courses/{}/courseware/(?P
[^/]*)/$'.format( settings.COURSE_ID_PATTERN, ), CoursewareIndex.as_view(), name='courseware_section', ), re_path( - r'^courses/{}/courseware/(?P[^/]*)/(?P
[^/]*)/(?P[^/]*)/?$'.format( + r'^courses/{}/courseware/(?P
[^/]*)/(?P[^/]*)/$'.format( + settings.COURSE_ID_PATTERN, + ), + CoursewareIndex.as_view(), + name='courseware_subsection', + ), + re_path( + r'^courses/{}/courseware/(?P
[^/]*)/(?P[^/]*)/(?P[^/]*)/?$'.format( settings.COURSE_ID_PATTERN, ), CoursewareIndex.as_view(), diff --git a/openedx/tests/xblock_integration/test_crowdsource_hinter.py b/openedx/tests/xblock_integration/test_crowdsource_hinter.py index 61f1097481..ecd3c09dd6 100644 --- a/openedx/tests/xblock_integration/test_crowdsource_hinter.py +++ b/openedx/tests/xblock_integration/test_crowdsource_hinter.py @@ -55,11 +55,11 @@ class TestCrowdsourceHinter(SharedModuleStoreTestCase, LoginEnrollmentTestCase): ) cls.course_url = reverse( - 'courseware_section', + 'courseware_subsection', kwargs={ 'course_id': str(cls.course.id), - 'chapter': 'Overview', - 'section': 'Welcome', + 'section': 'Overview', + 'subsection': 'Welcome', } )