diff --git a/cms/djangoapps/contentstore/rest_api/v0/tests/test_tabs.py b/cms/djangoapps/contentstore/rest_api/v0/tests/test_tabs.py index 3a159cf042..b5e53fadb9 100644 --- a/cms/djangoapps/contentstore/rest_api/v0/tests/test_tabs.py +++ b/cms/djangoapps/contentstore/rest_api/v0/tests/test_tabs.py @@ -6,6 +6,7 @@ import json from urllib.parse import urlencode import ddt +import random from django.urls import reverse from xmodule.modulestore.tests.factories import ItemFactory from xmodule.tabs import CourseTabList @@ -95,36 +96,38 @@ class TabsAPITests(CourseTestCase): content_type="application/json", ) - def test_reorder_tabs(self): + def test_reorder_static_tabs(self): """ - Test re-ordering of tabs + Test re-ordering of static tabs in a course. """ - # get the original tab ids - orig_tab_ids = [tab.tab_id for tab in self.course.tabs] - tab_ids = list(orig_tab_ids) - num_orig_tabs = len(orig_tab_ids) + # get the original tabs + course_tabs = list(self.course.tabs) + num_orig_tabs = len(self.course.tabs) # make sure we have enough tabs to play around with assert num_orig_tabs >= 5 - # reorder the last two tabs - tab_ids[num_orig_tabs - 1], tab_ids[num_orig_tabs - 2] = tab_ids[num_orig_tabs - 2], tab_ids[num_orig_tabs - 1] + # Randomize the order of static tabs, leaving the rest intact + course_tabs.sort(key=lambda tab: (100 + random.random()) if tab.type == 'static_tab' else tab.priority) - # remove the third to the last tab, the removed tab has to be a static tab - # (the code needs to handle the case where tabs requested for re-ordering is a subset of the tabs in the course) - removed_tab = tab_ids.pop(num_orig_tabs - 3) - self.assertEqual(len(tab_ids), num_orig_tabs - 1) + tabs_data = [ + {'tab_locator': str(self.course.id.make_usage_key("static_tab", tab.url_slug))} + for tab in course_tabs + if tab.type == 'static_tab' + ] + # Remove one tab randomly. This shouldn't delete the tab. + tabs_data.pop() - # post the request - resp = self.make_reorder_tabs_request([{"tab_id": tab_id} for tab_id in tab_ids if 'static' in tab_id]) + # post the request with the reordered static tabs only + resp = self.make_reorder_tabs_request(tabs_data) assert resp.status_code == 204 - # reload the course and verify the new tab order + # Reload the course and verify the new tab order self.reload_course() + reordered_tab_ids = [tab.tab_id for tab in course_tabs] new_tab_ids = [tab.tab_id for tab in self.course.tabs] - assert new_tab_ids == tab_ids + [removed_tab] - assert new_tab_ids != orig_tab_ids + assert new_tab_ids == reordered_tab_ids def test_reorder_tabs_invalid_list(self): """ diff --git a/cms/djangoapps/contentstore/rest_api/v0/views/advanced_settings.py b/cms/djangoapps/contentstore/rest_api/v0/views/advanced_settings.py index 320ee9054d..899513dfd1 100644 --- a/cms/djangoapps/contentstore/rest_api/v0/views/advanced_settings.py +++ b/cms/djangoapps/contentstore/rest_api/v0/views/advanced_settings.py @@ -24,7 +24,7 @@ class AdvancedCourseSettingsView(DeveloperErrorViewMixin, APIView): class FilterQuery(forms.Form): """ - Form for validating query marameters passed to advanced course settings view + Form for validating query parameters passed to advanced course settings view to filter the data it returns. """ filter_fields = forms.CharField(strip=True, empty_value=None, required=False) diff --git a/cms/djangoapps/contentstore/rest_api/v0/views/tabs.py b/cms/djangoapps/contentstore/rest_api/v0/views/tabs.py index e7f52d2143..ce5e72de43 100644 --- a/cms/djangoapps/contentstore/rest_api/v0/views/tabs.py +++ b/cms/djangoapps/contentstore/rest_api/v0/views/tabs.py @@ -177,7 +177,7 @@ class CourseTabReorderView(DeveloperErrorViewMixin, APIView): return super().handle_exception(exc) @apidocs.schema( - body=[TabIDLocatorSerializer], + body=TabIDLocatorSerializer(many=True), parameters=[ apidocs.string_parameter("course_id", apidocs.ParameterLocation.PATH, description="Course ID"), ], @@ -198,24 +198,12 @@ class CourseTabReorderView(DeveloperErrorViewMixin, APIView): Move course tabs: POST /api/contentstore/v0/tabs/{course_id}/reorder [ - { - "tab_id": "info" - }, - { - "tab_id": "courseware" - }, { "tab_locator": "block-v1:TstX+DemoX+Demo+type@static_tab+block@d26fcb0e93824fbfa5c9e5f100e2511a" }, { - "tab_id": "wiki" + "tab_locator": "block-v1:TstX+DemoX+Demo+type@static_tab+block@a011f1bd05af4578ae397ed8cabccf62" }, - { - "tab_id": "discussion" - }, - { - "tab_id": "progress" - } ] @@ -233,7 +221,7 @@ class CourseTabReorderView(DeveloperErrorViewMixin, APIView): tab_id_locators.is_valid(raise_exception=True) reorder_tabs_handler( course_module, - {"tabs": tab_id_locators.validated_data}, + tab_id_locators.validated_data, request.user, ) return Response(status=status.HTTP_204_NO_CONTENT) diff --git a/cms/djangoapps/contentstore/views/tabs.py b/cms/djangoapps/contentstore/views/tabs.py index 4788ddf4e7..5482be63dc 100644 --- a/cms/djangoapps/contentstore/views/tabs.py +++ b/cms/djangoapps/contentstore/views/tabs.py @@ -1,7 +1,7 @@ """ Views related to course tabs """ -from typing import Dict, Iterable, List, Optional +from typing import Dict, Iterable, List, Optional, Union from django.contrib.auth import get_user_model from django.contrib.auth.decorators import login_required @@ -110,7 +110,7 @@ def update_tabs_handler(course_item: CourseBlock, tabs_data: Dict, user: User) - """ if "tabs" in tabs_data: - reorder_tabs_handler(course_item, tabs_data, user) + reorder_tabs_handler(course_item, tabs_data["tabs"], user) elif "tab_id_locator" in tabs_data: edit_tab_handler(course_item, tabs_data, user) else: @@ -122,58 +122,45 @@ def reorder_tabs_handler(course_item, tabs_data, user): Helper function for handling reorder of static tabs request """ - # Tabs are identified by tab_id or locators. - # The locators are used to identify static tabs since they are xmodules. + # Static tabs are identified by locators (a UsageKey) instead of a tab id like + # other tabs. These can be used to identify static tabs since they are xmodules. # Although all tabs have tab_ids, newly created static tabs do not know # their tab_ids since the xmodule editor uses only locators to identify new objects. - requested_tab_id_locators = tabs_data["tabs"] - - #get original tab list of only static tabs with their original index(position) in the full course tabs list - old_tab_dict = {} - for idx, tab in enumerate(course_item.tabs): - if isinstance(tab, StaticTab): - old_tab_dict[tab] = idx - old_tab_list = list(old_tab_dict.keys()) - - new_tab_list = create_new_list(requested_tab_id_locators, old_tab_list) - - # Creates a full new course tab list of both default and static course tabs - # by looping through the new tab list of static only tabs and - # putting them in their new position in the list of course item tabs - # original_idx gives the list of positions of all static tabs in course tabs originally - full_new_tab_list = course_item.tabs - original_idx = list(old_tab_dict.values()) - for i in range(len(new_tab_list)): - full_new_tab_list[original_idx[i]] = new_tab_list[i] + new_tab_list = create_new_list(tabs_data, course_item.tabs) # validate the tabs to make sure everything is Ok (e.g., did the client try to reorder unmovable tabs?) try: - CourseTabList.validate_tabs(full_new_tab_list) + CourseTabList.validate_tabs(new_tab_list) except InvalidTabsException as exception: raise ValidationError({"error": f"New list of tabs is not valid: {str(exception)}."}) from exception - # persist the new order of the tabs - course_item.tabs = full_new_tab_list + course_item.tabs = new_tab_list + modulestore().update_item(course_item, user.id) -def create_new_list(requested_tab_id_locators, old_tab_list): +def create_new_list(tab_locators, old_tab_list): """ - Helper function for creating a new course tab list in the new order - of reordered tabs + Helper function for creating a new course tab list in the new order of + reordered tabs. + + It will take tab_locators for static tabs and resolve them to actual tab + instances. """ new_tab_list = [] - for tab_id_locator in requested_tab_id_locators: - tab = get_tab_by_tab_id_locator(old_tab_list, tab_id_locator) + for tab_locator in tab_locators: + tab = get_tab_by_tab_id_locator(old_tab_list, tab_locator) if tab is None: - raise ValidationError({"error": f"Tab with id_locator '{tab_id_locator}' does not exist."}) + raise ValidationError({"error": f"Tab with id_locator '{tab_locator}' does not exist."}) + if not isinstance(tab, StaticTab): + raise ValidationError({"error": f"Cannot reorder tabs of type '{tab.type}'"}) new_tab_list.append(tab) # the old_tab_list may contain additional tabs that were not rendered in the UI because of # global or course settings. so add those to the end of the list. non_displayed_tabs = set(old_tab_list) - set(new_tab_list) new_tab_list.extend(non_displayed_tabs) - return new_tab_list + return sorted(new_tab_list, key=lambda item: item.priority or float('inf')) def edit_tab_handler(course_item: CourseBlock, tabs_data: Dict, user: User): @@ -212,11 +199,12 @@ def get_tab_by_tab_id_locator(tab_list: List[CourseTab], tab_id_locator: Dict[st return tab -def get_tab_by_locator(tab_list: List[CourseTab], usage_key_string: str) -> Optional[CourseTab]: +def get_tab_by_locator(tab_list: List[CourseTab], tab_location: Union[str, UsageKey]) -> Optional[CourseTab]: """ Look for a tab with the specified locator. Returns the first matching tab. """ - tab_location = UsageKey.from_string(usage_key_string) + if isinstance(tab_location, str): + tab_location = UsageKey.from_string(tab_location) item = modulestore().get_item(tab_location) static_tab = StaticTab( name=item.display_name, diff --git a/cms/djangoapps/contentstore/views/tests/test_tabs.py b/cms/djangoapps/contentstore/views/tests/test_tabs.py index 0ccc8878ca..5ba7a4a9f1 100644 --- a/cms/djangoapps/contentstore/views/tests/test_tabs.py +++ b/cms/djangoapps/contentstore/views/tests/test_tabs.py @@ -1,6 +1,8 @@ """ Tests for tab functions (just primitive). """ import json +import random + from cms.djangoapps.contentstore.tests.utils import CourseTestCase from cms.djangoapps.contentstore.utils import reverse_course_url @@ -38,9 +40,9 @@ class TabsPageTests(CourseTestCase): def check_invalid_tab_id_response(self, resp): """Verify response is an error listing the invalid_tab_id""" - self.assertEqual(resp.status_code, 400) + assert resp.status_code == 400 resp_content = json.loads(resp.content.decode('utf-8')) - self.assertIn("error", resp_content) + assert "error" in resp_content def test_not_implemented(self): """Verify not implemented errors""" @@ -75,52 +77,38 @@ class TabsPageTests(CourseTestCase): def test_reorder_tabs(self): """Test re-ordering of tabs""" - # get the original tab ids - orig_tab_ids = [tab.tab_id for tab in self.course.tabs] - tab_ids = list(orig_tab_ids) - num_orig_tabs = len(orig_tab_ids) + # get the original tabs + course_tabs = list(self.course.tabs) + num_orig_tabs = len(self.course.tabs) # make sure we have enough tabs to play around with - self.assertGreaterEqual(num_orig_tabs, 5) + assert num_orig_tabs >= 5 - # reorder the last two tabs - tab_ids[num_orig_tabs - 1], tab_ids[num_orig_tabs - 2] = tab_ids[num_orig_tabs - 2], tab_ids[num_orig_tabs - 1] + # Randomise the order of static tabs, leave the rest intact + course_tabs.sort(key=lambda tab: (100 + random.random()) if tab.type == 'static_tab' else tab.priority) - # remove the third to the last tab, the removed tab has to be a static tab - # (the code needs to handle the case where tabs requested for re-ordering is a subset of the tabs in the course) - removed_tab = tab_ids.pop(num_orig_tabs - 3) - self.assertEqual(len(tab_ids), num_orig_tabs - 1) + tabs_data = [ + {'tab_locator': str(self.course.id.make_usage_key("static_tab", tab.url_slug))} + for tab in course_tabs + if tab.type == 'static_tab' + ] + # Remove one tab randomly. This shouldn't delete the tab. + tabs_data.pop() # post the request with the reordered static tabs only resp = self.client.ajax_post( self.url, - data={'tabs': [{'tab_id': tab_id} for tab_id in tab_ids if 'static' in tab_id]}, + data={ + 'tabs': tabs_data + }, ) - self.assertEqual(resp.status_code, 204) + assert resp.status_code == 204 # reload the course and verify the new tab order self.reload_course() + reordered_tab_ids = [tab.tab_id for tab in course_tabs] new_tab_ids = [tab.tab_id for tab in self.course.tabs] - self.assertEqual(new_tab_ids, tab_ids + [removed_tab]) - self.assertNotEqual(new_tab_ids, orig_tab_ids) - - def test_reorder_tabs_invalid_list(self): - """Test re-ordering of tabs with invalid tab list""" - - orig_tab_ids = [tab.tab_id for tab in self.course.tabs] - tab_ids = list(orig_tab_ids) - - # reorder the first two tabs - tab_ids[0], tab_ids[1] = tab_ids[1], tab_ids[0] - - # post the request - resp = self.client.ajax_post( - self.url, - data={'tabs': [{'tab_id': tab_id} for tab_id in tab_ids]}, - ) - self.assertEqual(resp.status_code, 400) - resp_content = json.loads(resp.content.decode('utf-8')) - self.assertIn("error", resp_content) + assert new_tab_ids == reordered_tab_ids def test_reorder_tabs_invalid_tab(self): """Test re-ordering of tabs with invalid tab""" @@ -141,7 +129,7 @@ class TabsPageTests(CourseTestCase): old_tab = CourseTabList.get_tab_by_type(self.course.tabs, tab_type) # visibility should be different from new setting - self.assertNotEqual(old_tab.is_hidden, new_is_hidden_setting) + assert old_tab.is_hidden != new_is_hidden_setting # post the request resp = self.client.ajax_post( @@ -151,12 +139,12 @@ class TabsPageTests(CourseTestCase): 'is_hidden': new_is_hidden_setting, }), ) - self.assertEqual(resp.status_code, 204) + assert resp.status_code == 204 # reload the course and verify the new visibility setting self.reload_course() new_tab = CourseTabList.get_tab_by_type(self.course.tabs, tab_type) - self.assertEqual(new_tab.is_hidden, new_is_hidden_setting) + assert new_tab.is_hidden == new_is_hidden_setting def test_toggle_tab_visibility(self): """Test toggling of tab visibility""" @@ -182,15 +170,15 @@ class TabsPageTests(CourseTestCase): preview_url = f'/xblock/{self.test_tabs[0].location}/{STUDENT_VIEW}' resp = self.client.get(preview_url, HTTP_ACCEPT='application/json') - self.assertEqual(resp.status_code, 200) + assert resp.status_code == 200 resp_content = json.loads(resp.content.decode('utf-8')) html = resp_content['html'] # Verify that the HTML contains the expected elements - self.assertIn('', html) - self.assertIn('Duplicate this component', html) - self.assertIn('Delete this component', html) - self.assertIn('', html) + assert '' in html + assert 'Duplicate this component' in html + assert 'Delete this component' in html + assert '' in html class PrimitiveTabEdit(ModuleStoreTestCase): @@ -205,16 +193,17 @@ 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'} tabs.primitive_delete(course, 2) - self.assertNotIn({'type': 'textbooks'}, course.tabs) + assert {'type': 'progress'} not in course.tabs # Check that discussion has shifted up - self.assertEqual(course.tabs[2], {'type': 'discussion', 'name': 'Discussion'}) + assert course.tabs[2] == {'type': 'discussion', 'name': 'Discussion'} def test_insert(self): """Test primitive tab insertion.""" course = CourseFactory.create() tabs.primitive_insert(course, 2, 'pdf_textbooks', 'aname') - self.assertEqual(course.tabs[2], {'type': 'pdf_textbooks', 'name': 'aname'}) + assert course.tabs[2] == {'type': 'pdf_textbooks', 'name': 'aname'} with self.assertRaises(ValueError): tabs.primitive_insert(course, 0, 'pdf_textbooks', 'aname') with self.assertRaises(ValueError): @@ -225,4 +214,4 @@ class PrimitiveTabEdit(ModuleStoreTestCase): course = CourseFactory.create() tabs.primitive_insert(course, 3, 'pdf_textbooks', 'aname') course2 = modulestore().get_course(course.id) - self.assertEqual(course2.tabs[3], {'type': 'pdf_textbooks', 'name': 'aname'}) + assert course2.tabs[3] == {'type': 'pdf_textbooks', 'name': 'aname'} diff --git a/cms/templates/edit-tabs.html b/cms/templates/edit-tabs.html index 056bff979d..472f3fa908 100644 --- a/cms/templates/edit-tabs.html +++ b/cms/templates/edit-tabs.html @@ -9,7 +9,7 @@ from openedx.core.djangolib.js_utils import js_escaped_string from cms.djangoapps.contentstore.utils import get_pages_and_resources_url %> -<%block name="title">${_("Pages")}%block> +<%block name="title">${_("Custom pages")}%block> <%block name="bodyclass">is-signedin course view-static-pages%block> <%block name="header_extras"> @@ -52,14 +52,14 @@