From b429e55cac6bdffd06931b51a3c26f1c624df9ec Mon Sep 17 00:00:00 2001 From: Sagirov Evgeniy <34642612+UvgenGen@users.noreply.github.com> Date: Wed, 14 Sep 2022 17:53:33 +0300 Subject: [PATCH] feat!: remove Studio editing for Old Mongo Courses This removes user-facing Studio edit support for Old Mongo courses (courses that have a CourseKey of the format {org}/{course}/{run}). This does not affect our normal courses, which have CourseKeys starting with "course-v1:". After this commit: * Old Mongo courses will continue to appear on the Studio course listing page, but are not clickable. * Any attempt to directly access an Old Mongo course in Studio via URL fail with a 404 error. * Course certificates will still be available for Old Mongo courses. * Old Mongo courses will continue to be returned by CourseOverviews and get_course_summaries() calls. We decided against removing Old Mongo courses from the listing entirely because that would require very expensive CourseOverviews query to filter them out. Making that query more efficient would involve a database migration to add appropriate indexing, which is something else that we are looking to avoid. CourseOverviews are used everywhere in the system, so we want to avoid changing how they work so that we can minimize risk. This is part of the Old Mongo Modulestore deprecation effort: https://github.com/openedx/public-engineering/issues/62 --- .../contentstore/tests/test_contentstore.py | 20 +++++++ .../contentstore/tests/test_course_listing.py | 3 +- cms/djangoapps/contentstore/views/course.py | 14 ++++- .../views/tests/test_course_index.py | 33 +++++++++-- .../views/tests/test_exam_settings_view.py | 12 +++- .../views/tests/test_header_menu.py | 14 ++++- .../studio/CourseOrLibraryListing.jsx | 55 +++++++++++-------- .../commands/tests/test_dump_course.py | 2 +- .../commands/tests/test_simulate_publish.py | 14 ++--- xmodule/modulestore/tests/test_semantics.py | 8 +++ 10 files changed, 133 insertions(+), 42 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 3f89bbe450..7d36753472 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -1485,6 +1485,12 @@ class ContentStoreTest(ContentStoreTestCase): """Test viewing the course overview page with an existing course""" course = CourseFactory.create() resp = self._show_course_overview(course.id) + + # course_handler raise 404 for old mongo course + if course.id.deprecated: + self.assertEqual(resp.status_code, 404) + return + self.assertContains( resp, '
'.format( # lint-amnesty, pylint: disable=line-too-long @@ -1550,6 +1556,12 @@ class ContentStoreTest(ContentStoreTestCase): course_key = course_items[0].id resp = self._show_course_overview(course_key) + + # course_handler raise 404 for old mongo course + if course_key.deprecated: + self.assertEqual(resp.status_code, 404) + return + self.assertEqual(resp.status_code, 200) self.assertContains(resp, 'Chapter 2') @@ -1879,6 +1891,10 @@ class RerunCourseTest(ContentStoreTestCase): rerun_course_data.update(destination_course_data) destination_course_key = _get_course_id(self.store, destination_course_data) + # course_handler raise 404 for old mongo course + if destination_course_key.deprecated: + raise SkipTest('OldMongo Deprecation') + # post the request course_url = get_url('course_handler', destination_course_key, 'course_key_string') response = self.client.ajax_post(course_url, rerun_course_data) @@ -2198,6 +2214,10 @@ def _create_course(test, course_key, course_data): """ Creates a course via an AJAX request and verifies the URL returned in the response. """ + # course_handler raise 404 for old mongo course + if course_key.deprecated: + raise SkipTest('OldMongo Deprecation') + course_url = get_url('course_handler', course_key, 'course_key_string') response = test.client.ajax_post(course_url, course_data) test.assertEqual(response.status_code, 200) diff --git a/cms/djangoapps/contentstore/tests/test_course_listing.py b/cms/djangoapps/contentstore/tests/test_course_listing.py index 9baa1c044f..44fe762f73 100644 --- a/cms/djangoapps/contentstore/tests/test_course_listing.py +++ b/cms/djangoapps/contentstore/tests/test_course_listing.py @@ -185,6 +185,7 @@ class TestCourseListing(ModuleStoreTestCase): # Fetch accessible courses list & verify their count courses_list_by_staff, __ = get_courses_accessible_to_user(self.request) + self.assertEqual(len(list(courses_list_by_staff)), TOTAL_COURSES_COUNT) # Verify fetched accessible courses list is a list of CourseSummery instances @@ -194,7 +195,7 @@ class TestCourseListing(ModuleStoreTestCase): with check_mongo_calls(mongo_calls): list(_accessible_courses_summary_iter(self.request)) - @ddt.data(ModuleStoreEnum.Type.split, ModuleStoreEnum.Type.mongo) + @ddt.data(ModuleStoreEnum.Type.split) def test_get_course_list_with_invalid_course_location(self, store): """ Test getting courses with invalid course location (course deleted from modulestore). diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index b8db950d74..2f6a6a0c6f 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -271,6 +271,11 @@ def course_handler(request, course_key_string=None): json: delete this branch from this course (leaving off /branch/draft would imply delete the course) """ try: + if course_key_string: + course_key = CourseKey.from_string(course_key_string) + if course_key.deprecated: + logging.error(f"User {request.user.id} tried to access Studio for Old Mongo course {course_key}.") + return HttpResponseNotFound() response_format = request.GET.get('format') or request.POST.get('format') or 'html' if response_format == 'json' or 'application/json' in request.META.get('HTTP_ACCEPT', 'application/json'): if request.method == 'GET': @@ -783,7 +788,7 @@ def _process_courses_list(courses_iter, in_process_course_actions, split_archive """ Return a dict of the data which the view requires for each course """ - return { + course_context = { 'display_name': course.display_name, 'course_key': str(course.location.course_key), 'url': reverse_course_url('course_handler', course.id), @@ -793,6 +798,13 @@ def _process_courses_list(courses_iter, in_process_course_actions, split_archive 'number': course.display_number_with_default, 'run': course.location.run } + if course.id.deprecated: + course_context.update({ + 'url': None, + 'lms_link': None, + 'rerun_link': None + }) + return course_context in_process_action_course_keys = {uca.course_key for uca in in_process_course_actions} active_courses = [] diff --git a/cms/djangoapps/contentstore/views/tests/test_course_index.py b/cms/djangoapps/contentstore/views/tests/test_course_index.py index 81821d947f..04b66498b0 100644 --- a/cms/djangoapps/contentstore/views/tests/test_course_index.py +++ b/cms/djangoapps/contentstore/views/tests/test_course_index.py @@ -5,7 +5,7 @@ Unit tests for getting the list of courses and the course outline. import datetime import json -from unittest import mock +from unittest import SkipTest, mock, skip from unittest.mock import patch import ddt @@ -58,7 +58,7 @@ class TestCourseIndex(CourseTestCase): display_name='dotted.course.name-2', ) - def check_courses_on_index(self, authed_client): + def check_courses_on_index(self, authed_client, expected_course_tab_len): """ Test that the React course listing is present. """ @@ -66,7 +66,7 @@ class TestCourseIndex(CourseTestCase): index_response = authed_client.get(index_url, {}, HTTP_ACCEPT='text/html') parsed_html = lxml.html.fromstring(index_response.content) courses_tab = parsed_html.find_class('react-course-listing') - self.assertEqual(len(courses_tab), 1) + self.assertEqual(len(courses_tab), expected_course_tab_len) def test_libraries_on_index(self): """ @@ -100,7 +100,7 @@ class TestCourseIndex(CourseTestCase): """ Test that people with is_staff see the courses and can navigate into them """ - self.check_courses_on_index(self.client) + self.check_courses_on_index(self.client, 1) def test_negative_conditions(self): """ @@ -110,7 +110,10 @@ class TestCourseIndex(CourseTestCase): # register a non-staff member and try to delete the course branch non_staff_client, _ = self.create_non_staff_authed_user_client() response = non_staff_client.delete(outline_url, {}, HTTP_ACCEPT='application/json') - self.assertEqual(response.status_code, 403) + if self.course.id.deprecated: + self.assertEqual(response.status_code, 404) + else: + self.assertEqual(response.status_code, 403) def test_course_staff_access(self): """ @@ -128,9 +131,10 @@ class TestCourseIndex(CourseTestCase): ) # test access - self.check_courses_on_index(course_staff_client) + self.check_courses_on_index(course_staff_client, 1) def test_json_responses(self): + outline_url = reverse_course_url('course_handler', self.course.id) chapter = ItemFactory.create(parent_location=self.course.location, category='chapter', display_name="Week 1") lesson = ItemFactory.create(parent_location=chapter.location, category='sequential', display_name="Lesson 1") @@ -142,6 +146,11 @@ class TestCourseIndex(CourseTestCase): ItemFactory.create(parent_location=subsection.location, category="video", display_name="My Video") resp = self.client.get(outline_url, HTTP_ACCEPT='application/json') + + if self.course.id.deprecated: + self.assertEqual(resp.status_code, 404) + return + json_response = json.loads(resp.content.decode('utf-8')) # First spot check some values in the root response @@ -307,6 +316,11 @@ class TestCourseIndex(CourseTestCase): course_outline_url = reverse_course_url('course_handler', updated_course.id) response = self.client.get_html(course_outline_url) + # course_handler raise 404 for old mongo course + if self.course.id.deprecated: + self.assertEqual(response.status_code, 404) + return + # Assert that response code is 200. self.assertEqual(response.status_code, 200) @@ -401,6 +415,7 @@ class TestCourseIndexArchived(CourseTestCase): archived_course_tab = parsed_html.find_class('archived-courses') self.assertEqual(len(archived_course_tab), 1 if separate_archived_courses else 0) + @skip('Skip test for old mongo course') @ddt.data( # Staff user has course staff access (True, 'staff', None, 0, 20), @@ -469,6 +484,10 @@ class TestCourseOutline(CourseTestCase): outline_url = reverse_course_url('course_handler', self.course.id) outline_url = outline_url + '?format=concise' if is_concise else outline_url resp = self.client.get(outline_url, HTTP_ACCEPT='application/json') + if self.course.id.deprecated: + self.assertEqual(resp.status_code, 404) + return + json_response = json.loads(resp.content.decode('utf-8')) # First spot check some values in the root response @@ -613,6 +632,8 @@ class TestCourseOutline(CourseTestCase): """ Test to check proctored exam settings mfe url is rendering properly """ + if self.course.id.deprecated: + raise SkipTest("Skip test for old mongo course") mock_validate_proctoring_settings.return_value = [ { 'key': 'proctoring_provider', diff --git a/cms/djangoapps/contentstore/views/tests/test_exam_settings_view.py b/cms/djangoapps/contentstore/views/tests/test_exam_settings_view.py index c45e4984e7..a7ee7f0ab0 100644 --- a/cms/djangoapps/contentstore/views/tests/test_exam_settings_view.py +++ b/cms/djangoapps/contentstore/views/tests/test_exam_settings_view.py @@ -1,7 +1,7 @@ """ Exam Settings View Tests """ - +from unittest import SkipTest from unittest.mock import patch import ddt @@ -104,6 +104,10 @@ class TestExamSettingsView(CourseTestCase, UrlResetMixin): self.course.enable_proctored_exams = True self.save_course() + # course_handler raise 404 for old mongo course + if self.course.id.deprecated: + raise SkipTest("course_handler raise 404 for old mongo course") + url = reverse_course_url(page_handler, self.course.id) resp = self.client.get(url, HTTP_ACCEPT='text/html') alert_text = self._get_exam_settings_alert_text(resp.content) @@ -139,6 +143,9 @@ class TestExamSettingsView(CourseTestCase, UrlResetMixin): self.course.enable_proctored_exams = True self.save_course() + # course_handler raise 404 for old mongo course + if self.course.id.deprecated and page_handler == 'course_handler': + raise SkipTest("course_handler raise 404 for old mongo course") url = reverse_course_url(page_handler, self.course.id) resp = self.client.get(url, HTTP_ACCEPT='text/html') alert_text = self._get_exam_settings_alert_text(resp.content) @@ -163,6 +170,9 @@ class TestExamSettingsView(CourseTestCase, UrlResetMixin): """ Alert should not be visible if no proctored exam setting error exists """ + # course_handler raise 404 for old mongo course + if self.course.id.deprecated and page_handler == 'course_handler': + raise SkipTest("course_handler raise 404 for old mongo course") url = reverse_course_url(page_handler, self.course.id) resp = self.client.get(url, HTTP_ACCEPT='text/html') parsed_html = lxml.html.fromstring(resp.content) diff --git a/cms/djangoapps/contentstore/views/tests/test_header_menu.py b/cms/djangoapps/contentstore/views/tests/test_header_menu.py index 496d5552aa..454fd7e257 100644 --- a/cms/djangoapps/contentstore/views/tests/test_header_menu.py +++ b/cms/djangoapps/contentstore/views/tests/test_header_menu.py @@ -1,7 +1,7 @@ """ Course Header Menu Tests. """ - +from unittest import SkipTest from django.conf import settings from django.test.utils import override_settings @@ -37,6 +37,9 @@ class TestHeaderMenu(CourseTestCase, UrlResetMixin): Tests course header menu should not have `Certificates` menu item if course has not web/HTML certificates enabled. """ + # course_handler raise 404 for old mongo course + if self.course.id.deprecated: + raise SkipTest("course_handler raise 404 for old mongo course") self.course.cert_html_view_enabled = False self.save_course() outline_url = reverse_course_url('course_handler', self.course.id) @@ -49,6 +52,9 @@ class TestHeaderMenu(CourseTestCase, UrlResetMixin): Tests course header menu should have `Certificates` menu item if course has web/HTML certificates enabled. """ + # course_handler raise 404 for old mongo course + if self.course.id.deprecated: + raise SkipTest("course_handler raise 404 for old mongo course") outline_url = reverse_course_url('course_handler', self.course.id) resp = self.client.get(outline_url, HTTP_ACCEPT='text/html') self.assertEqual(resp.status_code, 200) @@ -60,6 +66,9 @@ class TestHeaderMenu(CourseTestCase, UrlResetMixin): Tests course header menu should not have `Exam Settings` menu item if course does not have the Exam Settings view enabled. """ + # course_handler raise 404 for old mongo course + if self.course.id.deprecated: + raise SkipTest("course_handler raise 404 for old mongo course") outline_url = reverse_course_url('course_handler', self.course.id) resp = self.client.get(outline_url, HTTP_ACCEPT='text/html') self.assertEqual(resp.status_code, 200) @@ -71,6 +80,9 @@ class TestHeaderMenu(CourseTestCase, UrlResetMixin): Tests course header menu should have `Exam Settings` menu item if course does have Exam Settings view enabled. """ + # course_handler raise 404 for old mongo course + if self.course.id.deprecated: + raise SkipTest("course_handler raise 404 for old mongo course") outline_url = reverse_course_url('course_handler', self.course.id) resp = self.client.get(outline_url, HTTP_ACCEPT='text/html') self.assertEqual(resp.status_code, 200) diff --git a/cms/static/js/features_jsx/studio/CourseOrLibraryListing.jsx b/cms/static/js/features_jsx/studio/CourseOrLibraryListing.jsx index ef925419ef..7e7326ffe3 100644 --- a/cms/static/js/features_jsx/studio/CourseOrLibraryListing.jsx +++ b/cms/static/js/features_jsx/studio/CourseOrLibraryListing.jsx @@ -10,34 +10,45 @@ export function CourseOrLibraryListing(props) { const linkClass = props.linkClass; const idBase = props.idBase; + const renderCourseMetadata = (item, i) => ( +
+

{item.display_name}

+
+ + {gettext('Organization:')} + {item.org} + + + {gettext('Course Number:')} + {item.number} + + { item.run && + + {gettext('Course Run:')} + {item.run} + + } + { item.can_edit === false && + {gettext('(Read-only)')} + } +
+
+ ); + return (