diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 1c357a0588..e37fde4276 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,8 @@ These are notable changes in edx-platform. This is a rolling list of changes, in roughly chronological order, most recent first. Add your entries at or near the top. Include a label indicating the component affected. +Studio: Add ability to reorder Pages and hide the Wiki page. STUD-1375 + Blades: Added template for iFrames. BLD-611. Studio: Support for viewing built-in tabs on the Pages page. STUD-1193 diff --git a/cms/djangoapps/contentstore/features/pages.feature b/cms/djangoapps/contentstore/features/pages.feature index 22865e8ece..890c62301f 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 drag the first static page to the last + Then the static pages are switched And I reload the page - Then the static tabs are in the reverse order + Then the static pages are switched + + 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 drag the first page to the last + Then the built-in pages are switched + And I reload the page + Then the built-in pages are switched + + 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 drag the first page to the last + Then the pages are switched + And I reload the page + Then the pages are switched + + 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..e86177282d 100644 --- a/cms/djangoapps/contentstore/features/pages.py +++ b/cms/djangoapps/contentstore/features/pages.py @@ -3,7 +3,10 @@ # 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 + + +CSS_FOR_TAB_ELEMENT = "li[data-tab-id='{0}'] input.toggle-checkbox" @step(u'I go to the pages page$') @@ -33,15 +36,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,10 +54,92 @@ def change_name(step, new_name): world.css_click(save_button) -@step(u'I reorder the static tabs') -def reorder_tabs(_step): +@step(u'I drag the first static page to the last$') +def drag_first_static_page_to_last(step): + drag_first_to_last_with_css('.component') + + +@step(u'I have created a static page$') +def create_static_page(step): + step.given('I have opened the pages page in a new course') + step.given('I add a new static page') + + +@step(u'I have opened the pages page in a new course$') +def open_pages_page_in_new_course(step): + step.given('I have opened a new course in Studio') + step.given('I go to the pages page') + + +@step(u'I have created two different static pages$') +def create_two_pages(step): + step.given('I have created a static page') + 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 pages + _verify_page_names('First', 'Empty') + + +@step(u'the static pages are switched$') +def static_pages_are_switched(step): + _verify_page_names('Empty', 'First') + + +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 pages to be present" + ) + pages = world.css_find('.xmodule_StaticTabModule') + assert_equal(pages[0].text, first) + assert_equal(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 switched$') +def built_in_pages_switched(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 switched$$') +def pages_are_switched(step): + expected_pages = ['Courseware', 'Course Info', 'Wiki', 'Progress', 'First', 'Empty', 'Discussion'] + see_pages_in_expected_order(expected_pages) + + +@step(u'I drag the first page to the last$') +def drag_first_page_to_last(step): + drag_first_to_last_with_css('.is-movable') + + +@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_equal(world.css_find(CSS_FOR_TAB_ELEMENT.format(page_id)).checked, hidden) + + +@step(u'I toggle the visibility of the "([^"]*)" page$') +def page_toggle_visibility(step, page_id): + world.css_find(CSS_FOR_TAB_ELEMENT.format(page_id))[0].click() + + +def drag_first_to_last_with_css(css_class): # For some reason, the drag_and_drop method did not work in this case. - draggables = world.css_find('.component .drag-handle') + 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 @@ -71,39 +147,8 @@ def reorder_tabs(_step): source.action_chains.release().perform() -@step(u'I have created a static page') -def create_static_page(step): - step.given('I have opened the pages page in a new course') - step.given('I add a new static page') - - -@step(u'I have opened the pages page in a new course') -def open_pages_page_in_new_course(step): - step.given('I have opened a new course in Studio') - step.given('I go to the pages page') - - -@step(u'I have created two different static pages') -def create_two_pages(step): - step.given('I have created a static page') - 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') - - -@step(u'the static tabs are in the reverse order') -def tabs_in_reverse_order(step): - _verify_tab_names('Empty', 'First') - - -def _verify_tab_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" - ) - tabs = world.css_find('.xmodule_StaticTabModule') - assert tabs[0].text == first - assert tabs[1].text == second +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..9b7f4a7665 100644 --- a/cms/djangoapps/contentstore/management/commands/tests/test_git_export.py +++ b/cms/djangoapps/contentstore/management/commands/tests/test_git_export.py @@ -150,8 +150,8 @@ class TestGitExport(CourseTestCase): '--format=%an|%ae'], cwd=cwd) self.assertEqual(expect_string, git_log) - # Make changes to course so there is something commit - self.populateCourse() + # Make changes to course so there is something to commit + 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..0b028cdca0 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -400,23 +400,34 @@ 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}) + # create the requested tab_id_locators list + tab_id_locators = [ + { + 'tab_id': tab.tab_id + } for tab in built_in_tabs + ] + tab_id_locators.extend([ + { + 'tab_locator': unicode(self._get_tab_locator(course, tab)) + } for tab in reverse_static_tabs + ]) + + self.client.ajax_post(new_location.url_reverse('tabs'), {'tabs': tab_id_locators}) 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/course.py b/cms/djangoapps/contentstore/views/course.py index ce577cb903..02afd2ad13 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -918,9 +918,9 @@ def textbooks_detail_handler(request, tid, tag=None, package_id=None, branch=Non if not textbook: return JsonResponse(status=404) i = course.pdf_textbooks.index(textbook) - new_textbooks = course.pdf_textbooks[0:i] - new_textbooks.extend(course.pdf_textbooks[i + 1:]) - course.pdf_textbooks = new_textbooks + remaining_textbooks = course.pdf_textbooks[0:i] + remaining_textbooks.extend(course.pdf_textbooks[i + 1:]) + course.pdf_textbooks = remaining_textbooks store.update_item(course, request.user.id) return JsonResponse() diff --git a/cms/djangoapps/contentstore/views/tabs.py b/cms/djangoapps/contentstore/views/tabs.py index 69da777bcf..988fe64894 100644 --- a/cms/djangoapps/contentstore/views/tabs.py +++ b/cms/djangoapps/contentstore/views/tabs.py @@ -14,11 +14,9 @@ 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 _ +from ..utils import get_modulestore, get_lms_link_for_item __all__ = ['tabs_handler'] @@ -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 - ] + tab.locator = loc_mapper().translate_location( + course_item.location.course_id, static_tab_loc, 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, - 'course_locator': locator + 'tabs_to_render': tabs_to_render, + 'course_locator': locator, + 'lms_link': get_lms_link_for_item(course_item.location), }) 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. + requested_tab_id_locators = 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 requested_tab_id_locators: + 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..fd04089bc6 100644 --- a/cms/djangoapps/contentstore/views/tests/test_tabs.py +++ b/cms/djangoapps/contentstore/views/tests/test_tabs.py @@ -1,9 +1,176 @@ """ 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=json.dumps({ + 'tab_id_locator': {'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-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/envs/common.py b/cms/envs/common.py index 6e2618a65e..56d3fcc62e 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -43,9 +43,11 @@ FEATURES = { 'GITHUB_PUSH': False, - # for consistency in user-experience, keep the value of this setting in sync with the - # one in lms/envs/common.py + # for consistency in user-experience, keep the value of the following 3 settings + # in sync with the ones in lms/envs/common.py 'ENABLE_DISCUSSION_SERVICE': True, + 'ENABLE_TEXTBOOK': True, + 'ENABLE_STUDENT_NOTES': True, 'AUTH_USE_CERTIFICATES': False, diff --git a/cms/static/coffee/src/views/tabs.coffee b/cms/static/coffee/src/views/tabs.coffee index 202ea6c6c7..83f520f7ae 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-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: '> .is-movable' ) + 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 is-movable') editor.$el.addClass('new') setTimeout(=> editor.$el.removeClass('new') diff --git a/cms/static/sass/views/_static-pages.scss b/cms/static/sass/views/_static-pages.scss index cbe71bb973..6579c30c0f 100644 --- a/cms/static/sass/views/_static-pages.scss +++ b/cms/static/sass/views/_static-pages.scss @@ -180,7 +180,7 @@ } .component, - .course-nav-tab { + .course-nav-item { position: relative; border: 1px solid $mediumGrey; border-top: none; @@ -239,7 +239,7 @@ } .component-actions, - .course-nav-tab-actions { + .course-nav-item-actions { display: inline-block; float: right; margin-right: ($baseline*2); @@ -289,30 +289,31 @@ } // basic course nav items - overrides from above - .course-nav-tab { + .course-nav-item { padding: ($baseline*.75) ($baseline/4) ($baseline*.75) $baseline; + background: $white; - &.fixed { + &.is-fixed { + @extend %ui-disabled; @include transition(opacity $tmg-f2 ease-in-out 0s); - opacity: .7; - - &:hover { - opacity: 1; - } + opacity: 0.5; } - .course-nav-tab-header { + .course-nav-item-header { display: inline-block; width:80%; .title { @extend %t-title4; - font-weight: 300; - color: $gray; + } + + .title-sub { + @extend %t-title7; + color: $gray-l2; } } - .course-nav-tab-actions { + .course-nav-item-actions { display: inline-block; padding: ($baseline/10); } @@ -335,7 +336,6 @@ @include transition(background-color $tmg-s3 linear 0s); padding: 20px 20px 22px; font-size: 24px; - font-weight: 300; background: #fff; } @@ -395,4 +395,3 @@ outline: 0; } } - diff --git a/cms/templates/edit-tabs.html b/cms/templates/edit-tabs.html index 8be22b81fa..2e3ce617af 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 @@ -39,7 +40,10 @@

