From d865e9096e352498b0412a60e91e28f08c54b67b Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Tue, 25 Mar 2014 12:54:27 -0400 Subject: [PATCH 1/3] Add the ability to reorder Pages and hide the Wiki page. --- .../contentstore/features/pages.feature | 39 +++- cms/djangoapps/contentstore/features/pages.py | 101 +++++++--- .../commands/tests/test_git_export.py | 2 +- .../contentstore/tests/test_contentstore.py | 21 ++- .../tests/test_course_settings.py | 2 +- .../contentstore/tests/test_export_git.py | 2 +- .../contentstore/tests/test_orphan.py | 2 +- cms/djangoapps/contentstore/tests/utils.py | 19 +- cms/djangoapps/contentstore/views/tabs.py | 177 ++++++++++------- .../views/tests/test_course_index.py | 4 +- .../contentstore/views/tests/test_tabs.py | 166 +++++++++++++++- .../views/tests/test_textbooks.py | 25 +-- cms/static/coffee/src/views/tabs.coffee | 35 +++- cms/templates/edit-tabs.html | 72 ++++--- .../xmodule/xmodule/modulestore/mongo/base.py | 2 +- common/lib/xmodule/xmodule/modulestore/xml.py | 2 +- common/lib/xmodule/xmodule/tabs.py | 178 +++++++++++++----- common/lib/xmodule/xmodule/tests/test_tabs.py | 113 ++++++++--- lms/djangoapps/courseware/tests/test_tabs.py | 2 +- lms/djangoapps/courseware/views.py | 2 +- .../courseware/course_navigation.html | 2 +- 21 files changed, 727 insertions(+), 241 deletions(-) diff --git a/cms/djangoapps/contentstore/features/pages.feature b/cms/djangoapps/contentstore/features/pages.feature index 22865e8ece..e0c4fa0899 100644 --- a/cms/djangoapps/contentstore/features/pages.feature +++ b/cms/djangoapps/contentstore/features/pages.feature @@ -15,10 +15,6 @@ Feature: CMS.Pages When I confirm the prompt Then I should not see any static pages - Scenario: Users can see built-in pages - Given I have opened the pages page in a new course - Then I should see the default built-in pages - # Safari won't update the name properly @skip_safari Scenario: Users can edit static pages @@ -31,7 +27,36 @@ Feature: CMS.Pages @skip_safari Scenario: Users can reorder static pages Given I have created two different static pages - When I reorder the static tabs - Then the static tabs are in the reverse order + When I reorder the static pages + Then the static pages are in the reverse order And I reload the page - Then the static tabs are in the reverse order + Then the static pages are in the reverse order + + Scenario: Users can reorder built-in pages + Given I have opened the pages page in a new course + Then the built-in pages are in the default order + When I reorder the pages + Then the built-in pages are in the reverse order + And I reload the page + Then the built-in pages are in the reverse order + + Scenario: Users can reorder built-in pages amongst static pages + Given I have created two different static pages + Then the pages are in the default order + When I reorder the pages + Then the pages are in the reverse order + And I reload the page + Then the pages are in the reverse order + + Scenario: Users can toggle visibility on hideable pages + Given I have opened the pages page in a new course + Then I should see the "wiki" page as "visible" + When I toggle the visibility of the "wiki" page + Then I should see the "wiki" page as "hidden" + And I reload the page + Then I should see the "wiki" page as "hidden" + When I toggle the visibility of the "wiki" page + Then I should see the "wiki" page as "visible" + And I reload the page + Then I should see the "wiki" page as "visible" + diff --git a/cms/djangoapps/contentstore/features/pages.py b/cms/djangoapps/contentstore/features/pages.py index f4de3455da..f91eb3bd6a 100644 --- a/cms/djangoapps/contentstore/features/pages.py +++ b/cms/djangoapps/contentstore/features/pages.py @@ -3,7 +3,7 @@ # pylint: disable=W0613 from lettuce import world, step -from nose.tools import assert_equal # pylint: disable=E0611 +from nose.tools import assert_equal, assert_in # pylint: disable=E0611 @step(u'I go to the pages page$') @@ -33,15 +33,6 @@ def not_see_any_static_pages(step): assert (world.is_css_not_present(pages_css, wait_time=30)) -@step(u'I should see the default built-in pages') -def see_default_built_in_pages(step): - expected_pages = ['Courseware', 'Course Info', 'Discussion', 'Wiki', 'Progress'] - pages = world.css_find("div.course-nav-tab-header h3.title") - assert_equal(len(expected_pages), len(pages)) - for i, page_name in enumerate(expected_pages): - assert_equal(pages[i].text, page_name) - - @step(u'I "(edit|delete)" the static page$') def click_edit_or_delete(step, edit_or_delete): button_css = 'ul.component-actions a.%s-button' % edit_or_delete @@ -60,15 +51,9 @@ def change_name(step, new_name): world.css_click(save_button) -@step(u'I reorder the static tabs') -def reorder_tabs(_step): - # For some reason, the drag_and_drop method did not work in this case. - draggables = world.css_find('.component .drag-handle') - source = draggables.first - target = draggables.last - source.action_chains.click_and_hold(source._element).perform() # pylint: disable=protected-access - source.action_chains.move_to_element_with_offset(target._element, 0, 50).perform() # pylint: disable=protected-access - source.action_chains.release().perform() +@step(u'I reorder the static pages') +def reorder_static_pages(_step): + reorder_pages_with_css_class('.component') @step(u'I have created a static page') @@ -89,21 +74,79 @@ def create_two_pages(step): step.given('I "edit" the static page') step.given('I change the name to "First"') step.given('I add a new static page') - # Verify order of tabs - _verify_tab_names('First', 'Empty') + # Verify order of pages + _verify_page_names('First', 'Empty') -@step(u'the static tabs are in the reverse order') -def tabs_in_reverse_order(step): - _verify_tab_names('Empty', 'First') +@step(u'the static pages are in the reverse order') +def static_pages_in_reverse_order(step): + _verify_page_names('Empty', 'First') -def _verify_tab_names(first, second): +def _verify_page_names(first, second): world.wait_for( func=lambda _: len(world.css_find('.xmodule_StaticTabModule')) == 2, timeout=200, - timeout_msg="Timed out waiting for two tabs to be present" + timeout_msg="Timed out waiting for two pages to be present" ) - tabs = world.css_find('.xmodule_StaticTabModule') - assert tabs[0].text == first - assert tabs[1].text == second + pages = world.css_find('.xmodule_StaticTabModule') + assert pages[0].text == first + assert pages[1].text == second + + +@step(u'the built-in pages are in the default order') +def built_in_pages_in_default_order(step): + expected_pages = ['Courseware', 'Course Info', 'Discussion', 'Wiki', 'Progress'] + see_pages_in_expected_order(expected_pages) + + +@step(u'the built-in pages are in the reverse order') +def built_in_pages_in_reverse_order(step): + expected_pages = ['Courseware', 'Course Info', 'Wiki', 'Progress', 'Discussion'] + see_pages_in_expected_order(expected_pages) + + +@step(u'the pages are in the default order') +def pages_in_default_order(step): + expected_pages = ['Courseware', 'Course Info', 'Discussion', 'Wiki', 'Progress', 'First', 'Empty'] + see_pages_in_expected_order(expected_pages) + + +@step(u'the pages are in the reverse order') +def pages_in_reverse_order(step): + expected_pages = ['Courseware', 'Course Info', 'Wiki', 'Progress', 'First', 'Empty', 'Discussion'] + see_pages_in_expected_order(expected_pages) + + +@step(u'I reorder the pages') +def reorder_pages(step): + reorder_pages_with_css_class('.sortable-tab') + + +@step(u'I should see the "([^"]*)" page as "(visible|hidden)"$') +def page_is_visible_or_hidden(step, page_id, visible_or_hidden): + hidden = visible_or_hidden == "hidden" + assert world.css_find("li[data-tab-id='{0}'] input.toggle-checkbox".format(page_id)).checked == hidden + + +@step(u'I toggle the visibility of the "([^"]*)" page') +def page_toggle_visibility(step, page_id): + world.css_find("li[data-tab-id='{0}'] input.toggle-checkbox".format(page_id))[0].click() + + +def reorder_pages_with_css_class(css_class): + # For some reason, the drag_and_drop method did not work in this case. + draggables = world.css_find(css_class + ' .drag-handle') + source = draggables.first + target = draggables.last + source.action_chains.click_and_hold(source._element).perform() # pylint: disable=protected-access + source.action_chains.move_to_element_with_offset(target._element, 0, 50).perform() # pylint: disable=protected-access + source.action_chains.release().perform() + + +def see_pages_in_expected_order(page_names_in_expected_order): + pages = world.css_find("li.course-tab") + assert_equal(len(page_names_in_expected_order), len(pages)) + for i, page_name in enumerate(page_names_in_expected_order): + assert_in(page_name, pages[i].text) + diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_git_export.py b/cms/djangoapps/contentstore/management/commands/tests/test_git_export.py index 228571035a..1fa81e9a64 100644 --- a/cms/djangoapps/contentstore/management/commands/tests/test_git_export.py +++ b/cms/djangoapps/contentstore/management/commands/tests/test_git_export.py @@ -151,7 +151,7 @@ class TestGitExport(CourseTestCase): self.assertEqual(expect_string, git_log) # Make changes to course so there is something commit - self.populateCourse() + self.populate_course() git_export_utils.export_to_git( self.course.id, 'file://{0}'.format(self.bare_repo_dir), diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index eb530cba89..ca23c4e51d 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -400,23 +400,24 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): course = module_store.get_item(course_location) - # reverse the ordering - reverse_tabs = [] + # reverse the ordering of the static tabs + reverse_static_tabs = [] + built_in_tabs = [] for tab in course.tabs: if tab['type'] == 'static_tab': - reverse_tabs.insert(0, unicode(self._get_tab_locator(course, tab))) + reverse_static_tabs.insert(0, tab) + else: + built_in_tabs.append(tab) - self.client.ajax_post(new_location.url_reverse('tabs'), {'tabs': reverse_tabs}) + tab_ids = [{'tab_id': tab.tab_id} for tab in (built_in_tabs + reverse_static_tabs)] + + self.client.ajax_post(new_location.url_reverse('tabs'), {'tabs': tab_ids}) course = module_store.get_item(course_location) # compare to make sure that the tabs information is in the expected order after the server call - course_tabs = [] - for tab in course.tabs: - if tab['type'] == 'static_tab': - course_tabs.append(unicode(self._get_tab_locator(course, tab))) - - self.assertEqual(reverse_tabs, course_tabs) + new_static_tabs = [tab for tab in course.tabs if (tab['type'] == 'static_tab')] + self.assertEqual(reverse_static_tabs, new_static_tabs) def test_static_tab_deletion(self): module_store, course_location, _ = self._create_static_tabs() diff --git a/cms/djangoapps/contentstore/tests/test_course_settings.py b/cms/djangoapps/contentstore/tests/test_course_settings.py index b3ef6bd80a..1af9c6c710 100644 --- a/cms/djangoapps/contentstore/tests/test_course_settings.py +++ b/cms/djangoapps/contentstore/tests/test_course_settings.py @@ -410,7 +410,7 @@ class CourseGradingTest(CourseTestCase): """ Populate the course, grab a section, get the url for the assignment type access """ - self.populateCourse() + self.populate_course() sections = get_modulestore(self.course_location).get_items( self.course_location.replace(category="sequential", name=None) ) diff --git a/cms/djangoapps/contentstore/tests/test_export_git.py b/cms/djangoapps/contentstore/tests/test_export_git.py index 9a29ae6058..41f61b8807 100644 --- a/cms/djangoapps/contentstore/tests/test_export_git.py +++ b/cms/djangoapps/contentstore/tests/test_export_git.py @@ -102,7 +102,7 @@ class TestExportGit(CourseTestCase): subprocess.check_output(['git', '--bare', 'init', ], cwd=bare_repo_dir) - self.populateCourse() + self.populate_course() self.course_module.giturl = 'file://{}'.format(bare_repo_dir) get_modulestore(self.course_module.location).update_item(self.course_module) diff --git a/cms/djangoapps/contentstore/tests/test_orphan.py b/cms/djangoapps/contentstore/tests/test_orphan.py index 2bda394f73..80c2f76da8 100644 --- a/cms/djangoapps/contentstore/tests/test_orphan.py +++ b/cms/djangoapps/contentstore/tests/test_orphan.py @@ -75,7 +75,7 @@ class TestOrphan(CourseTestCase): """ Test that auth restricts get and delete appropriately """ - test_user_client, test_user = self.createNonStaffAuthedUserClient() + test_user_client, test_user = self.create_non_staff_authed_user_client() CourseEnrollment.enroll(test_user, self.course.location.course_id) locator = loc_mapper().translate_location(self.course.location.course_id, self.course.location, False, True) orphan_url = locator.url_reverse('orphan/', '') diff --git a/cms/djangoapps/contentstore/tests/utils.py b/cms/djangoapps/contentstore/tests/utils.py index 6db416f04b..9c2e46ab82 100644 --- a/cms/djangoapps/contentstore/tests/utils.py +++ b/cms/djangoapps/contentstore/tests/utils.py @@ -12,6 +12,7 @@ from django.test.utils import override_settings from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from contentstore.tests.modulestore_config import TEST_MODULESTORE +from contentstore.utils import get_modulestore from xmodule.modulestore.django import loc_mapper @@ -95,8 +96,9 @@ class CourseTestCase(ModuleStoreTestCase): self.course_locator = loc_mapper().translate_location( self.course.location.course_id, self.course.location, False, True ) + self.store = get_modulestore(self.course.location) - def createNonStaffAuthedUserClient(self): + def create_non_staff_authed_user_client(self): """ Create a non-staff user, log them in, and return the client, user to use for testing. """ @@ -114,7 +116,7 @@ class CourseTestCase(ModuleStoreTestCase): client.login(username=uname, password=password) return client, nonstaff - def populateCourse(self): + def populate_course(self): """ Add 2 chapters, 4 sections, 8 verticals, 16 problems to self.course (branching 2) """ @@ -126,3 +128,16 @@ class CourseTestCase(ModuleStoreTestCase): descend(child, stack) descend(self.course, ['chapter', 'sequential', 'vertical', 'problem']) + + def reload_course(self): + """ + Reloads the course object from the database + """ + self.course = self.store.get_item(self.course.location) + + def save_course(self): + """ + Updates the course object in the database + """ + self.course.save() + self.store.update_item(self.course, self.user.id) diff --git a/cms/djangoapps/contentstore/views/tabs.py b/cms/djangoapps/contentstore/views/tabs.py index 69da777bcf..695d390138 100644 --- a/cms/djangoapps/contentstore/views/tabs.py +++ b/cms/djangoapps/contentstore/views/tabs.py @@ -14,12 +14,10 @@ from edxmako.shortcuts import render_to_response from xmodule.modulestore.django import modulestore from xmodule.modulestore.django import loc_mapper from xmodule.modulestore.locator import BlockUsageLocator -from xmodule.tabs import CourseTabList, StaticTab, CourseTab +from xmodule.tabs import CourseTabList, StaticTab, CourseTab, InvalidTabsException from ..utils import get_modulestore -from django.utils.translation import ugettext as _ - __all__ = ['tabs_handler'] @expect_json @@ -53,85 +51,132 @@ def tabs_handler(request, tag=None, package_id=None, branch=None, version_guid=N raise NotImplementedError('coming soon') else: if 'tabs' in request.json: - def get_location_for_tab(tab): - """ Returns the location (old-style) for a tab. """ - return loc_mapper().translate_locator_to_location(BlockUsageLocator(tab)) - - tabs = request.json['tabs'] - - # get list of existing static tabs in course - # make sure they are the same lengths (i.e. the number of passed in tabs equals the number - # that we know about) otherwise we will inadvertently drop some! - existing_static_tabs = [t for t in course_item.tabs if t['type'] == 'static_tab'] - if len(existing_static_tabs) != len(tabs): - return JsonResponse( - {"error": "number of tabs must be {}".format(len(existing_static_tabs))}, status=400 - ) - - # load all reference tabs, return BadRequest if we can't find any of them - tab_items = [] - for tab in tabs: - item = modulestore('direct').get_item(get_location_for_tab(tab)) - if item is None: - return JsonResponse( - {"error": "no tab for found location {}".format(tab)}, status=400 - ) - - tab_items.append(item) - - # now just go through the existing course_tabs and re-order the static tabs - reordered_tabs = [] - static_tab_idx = 0 - for tab in course_item.tabs: - if isinstance(tab, StaticTab): - reordered_tabs.append( - StaticTab( - name=tab_items[static_tab_idx].display_name, - url_slug=tab_items[static_tab_idx].location.name, - ) - ) - static_tab_idx += 1 - else: - reordered_tabs.append(tab) - - # OK, re-assemble the static tabs in the new order - course_item.tabs = reordered_tabs - modulestore('direct').update_item(course_item, request.user.id) - return JsonResponse() + return reorder_tabs_handler(course_item, request) + elif 'tab_id_locator' in request.json: + return edit_tab_handler(course_item, request) else: raise NotImplementedError('Creating or changing tab content is not supported.') + elif request.method == 'GET': # assume html # get all tabs from the tabs list: static tabs (a.k.a. user-created tabs) and built-in tabs - # we do this because this is also the order in which items are displayed in the LMS + # present in the same order they are displayed in LMS - static_tabs = [] - built_in_tabs = [] - for tab in CourseTabList.iterate_displayable(course_item, settings, include_instructor_tab=False): + tabs_to_render = [] + for tab in CourseTabList.iterate_displayable_cms( + course_item, + settings, + ): if isinstance(tab, StaticTab): + # static tab needs its locator information to render itself as an xmodule static_tab_loc = old_location.replace(category='static_tab', name=tab.url_slug) - static_tabs.append(modulestore('direct').get_item(static_tab_loc)) - else: - built_in_tabs.append(tab) - - # create a list of components for each static tab - components = [ - loc_mapper().translate_location( - course_item.location.course_id, static_tab.location, False, True - ) - for static_tab - in static_tabs - ] + static_tab = modulestore('direct').get_item(static_tab_loc) + tab.locator = loc_mapper().translate_location( + course_item.location.course_id, static_tab.location, False, True + ) + tabs_to_render.append(tab) return render_to_response('edit-tabs.html', { 'context_course': course_item, - 'built_in_tabs': built_in_tabs, - 'components': components, + 'tabs_to_render': tabs_to_render, 'course_locator': locator }) else: return HttpResponseNotFound() +def reorder_tabs_handler(course_item, request): + """ + Helper function for handling reorder of tabs request + """ + + # Tabs are identified by tab_id or locators. + # The locators are 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. + ids_locators_of_new_tab_order = request.json['tabs'] + + # original tab list in original order + old_tab_list = course_item.tabs + + # create a new list in the new order + new_tab_list = [] + for tab_id_locator in ids_locators_of_new_tab_order: + tab = get_tab_by_tab_id_locator(old_tab_list, tab_id_locator) + if tab is None: + return JsonResponse( + {"error": "Tab with id_locator '{0}' does not exist.".format(tab_id_locator)}, status=400 + ) + 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) + + # validate the tabs to make sure everything is Ok (e.g., did the client try to reorder unmovable tabs?) + try: + CourseTabList.validate_tabs(new_tab_list) + except InvalidTabsException, exception: + return JsonResponse( + {"error": "New list of tabs is not valid: {0}.".format(str(exception))}, status=400 + ) + + # persist the new order of the tabs + course_item.tabs = new_tab_list + modulestore('direct').update_item(course_item, request.user.id) + + return JsonResponse() + + +def edit_tab_handler(course_item, request): + """ + Helper function for handling requests to edit settings of a single tab + """ + + # Tabs are identified by tab_id or locator + tab_id_locator = request.json['tab_id_locator'] + + # Find the given tab in the course + tab = get_tab_by_tab_id_locator(course_item.tabs, tab_id_locator) + if tab is None: + return JsonResponse( + {"error": "Tab with id_locator '{0}' does not exist.".format(tab_id_locator)}, status=400 + ) + + if 'is_hidden' in request.json: + # set the is_hidden attribute on the requested tab + tab.is_hidden = request.json['is_hidden'] + modulestore('direct').update_item(course_item, request.user.id) + else: + raise NotImplementedError('Unsupported request to edit tab: {0}'.format(request.json)) + + return JsonResponse() + + +def get_tab_by_tab_id_locator(tab_list, tab_id_locator): + """ + Look for a tab with the specified tab_id or locator. Returns the first matching tab. + """ + if 'tab_id' in tab_id_locator: + tab = CourseTabList.get_tab_by_id(tab_list, tab_id_locator['tab_id']) + elif 'tab_locator' in tab_id_locator: + tab = get_tab_by_locator(tab_list, tab_id_locator['tab_locator']) + return tab + + +def get_tab_by_locator(tab_list, tab_locator): + """ + Look for a tab with the specified locator. Returns the first matching tab. + """ + tab_location = loc_mapper().translate_locator_to_location(BlockUsageLocator(tab_locator)) + item = modulestore('direct').get_item(tab_location) + static_tab = StaticTab( + name=item.display_name, + url_slug=item.location.name, + ) + return CourseTabList.get_tab_by_id(tab_list, static_tab.tab_id) + + # "primitive" tab edit functions driven by the command line. # These should be replaced/deleted by a more capable GUI someday. # Note that the command line UI identifies the tabs with 1-based diff --git a/cms/djangoapps/contentstore/views/tests/test_course_index.py b/cms/djangoapps/contentstore/views/tests/test_course_index.py index 4ca23ee43b..3822ca0262 100644 --- a/cms/djangoapps/contentstore/views/tests/test_course_index.py +++ b/cms/djangoapps/contentstore/views/tests/test_course_index.py @@ -61,7 +61,7 @@ class TestCourseIndex(CourseTestCase): """ outline_url = self.course_locator.url_reverse('course/', '') # register a non-staff member and try to delete the course branch - non_staff_client, _ = self.createNonStaffAuthedUserClient() + 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) @@ -69,7 +69,7 @@ class TestCourseIndex(CourseTestCase): """ Make and register an course_staff and ensure they can access the courses """ - course_staff_client, course_staff = self.createNonStaffAuthedUserClient() + course_staff_client, course_staff = self.create_non_staff_authed_user_client() for course in [self.course, self.odd_course]: new_location = loc_mapper().translate_location(course.location.course_id, course.location, False, True) permission_url = new_location.url_reverse("course_team/", course_staff.email) diff --git a/cms/djangoapps/contentstore/views/tests/test_tabs.py b/cms/djangoapps/contentstore/views/tests/test_tabs.py index 0a8bb1198b..9e92c4f27d 100644 --- a/cms/djangoapps/contentstore/views/tests/test_tabs.py +++ b/cms/djangoapps/contentstore/views/tests/test_tabs.py @@ -1,9 +1,173 @@ """ Tests for tab functions (just primitive). """ +import json from contentstore.views import tabs +from contentstore.tests.utils import CourseTestCase from django.test import TestCase -from xmodule.modulestore.tests.factories import CourseFactory +from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from courseware.courses import get_course_by_id +from xmodule.tabs import CourseTabList, WikiTab + + +class TabsPageTests(CourseTestCase): + """Test cases for Tabs (a.k.a Pages) page""" + + def setUp(self): + """Common setup for tests""" + + # call super class to setup course, etc. + super(TabsPageTests, self).setUp() + + # Set the URL for tests + self.url = self.course_locator.url_reverse('tabs') + + # add a static tab to the course, for code coverage + ItemFactory.create( + parent_location=self.course_location, + category="static_tab", + display_name="Static_1" + ) + self.reload_course() + + def check_invalid_tab_id_response(self, resp): + """Verify response is an error listing the invalid_tab_id""" + + self.assertEqual(resp.status_code, 400) + resp_content = json.loads(resp.content) + self.assertIn("error", resp_content) + self.assertIn("invalid_tab_id", resp_content['error']) + + def test_not_implemented(self): + """Verify not implemented errors""" + + # JSON GET request not supported + with self.assertRaises(NotImplementedError): + self.client.get(self.url) + + # JSON POST request not supported + with self.assertRaises(NotImplementedError): + self.client.ajax_post( + self.url, + data={'tab_id': WikiTab.type, 'unsupported_request': None} + ) + + # invalid JSON POST request + with self.assertRaises(NotImplementedError): + self.client.ajax_post( + self.url, + data={'invalid_request': None} + ) + + def test_view_index(self): + """Basic check that the Pages page responds correctly""" + + resp = self.client.get_html(self.url) + self.assertEqual(resp.status_code, 200) + self.assertIn('course-nav-tab-list', resp.content) + + 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) + + # make sure we have enough tabs to play around with + self.assertTrue(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] + + # remove the middle 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 / 2) + self.assertTrue(len(tab_ids) == num_orig_tabs - 1) + + # 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, 204) + + # reload the course and verify the new tab order + self.reload_course() + 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) + self.assertIn("error", resp_content) + + def test_reorder_tabs_invalid_tab(self): + """Test re-ordering of tabs with invalid tab""" + + invalid_tab_ids = ['courseware', 'info', 'invalid_tab_id'] + + # post the request + resp = self.client.ajax_post( + self.url, + data={'tabs': [{'tab_id': tab_id} for tab_id in invalid_tab_ids]} + ) + self.check_invalid_tab_id_response(resp) + + def check_toggle_tab_visiblity(self, tab_type, new_is_hidden_setting): + """Helper method to check changes in tab visibility""" + + # find the tab + 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) + + # post the request + resp = self.client.ajax_post( + self.url, + data=json.dumps({ + 'tab_id_locator': {'tab_id': old_tab.tab_id}, + 'is_hidden': new_is_hidden_setting + }), + ) + self.assertEqual(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) + + def test_toggle_tab_visibility(self): + """Test toggling of tab visiblity""" + + self.check_toggle_tab_visiblity(WikiTab.type, True) + self.check_toggle_tab_visiblity(WikiTab.type, False) + + def test_toggle_invalid_tab_visibility(self): + """Test toggling visibility of an invalid tab""" + + # post the request + resp = self.client.ajax_post( + self.url, + data=json.dumps({ + 'tab_id_locator': {'tab_id': 'invalid_tab_id'} + }), + ) + self.check_invalid_tab_id_response(resp) class PrimitiveTabEdit(TestCase): diff --git a/cms/djangoapps/contentstore/views/tests/test_textbooks.py b/cms/djangoapps/contentstore/views/tests/test_textbooks.py index 7312d0c70a..d86dd13558 100644 --- a/cms/djangoapps/contentstore/views/tests/test_textbooks.py +++ b/cms/djangoapps/contentstore/views/tests/test_textbooks.py @@ -56,8 +56,7 @@ class TextbookIndexTestCase(CourseTestCase): } ] self.course.pdf_textbooks = content - store = get_modulestore(self.course.location) - store.update_item(self.course, self.user.id) + self.save_course() resp = self.client.get( self.url, @@ -83,12 +82,10 @@ class TextbookIndexTestCase(CourseTestCase): ) self.assertEqual(resp.status_code, 200) - # reload course - store = get_modulestore(self.course.location) - course = store.get_item(self.course.location) # should be the same, except for added ID no_ids = [] - for textbook in course.pdf_textbooks: + self.reload_course() + for textbook in self.course.pdf_textbooks: del textbook["id"] no_ids.append(textbook) self.assertEqual(no_ids, textbooks) @@ -193,9 +190,7 @@ class TextbookDetailTestCase(CourseTestCase): self.course.pdf_textbooks = [self.textbook1, self.textbook2] # Save the data that we've just changed to the underlying # MongoKeyValueStore before we update the mongo datastore. - self.course.save() - self.store = get_modulestore(self.course.location) - self.store.update_item(self.course, self.user.id) + self.save_course() self.url_nonexist = self.course_locator.url_reverse("textbooks", "20") def test_get_1(self): @@ -221,15 +216,15 @@ class TextbookDetailTestCase(CourseTestCase): "Delete a textbook by ID" resp = self.client.delete(self.url1) self.assertEqual(resp.status_code, 204) - course = self.store.get_item(self.course.location) - self.assertEqual(course.pdf_textbooks, [self.textbook2]) + self.reload_course() + self.assertEqual(self.course.pdf_textbooks, [self.textbook2]) def test_delete_nonexistant(self): "Delete a textbook by ID, when the ID doesn't match an existing textbook" resp = self.client.delete(self.url_nonexist) self.assertEqual(resp.status_code, 404) - course = self.store.get_item(self.course.location) - self.assertEqual(course.pdf_textbooks, [self.textbook1, self.textbook2]) + self.reload_course() + self.assertEqual(self.course.pdf_textbooks, [self.textbook1, self.textbook2]) def test_create_new_by_id(self): "Create a textbook by ID" @@ -249,9 +244,9 @@ class TextbookDetailTestCase(CourseTestCase): self.assertEqual(resp2.status_code, 200) compare = json.loads(resp2.content) self.assertEqual(compare, textbook) - course = self.store.get_item(self.course.location) + self.reload_course() self.assertEqual( - course.pdf_textbooks, + self.course.pdf_textbooks, [self.textbook1, self.textbook2, textbook] ) diff --git a/cms/static/coffee/src/views/tabs.coffee b/cms/static/coffee/src/views/tabs.coffee index 202ea6c6c7..0262886685 100644 --- a/cms/static/coffee/src/views/tabs.coffee +++ b/cms/static/coffee/src/views/tabs.coffee @@ -18,7 +18,8 @@ define ["jquery", "jquery.ui", "backbone", "js/views/feedback_prompt", "js/views @options.mast.find('.new-tab').on('click', @addNewTab) $('.add-pages .new-tab').on('click', @addNewTab) - @$('.components').sortable( + $('.toggle-checkbox').on('click', @toggleVisibilityOfTab) + @$('.course-nav-tab-list').sortable( handle: '.drag-handle' update: @tabMoved helper: 'clone' @@ -26,13 +27,38 @@ define ["jquery", "jquery.ui", "backbone", "js/views/feedback_prompt", "js/views placeholder: 'component-placeholder' forcePlaceholderSize: true axis: 'y' - items: '> .component' + items: '> .sortable-tab' ) + toggleVisibilityOfTab: (event, ui) => + checkbox_element = event.srcElement + tab_element = $(checkbox_element).parents(".course-tab")[0] + + saving = new NotificationView.Mini({title: gettext("Saving…")}) + saving.show() + + $.ajax({ + type:'POST', + url: @model.url(), + data: JSON.stringify({ + tab_id_locator : { + tab_id: $(tab_element).data('tab-id'), + tab_locator: $(tab_element).data('locator') + }, + is_hidden : $(checkbox_element).is(':checked') + }), + contentType: 'application/json' + }).success(=> saving.hide()) + tabMoved: (event, ui) => tabs = [] - @$('.component').each((idx, element) => - tabs.push($(element).data('locator')) + @$('.course-tab').each((idx, element) => + tabs.push( + { + tab_id: $(element).data('tab-id'), + tab_locator: $(element).data('locator') + } + ) ) analytics.track "Reordered Pages", @@ -59,6 +85,7 @@ define ["jquery", "jquery.ui", "backbone", "js/views/feedback_prompt", "js/views ) $('.new-component-item').before(editor.$el) + editor.$el.addClass('course-tab sortable-tab') editor.$el.addClass('new') setTimeout(=> editor.$el.removeClass('new') diff --git a/cms/templates/edit-tabs.html b/cms/templates/edit-tabs.html index 8be22b81fa..58a848ca9a 100644 --- a/cms/templates/edit-tabs.html +++ b/cms/templates/edit-tabs.html @@ -3,6 +3,7 @@ <%! from django.utils.translation import ugettext as _ from django.core.urlresolvers import reverse + from xmodule.tabs import StaticTab %> <%block name="title">${_("Pages")} <%block name="bodyclass">is-signedin course view-static-pages @@ -56,37 +57,54 @@
    - % for tab in built_in_tabs: -
  1. -
    -

    ${_(tab.name)}

    -
    -
    -
      + % for tab in tabs_to_render: + % if isinstance(tab, StaticTab): +
    • + % else: - % if tab.is_hideable: -
    • - - -
      -
      • - %endif + <% + tab_name = _(tab.name) + if tab.is_collection: + item_names = [_(item.name) for item in tab.items(context_course)] + num_items = sum(1 for item in tab.items(context_course)) + tab_name = tab_name + " ({0}): {1}".format(num_items, ", ".join(item_names)) + css_class = "course-nav-tab course-tab" + if tab.is_movable: + css_class = css_class + " sortable-tab" + %> -
    -
    -
    - ${_("Fixed page")} -
    -
    2. +
  2. +
    +

    ${tab_name}

    +
    +
    +
      + + % if tab.is_hideable: +
    • + + % if tab.is_hidden: + + % else: + + % endif +
      +
      • + % endif + +
    +
    + + % if tab.is_movable: +
    + ${_("Fixed page")} +
    + % endif +
    3. + % endif % endfor - % for locator in components: -
  3. - % endfor - -
  4. - -
    5. +
diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index f872e1466a..83d855bc0c 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -797,7 +797,7 @@ class MongoModuleStore(ModuleStoreWriteBase): if xblock.category == 'static_tab': course = self._get_course_for_item(xblock.location) # find the course's reference to this tab and update the name. - static_tab = CourseTabList.get_tab_by_slug(course, xblock.location.name) + static_tab = CourseTabList.get_tab_by_slug(course.tabs, xblock.location.name) # only update if changed if static_tab and static_tab['name'] != xblock.display_name: static_tab['name'] = xblock.display_name diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index b8023c9ea4..304f5f1c0c 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -663,7 +663,7 @@ class XMLModuleStore(ModuleStoreReadBase): # Hack because we need to pull in the 'display_name' for static tabs (because we need to edit them) # from the course policy if category == "static_tab": - tab = CourseTabList.get_tab_by_slug(course=course_descriptor, url_slug=slug) + tab = CourseTabList.get_tab_by_slug(tab_list=course_descriptor.tabs, url_slug=slug) if tab: module.display_name = tab.name module.data_dir = course_dir diff --git a/common/lib/xmodule/xmodule/tabs.py b/common/lib/xmodule/xmodule/tabs.py index c3193a540b..c215d56f34 100644 --- a/common/lib/xmodule/xmodule/tabs.py +++ b/common/lib/xmodule/xmodule/tabs.py @@ -28,7 +28,16 @@ class CourseTab(object): # pylint: disable=incomplete-protocol # subclass, shared by all instances of the subclass. type = '' - def __init__(self, name, tab_id, link_func): + # Class property that specifies whether the tab can be hidden for a particular course + is_hideable = False + + # Class property that specifies whether the tab can be moved within a course's list of tabs + is_movable = True + + # Class property that specifies whether the tab is a collection of other tabs + is_collection = False + + def __init__(self, name, tab_id, link_func, tab): """ Initializes class members with values passed in by subclasses. @@ -48,8 +57,7 @@ class CourseTab(object): # pylint: disable=incomplete-protocol self.link_func = link_func - # indicates whether the tab can be hidden for a particular course - self.is_hideable = False + self.is_hidden = tab.get('is_hidden', False) if tab else False def can_display(self, course, settings, is_user_authenticated, is_user_staff): # pylint: disable=unused-argument """ @@ -97,6 +105,8 @@ class CourseTab(object): # pylint: disable=incomplete-protocol return self.name elif key == 'type': return self.type + elif key == 'is_hidden': + return self.is_hidden elif key == 'tab_id': return self.tab_id else: @@ -113,6 +123,8 @@ class CourseTab(object): # pylint: disable=incomplete-protocol self.name = value elif key == 'tab_id': self.tab_id = value + elif key == 'is_hidden': + self.is_hidden = value else: raise KeyError('Key {0} cannot be set in tab {1}'.format(key, self.to_json())) @@ -130,8 +142,8 @@ class CourseTab(object): # pylint: disable=incomplete-protocol # allow tabs without names; if a name is required, its presence was checked in the validator. name_is_eq = (other.get('name') is None or self.name == other['name']) - # only compare the persisted/serialized members: 'type' and 'name' - return self.type == other.get('type') and name_is_eq + # only compare the persisted/serialized members: 'type', 'name', and 'is_hidden' + return self.type == other.get('type') and name_is_eq and self.is_hidden == other.get('is_hidden', False) def __ne__(self, other): """ @@ -140,7 +152,7 @@ class CourseTab(object): # pylint: disable=incomplete-protocol return not (self == other) @classmethod - def validate(cls, tab, raise_error=True): # pylint: disable=unused-argument + def validate(cls, tab, raise_error=True): """ Validates the given dict-type tab object to ensure it contains the expected keys. This method should be overridden by subclasses that require certain keys to be persisted in the tab. @@ -155,7 +167,10 @@ class CourseTab(object): # pylint: disable=incomplete-protocol Returns: a dictionary with keys for the properties of the CourseTab object. """ - return {'type': self.type, 'name': self.name} + to_json_val = {'type': self.type, 'name': self.name} + if self.is_hidden: + to_json_val.update({'is_hidden': True}) + return to_json_val @staticmethod def from_json(tab): @@ -224,13 +239,15 @@ class CoursewareTab(CourseTab): """ type = 'courseware' + is_movable = False - def __init__(self, tab=None): # pylint: disable=unused-argument + def __init__(self, tab=None): super(CoursewareTab, self).__init__( # Translators: 'Courseware' refers to the tab in the courseware that leads to the content of a course name=_('Courseware'), # support fixed name for the courseware tab tab_id=self.type, link_func=link_reverse_func(self.type), + tab=tab, ) @@ -240,6 +257,7 @@ class CourseInfoTab(CourseTab): """ type = 'course_info' + is_movable = False def __init__(self, tab=None): super(CourseInfoTab, self).__init__( @@ -247,6 +265,7 @@ class CourseInfoTab(CourseTab): name=tab['name'] if tab else _('Course Info'), tab_id='info', link_func=link_reverse_func('info'), + tab=tab, ) @classmethod @@ -267,6 +286,7 @@ class ProgressTab(AuthenticatedCourseTab): name=tab['name'] if tab else _('Progress'), tab_id=self.type, link_func=link_reverse_func(self.type), + tab=tab, ) def can_display(self, course, settings, is_user_authenticated, is_user_staff): @@ -283,16 +303,15 @@ class WikiTab(CourseTab): """ type = 'wiki' + is_hideable = True def __init__(self, tab=None): - # LATER - enable the following flag to enable hiding of the Wiki page - # self.is_hideable = True - super(WikiTab, self).__init__( # Translators: "Wiki" is the name of the course's wiki page name=tab['name'] if tab else _('Wiki'), tab_id=self.type, link_func=link_reverse_func('course_wiki'), + tab=tab, ) def can_display(self, course, settings, is_user_authenticated, is_user_staff): @@ -316,6 +335,7 @@ class DiscussionTab(CourseTab): name=tab['name'] if tab else _('Discussion'), tab_id=self.type, link_func=link_reverse_func('django_comment_client.forum.views.forum_form_discussion'), + tab=tab, ) def can_display(self, course, settings, is_user_authenticated, is_user_staff): @@ -332,12 +352,13 @@ class LinkTab(CourseTab): """ link_value = '' - def __init__(self, name, tab_id, link_value): + def __init__(self, name, tab_id, link_value, tab): self.link_value = link_value super(LinkTab, self).__init__( name=name, tab_id=tab_id, link_func=link_value_func(self.link_value), + tab=tab, ) def __getitem__(self, key): @@ -380,6 +401,7 @@ class ExternalDiscussionTab(LinkTab): name=_('Discussion'), tab_id='discussion', link_value=tab['link'] if tab else link_value, + tab=tab, ) @@ -394,6 +416,7 @@ class ExternalLinkTab(LinkTab): name=tab['name'], tab_id=None, # External links are never active. link_value=tab['link'], + tab=tab, ) @@ -402,7 +425,6 @@ class StaticTab(CourseTab): A custom tab. """ type = 'static_tab' - url_slug = '' @classmethod def validate(cls, tab, raise_error=True): @@ -410,11 +432,11 @@ class StaticTab(CourseTab): def __init__(self, tab=None, name=None, url_slug=None): self.url_slug = tab['url_slug'] if tab else url_slug - tab_name = tab['name'] if tab else name super(StaticTab, self).__init__( - name=tab_name, + name=tab['name'] if tab else name, tab_id='static_tab_{0}'.format(self.url_slug), link_func=lambda course, reverse_func: reverse_func(self.type, args=[course.id, self.url_slug]), + tab=tab, ) def __getitem__(self, key): @@ -446,6 +468,8 @@ class SingleTextbookTab(CourseTab): Textbook collection tab. It should not be serialized or persisted. """ type = 'single_textbook' + is_movable = False + is_collection_item = True def to_json(self): raise NotImplementedError('SingleTextbookTab should not be serialized.') @@ -455,11 +479,18 @@ class TextbookTabsBase(AuthenticatedCourseTab): """ Abstract class for textbook collection tabs classes. """ - def __init__(self, tab=None): # pylint: disable=unused-argument - super(TextbookTabsBase, self).__init__('', '', '') + is_collection = True + + def __init__(self, tab_id, tab): + # Translators: 'Textbooks' refers to the tab in the course that leads to the course' textbooks + super(TextbookTabsBase, self).__init__( + name=_("Textbooks"), + tab_id=tab_id, + link_func=None, tab=tab + ) @abstractmethod - def books(self, course): + def items(self, course): """ A generator for iterating through all the SingleTextbookTab book objects associated with this collection of textbooks. @@ -473,15 +504,22 @@ class TextbookTabs(TextbookTabsBase): """ type = 'textbooks' + def __init__(self, tab=None): + super(TextbookTabs, self).__init__( + tab_id=self.type, + tab=tab, + ) + def can_display(self, course, settings, is_user_authenticated, is_user_staff): return settings.FEATURES.get('ENABLE_TEXTBOOK') - def books(self, course): + def items(self, course): for index, textbook in enumerate(course.textbooks): yield SingleTextbookTab( name=textbook.title, tab_id='textbook/{0}'.format(index), link_func=lambda course, reverse_func: reverse_func('book', args=[course.id, index]), + tab=None ) @@ -491,12 +529,19 @@ class PDFTextbookTabs(TextbookTabsBase): """ type = 'pdf_textbooks' - def books(self, course): + def __init__(self, tab=None): + super(PDFTextbookTabs, self).__init__( + tab_id=self.type, + tab=tab, + ) + + def items(self, course): for index, textbook in enumerate(course.pdf_textbooks): yield SingleTextbookTab( name=textbook['tab_title'], tab_id='pdftextbook/{0}'.format(index), link_func=lambda course, reverse_func: reverse_func('pdf_book', args=[course.id, index]), + tab=None ) @@ -506,12 +551,19 @@ class HtmlTextbookTabs(TextbookTabsBase): """ type = 'html_textbooks' - def books(self, course): + def __init__(self, tab=None): + super(HtmlTextbookTabs, self).__init__( + tab_id=self.type, + tab=tab, + ) + + def items(self, course): for index, textbook in enumerate(course.html_textbooks): yield SingleTextbookTab( name=textbook['tab_title'], tab_id='htmltextbook/{0}'.format(index), link_func=lambda course, reverse_func: reverse_func('html_book', args=[course.id, index]), + tab=None ) @@ -528,13 +580,14 @@ class StaffGradingTab(StaffTab, GradingTab): """ type = 'staff_grading' - def __init__(self, tab=None): # pylint: disable=unused-argument + def __init__(self, tab=None): super(StaffGradingTab, self).__init__( # Translators: "Staff grading" appears on a tab that allows # staff to view open-ended problems that require staff grading name=_("Staff grading"), tab_id=self.type, link_func=link_reverse_func(self.type), + tab=tab, ) @@ -544,13 +597,14 @@ class PeerGradingTab(AuthenticatedCourseTab, GradingTab): """ type = 'peer_grading' - def __init__(self, tab=None): # pylint: disable=unused-argument + def __init__(self, tab=None): super(PeerGradingTab, self).__init__( # Translators: "Peer grading" appears on a tab that allows # students to view open-ended problems that require grading name=_("Peer grading"), tab_id=self.type, link_func=link_reverse_func(self.type), + tab=tab, ) @@ -560,13 +614,14 @@ class OpenEndedGradingTab(AuthenticatedCourseTab, GradingTab): """ type = 'open_ended' - def __init__(self, tab=None): # pylint: disable=unused-argument + def __init__(self, tab=None): super(OpenEndedGradingTab, self).__init__( # Translators: "Open Ended Panel" appears on a tab that, when clicked, opens up a panel that # displays information about open-ended problems that a user has submitted or needs to grade name=_("Open Ended Panel"), tab_id=self.type, link_func=link_reverse_func('open_ended_notifications'), + tab=tab, ) @@ -579,12 +634,13 @@ class SyllabusTab(CourseTab): def can_display(self, course, settings, is_user_authenticated, is_user_staff): return hasattr(course, 'syllabus_present') and course.syllabus_present - def __init__(self, tab=None): # pylint: disable=unused-argument + def __init__(self, tab=None): super(SyllabusTab, self).__init__( # Translators: "Syllabus" appears on a tab that, when clicked, opens the syllabus of the course. name=_('Syllabus'), tab_id=self.type, link_func=link_reverse_func(self.type), + tab=tab, ) @@ -602,6 +658,7 @@ class NotesTab(AuthenticatedCourseTab): name=tab['name'], tab_id=self.type, link_func=link_reverse_func(self.type), + tab=tab, ) @classmethod @@ -615,13 +672,14 @@ class InstructorTab(StaffTab): """ type = 'instructor' - def __init__(self, tab=None): # pylint: disable=unused-argument + def __init__(self, tab=None): super(InstructorTab, self).__init__( # Translators: 'Instructor' appears on the tab that leads to the instructor dashboard, which is # a portal where an instructor can get data and perform various actions on their course name=_('Instructor'), tab_id=self.type, link_func=link_reverse_func('instructor_dashboard'), + tab=tab, ) @@ -681,36 +739,63 @@ class CourseTabList(List): return None @staticmethod - def get_tab_by_slug(course, url_slug): + def get_tab_by_slug(tab_list, url_slug): """ Look for a tab with the specified 'url_slug'. Returns the tab or None if not found. """ - for tab in course.tabs: - # The validation code checks that these exist. - if tab.get('url_slug') == url_slug: - return tab - return None + return next((tab for tab in tab_list if tab.get('url_slug') == url_slug), None) @staticmethod - def iterate_displayable(course, settings, is_user_authenticated=True, is_user_staff=True, include_instructor_tab=False): + def get_tab_by_type(tab_list, tab_type): + """ + Look for a tab with the specified type. Returns the first matching tab. + """ + return next((tab for tab in tab_list if tab.type == tab_type), None) + + @staticmethod + def get_tab_by_id(tab_list, tab_id): + """ + Look for a tab with the specified tab_id. Returns the first matching tab. + """ + return next((tab for tab in tab_list if tab.tab_id == tab_id), None) + + @staticmethod + def iterate_displayable( + course, + settings, + is_user_authenticated=True, + is_user_staff=True, + ): """ Generator method for iterating through all tabs that can be displayed for the given course and the given user with the provided access settings. """ for tab in course.tabs: - if tab.can_display(course, settings, is_user_authenticated, is_user_staff): - if isinstance(tab, TextbookTabsBase): - for book in tab.books(course): - yield book + if tab.can_display(course, settings, is_user_authenticated, is_user_staff) and not tab.is_hidden: + if tab.is_collection: + for item in tab.items(course): + yield item else: yield tab - if include_instructor_tab: - instructor_tab = InstructorTab() - if instructor_tab.can_display(course, settings, is_user_authenticated, is_user_staff): - yield instructor_tab + instructor_tab = InstructorTab() + if instructor_tab.can_display(course, settings, is_user_authenticated, is_user_staff): + yield instructor_tab + + @staticmethod + def iterate_displayable_cms( + course, + settings + ): + """ + Generator method for iterating through all tabs that can be displayed for the given course and + the given user with the provided access settings. + """ + for tab in course.tabs: + if tab.can_display(course, settings, is_user_authenticated=True, is_user_staff=True): + yield tab @classmethod - def _validate_tabs(cls, tabs): + def validate_tabs(cls, tabs): """ Check that the tabs set for the specified course is valid. If it isn't, raise InvalidTabsException with the complaint. @@ -776,7 +861,7 @@ class CourseTabList(List): """ Overrides the from_json method to de-serialize the CourseTab objects from a json-like representation. """ - self._validate_tabs(values) + self.validate_tabs(values) return [CourseTab.from_json(tab) for tab in values] @@ -833,3 +918,10 @@ class InvalidTabsException(Exception): A complaint about invalid tabs. """ pass + + +class UnequalTabsException(Exception): + """ + A complaint about tab lists being unequal + """ + pass diff --git a/common/lib/xmodule/xmodule/tests/test_tabs.py b/common/lib/xmodule/xmodule/tests/test_tabs.py index d41580c881..86f1c1e026 100644 --- a/common/lib/xmodule/xmodule/tests/test_tabs.py +++ b/common/lib/xmodule/xmodule/tests/test_tabs.py @@ -14,6 +14,16 @@ class TabTestCase(unittest.TestCase): self.settings = MagicMock() self.settings.FEATURES = {} self.reverse = lambda name, args: "name/{0}/args/{1}".format(name, ",".join(str(a) for a in args)) + self.books = None + + def set_up_books(self, num_books): + """initializes the textbooks in the course and adds the given number of books to each textbook""" + self.books = [MagicMock() for _ in range(num_books)] + for book_index, book in enumerate(self.books): + book.title = 'Book{0}'.format(book_index) + self.course.textbooks = self.books + self.course.pdf_textbooks = self.books + self.course.html_textbooks = self.books def check_tab( self, @@ -57,22 +67,30 @@ class TabTestCase(unittest.TestCase): self.check_get_and_set_methods(tab) # check to_json and from_json methods - serialized_tab = tab.to_json() - deserialized_tab = tab_class.from_json(serialized_tab) - self.assertEquals(serialized_tab, deserialized_tab) + self.check_tab_json_methods(tab) # check equality methods + self.check_tab_equality(tab, dict_tab) + + # return tab for any additional tests + return tab + + def check_tab_equality(self, tab, dict_tab): + """tests the equality methods on the given tab""" self.assertEquals(tab, dict_tab) # test __eq__ ne_dict_tab = dict_tab ne_dict_tab['type'] = 'fake_type' self.assertNotEquals(tab, ne_dict_tab) # test __ne__: incorrect type self.assertNotEquals(tab, {'fake_key': 'fake_value'}) # test __ne__: missing type - # return tab for any additional tests - return tab + def check_tab_json_methods(self, tab): + """tests the json from and to methods on the given tab""" + serialized_tab = tab.to_json() + deserialized_tab = tab.from_json(serialized_tab) + self.assertEquals(serialized_tab, deserialized_tab) def check_can_display_results(self, tab, expected_value=True, for_authenticated_users_only=False, for_staff_only=False): - """Check can display results for various users""" + """checks can display results for various users""" if for_staff_only: self.assertEquals( expected_value, @@ -149,17 +167,31 @@ class WikiTestCase(TabTestCase): ) def test_wiki_enabled(self): + """Test wiki tab when Enabled setting is True""" self.settings.WIKI_ENABLED = True tab = self.check_wiki_tab() self.check_can_display_results(tab) def test_wiki_enabled_false(self): + """Test wiki tab when Enabled setting is False""" self.settings.WIKI_ENABLED = False tab = self.check_wiki_tab() self.check_can_display_results(tab, expected_value=False) + def test_wiki_visibility(self): + """Test toggling of visibility of wiki tab""" + + wiki_tab = tabs.WikiTab() + self.assertTrue(wiki_tab.is_hideable) + wiki_tab.is_hidden = True + self.assertTrue(wiki_tab['is_hidden']) + self.check_tab_json_methods(wiki_tab) + self.check_tab_equality(wiki_tab, wiki_tab.to_json()) + wiki_tab['is_hidden'] = False + self.assertFalse(wiki_tab.is_hidden) + class ExternalLinkTestCase(TabTestCase): """Test cases for External Link Tab.""" @@ -202,15 +234,9 @@ class TextbooksTestCase(TabTestCase): def setUp(self): super(TextbooksTestCase, self).setUp() + self.set_up_books(2) + self.dict_tab = MagicMock() - book1 = MagicMock() - book2 = MagicMock() - book1.title = 'Book1: Algebra' - book2.title = 'Book2: Topology' - books = [book1, book2] - self.course.textbooks = books - self.course.pdf_textbooks = books - self.course.html_textbooks = books self.course.tabs = [ tabs.CoursewareTab(), tabs.CourseInfoTab(), @@ -219,7 +245,7 @@ class TextbooksTestCase(TabTestCase): tabs.HtmlTextbookTabs(), ] self.num_textbook_tabs = sum(1 for tab in self.course.tabs if isinstance(tab, tabs.TextbookTabsBase)) - self.num_textbooks = self.num_textbook_tabs * len(books) + self.num_textbooks = self.num_textbook_tabs * len(self.books) def test_textbooks_enabled(self): @@ -233,7 +259,7 @@ class TextbooksTestCase(TabTestCase): book_type, book_index = tab.tab_id.split("/", 1) expected_link = self.reverse(type_to_reverse_name[book_type], args=[self.course.id, book_index]) self.assertEqual(tab.link_func(self.course, self.reverse), expected_link) - self.assertTrue(tab.name.startswith('Book{0}:'.format(1 + int(book_index)))) + self.assertTrue(tab.name.startswith('Book{0}'.format(book_index))) num_textbooks_found = num_textbooks_found + 1 self.assertEquals(num_textbooks_found, self.num_textbooks) @@ -381,10 +407,11 @@ class NeedNameTestCase(unittest.TestCase): tabs.need_name(self.invalid_dict) -class ValidateTabsTestCase(unittest.TestCase): - """Test cases for validating tabs.""" +class TabListTestCase(TabTestCase): + """Base class for Test cases involving tab lists.""" def setUp(self): + super(TabListTestCase, self).setUp() # invalid tabs self.invalid_tabs = [ @@ -447,6 +474,12 @@ class ValidateTabsTestCase(unittest.TestCase): ], ] + self.all_valid_tab_list = tabs.CourseTabList().from_json(self.valid_tabs[1]) + + +class ValidateTabsTestCase(TabListTestCase): + """Test cases for validating tabs.""" + def test_validate_tabs(self): tab_list = tabs.CourseTabList() for invalid_tab_list in self.invalid_tabs: @@ -458,7 +491,7 @@ class ValidateTabsTestCase(unittest.TestCase): self.assertEquals(len(from_json_result), len(valid_tab_list)) -class CourseTabListTestCase(TabTestCase): +class CourseTabListTestCase(TabListTestCase): """Testing the generator method for iterating through displayable tabs""" def test_initialize_default_without_syllabus(self): @@ -488,23 +521,51 @@ class CourseTabListTestCase(TabTestCase): self.assertTrue(tabs.DiscussionTab() in self.course.tabs) def test_iterate_displayable(self): + # enable all tab types self.settings.FEATURES['ENABLE_TEXTBOOK'] = True - self.course.tabs = [ - tabs.CoursewareTab(), - tabs.CourseInfoTab(), - tabs.WikiTab(), - ] + self.settings.FEATURES['ENABLE_DISCUSSION_SERVICE'] = True + self.settings.FEATURES['ENABLE_STUDENT_NOTES'] = True + self.course.hide_progress_tab = False + # create 1 book per textbook type + self.set_up_books(1) + + # initialize the course tabs to a list of all valid tabs + self.course.tabs = self.all_valid_tab_list + + # enumerate the tabs using the CMS call + for i, tab in enumerate(tabs.CourseTabList.iterate_displayable_cms( + self.course, + self.settings, + )): + self.assertEquals(tab.type, self.course.tabs[i].type) + + # enumerate the tabs and verify textbooks and the instructor tab for i, tab in enumerate(tabs.CourseTabList.iterate_displayable( self.course, self.settings, - include_instructor_tab=True, )): - if i == len(self.course.tabs): + if getattr(tab, 'is_collection_item', False): + # a collection item was found as a result of a collection tab + self.assertTrue(getattr(self.course.tabs[i], 'is_collection', False)) + elif i == len(self.course.tabs): + # the last tab must be the Instructor tab self.assertEquals(tab.type, tabs.InstructorTab.type) else: + # all other tabs must match the expected type self.assertEquals(tab.type, self.course.tabs[i].type) + def test_get_tab_by_methods(self): + """tests the get_tab methods in CourseTabList""" + self.course.tabs = self.all_valid_tab_list + for tab in self.course.tabs: + + # get tab by type + self.assertEquals(tabs.CourseTabList.get_tab_by_type(self.course.tabs, tab.type), tab) + + # get tab by id + self.assertEquals(tabs.CourseTabList.get_tab_by_id(self.course.tabs, tab.tab_id), tab) + class DiscussionLinkTestCase(TabTestCase): """Test cases for discussion link tab.""" diff --git a/lms/djangoapps/courseware/tests/test_tabs.py b/lms/djangoapps/courseware/tests/test_tabs.py index 51a2dd5b0d..f69c1a83eb 100644 --- a/lms/djangoapps/courseware/tests/test_tabs.py +++ b/lms/djangoapps/courseware/tests/test_tabs.py @@ -45,7 +45,7 @@ class StaticTabDateTestCase(LoginEnrollmentTestCase, ModuleStoreTestCase): def test_get_static_tab_contents(self): course = get_course_by_id('edX/toy/2012_Fall') request = get_request_for_user(UserFactory.create()) - tab = CourseTabList.get_tab_by_slug(course, 'resources') + tab = CourseTabList.get_tab_by_slug(course.tabs, 'resources') # Test render works okay tab_content = get_static_tab_contents(request, course, tab) diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index 854ba5e5f6..9ee7a700a6 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -482,7 +482,7 @@ def static_tab(request, course_id, tab_slug): """ course = get_course_with_access(request.user, course_id, 'load') - tab = CourseTabList.get_tab_by_slug(course, tab_slug) + tab = CourseTabList.get_tab_by_slug(course.tabs, tab_slug) if tab is None: raise Http404 diff --git a/lms/templates/courseware/course_navigation.html b/lms/templates/courseware/course_navigation.html index 7ce6075aee..de27004e4c 100644 --- a/lms/templates/courseware/course_navigation.html +++ b/lms/templates/courseware/course_navigation.html @@ -23,7 +23,7 @@ def url_class(is_active): @@ -58,39 +61,34 @@
    % for tab in tabs_to_render: + <% + css_class = "course-tab" + if tab.is_movable: + css_class = css_class + " is-movable" + elif (not tab.is_movable) and (not tab.is_hideable): + css_class = css_class + " is-fixed" + %> + % if isinstance(tab, StaticTab): -
  1. +
  2. + % else: - - <% - tab_name = _(tab.name) - item_names_formatted = "" - item_names = [] - num_items = 0 - if tab.is_collection: - item_names = [_(item.name) for item in tab.items(context_course)] - num_items = sum(1 for item in tab.items(context_course)) - css_class = "course-nav-item course-nav-tab course-tab" - if tab.is_movable: - css_class = css_class + " sortable-tab" - %> - - % if tab.is_hideable or tab.is_movable: -
  3. +
  4. - % if tab.is_collection: -

    ${tab_name}

    -
      - % for item_name in item_names: -
    • - ${item_name} -
      • - % endfor -
    + % if tab.is_collection: + +

    ${_(tab.name)}

    +
      + % for item in tab.items(context_course): +
    • + ${_(item.name)} +
      • + % endfor +
    % else: -

    ${tab_name}

    +

    ${_(tab.name)}

    % endif
    @@ -99,13 +97,13 @@ % if tab.is_hideable:
  5. - - % if tab.is_hidden: - - % else: - - % endif -
    + + % if tab.is_hidden: + + % else: + + % endif +
    6. % endif @@ -123,30 +121,7 @@ % endif - % else: -
  6. -
    -

    ${tab_name}

    - - % if tab.is_collection: -
      - % for item_name in item_names: -
    • - ${item_name} -
      • - % endfor -
    - % endif - -
    - -
    - ${_("This page cannot be reordered")} -
    -
    7. - % endif - - % endif + % endif % endfor
  7. diff --git a/common/lib/xmodule/xmodule/tabs.py b/common/lib/xmodule/xmodule/tabs.py index c215d56f34..cd1f5101db 100644 --- a/common/lib/xmodule/xmodule/tabs.py +++ b/common/lib/xmodule/xmodule/tabs.py @@ -37,7 +37,7 @@ class CourseTab(object): # pylint: disable=incomplete-protocol # Class property that specifies whether the tab is a collection of other tabs is_collection = False - def __init__(self, name, tab_id, link_func, tab): + def __init__(self, name, tab_id, link_func): """ Initializes class members with values passed in by subclasses. @@ -57,8 +57,6 @@ class CourseTab(object): # pylint: disable=incomplete-protocol self.link_func = link_func - self.is_hidden = tab.get('is_hidden', False) if tab else False - def can_display(self, course, settings, is_user_authenticated, is_user_staff): # pylint: disable=unused-argument """ Determines whether the tab should be displayed in the UI for the given course and a particular user. @@ -105,8 +103,6 @@ class CourseTab(object): # pylint: disable=incomplete-protocol return self.name elif key == 'type': return self.type - elif key == 'is_hidden': - return self.is_hidden elif key == 'tab_id': return self.tab_id else: @@ -123,8 +119,6 @@ class CourseTab(object): # pylint: disable=incomplete-protocol self.name = value elif key == 'tab_id': self.tab_id = value - elif key == 'is_hidden': - self.is_hidden = value else: raise KeyError('Key {0} cannot be set in tab {1}'.format(key, self.to_json())) @@ -142,8 +136,8 @@ class CourseTab(object): # pylint: disable=incomplete-protocol # allow tabs without names; if a name is required, its presence was checked in the validator. name_is_eq = (other.get('name') is None or self.name == other['name']) - # only compare the persisted/serialized members: 'type', 'name', and 'is_hidden' - return self.type == other.get('type') and name_is_eq and self.is_hidden == other.get('is_hidden', False) + # only compare the persisted/serialized members: 'type' and 'name' + return self.type == other.get('type') and name_is_eq def __ne__(self, other): """ @@ -152,12 +146,12 @@ class CourseTab(object): # pylint: disable=incomplete-protocol return not (self == other) @classmethod - def validate(cls, tab, raise_error=True): + def validate(cls, tab_dict, raise_error=True): """ Validates the given dict-type tab object to ensure it contains the expected keys. This method should be overridden by subclasses that require certain keys to be persisted in the tab. """ - return key_checker(['type'])(tab, raise_error) + return key_checker(['type'])(tab_dict, raise_error) def to_json(self): """ @@ -167,13 +161,10 @@ class CourseTab(object): # pylint: disable=incomplete-protocol Returns: a dictionary with keys for the properties of the CourseTab object. """ - to_json_val = {'type': self.type, 'name': self.name} - if self.is_hidden: - to_json_val.update({'is_hidden': True}) - return to_json_val + return {'type': self.type, 'name': self.name} @staticmethod - def from_json(tab): + def from_json(tab_dict): """ Deserializes a CourseTab from a json-like representation. @@ -206,15 +197,15 @@ class CourseTab(object): # pylint: disable=incomplete-protocol 'instructor': InstructorTab, # not persisted } - tab_type = tab.get('type') + tab_type = tab_dict.get('type') if tab_type not in sub_class_types: raise InvalidTabsException( 'Unknown tab type {0}. Known types: {1}'.format(tab_type, sub_class_types) ) - tab_class = sub_class_types[tab['type']] - tab_class.validate(tab) - return tab_class(tab=tab) + tab_class = sub_class_types[tab_dict['type']] + tab_class.validate(tab_dict) + return tab_class(tab_dict=tab_dict) class AuthenticatedCourseTab(CourseTab): @@ -233,6 +224,44 @@ class StaffTab(AuthenticatedCourseTab): return is_user_staff +class HideableTab(CourseTab): + """ + Abstract class for tabs that are hideable + """ + is_hideable = True + + def __init__(self, name, tab_id, link_func, tab_dict): + super(HideableTab, self).__init__( + name=name, + tab_id=tab_id, + link_func=link_func, + ) + self.is_hidden = tab_dict.get('is_hidden', False) if tab_dict else False + + def __getitem__(self, key): + if key == 'is_hidden': + return self.is_hidden + else: + return super(HideableTab, self).__getitem__(key) + + def __setitem__(self, key, value): + if key == 'is_hidden': + self.is_hidden = value + else: + super(HideableTab, self).__setitem__(key, value) + + def to_json(self): + to_json_val = super(HideableTab, self).to_json() + if self.is_hidden: + to_json_val.update({'is_hidden': True}) + return to_json_val + + def __eq__(self, other): + if not super(HideableTab, self).__eq__(other): + return False + return self.is_hidden == other.get('is_hidden', False) + + class CoursewareTab(CourseTab): """ A tab containing the course content. @@ -241,13 +270,12 @@ class CoursewareTab(CourseTab): type = 'courseware' is_movable = False - def __init__(self, tab=None): + def __init__(self, tab_dict=None): # pylint: disable=unused-argument super(CoursewareTab, self).__init__( # Translators: 'Courseware' refers to the tab in the courseware that leads to the content of a course name=_('Courseware'), # support fixed name for the courseware tab tab_id=self.type, link_func=link_reverse_func(self.type), - tab=tab, ) @@ -259,18 +287,17 @@ class CourseInfoTab(CourseTab): type = 'course_info' is_movable = False - def __init__(self, tab=None): + def __init__(self, tab_dict=None): super(CourseInfoTab, self).__init__( # Translators: "Course Info" is the name of the course's information and updates page - name=tab['name'] if tab else _('Course Info'), + name=tab_dict['name'] if tab_dict else _('Course Info'), tab_id='info', link_func=link_reverse_func('info'), - tab=tab, ) @classmethod - def validate(cls, tab, raise_error=True): - return super(CourseInfoTab, cls).validate(tab, raise_error) and need_name(tab, raise_error) + def validate(cls, tab_dict, raise_error=True): + return super(CourseInfoTab, cls).validate(tab_dict, raise_error) and need_name(tab_dict, raise_error) class ProgressTab(AuthenticatedCourseTab): @@ -280,46 +307,44 @@ class ProgressTab(AuthenticatedCourseTab): type = 'progress' - def __init__(self, tab=None): + def __init__(self, tab_dict=None): super(ProgressTab, self).__init__( # Translators: "Progress" is the name of the student's course progress page - name=tab['name'] if tab else _('Progress'), + name=tab_dict['name'] if tab_dict else _('Progress'), tab_id=self.type, link_func=link_reverse_func(self.type), - tab=tab, ) def can_display(self, course, settings, is_user_authenticated, is_user_staff): return not course.hide_progress_tab @classmethod - def validate(cls, tab, raise_error=True): - return super(ProgressTab, cls).validate(tab, raise_error) and need_name(tab, raise_error) + def validate(cls, tab_dict, raise_error=True): + return super(ProgressTab, cls).validate(tab_dict, raise_error) and need_name(tab_dict, raise_error) -class WikiTab(CourseTab): +class WikiTab(HideableTab): """ - A tab containing the course wiki. + A tab_dict containing the course wiki. """ type = 'wiki' - is_hideable = True - def __init__(self, tab=None): + def __init__(self, tab_dict=None): super(WikiTab, self).__init__( # Translators: "Wiki" is the name of the course's wiki page - name=tab['name'] if tab else _('Wiki'), + name=tab_dict['name'] if tab_dict else _('Wiki'), tab_id=self.type, link_func=link_reverse_func('course_wiki'), - tab=tab, + tab_dict=tab_dict, ) def can_display(self, course, settings, is_user_authenticated, is_user_staff): return settings.WIKI_ENABLED @classmethod - def validate(cls, tab, raise_error=True): - return super(WikiTab, cls).validate(tab, raise_error) and need_name(tab, raise_error) + def validate(cls, tab_dict, raise_error=True): + return super(WikiTab, cls).validate(tab_dict, raise_error) and need_name(tab_dict, raise_error) class DiscussionTab(CourseTab): @@ -329,21 +354,20 @@ class DiscussionTab(CourseTab): type = 'discussion' - def __init__(self, tab=None): + def __init__(self, tab_dict=None): super(DiscussionTab, self).__init__( # Translators: "Discussion" is the title of the course forum page - name=tab['name'] if tab else _('Discussion'), + name=tab_dict['name'] if tab_dict else _('Discussion'), tab_id=self.type, link_func=link_reverse_func('django_comment_client.forum.views.forum_form_discussion'), - tab=tab, ) def can_display(self, course, settings, is_user_authenticated, is_user_staff): return settings.FEATURES.get('ENABLE_DISCUSSION_SERVICE') @classmethod - def validate(cls, tab, raise_error=True): - return super(DiscussionTab, cls).validate(tab, raise_error) and need_name(tab, raise_error) + def validate(cls, tab_dict, raise_error=True): + return super(DiscussionTab, cls).validate(tab_dict, raise_error) and need_name(tab_dict, raise_error) class LinkTab(CourseTab): @@ -352,13 +376,12 @@ class LinkTab(CourseTab): """ link_value = '' - def __init__(self, name, tab_id, link_value, tab): + def __init__(self, name, tab_id, link_value): self.link_value = link_value super(LinkTab, self).__init__( name=name, tab_id=tab_id, link_func=link_value_func(self.link_value), - tab=tab, ) def __getitem__(self, key): @@ -384,8 +407,8 @@ class LinkTab(CourseTab): return self.link_value == other.get('link') @classmethod - def validate(cls, tab, raise_error=True): - return super(LinkTab, cls).validate(tab, raise_error) and key_checker(['link'])(tab, raise_error) + def validate(cls, tab_dict, raise_error=True): + return super(LinkTab, cls).validate(tab_dict, raise_error) and key_checker(['link'])(tab_dict, raise_error) class ExternalDiscussionTab(LinkTab): @@ -395,13 +418,12 @@ class ExternalDiscussionTab(LinkTab): type = 'external_discussion' - def __init__(self, tab=None, link_value=None): + def __init__(self, tab_dict=None, link_value=None): super(ExternalDiscussionTab, self).__init__( # Translators: 'Discussion' refers to the tab in the courseware that leads to the discussion forums name=_('Discussion'), tab_id='discussion', - link_value=tab['link'] if tab else link_value, - tab=tab, + link_value=tab_dict['link'] if tab_dict else link_value, ) @@ -411,12 +433,11 @@ class ExternalLinkTab(LinkTab): """ type = 'external_link' - def __init__(self, tab): + def __init__(self, tab_dict): super(ExternalLinkTab, self).__init__( - name=tab['name'], + name=tab_dict['name'], tab_id=None, # External links are never active. - link_value=tab['link'], - tab=tab, + link_value=tab_dict['link'], ) @@ -427,16 +448,15 @@ class StaticTab(CourseTab): type = 'static_tab' @classmethod - def validate(cls, tab, raise_error=True): - return super(StaticTab, cls).validate(tab, raise_error) and key_checker(['name', 'url_slug'])(tab, raise_error) + def validate(cls, tab_dict, raise_error=True): + return super(StaticTab, cls).validate(tab_dict, raise_error) and key_checker(['name', 'url_slug'])(tab_dict, raise_error) - def __init__(self, tab=None, name=None, url_slug=None): - self.url_slug = tab['url_slug'] if tab else url_slug + def __init__(self, tab_dict=None, name=None, url_slug=None): + self.url_slug = tab_dict['url_slug'] if tab_dict else url_slug super(StaticTab, self).__init__( - name=tab['name'] if tab else name, + name=tab_dict['name'] if tab_dict else name, tab_id='static_tab_{0}'.format(self.url_slug), link_func=lambda course, reverse_func: reverse_func(self.type, args=[course.id, self.url_slug]), - tab=tab, ) def __getitem__(self, key): @@ -481,12 +501,12 @@ class TextbookTabsBase(AuthenticatedCourseTab): """ is_collection = True - def __init__(self, tab_id, tab): + def __init__(self, tab_id): # Translators: 'Textbooks' refers to the tab in the course that leads to the course' textbooks super(TextbookTabsBase, self).__init__( name=_("Textbooks"), tab_id=tab_id, - link_func=None, tab=tab + link_func=None, ) @abstractmethod @@ -504,10 +524,9 @@ class TextbookTabs(TextbookTabsBase): """ type = 'textbooks' - def __init__(self, tab=None): + def __init__(self, tab_dict=None): # pylint: disable=unused-argument super(TextbookTabs, self).__init__( tab_id=self.type, - tab=tab, ) def can_display(self, course, settings, is_user_authenticated, is_user_staff): @@ -519,7 +538,6 @@ class TextbookTabs(TextbookTabsBase): name=textbook.title, tab_id='textbook/{0}'.format(index), link_func=lambda course, reverse_func: reverse_func('book', args=[course.id, index]), - tab=None ) @@ -529,10 +547,9 @@ class PDFTextbookTabs(TextbookTabsBase): """ type = 'pdf_textbooks' - def __init__(self, tab=None): + def __init__(self, tab_dict=None): # pylint: disable=unused-argument super(PDFTextbookTabs, self).__init__( tab_id=self.type, - tab=tab, ) def items(self, course): @@ -541,7 +558,6 @@ class PDFTextbookTabs(TextbookTabsBase): name=textbook['tab_title'], tab_id='pdftextbook/{0}'.format(index), link_func=lambda course, reverse_func: reverse_func('pdf_book', args=[course.id, index]), - tab=None ) @@ -551,10 +567,9 @@ class HtmlTextbookTabs(TextbookTabsBase): """ type = 'html_textbooks' - def __init__(self, tab=None): + def __init__(self, tab_dict=None): # pylint: disable=unused-argument super(HtmlTextbookTabs, self).__init__( tab_id=self.type, - tab=tab, ) def items(self, course): @@ -563,7 +578,6 @@ class HtmlTextbookTabs(TextbookTabsBase): name=textbook['tab_title'], tab_id='htmltextbook/{0}'.format(index), link_func=lambda course, reverse_func: reverse_func('html_book', args=[course.id, index]), - tab=None ) @@ -580,14 +594,13 @@ class StaffGradingTab(StaffTab, GradingTab): """ type = 'staff_grading' - def __init__(self, tab=None): + def __init__(self, tab_dict=None): # pylint: disable=unused-argument super(StaffGradingTab, self).__init__( # Translators: "Staff grading" appears on a tab that allows # staff to view open-ended problems that require staff grading name=_("Staff grading"), tab_id=self.type, link_func=link_reverse_func(self.type), - tab=tab, ) @@ -597,14 +610,13 @@ class PeerGradingTab(AuthenticatedCourseTab, GradingTab): """ type = 'peer_grading' - def __init__(self, tab=None): + def __init__(self, tab_dict=None): # pylint: disable=unused-argument super(PeerGradingTab, self).__init__( # Translators: "Peer grading" appears on a tab that allows # students to view open-ended problems that require grading name=_("Peer grading"), tab_id=self.type, link_func=link_reverse_func(self.type), - tab=tab, ) @@ -614,14 +626,13 @@ class OpenEndedGradingTab(AuthenticatedCourseTab, GradingTab): """ type = 'open_ended' - def __init__(self, tab=None): + def __init__(self, tab_dict=None): # pylint: disable=unused-argument super(OpenEndedGradingTab, self).__init__( # Translators: "Open Ended Panel" appears on a tab that, when clicked, opens up a panel that # displays information about open-ended problems that a user has submitted or needs to grade name=_("Open Ended Panel"), tab_id=self.type, link_func=link_reverse_func('open_ended_notifications'), - tab=tab, ) @@ -634,13 +645,12 @@ class SyllabusTab(CourseTab): def can_display(self, course, settings, is_user_authenticated, is_user_staff): return hasattr(course, 'syllabus_present') and course.syllabus_present - def __init__(self, tab=None): + def __init__(self, tab_dict=None): # pylint: disable=unused-argument super(SyllabusTab, self).__init__( # Translators: "Syllabus" appears on a tab that, when clicked, opens the syllabus of the course. name=_('Syllabus'), tab_id=self.type, link_func=link_reverse_func(self.type), - tab=tab, ) @@ -653,17 +663,16 @@ class NotesTab(AuthenticatedCourseTab): def can_display(self, course, settings, is_user_authenticated, is_user_staff): return settings.FEATURES.get('ENABLE_STUDENT_NOTES') - def __init__(self, tab=None): + def __init__(self, tab_dict=None): super(NotesTab, self).__init__( - name=tab['name'], + name=tab_dict['name'], tab_id=self.type, link_func=link_reverse_func(self.type), - tab=tab, ) @classmethod - def validate(cls, tab, raise_error=True): - return super(NotesTab, cls).validate(tab, raise_error) and need_name(tab, raise_error) + def validate(cls, tab_dict, raise_error=True): + return super(NotesTab, cls).validate(tab_dict, raise_error) and need_name(tab_dict, raise_error) class InstructorTab(StaffTab): @@ -672,14 +681,13 @@ class InstructorTab(StaffTab): """ type = 'instructor' - def __init__(self, tab=None): + def __init__(self, tab_dict=None): # pylint: disable=unused-argument super(InstructorTab, self).__init__( # Translators: 'Instructor' appears on the tab that leads to the instructor dashboard, which is # a portal where an instructor can get data and perform various actions on their course name=_('Instructor'), tab_id=self.type, link_func=link_reverse_func('instructor_dashboard'), - tab=tab, ) @@ -771,7 +779,9 @@ class CourseTabList(List): the given user with the provided access settings. """ for tab in course.tabs: - if tab.can_display(course, settings, is_user_authenticated, is_user_staff) and not tab.is_hidden: + if tab.can_display( + course, settings, is_user_authenticated, is_user_staff + ) and (not tab.is_hideable or not tab.is_hidden): if tab.is_collection: for item in tab.items(course): yield item @@ -787,11 +797,14 @@ class CourseTabList(List): settings ): """ - Generator method for iterating through all tabs that can be displayed for the given course and - the given user with the provided access settings. + Generator method for iterating through all tabs that can be displayed for the given course + with the provided settings. """ for tab in course.tabs: if tab.can_display(course, settings, is_user_authenticated=True, is_user_staff=True): + if tab.is_collection and not len(list(tab.items(course))): + # do not yield collections that have no items + continue yield tab @classmethod @@ -862,7 +875,7 @@ class CourseTabList(List): Overrides the from_json method to de-serialize the CourseTab objects from a json-like representation. """ self.validate_tabs(values) - return [CourseTab.from_json(tab) for tab in values] + return [CourseTab.from_json(tab_dict) for tab_dict in values] #### Link Functions diff --git a/common/lib/xmodule/xmodule/tests/test_tabs.py b/common/lib/xmodule/xmodule/tests/test_tabs.py index 86f1c1e026..311cced077 100644 --- a/common/lib/xmodule/xmodule/tests/test_tabs.py +++ b/common/lib/xmodule/xmodule/tests/test_tabs.py @@ -17,7 +17,7 @@ class TabTestCase(unittest.TestCase): self.books = None def set_up_books(self, num_books): - """initializes the textbooks in the course and adds the given number of books to each textbook""" + """Initializes the textbooks in the course and adds the given number of books to each textbook""" self.books = [MagicMock() for _ in range(num_books)] for book_index, book in enumerate(self.books): book.title = 'Book{0}'.format(book_index) @@ -76,7 +76,7 @@ class TabTestCase(unittest.TestCase): return tab def check_tab_equality(self, tab, dict_tab): - """tests the equality methods on the given tab""" + """Tests the equality methods on the given tab""" self.assertEquals(tab, dict_tab) # test __eq__ ne_dict_tab = dict_tab ne_dict_tab['type'] = 'fake_type' @@ -84,13 +84,13 @@ class TabTestCase(unittest.TestCase): self.assertNotEquals(tab, {'fake_key': 'fake_value'}) # test __ne__: missing type def check_tab_json_methods(self, tab): - """tests the json from and to methods on the given tab""" + """Tests the json from and to methods on the given tab""" serialized_tab = tab.to_json() deserialized_tab = tab.from_json(serialized_tab) self.assertEquals(serialized_tab, deserialized_tab) def check_can_display_results(self, tab, expected_value=True, for_authenticated_users_only=False, for_staff_only=False): - """checks can display results for various users""" + """Checks can display results for various users""" if for_staff_only: self.assertEquals( expected_value, @@ -108,7 +108,7 @@ class TabTestCase(unittest.TestCase): ) def check_get_and_set_methods(self, tab): - """test __getitem__ and __setitem__ calls""" + """Test __getitem__ and __setitem__ calls""" self.assertEquals(tab['type'], tab.type) self.assertEquals(tab['tab_id'], tab.tab_id) with self.assertRaises(KeyError): @@ -120,7 +120,7 @@ class TabTestCase(unittest.TestCase): tab['invalid_key'] = 'New Value' def check_get_and_set_method_for_key(self, tab, key): - """test __getitem__ and __setitem__ for the given key""" + """Test __getitem__ and __setitem__ for the given key""" old_value = tab[key] new_value = 'New Value' tab[key] = new_value @@ -540,7 +540,7 @@ class CourseTabListTestCase(TabListTestCase): )): self.assertEquals(tab.type, self.course.tabs[i].type) - # enumerate the tabs and verify textbooks and the instructor tab + # enumerate the tabs and verify textbooks and the instructor tab for i, tab in enumerate(tabs.CourseTabList.iterate_displayable( self.course, self.settings, @@ -555,8 +555,21 @@ class CourseTabListTestCase(TabListTestCase): # all other tabs must match the expected type self.assertEquals(tab.type, self.course.tabs[i].type) + # test including non-empty collections + self.assertIn( + tabs.HtmlTextbookTabs(), + list(tabs.CourseTabList.iterate_displayable_cms(self.course, self.settings)), + ) + + # test not including empty collections + self.course.html_textbooks = [] + self.assertNotIn( + tabs.HtmlTextbookTabs(), + list(tabs.CourseTabList.iterate_displayable_cms(self.course, self.settings)), + ) + def test_get_tab_by_methods(self): - """tests the get_tab methods in CourseTabList""" + """Tests the get_tab methods in CourseTabList""" self.course.tabs = self.all_valid_tab_list for tab in self.course.tabs: @@ -587,7 +600,7 @@ class DiscussionLinkTestCase(TabTestCase): @staticmethod def _reverse(course): - """custom reverse function""" + """Custom reverse function""" def reverse_discussion_link(viewname, args): """reverse lookup for discussion link""" if viewname == "django_comment_client.forum.views.forum_form_discussion" and args == [course.id]: diff --git a/lms/envs/common.py b/lms/envs/common.py index 4cef6a88d6..e61ee8740d 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -76,10 +76,11 @@ FEATURES = { 'FORCE_UNIVERSITY_DOMAIN': False, # set this to the university domain to use, as an override to HTTP_HOST # set to None to do no university selection - 'ENABLE_TEXTBOOK': True, - - # for consistency in user-experience, keep the value of this setting in sync with the one in cms/envs/common.py + # for consistency in user-experience, keep the value of the following 3 settings + # in sync with the corresponding ones in cms/envs/common.py 'ENABLE_DISCUSSION_SERVICE': True, + 'ENABLE_TEXTBOOK': True, + 'ENABLE_STUDENT_NOTES': True, # enables the student notes API and UI. # discussion home panel, which includes a subscription on/off setting for discussion digest emails. # this should remain off in production until digest notifications are online. @@ -146,9 +147,6 @@ FEATURES = { # segment.io for LMS--need to explicitly turn it on for production. 'SEGMENT_IO_LMS': False, - # Enables the student notes API and UI. - 'ENABLE_STUDENT_NOTES': True, - # Provide a UI to allow users to submit feedback from the LMS (left-hand help modal) 'ENABLE_FEEDBACK_SUBMISSION': False,