From 61b93d953d8ebf2a181f5a2c01335ceff9a86017 Mon Sep 17 00:00:00 2001 From: Dillon Dumesnil Date: Wed, 9 Mar 2022 12:27:18 -0500 Subject: [PATCH] feat: Add backfill course tabs management command Previously, course tabs would only be created once and never try to update the default tabs again. This leads to an issue if you ever want to add a new tab. With this command, you can now update the default tabs for all existing courses and new courses will pick it up upon creation when CourseTabList.initialize_default is called. --- .../commands/backfill_course_tabs.py | 53 +++++++ .../tests/test_backfill_course_tabs.py | 131 ++++++++++++++++ cms/djangoapps/contentstore/tasks.py | 9 +- .../contentstore/views/tests/test_tabs.py | 7 +- .../test_cross_modulestore_import_export.py | 4 +- .../tests/test_mixed_modulestore.py | 9 +- .../xmodule/modulestore/tests/test_mongo.py | 91 +++++------ .../tests/test_split_modulestore.py | 145 +++++------------- .../xmodule/modulestore/tests/utils.py | 8 - common/lib/xmodule/xmodule/tabs.py | 18 ++- lms/djangoapps/courseware/tabs.py | 6 +- .../management/commands/simulate_publish.py | 10 +- .../tests/test_course_overviews.py | 2 +- 13 files changed, 304 insertions(+), 189 deletions(-) create mode 100644 cms/djangoapps/contentstore/management/commands/backfill_course_tabs.py create mode 100644 cms/djangoapps/contentstore/management/commands/tests/test_backfill_course_tabs.py 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']