${_("Page Actions")}

@@ -54,39 +58,73 @@
-
    +
      - % for tab in built_in_tabs: -
    1. -
      -

      ${_(tab.name)}

      -
      -
      -
        + % 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 tab.is_hideable: -
      • - - -
        -
      • - %endif + % if isinstance(tab, StaticTab): +
      • -
      -
      -
      - ${_("Fixed page")} -
      -
    2. + % else: +
    3. +
      + + % if tab.is_collection: + +

      ${_(tab.name)}

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

      ${_(tab.name)}

      + % endif +
      + +
      +
        + + % if tab.is_hideable: +
      • + + % if tab.is_hidden: + + % else: + + % endif +
        +
      • + % endif + +
      +
      + + % if tab.is_movable: +
      + ${_("Drag to reorder")} +
      + % else: +
      + ${_("This page cannot be reordered")} +
      + % endif +
    4. + + % endif % endfor - % for locator in components: -
    5. - % endfor - -
    6. - -
    7. +
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..cd1f5101db 100644 --- a/common/lib/xmodule/xmodule/tabs.py +++ b/common/lib/xmodule/xmodule/tabs.py @@ -28,6 +28,15 @@ class CourseTab(object): # pylint: disable=incomplete-protocol # subclass, shared by all instances of the subclass. type = '' + # 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): """ Initializes class members with values passed in by subclasses. @@ -48,9 +57,6 @@ 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 - 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. @@ -140,12 +146,12 @@ 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_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): """ @@ -158,7 +164,7 @@ class CourseTab(object): # pylint: disable=incomplete-protocol 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. @@ -191,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): @@ -218,14 +224,53 @@ 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. """ type = 'courseware' + is_movable = False - def __init__(self, tab=None): # pylint: disable=unused-argument + 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 @@ -240,18 +285,19 @@ 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'), ) @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): @@ -261,10 +307,10 @@ 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), ) @@ -273,34 +319,32 @@ class ProgressTab(AuthenticatedCourseTab): 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' - def __init__(self, tab=None): - # LATER - enable the following flag to enable hiding of the Wiki page - # self.is_hideable = True - + 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_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): @@ -310,10 +354,10 @@ 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'), ) @@ -322,8 +366,8 @@ class DiscussionTab(CourseTab): 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): @@ -363,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): @@ -374,12 +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, + link_value=tab_dict['link'] if tab_dict else link_value, ) @@ -389,11 +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'], + link_value=tab_dict['link'], ) @@ -402,17 +446,15 @@ class StaticTab(CourseTab): A custom tab. """ type = 'static_tab' - url_slug = '' @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 - tab_name = tab['name'] if tab else name + 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, + 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]), ) @@ -446,6 +488,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 +499,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): + # 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, + ) @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,10 +524,15 @@ class TextbookTabs(TextbookTabsBase): """ type = 'textbooks' + def __init__(self, tab_dict=None): # pylint: disable=unused-argument + super(TextbookTabs, self).__init__( + tab_id=self.type, + ) + 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, @@ -491,7 +547,12 @@ class PDFTextbookTabs(TextbookTabsBase): """ type = 'pdf_textbooks' - def books(self, course): + def __init__(self, tab_dict=None): # pylint: disable=unused-argument + super(PDFTextbookTabs, self).__init__( + tab_id=self.type, + ) + + def items(self, course): for index, textbook in enumerate(course.pdf_textbooks): yield SingleTextbookTab( name=textbook['tab_title'], @@ -506,7 +567,12 @@ class HtmlTextbookTabs(TextbookTabsBase): """ type = 'html_textbooks' - def books(self, course): + def __init__(self, tab_dict=None): # pylint: disable=unused-argument + super(HtmlTextbookTabs, self).__init__( + tab_id=self.type, + ) + + def items(self, course): for index, textbook in enumerate(course.html_textbooks): yield SingleTextbookTab( name=textbook['tab_title'], @@ -528,7 +594,7 @@ class StaffGradingTab(StaffTab, GradingTab): """ type = 'staff_grading' - def __init__(self, tab=None): # pylint: disable=unused-argument + 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 @@ -544,7 +610,7 @@ class PeerGradingTab(AuthenticatedCourseTab, GradingTab): """ type = 'peer_grading' - def __init__(self, tab=None): # pylint: disable=unused-argument + 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 @@ -560,7 +626,7 @@ class OpenEndedGradingTab(AuthenticatedCourseTab, GradingTab): """ type = 'open_ended' - def __init__(self, tab=None): # pylint: disable=unused-argument + 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 @@ -579,7 +645,7 @@ 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_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'), @@ -597,16 +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), ) @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): @@ -615,7 +681,7 @@ class InstructorTab(StaffTab): """ type = 'instructor' - def __init__(self, tab=None): # pylint: disable=unused-argument + 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 @@ -681,36 +747,68 @@ 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_hideable or 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 + 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 - 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,8 +874,8 @@ 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] + self.validate_tabs(values) + return [CourseTab.from_json(tab_dict) for tab_dict in values] #### Link Functions @@ -833,3 +931,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..311cced077 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, @@ -90,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): @@ -102,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 @@ -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,64 @@ 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) + # 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""" + 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.""" @@ -526,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/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/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, 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):