diff --git a/cms/djangoapps/contentstore/management/commands/backfill_course_tabs.py b/cms/djangoapps/contentstore/management/commands/backfill_course_tabs.py new file mode 100644 index 0000000000..b163c573e5 --- /dev/null +++ b/cms/djangoapps/contentstore/management/commands/backfill_course_tabs.py @@ -0,0 +1,53 @@ +""" +Management command to backfill default course tabs for all courses. This command is essentially for +when a new default tab is added and we need to update all existing courses. Any new courses will pick +up the new tab automatically via course creation and the CourseTabList.initialize_default method. +People updating to Nutmeg release should run this command as part of the upgrade process. + +This should be invoked from the Studio process. +""" +import logging + +from django.core.management.base import BaseCommand + +from xmodule.tabs import CourseTabList +from xmodule.modulestore import ModuleStoreEnum +from xmodule.modulestore.django import modulestore + +logger = logging.getLogger(__name__) + + +class Command(BaseCommand): + """ + Invoke with: + python manage.py cms backfill_course_tabs + """ + help = ( + 'Backfill default course tabs for all courses. This command is essentially for when a new default ' + 'tab is added and we need to update all existing courses. Any new courses will pick up the new ' + 'tab automatically via course creation and the CourseTabList.initialize_default method.' + ) + + def handle(self, *args, **options): + """ + Gathers all course keys in the modulestore and updates the course tabs + if there are any new default course tabs. Else, makes no updates. + """ + store = modulestore() + course_keys = sorted( + (course.id for course in store.get_course_summaries()), + key=str # Different types of CourseKeys can't be compared without this. + ) + logger.info(f'{len(course_keys)} courses read from modulestore.') + + for course_key in course_keys: + course = store.get_course(course_key, depth=1) + existing_tabs = {tab.type for tab in course.tabs} + CourseTabList.initialize_default(course) + new_tabs = {tab.type for tab in course.tabs} + + if existing_tabs != new_tabs: + # This will trigger the Course Published Signal which is necessary to update + # the corresponding Course Overview + logger.info(f'Updating tabs for {course_key}.') + store.update_item(course, ModuleStoreEnum.UserID.mgmt_command) diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_backfill_course_tabs.py b/cms/djangoapps/contentstore/management/commands/tests/test_backfill_course_tabs.py new file mode 100644 index 0000000000..d63979d2fe --- /dev/null +++ b/cms/djangoapps/contentstore/management/commands/tests/test_backfill_course_tabs.py @@ -0,0 +1,131 @@ +""" +Tests for `backfill_course_outlines` Studio (cms) management command. +""" +from unittest import mock + +from django.core.management import call_command + +from xmodule.modulestore import ModuleStoreEnum +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.factories import CourseFactory +from xmodule.tabs import InvalidTabsException + + +class BackfillCourseTabsTest(ModuleStoreTestCase): + """ + Test `backfill_course_tabs` + """ + @mock.patch('cms.djangoapps.contentstore.management.commands.backfill_course_tabs.logger') + def test_no_tabs_to_add(self, mock_logger): + """ Calls command with a course already having all default tabs. """ + course = CourseFactory() + tabs_before = course.tabs + + call_command('backfill_course_tabs') + + course = self.store.get_course(course.id) + tabs_after = course.tabs + assert tabs_before == tabs_after + # Ensure update_item was never called since there were no changes necessary + # Using logger as a proxy. First time is number of courses read. + assert mock_logger.info.call_count == 1 + + @mock.patch('cms.djangoapps.contentstore.management.commands.backfill_course_tabs.logger') + def test_add_one_tab(self, mock_logger): + """ + Calls command on a course with existing tabs, but not all default ones. + """ + course = CourseFactory() + course.tabs = [tab for tab in course.tabs if tab.type != 'dates'] + self.update_course(course, ModuleStoreEnum.UserID.test) + assert len(course.tabs) == 6 + assert 'dates' not in {tab.type for tab in course.tabs} + + call_command('backfill_course_tabs') + + course = self.store.get_course(course.id) + assert len(course.tabs) == 7 + assert 'dates' in {tab.type for tab in course.tabs} + mock_logger.info.assert_called_with(f'Updating tabs for {course.id}.') + assert mock_logger.info.call_count == 2 + + call_command('backfill_course_tabs') + # Ensure rerunning the command does not require another update on the course. + # Goes up by one from the courses read log. + assert mock_logger.info.call_count == 3 + + @mock.patch('cms.djangoapps.contentstore.management.commands.backfill_course_tabs.logger') + def test_multiple_courses_one_update(self, mock_logger): + """ + Calls command on multiple courses, some that already have all their tabs, and some that need updates. + """ + CourseFactory() + CourseFactory() + CourseFactory() + course = CourseFactory() + course.tabs = [tab for tab in course.tabs if tab.type in ('course_info', 'courseware')] + self.update_course(course, ModuleStoreEnum.UserID.test) + assert len(course.tabs) == 2 + assert 'dates' not in {tab.type for tab in course.tabs} + assert 'progress' not in {tab.type for tab in course.tabs} + + call_command('backfill_course_tabs') + + course = self.store.get_course(course.id) + assert len(course.tabs) == 7 + assert 'dates' in {tab.type for tab in course.tabs} + assert 'progress' in {tab.type for tab in course.tabs} + mock_logger.info.assert_any_call('4 courses read from modulestore.') + mock_logger.info.assert_called_with(f'Updating tabs for {course.id}.') + assert mock_logger.info.call_count == 2 + + call_command('backfill_course_tabs') + # Ensure rerunning the command does not require another update on the course. + # Goes up by one from the courses read log. + assert mock_logger.info.call_count == 3 + + @mock.patch('cms.djangoapps.contentstore.management.commands.backfill_course_tabs.logger') + def test_multiple_courses_all_updated(self, mock_logger): + """ + Calls command on multiple courses where all of them need updates. + """ + course_1 = CourseFactory() + course_1.tabs = [tab for tab in course_1.tabs if tab.type != 'dates'] + self.update_course(course_1, ModuleStoreEnum.UserID.test) + course_2 = CourseFactory() + course_2.tabs = [tab for tab in course_2.tabs if tab.type != 'progress'] + self.update_course(course_2, ModuleStoreEnum.UserID.test) + assert len(course_1.tabs) == 6 + assert len(course_2.tabs) == 6 + assert 'dates' not in {tab.type for tab in course_1.tabs} + assert 'progress' not in {tab.type for tab in course_2.tabs} + + call_command('backfill_course_tabs') + + course_1 = self.store.get_course(course_1.id) + course_2 = self.store.get_course(course_2.id) + assert len(course_1.tabs) == 7 + assert len(course_2.tabs) == 7 + assert 'dates' in {tab.type for tab in course_1.tabs} + assert 'progress' in {tab.type for tab in course_2.tabs} + mock_logger.info.assert_any_call('2 courses read from modulestore.') + mock_logger.info.assert_any_call(f'Updating tabs for {course_1.id}.') + mock_logger.info.assert_any_call(f'Updating tabs for {course_2.id}.') + assert mock_logger.info.call_count == 3 + + call_command('backfill_course_tabs') + # Ensure rerunning the command does not require another update on any courses. + # Goes up by one from the courses read log. + assert mock_logger.info.call_count == 4 + + @mock.patch('cms.djangoapps.contentstore.management.commands.backfill_course_tabs.logger') + def test_command_fails_if_error_raised(self, mock_logger): + CourseFactory() + with mock.patch( + 'cms.djangoapps.contentstore.management.commands.backfill_course_tabs.CourseTabList.initialize_default', + side_effect=InvalidTabsException + ): + with self.assertRaises(InvalidTabsException): + call_command('backfill_course_tabs') + # Never calls the update, but does make it through grabbing the courses + mock_logger.info.assert_called_once_with('1 courses read from modulestore.') diff --git a/cms/djangoapps/contentstore/tasks.py b/cms/djangoapps/contentstore/tasks.py index f4c9c5d0d4..20146caf1f 100644 --- a/cms/djangoapps/contentstore/tasks.py +++ b/cms/djangoapps/contentstore/tasks.py @@ -5,10 +5,10 @@ This file contains celery tasks for contentstore views import base64 import json import os -import shutil # lint-amnesty, pylint: disable=wrong-import-order -import tarfile # lint-amnesty, pylint: disable=wrong-import-order -from datetime import datetime # lint-amnesty, pylint: disable=wrong-import-order -from tempfile import NamedTemporaryFile, mkdtemp # lint-amnesty, pylint: disable=wrong-import-order +import shutil +import tarfile +from datetime import datetime +from tempfile import NamedTemporaryFile, mkdtemp import olxcleaner import pkg_resources @@ -17,7 +17,6 @@ from celery import shared_task from celery.utils.log import get_task_logger from django.conf import settings from django.contrib.auth import get_user_model -from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user from django.core.exceptions import SuspiciousOperation from django.core.files import File from django.test import RequestFactory diff --git a/cms/djangoapps/contentstore/views/tests/test_tabs.py b/cms/djangoapps/contentstore/views/tests/test_tabs.py index fae6ee1f7f..e366420d22 100644 --- a/cms/djangoapps/contentstore/views/tests/test_tabs.py +++ b/cms/djangoapps/contentstore/views/tests/test_tabs.py @@ -193,11 +193,12 @@ class PrimitiveTabEdit(ModuleStoreTestCase): tabs.primitive_delete(course, 1) with self.assertRaises(IndexError): tabs.primitive_delete(course, 7) - assert course.tabs[2] != {'type': 'discussion', 'name': 'Discussion'} + + assert course.tabs[2] != {'type': 'dates', 'name': 'Dates'} tabs.primitive_delete(course, 2) assert {'type': 'progress'} not in course.tabs - # Check that discussion has shifted up - assert course.tabs[2] == {'type': 'discussion', 'name': 'Discussion'} + # Check that dates has shifted up + assert course.tabs[2] == {'type': 'dates', 'name': 'Dates'} def test_insert(self): """Test primitive tab insertion.""" diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py b/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py index 365bc8cace..21d9ddccd5 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py @@ -30,7 +30,6 @@ from xmodule.modulestore.tests.utils import ( SPLIT_MODULESTORE_SETUP, TEST_DATA_DIR, MongoContentstoreBuilder, - mock_tab_from_json ) from xmodule.modulestore.xml_exporter import export_course_to_xml from xmodule.modulestore.xml_importer import import_course_from_xml @@ -61,7 +60,6 @@ class CrossStoreXMLRoundtrip(CourseComparisonTest, PartitionTestCase): self.addCleanup(rmtree, self.export_dir, ignore_errors=True) @patch('xmodule.video_module.video_module.edxval_api', None) - @patch('xmodule.tabs.CourseTab.from_json', side_effect=mock_tab_from_json) @ddt.data(*itertools.product( MODULESTORE_SETUPS, MODULESTORE_SETUPS, @@ -72,7 +70,7 @@ class CrossStoreXMLRoundtrip(CourseComparisonTest, PartitionTestCase): @ddt.unpack def test_round_trip( self, source_builder, dest_builder, source_content_builder, - dest_content_builder, course_data_name, _mock_tab_from_json + dest_content_builder, course_data_name, ): # Construct the contentstore for storing the first import with source_content_builder.build() as source_content: diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py index 8d1fc06e29..5d50dfb5ad 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -50,11 +50,7 @@ from xmodule.modulestore.store_utilities import DETACHED_XBLOCK_TYPES from xmodule.modulestore.tests.factories import check_exact_number_of_calls, check_mongo_calls, mongo_uses_error_check from xmodule.modulestore.tests.mongo_connection import MONGO_HOST, MONGO_PORT_NUM from xmodule.modulestore.tests.test_asides import AsideTestType -from xmodule.modulestore.tests.utils import ( - MongoContentstoreBuilder, - create_modulestore_instance, - mock_tab_from_json -) +from xmodule.modulestore.tests.utils import MongoContentstoreBuilder, create_modulestore_instance from xmodule.modulestore.xml_exporter import export_course_to_xml from xmodule.modulestore.xml_importer import LocationMixin, import_course_from_xml from xmodule.tests import DATA_DIR, CourseComparisonTest @@ -2659,9 +2655,8 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup): self.store.clone_course(course_key, dest_course_id, self.user_id) signal_handler.send.assert_called_with('course_published', course_key=dest_course_id) - @patch('xmodule.tabs.CourseTab.from_json', side_effect=mock_tab_from_json) @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) - def test_course_publish_signal_import_firing(self, default, _from_json): + def test_course_publish_signal_import_firing(self, default): with MongoContentstoreBuilder().build() as contentstore: signal_handler = Mock(name='signal_handler') self.store = MixedModuleStore( diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py index f4bbbfdbb1..cc2cca9c1b 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py @@ -35,7 +35,6 @@ from xmodule.modulestore.inheritance import InheritanceMixin from xmodule.modulestore.mongo import MongoKeyValueStore from xmodule.modulestore.mongo.base import as_draft from xmodule.modulestore.tests.mongo_connection import MONGO_HOST, MONGO_PORT_NUM -from xmodule.modulestore.tests.utils import mock_tab_from_json from xmodule.modulestore.xml_exporter import export_course_to_xml from xmodule.modulestore.xml_importer import LocationMixin, import_course_from_xml, perform_xlint from xmodule.tests import DATA_DIR @@ -131,37 +130,46 @@ class TestMongoModuleStoreBase(TestCase): ) - with patch('xmodule.tabs.CourseTab.from_json', side_effect=mock_tab_from_json): - import_course_from_xml( - draft_store, - 999, - DATA_DIR, - cls.courses, - static_content_store=content_store - ) + import_course_from_xml( + draft_store, + 999, + DATA_DIR, + cls.courses, + static_content_store=content_store + ) - # also test a course with no importing of static content - import_course_from_xml( - draft_store, - 999, - DATA_DIR, - ['test_import_course'], - static_content_store=content_store, - do_import_static=False, - verbose=True - ) + # also test a course with no importing of static content + import_course_from_xml( + draft_store, + 999, + DATA_DIR, + ['test_import_course'], + static_content_store=content_store, + do_import_static=False, + verbose=True + ) - # also import a course under a different course_id (especially ORG) - import_course_from_xml( - draft_store, - 999, - DATA_DIR, - ['test_import_course'], - static_content_store=content_store, - do_import_static=False, - verbose=True, - target_id=CourseKey.from_string('guestx/foo/bar') - ) + # also import a course under a different course_id (especially ORG) + import_course_from_xml( + draft_store, + 999, + DATA_DIR, + ['test_import_course'], + static_content_store=content_store, + do_import_static=False, + verbose=True, + target_id=CourseKey.from_string('guestx/foo/bar') + ) + + # Import a course for `test_reference_converters` since it manipulates the saved course + # which can cause any other test using the same course to have a flakey error + import_course_from_xml( + draft_store, + 999, + DATA_DIR, + ['test_import_course_2'], + static_content_store=content_store + ) return content_store, draft_store @@ -206,12 +214,11 @@ class TestMongoModuleStore(TestMongoModuleStoreBase): ) assert store.get_modulestore_type('') == ModuleStoreEnum.Type.mongo - @patch('xmodule.tabs.CourseTab.from_json', side_effect=mock_tab_from_json) - def test_get_courses(self, _from_json): + def test_get_courses(self): '''Make sure the course objects loaded properly''' courses = self.draft_store.get_courses() - assert len(courses) == 6 + assert len(courses) == 7 course_ids = [course.id for course in courses] for course_key in [ @@ -224,6 +231,7 @@ class TestMongoModuleStore(TestMongoModuleStoreBase): ['edX', 'test_unicode', '2012_Fall'], ['edX', 'toy', '2012_Fall'], ['guestx', 'foo', 'bar'], + ['edX', 'test_import', '2014_Fall'], ] ]: assert course_key in course_ids @@ -236,8 +244,7 @@ class TestMongoModuleStore(TestMongoModuleStoreBase): assert not self.draft_store.has_course(mix_cased) assert self.draft_store.has_course(mix_cased, ignore_case=True) - @patch('xmodule.tabs.CourseTab.from_json', side_effect=mock_tab_from_json) - def test_get_org_courses(self, _from_json): + def test_get_org_courses(self): """ Make sure that we can query for a filtered list of courses for a given ORG """ @@ -255,7 +262,7 @@ class TestMongoModuleStore(TestMongoModuleStoreBase): assert course_key in course_ids courses = self.draft_store.get_courses(org='edX') - assert len(courses) == 5 + assert len(courses) == 6 course_ids = [course.id for course in courses] for course_key in [ @@ -266,6 +273,7 @@ class TestMongoModuleStore(TestMongoModuleStoreBase): ['edX', 'test_import_course', '2012_Fall'], ['edX', 'test_unicode', '2012_Fall'], ['edX', 'toy', '2012_Fall'], + ['edX', 'test_import', '2014_Fall'], ] ]: assert course_key in course_ids @@ -431,8 +439,7 @@ class TestMongoModuleStore(TestMongoModuleStoreBase): {'displayname': 'hello'} ) - @patch('xmodule.tabs.CourseTab.from_json', side_effect=mock_tab_from_json) - def test_get_courses_for_wiki(self, _from_json): + def test_get_courses_for_wiki(self): """ Test the get_courses_for_wiki method """ @@ -473,7 +480,7 @@ class TestMongoModuleStore(TestMongoModuleStoreBase): """ Test that references types get deserialized correctly """ - course_key = CourseKey.from_string('edX/toy/2012_Fall') + course_key = CourseKey.from_string('edX/test_import/2014_Fall') def setup_test(): course = self.draft_store.get_course(course_key) @@ -548,8 +555,7 @@ class TestMongoModuleStore(TestMongoModuleStoreBase): check_mongo_fields() @patch('xmodule.video_module.video_module.edxval_api', None) - @patch('xmodule.tabs.CourseTab.from_json', side_effect=mock_tab_from_json) - def test_export_course_image(self, _from_json): + def test_export_course_image(self): """ Test to make sure that we have a course image in the contentstore, then export it to ensure it gets copied to both file locations. @@ -567,8 +573,7 @@ class TestMongoModuleStore(TestMongoModuleStoreBase): assert path(root_dir / 'test_export/static/images_course_image.jpg').isfile() @patch('xmodule.video_module.video_module.edxval_api', None) - @patch('xmodule.tabs.CourseTab.from_json', side_effect=mock_tab_from_json) - def test_export_course_image_nondefault(self, _from_json): + def test_export_course_image_nondefault(self): """ Make sure that if a non-default image path is specified that we don't export it to the static default location diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py index 3550ef69ea..55623b217e 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py @@ -39,7 +39,7 @@ from xmodule.modulestore.split_mongo.split import SplitMongoModuleStore from xmodule.modulestore.tests.factories import check_mongo_calls from xmodule.modulestore.tests.mongo_connection import MONGO_HOST, MONGO_PORT_NUM from xmodule.modulestore.tests.test_modulestore import check_has_course_method -from xmodule.modulestore.tests.utils import mock_tab_from_json +from xmodule.tabs import CourseTab from xmodule.x_module import XModuleMixin BRANCH_NAME_DRAFT = ModuleStoreEnum.BranchName.draft @@ -96,21 +96,10 @@ class SplitModuleTest(unittest.TestCase): "user_id": TEST_USER_ID, "fields": { "tabs": [ - { - "type": "courseware" - }, - { - "type": "course_info", - "name": "Course Info" - }, - { - "type": "discussion", - "name": "Discussion" - }, - { - "type": "wiki", - "name": "Wiki" - } + CourseTab.load('courseware'), + CourseTab.load('course_info'), + CourseTab.load('discussion'), + CourseTab.load('wiki'), ], "start": _date_field.from_json("2013-02-14T05:00"), "display_name": "The Ancient Greek Hero", @@ -157,31 +146,16 @@ class SplitModuleTest(unittest.TestCase): ("course", "head12345"): { "end": _date_field.from_json("2013-04-13T04:30"), "tabs": [ - { - "type": "courseware" - }, - { - "type": "course_info", - "name": "Course Info" - }, - { - "type": "discussion", - "name": "Discussion" - }, - { - "type": "wiki", - "name": "Wiki" - }, - { - "type": "static_tab", - "name": "Syllabus", - "url_slug": "01356a17b5924b17a04b7fc2426a3798" - }, - { - "type": "static_tab", - "name": "Advice for Students", - "url_slug": "57e9991c0d794ff58f7defae3e042e39" - } + CourseTab.load('courseware'), + CourseTab.load('course_info'), + CourseTab.load('discussion'), + CourseTab.load('wiki'), + CourseTab.load( + 'static_tab', name="Syllabus", url_slug="01356a17b5924b17a04b7fc2426a3798" + ), + CourseTab.load( + 'static_tab', name="Advice for Students", url_slug="57e9991c0d794ff58f7defae3e042e" + ), ], "graceperiod": _time_delta_field.from_json("2 hours 0 minutes 0 seconds"), "grading_policy": { @@ -345,21 +319,10 @@ class SplitModuleTest(unittest.TestCase): "user_id": TEST_USER_ID, "fields": { "tabs": [ - { - "type": "courseware" - }, - { - "type": "course_info", - "name": "Course Info" - }, - { - "type": "discussion", - "name": "Discussion" - }, - { - "type": "wiki", - "name": "Wiki" - } + CourseTab.load('courseware'), + CourseTab.load('course_info'), + CourseTab.load('discussion'), + CourseTab.load('wiki'), ], "start": _date_field.from_json("2013-02-14T05:00"), "display_name": "A wonderful course", @@ -453,21 +416,10 @@ class SplitModuleTest(unittest.TestCase): "user_id": TEST_GUEST_USER_ID, "fields": { "tabs": [ - { - "type": "courseware" - }, - { - "type": "course_info", - "name": "Course Info" - }, - { - "type": "discussion", - "name": "Discussion" - }, - { - "type": "wiki", - "name": "Wiki" - } + CourseTab.load('courseware'), + CourseTab.load('course_info'), + CourseTab.load('discussion'), + CourseTab.load('wiki'), ], "start": _date_field.from_json("2013-03-14T05:00"), "display_name": "Yet another contender", @@ -582,8 +534,7 @@ class SplitModuleTest(unittest.TestCase): class TestHasChildrenAtDepth(SplitModuleTest): """Test the has_children_at_depth method of XModuleMixin. """ - @patch('xmodule.tabs.CourseTab.from_json', side_effect=mock_tab_from_json) - def test_has_children_at_depth(self, _from_json): + def test_has_children_at_depth(self): course_locator = CourseLocator( org='testx', course='GreekHero', run="run", branch=BRANCH_NAME_DRAFT ) @@ -622,8 +573,7 @@ class SplitModuleCourseTests(SplitModuleTest): Course CRUD operation tests ''' - @patch('xmodule.tabs.CourseTab.from_json', side_effect=mock_tab_from_json) - def test_get_courses(self, _from_json): + def test_get_courses(self): courses = modulestore().get_courses(branch=BRANCH_NAME_DRAFT) # should have gotten 3 draft courses assert len(courses) == 3, 'Wrong number of courses' @@ -639,8 +589,7 @@ class SplitModuleCourseTests(SplitModuleTest): assert course.edited_by == TEST_ASSISTANT_USER_ID self.assertDictEqual(course.grade_cutoffs, {"Pass": 0.45}) - @patch('xmodule.tabs.CourseTab.from_json', side_effect=mock_tab_from_json) - def test_get_courses_with_same_course_index(self, _from_json): + def test_get_courses_with_same_course_index(self): """ Test that if two courses point to same course index, `get_courses` should return both courses. @@ -659,8 +608,7 @@ class SplitModuleCourseTests(SplitModuleTest): assert len(courses) == 4 assert new_draft_course.id.version_agnostic() in [c.id for c in courses] - @patch('xmodule.tabs.CourseTab.from_json', side_effect=mock_tab_from_json) - def test_get_org_courses(self, _from_json): + def test_get_org_courses(self): courses = modulestore().get_courses(branch=BRANCH_NAME_DRAFT, org='guestx') # should have gotten 1 draft courses @@ -677,8 +625,7 @@ class SplitModuleCourseTests(SplitModuleTest): courses = modulestore().get_courses(branch=BRANCH_NAME_DRAFT) assert len(courses) == 3 - @patch('xmodule.tabs.CourseTab.from_json', side_effect=mock_tab_from_json) - def test_branch_requests(self, _from_json): + def test_branch_requests(self): # query w/ branch qualifier (both draft and published) def _verify_published_course(courses_published): """ Helper function for verifying published course. """ @@ -706,8 +653,7 @@ class SplitModuleCourseTests(SplitModuleTest): locator_key_fields=['org', 'course', 'run'] ) - @patch('xmodule.tabs.CourseTab.from_json', side_effect=mock_tab_from_json) - def test_get_course(self, _from_json): + def test_get_course(self): ''' Test the various calling forms for get_course ''' @@ -760,8 +706,7 @@ class SplitModuleCourseTests(SplitModuleTest): with pytest.raises(ItemNotFoundError): modulestore().get_course(CourseLocator(org='testx', course='GreekHero', run="run", branch=BRANCH_NAME_PUBLISHED)) # lint-amnesty, pylint: disable=line-too-long - @patch('xmodule.tabs.CourseTab.from_json', side_effect=mock_tab_from_json) - def test_cache(self, _from_json): + def test_cache(self): """ Test that the mechanics of caching work. """ @@ -773,8 +718,7 @@ class SplitModuleCourseTests(SplitModuleTest): assert BlockKey('chapter', 'chapter1') in block_map assert BlockKey('problem', 'problem3_2') in block_map - @patch('xmodule.tabs.CourseTab.from_json', side_effect=mock_tab_from_json) - def test_persist_dag(self, _from_json): + def test_persist_dag(self): """ try saving temporary xblocks """ @@ -919,8 +863,7 @@ class SplitModuleItemTests(SplitModuleTest): Item read tests including inheritance ''' - @patch('xmodule.tabs.CourseTab.from_json', side_effect=mock_tab_from_json) - def test_has_item(self, _from_json): + def test_has_item(self): ''' has_item(BlockUsageLocator) ''' @@ -969,8 +912,7 @@ class SplitModuleItemTests(SplitModuleTest): ) assert not modulestore().has_item(locator) - @patch('xmodule.tabs.CourseTab.from_json', side_effect=mock_tab_from_json) - def test_get_item(self, _from_json): + def test_get_item(self): ''' get_item(blocklocator) ''' @@ -1126,8 +1068,7 @@ class SplitModuleItemTests(SplitModuleTest): parent = modulestore().get_parent_location(locator) assert parent is None - @patch('xmodule.tabs.CourseTab.from_json', side_effect=mock_tab_from_json) - def test_get_children(self, _from_json): + def test_get_children(self): """ Test the existing get_children method on xdescriptors """ @@ -1482,8 +1423,7 @@ class TestItemCrud(SplitModuleTest): other_updated = modulestore().update_item(other_block, self.user_id) assert moved_child.version_agnostic() in version_agnostic(other_updated.children) - @patch('xmodule.tabs.CourseTab.from_json', side_effect=mock_tab_from_json) - def test_update_definition(self, _from_json): + def test_update_definition(self): """ test updating an item's definition: ensure it gets versioned as well as the course getting versioned """ @@ -1752,8 +1692,7 @@ class TestCourseCreation(SplitModuleTest): fields['grading_policy']['GRADE_CUTOFFS'] ) - @patch('xmodule.tabs.CourseTab.from_json', side_effect=mock_tab_from_json) - def test_update_course_index(self, _from_json): + def test_update_course_index(self): """ Test the versions pointers. NOTE: you can change the org, course, or other things, but it's not clear how you'd find them again or associate them w/ existing student history since @@ -1808,8 +1747,7 @@ class TestCourseCreation(SplitModuleTest): dupe_course_key.org, dupe_course_key.course, dupe_course_key.run, user, BRANCH_NAME_DRAFT ) - @patch('xmodule.tabs.CourseTab.from_json', side_effect=mock_tab_from_json) - def test_bulk_ops_get_courses(self, _from_json): + def test_bulk_ops_get_courses(self): """ Test get_courses when some are created, updated, and deleted w/in a bulk operation """ @@ -1848,8 +1786,7 @@ class TestInheritance(SplitModuleTest): """ Test the metadata inheritance mechanism. """ - @patch('xmodule.tabs.CourseTab.from_json', side_effect=mock_tab_from_json) - def test_inheritance(self, _from_json): + def test_inheritance(self): """ The actual test """ @@ -1923,8 +1860,7 @@ class TestPublish(SplitModuleTest): """ Test the publishing api """ - @patch('xmodule.tabs.CourseTab.from_json', side_effect=mock_tab_from_json) - def test_publish_safe(self, _from_json): + def test_publish_safe(self): """ Test the standard patterns: publish to new branch, revise and publish """ @@ -1991,8 +1927,7 @@ class TestPublish(SplitModuleTest): with pytest.raises(ItemNotFoundError): modulestore().copy(self.user_id, source_course, destination_course, [problem1], []) - @patch('xmodule.tabs.CourseTab.from_json', side_effect=mock_tab_from_json) - def test_move_delete(self, _from_json): + def test_move_delete(self): """ Test publishing moves and deletes. """ diff --git a/common/lib/xmodule/xmodule/modulestore/tests/utils.py b/common/lib/xmodule/xmodule/modulestore/tests/utils.py index f59d7b6b75..78e2760ab2 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/utils.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/utils.py @@ -64,14 +64,6 @@ def create_modulestore_instance( ) -def mock_tab_from_json(tab_dict): - """ - Mocks out the CourseTab.from_json to just return the tab_dict itself so that we don't have to deal - with plugin errors. - """ - return tab_dict - - def add_temp_files_from_dict(file_dict, dir): # lint-amnesty, pylint: disable=redefined-builtin """ Takes in a dict formatted as: { file_name: content }, and adds files to directory diff --git a/common/lib/xmodule/xmodule/tabs.py b/common/lib/xmodule/xmodule/tabs.py index 099aa84a45..1292db60ac 100644 --- a/common/lib/xmodule/xmodule/tabs.py +++ b/common/lib/xmodule/xmodule/tabs.py @@ -381,7 +381,6 @@ class CourseTabList(List): __init__ method. This is because the default values are dependent on other information from within the course. """ - course_tabs = [ CourseTab.load('course_info'), CourseTab.load('courseware') @@ -406,11 +405,20 @@ class CourseTabList(List): discussion_tab, CourseTab.load('wiki'), CourseTab.load('progress'), + CourseTab.load('dates'), ]) - # While you should be able to do `tab.priority`, a lot of tests mock tabs to be a dict - # which causes them to throw an error on this line - course_tabs.sort(key=lambda tab: getattr(tab, 'priority', None) or float('inf')) - course.tabs.extend(course_tabs) + + # Cross reference existing slugs with slugs this method would add to not add duplicates. + existing_tab_slugs = {tab.type for tab in course.tabs if course.tabs} + tabs_to_add = [] + for tab in course_tabs: + if tab.type not in existing_tab_slugs: + tabs_to_add.append(tab) + + if tabs_to_add: + tabs_to_add.extend(course.tabs) + tabs_to_add.sort(key=lambda tab: tab.priority or float('inf')) + course.tabs = tabs_to_add @staticmethod def get_discussion(course): diff --git a/lms/djangoapps/courseware/tabs.py b/lms/djangoapps/courseware/tabs.py index 9c6a8f283e..54190fa833 100644 --- a/lms/djangoapps/courseware/tabs.py +++ b/lms/djangoapps/courseware/tabs.py @@ -367,10 +367,8 @@ def get_course_tab_list(user, course): if tab.type == 'static_tab' and tab.course_staff_only and \ not bool(user and has_access(user, 'staff', course, course.id)): continue - # We had initially created a CourseTab.load() for dates that ended up - # persisting the dates tab tomodulestore on Course Run creation, but - # ignoring any static dates tab here we can fix forward without - # allowing the bug to continue to surface + # We are phasing this out in https://github.com/openedx/edx-platform/pull/30045/, but need this + # until the backfill course tabs command is completed if tab.type == 'dates': continue course_tab_list.append(tab) diff --git a/openedx/core/djangoapps/content/course_overviews/management/commands/simulate_publish.py b/openedx/core/djangoapps/content/course_overviews/management/commands/simulate_publish.py index 177fb8f31f..f4736c6b3e 100644 --- a/openedx/core/djangoapps/content/course_overviews/management/commands/simulate_publish.py +++ b/openedx/core/djangoapps/content/course_overviews/management/commands/simulate_publish.py @@ -85,7 +85,7 @@ class Command(BaseCommand): dest='show_receivers', action='store_true', help=('Display the list of possible receiver functions and exit.') - ), # lint-amnesty, pylint: disable=trailing-comma-tuple + ) parser.add_argument( '--dry-run', dest='dry_run', @@ -95,7 +95,7 @@ class Command(BaseCommand): "expensive modulestore query to find courses, but it will " "not emit any signals." ) - ), # lint-amnesty, pylint: disable=trailing-comma-tuple + ) parser.add_argument( '--receivers', dest='receivers', @@ -141,7 +141,7 @@ class Command(BaseCommand): "process. However, if you know what you're doing and need to " "override that behavior, use this flag." ) - ), # lint-amnesty, pylint: disable=trailing-comma-tuple + ) parser.add_argument( '--skip-ccx', dest='skip_ccx', @@ -155,12 +155,12 @@ class Command(BaseCommand): "if you know what you're doing, you can disable this behavior " "with this flag, so that CCX receivers are omitted." ) - ), # lint-amnesty, pylint: disable=trailing-comma-tuple + ) parser.add_argument( '--args-from-database', action='store_true', help='Use arguments from the SimulateCoursePublishConfig model instead of the command line.', - ), # lint-amnesty, pylint: disable=trailing-comma-tuple + ) def get_args_from_database(self): """ Returns an options dictionary from the current SimulateCoursePublishConfig model. """ diff --git a/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py b/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py index 04ed90de15..d9b8859a60 100644 --- a/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py +++ b/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py @@ -62,7 +62,7 @@ class CourseOverviewTestCase(CatalogIntegrationMixin, ModuleStoreTestCase, Cache None: None, } - COURSE_OVERVIEW_TABS = {'courseware', 'info', 'textbooks', 'discussion', 'wiki', 'progress'} + COURSE_OVERVIEW_TABS = {'courseware', 'info', 'textbooks', 'discussion', 'wiki', 'progress', 'dates'} ENABLED_SIGNALS = ['course_deleted', 'course_published']