diff --git a/common/lib/xmodule/xmodule/css/sequence/display.scss b/common/lib/xmodule/xmodule/css/sequence/display.scss index e42b885218..31f519b874 100644 --- a/common/lib/xmodule/xmodule/css/sequence/display.scss +++ b/common/lib/xmodule/xmodule/css/sequence/display.scss @@ -30,6 +30,23 @@ $link-color: rgb(26, 161, 222) !default; } } +%ui-clear-button { + background-color: rgba(0,0,0,0); + background-image: none; + background-position: center 14px; + background-repeat: no-repeat; + border: none; + border-radius: 0; + + background-clip: border-box; + box-shadow: none; + box-sizing: content-box; + font-family: inherit; + font-size: inherit; + font-weight: inherit; +} + + // ==================== .sequence-nav { @@ -90,7 +107,7 @@ $link-color: rgb(26, 161, 222) !default; padding: 0 ($baseline/2); width: 100%; - a { + button { @extend .block-link; } @@ -98,15 +115,14 @@ $link-color: rgb(26, 161, 222) !default; display: table-cell; min-width: 20px; - a { + button { @extend %ui-fake-link; + @extend %ui-clear-button; @include transition(none); width: 100%; height: 42px; margin: 0; - background-position: center 14px; - background-repeat: no-repeat; - border: 1px solid transparent; + display: block; padding: 0; position: relative; @@ -197,7 +213,6 @@ $link-color: rgb(26, 161, 222) !default; .sequence-tooltip { @extend %ui-depth2; - display: none; margin-top: ($baseline/5); background: #333; color: $white; @@ -209,7 +224,6 @@ $link-color: rgb(26, 161, 222) !default; top: 48px; text-shadow: 0 -1px 0 $black; @include text-align(left); - @include transition(all .1s $ease-in-out-quart 0s); white-space: pre; pointer-events: none; @@ -233,17 +247,11 @@ $link-color: rgb(26, 161, 222) !default; width: 10px; } } - - &:hover, &:focus { - .sequence-tooltip { - display: block; - } - } } } } - body.touch-based-device & ol li a:hover p { + body.touch-based-device & ol li button:hover p { display: none; } } diff --git a/common/lib/xmodule/xmodule/js/fixtures/sequence.html b/common/lib/xmodule/xmodule/js/fixtures/sequence.html index 53e9531dd2..f8660ea286 100644 --- a/common/lib/xmodule/xmodule/js/fixtures/sequence.html +++ b/common/lib/xmodule/xmodule/js/fixtures/sequence.html @@ -4,8 +4,8 @@ @@ -13,8 +13,8 @@ diff --git a/common/lib/xmodule/xmodule/js/src/sequence/display.coffee b/common/lib/xmodule/xmodule/js/src/sequence/display.coffee index 9c098efbcc..0025ffb663 100644 --- a/common/lib/xmodule/xmodule/js/src/sequence/display.coffee +++ b/common/lib/xmodule/xmodule/js/src/sequence/display.coffee @@ -8,6 +8,8 @@ class @Sequence @num_contents = @contents.length @id = @el.data('id') @ajaxUrl = @el.data('ajax-url') + @nextUrl = @el.data('next-url') + @prevUrl = @el.data('prev-url') @base_page_title = " | " + document.title @initProgress() @bind() @@ -17,9 +19,17 @@ class @Sequence $(selector, @el) bind: -> - @$('#sequence-list a').click @goto + @$('#sequence-list .nav-item').click @goto @el.on 'bookmark:add', @addBookmarkIconToActiveNavItem @el.on 'bookmark:remove', @removeBookmarkIconFromActiveNavItem + @$('#sequence-list .nav-item').on('focus mouseenter', @displayTabTooltip) + @$('#sequence-list .nav-item').on('blur mouseleave', @hideTabTooltip) + + displayTabTooltip: (event) => + $(event.currentTarget).find('.sequence-tooltip').removeClass('sr') + + hideTabTooltip: (event) => + $(event.currentTarget).find('.sequence-tooltip').addClass('sr') initProgress: -> @progressTable = {} # "#problem_#{id}" -> progress @@ -73,23 +83,35 @@ class @Sequence when 'in_progress' then element.addClass('progress-some') when 'done' then element.addClass('progress-done') + enableButton: (button_class, button_action) -> + @$(button_class).removeClass('disabled').removeAttr('disabled').click(button_action) + + disableButton: (button_class) -> + @$(button_class).addClass('disabled').attr('disabled', true) + + setButtonLabel: (button_class, button_label) -> + @$(button_class + ' .sr').html(button_label) + + updateButtonState: (button_class, button_action, action_label_prefix, is_at_boundary, boundary_url) -> + if is_at_boundary and boundary_url == 'None' + @disableButton(button_class) + else + button_label = action_label_prefix + (if is_at_boundary then ' Section' else ' Unit') + @setButtonLabel(button_class, button_label) + @enableButton(button_class, button_action) + toggleArrows: => @$('.sequence-nav-button').unbind('click') - if @contents.length == 0 ## There are no modules to display, and therefore no nav to build. - @$('.sequence-nav-button.button-previous').addClass('disabled').attr('disabled', true) - @$('.sequence-nav-button.button-next').addClass('disabled').attr('disabled', true) - return + # previous button + first_tab = @position == 1 + previous_button_class = '.sequence-nav-button.button-previous' + @updateButtonState(previous_button_class, @previous, 'Previous', first_tab, @prevUrl) - if @position == 1 ## 1 != 0 here. 1 is the first item in the sequence nav. - @$('.sequence-nav-button.button-previous').addClass('disabled').attr('disabled', true) - else - @$('.sequence-nav-button.button-previous').removeClass('disabled').removeAttr('disabled').click(@previous) - - if @position == @contents.length ## If the final position on the nav matches the total contents. - @$('.sequence-nav-button.button-next').addClass('disabled').attr('disabled', true) - else - @$('.sequence-nav-button.button-next').removeClass('disabled').removeAttr('disabled').click(@next) + # next button + last_tab = @position >= @contents.length # use inequality in case contents.length is 0 and position is 1. + next_button_class = '.sequence-nav-button.button-next' + @updateButtonState(next_button_class, @next, 'Next', last_tab, @nextUrl) render: (new_position) -> if @position != new_position @@ -122,7 +144,6 @@ class @Sequence @el.find('.path').text(@el.find('.nav-item.active').data('path')) @sr_container.focus(); - # @$("a.active").blur() goto: (event) => event.preventDefault() @@ -164,13 +185,18 @@ class @Sequence new: new_position id: @id - # If the bottom nav is used, scroll to the top of the page on change. - if $(event.target).closest('nav[class="sequence-bottom"]').length > 0 - $.scrollTo 0, 150 - @render new_position + if (direction == "seq_next") and (@position == @contents.length) + window.location.href = @nextUrl + else if (direction == "seq_prev") and (@position == 1) + window.location.href = @prevUrl + else + # If the bottom nav is used, scroll to the top of the page on change. + if $(event.target).closest('nav[class="sequence-bottom"]').length > 0 + $.scrollTo 0, 150 + @render new_position link_for: (position) -> - @$("#sequence-list a[data-element=#{position}]") + @$("#sequence-list .nav-item[data-element=#{position}]") mark_visited: (position) -> # Don't overwrite class attribute to avoid changing Progress class diff --git a/common/lib/xmodule/xmodule/seq_module.py b/common/lib/xmodule/xmodule/seq_module.py index 2e9c561fcc..9548bfa57d 100644 --- a/common/lib/xmodule/xmodule/seq_module.py +++ b/common/lib/xmodule/xmodule/seq_module.py @@ -141,16 +141,8 @@ class SequenceModule(SequenceFields, ProctoringFields, XModule): # If position is specified in system, then use that instead. position = getattr(self.system, 'position', None) if position is not None: - try: - self.position = int(self.system.position) - except (ValueError, TypeError): - # Check for https://openedx.atlassian.net/browse/LMS-6496 - warnings.warn( - "Sequential position cannot be converted to an integer: {pos!r}".format( - pos=self.system.position, - ), - RuntimeWarning, - ) + assert isinstance(position, int) + self.position = self.system.position def get_progress(self): ''' Return the total progress, adding total done and total available. @@ -177,9 +169,16 @@ class SequenceModule(SequenceFields, ProctoringFields, XModule): raise NotFoundError('Unexpected dispatch type') def student_view(self, context): + display_items = self.get_display_items() + # If we're rendering this sequence, but no position is set yet, + # or exceeds the length of the displayable items, # default the position to the first element - if self.position is None: + if context.get('requested_child') == 'first': + self.position = 1 + elif context.get('requested_child') == 'last': + self.position = len(display_items) or None + elif self.position is None or self.position > len(display_items): self.position = 1 ## Returns a set of all types of all sub-children @@ -211,7 +210,6 @@ class SequenceModule(SequenceFields, ProctoringFields, XModule): fragment.add_content(view_html) return fragment - display_items = self.get_display_items() for child in display_items: is_bookmarked = bookmarks_service.is_bookmarked(usage_key=child.scope_ids.usage_id) context["bookmarked"] = is_bookmarked @@ -245,6 +243,16 @@ class SequenceModule(SequenceFields, ProctoringFields, XModule): 'position': self.position, 'tag': self.location.category, 'ajax_url': self.system.ajax_url, + 'next_url': _compute_next_url( + self.location, + parent_module, + context.get('redirect_url_func'), + ), + 'prev_url': _compute_previous_url( + self.location, + parent_module, + context.get('redirect_url_func'), + ), } fragment.add_content(self.system.render_template("seq_module.html", params)) @@ -453,3 +461,88 @@ class SequenceDescriptor(SequenceFields, ProctoringFields, MakoModuleDescriptor, xblock_body["content_type"] = "Sequence" return xblock_body + + +def _compute_next_url(block_location, parent_block, redirect_url_func): + """ + Returns the url for the next block after the given block. + """ + def get_next_block_location(parent_block, index_in_parent): + """ + Returns the next block in the parent_block after the block with the given + index_in_parent. + """ + if index_in_parent + 1 < len(parent_block.children): + return parent_block.children[index_in_parent + 1] + else: + return None + + return _compute_next_or_prev_url( + block_location, + parent_block, + redirect_url_func, + get_next_block_location, + 'first', + ) + + +def _compute_previous_url(block_location, parent_block, redirect_url_func): + """ + Returns the url for the previous block after the given block. + """ + def get_previous_block_location(parent_block, index_in_parent): + """ + Returns the previous block in the parent_block before the block with the given + index_in_parent. + """ + return parent_block.children[index_in_parent - 1] if index_in_parent else None + + return _compute_next_or_prev_url( + block_location, + parent_block, + redirect_url_func, + get_previous_block_location, + 'last', + ) + + +def _compute_next_or_prev_url( + block_location, + parent_block, + redirect_url_func, + get_next_or_prev_block, + redirect_url_child_param, +): + """ + Returns the url for the next or previous block from the given block. + + Arguments: + block_location: Location of the block that is being navigated. + parent_block: Parent block of the given block. + redirect_url_func: Function that computes a redirect URL directly to + a block, given the block's location. + get_next_or_prev_block: Function that returns the next or previous + block in the parent, or None if doesn't exist. + redirect_url_child_param: Value to pass for the child parameter to the + redirect_url_func. + """ + if redirect_url_func: + index_in_parent = parent_block.children.index(block_location) + next_or_prev_block_location = get_next_or_prev_block(parent_block, index_in_parent) + if next_or_prev_block_location: + return redirect_url_func( + block_location.course_key, + next_or_prev_block_location, + child=redirect_url_child_param, + ) + else: + grandparent = parent_block.get_parent() + if grandparent: + return _compute_next_or_prev_url( + parent_block.location, + grandparent, + redirect_url_func, + get_next_or_prev_block, + redirect_url_child_param, + ) + return None diff --git a/common/lib/xmodule/xmodule/tests/test_sequence.py b/common/lib/xmodule/xmodule/tests/test_sequence.py new file mode 100644 index 0000000000..9e4e1aba36 --- /dev/null +++ b/common/lib/xmodule/xmodule/tests/test_sequence.py @@ -0,0 +1,163 @@ +""" +Tests for sequence module. +""" +# pylint: disable=no-member +from mock import Mock +from xblock.reference.user_service import XBlockUser, UserService +from xmodule.tests import get_test_system +from xmodule.tests.xml import XModuleXmlImportTest +from xmodule.tests.xml import factories as xml +from xmodule.x_module import STUDENT_VIEW +from xmodule.seq_module import _compute_next_url, _compute_previous_url, SequenceModule + + +class StubUserService(UserService): + """ + Stub UserService for testing the sequence module. + """ + def get_current_user(self): + """ + Implements abstract method for getting the current user. + """ + user = XBlockUser() + user.opt_attrs['edx-platform.username'] = 'test user' + return user + + +class SequenceBlockTestCase(XModuleXmlImportTest): + """ + Tests for the Sequence Module. + """ + @classmethod + def setUpClass(cls): + super(SequenceBlockTestCase, cls).setUpClass() + + course_xml = cls._set_up_course_xml() + cls.course = cls.process_xml(course_xml) + cls._set_up_module_system(cls.course) + + for chapter_index in range(len(cls.course.get_children())): + chapter = cls._set_up_block(cls.course, chapter_index) + setattr(cls, 'chapter_{}'.format(chapter_index + 1), chapter) + + for sequence_index in range(len(chapter.get_children())): + sequence = cls._set_up_block(chapter, sequence_index) + setattr(cls, 'sequence_{}_{}'.format(chapter_index + 1, sequence_index + 1), sequence) + + @classmethod + def _set_up_course_xml(cls): + """ + Sets up and returns XML course structure. + """ + course = xml.CourseFactory.build() + + chapter_1 = xml.ChapterFactory.build(parent=course) # has 2 child sequences + xml.ChapterFactory.build(parent=course) # has 0 child sequences + chapter_3 = xml.ChapterFactory.build(parent=course) # has 1 child sequence + chapter_4 = xml.ChapterFactory.build(parent=course) # has 2 child sequences + + xml.SequenceFactory.build(parent=chapter_1) + xml.SequenceFactory.build(parent=chapter_1) + sequence_3_1 = xml.SequenceFactory.build(parent=chapter_3) # has 3 verticals + xml.SequenceFactory.build(parent=chapter_4) + xml.SequenceFactory.build(parent=chapter_4) + + for _ in range(3): + xml.VerticalFactory.build(parent=sequence_3_1) + + return course + + @classmethod + def _set_up_block(cls, parent, index_in_parent): + """ + Sets up the stub sequence module for testing. + """ + block = parent.get_children()[index_in_parent] + + cls._set_up_module_system(block) + + block.xmodule_runtime._services['bookmarks'] = Mock() # pylint: disable=protected-access + block.xmodule_runtime._services['user'] = StubUserService() # pylint: disable=protected-access + block.xmodule_runtime.xmodule_instance = getattr(block, '_xmodule', None) # pylint: disable=protected-access + block.parent = parent.location + return block + + @classmethod + def _set_up_module_system(cls, block): + """ + Sets up the test module system for the given block. + """ + module_system = get_test_system() + module_system.descriptor_runtime = block._runtime # pylint: disable=protected-access + block.xmodule_runtime = module_system + + def test_student_view_init(self): + seq_module = SequenceModule(runtime=Mock(position=2), descriptor=Mock(), scope_ids=Mock()) + self.assertEquals(seq_module.position, 2) # matches position set in the runtime + + def test_render_student_view(self): + html = self._get_rendered_student_view(self.sequence_3_1, requested_child=None) + self._assert_view_at_position(html, expected_position=1) + self.assertIn(unicode(self.sequence_3_1.location), html) + self.assertIn("'next_url': u'{}'".format(unicode(self.chapter_4.location)), html) + self.assertIn("'prev_url': u'{}'".format(unicode(self.chapter_2.location)), html) + + def test_student_view_first_child(self): + html = self._get_rendered_student_view(self.sequence_3_1, requested_child='first') + self._assert_view_at_position(html, expected_position=1) + + def test_student_view_last_child(self): + html = self._get_rendered_student_view(self.sequence_3_1, requested_child='last') + self._assert_view_at_position(html, expected_position=3) + + def _get_rendered_student_view(self, sequence, requested_child): + """ + Returns the rendered student view for the given sequence and the + requested_child parameter. + """ + return sequence.xmodule_runtime.render( + sequence, + STUDENT_VIEW, + { + 'redirect_url_func': lambda course_key, block_location, child: unicode(block_location), + 'requested_child': requested_child, + }, + ).content + + def _assert_view_at_position(self, rendered_html, expected_position): + """ + Verifies that the rendered view contains the expected position. + """ + self.assertIn("'position': {}".format(expected_position), rendered_html) + + def test_compute_next_url(self): + + for sequence, parent, expected_next_sequence_location in [ + (self.sequence_1_1, self.chapter_1, self.sequence_1_2.location), + (self.sequence_1_2, self.chapter_1, self.chapter_2.location), + (self.sequence_3_1, self.chapter_3, self.chapter_4.location), + (self.sequence_4_1, self.chapter_4, self.sequence_4_2.location), + (self.sequence_4_2, self.chapter_4, None), + ]: + actual_next_sequence_location = _compute_next_url( + sequence.location, + parent, + lambda course_key, block_location, child: block_location, + ) + self.assertEquals(actual_next_sequence_location, expected_next_sequence_location) + + def test_compute_previous_url(self): + + for sequence, parent, expected_prev_sequence_location in [ + (self.sequence_1_1, self.chapter_1, None), + (self.sequence_1_2, self.chapter_1, self.sequence_1_1.location), + (self.sequence_3_1, self.chapter_3, self.chapter_2.location), + (self.sequence_4_1, self.chapter_4, self.chapter_3.location), + (self.sequence_4_2, self.chapter_4, self.sequence_4_1.location), + ]: + actual_next_sequence_location = _compute_previous_url( + sequence.location, + parent, + lambda course_key, block_location, child: block_location, + ) + self.assertEquals(actual_next_sequence_location, expected_prev_sequence_location) diff --git a/common/lib/xmodule/xmodule/tests/xml/__init__.py b/common/lib/xmodule/xmodule/tests/xml/__init__.py index 3beeebf271..2c777c69c6 100644 --- a/common/lib/xmodule/xmodule/tests/xml/__init__.py +++ b/common/lib/xmodule/xmodule/tests/xml/__init__.py @@ -57,7 +57,8 @@ class InMemorySystem(XMLParsingSystem, MakoDescriptorSystem): # pylint: disable class XModuleXmlImportTest(TestCase): """Base class for tests that use basic XML parsing""" - def process_xml(self, xml_import_data): + @classmethod + def process_xml(cls, xml_import_data): """Use the `xml_import_data` to import an :class:`XBlock` from XML.""" system = InMemorySystem(xml_import_data) return system.process_xml(xml_import_data.xml_string) diff --git a/common/lib/xmodule/xmodule/tests/xml/factories.py b/common/lib/xmodule/xmodule/tests/xml/factories.py index b131a65c76..903f2d925e 100644 --- a/common/lib/xmodule/xmodule/tests/xml/factories.py +++ b/common/lib/xmodule/xmodule/tests/xml/factories.py @@ -8,6 +8,7 @@ from fs.memoryfs import MemoryFS from factory import Factory, lazy_attribute, post_generation, Sequence from lxml import etree +from xblock.mixins import HierarchyMixin from xmodule.modulestore.inheritance import InheritanceMixin from xmodule.x_module import XModuleMixin from xmodule.modulestore import only_xmodules @@ -68,7 +69,7 @@ class XmlImportFactory(Factory): model = XmlImportData filesystem = MemoryFS() - xblock_mixins = (InheritanceMixin, XModuleMixin) + xblock_mixins = (InheritanceMixin, XModuleMixin, HierarchyMixin) xblock_select = only_xmodules url_name = Sequence(str) attribs = {} @@ -142,6 +143,11 @@ class CourseFactory(XmlImportFactory): static_asset_path = 'xml_test_course' +class ChapterFactory(XmlImportFactory): + """Factory for nodes""" + tag = 'chapter' + + class SequenceFactory(XmlImportFactory): """Factory for nodes""" tag = 'sequential' diff --git a/common/test/acceptance/pages/lms/course_nav.py b/common/test/acceptance/pages/lms/course_nav.py index bc68b55077..7892019698 100644 --- a/common/test/acceptance/pages/lms/course_nav.py +++ b/common/test/acceptance/pages/lms/course_nav.py @@ -3,7 +3,7 @@ Course navigation page object """ import re -from bok_choy.page_object import PageObject +from bok_choy.page_object import PageObject, unguarded from bok_choy.promise import EmptyPromise @@ -57,7 +57,7 @@ class CourseNavPage(PageObject): Example return value: ['Chemical Bonds Video', 'Practice Problems', 'Homework'] """ - seq_css = 'ol#sequence-list>li>a>.sequence-tooltip' + seq_css = 'ol#sequence-list>li>.nav-item>.sequence-tooltip' return self.q(css=seq_css).map(self._clean_seq_titles).results def go_to_section(self, section_title, subsection_title): @@ -124,7 +124,7 @@ class CourseNavPage(PageObject): # Click on the sequence item at the correct index # Convert the list index (starts at 0) to a CSS index (starts at 1) - seq_css = "ol#sequence-list>li:nth-of-type({0})>a".format(seq_index + 1) + seq_css = "ol#sequence-list>li:nth-of-type({0})>.nav-item".format(seq_index + 1) self.q(css=seq_css).first.click() # Click triggers an ajax event self.wait_for_ajax() @@ -165,10 +165,11 @@ class CourseNavPage(PageObject): """ desc = "currently at section '{0}' and subsection '{1}'".format(section_title, subsection_title) return EmptyPromise( - lambda: self._is_on_section(section_title, subsection_title), desc + lambda: self.is_on_section(section_title, subsection_title), desc ) - def _is_on_section(self, section_title, subsection_title): + @unguarded + def is_on_section(self, section_title, subsection_title): """ Return a boolean indicating whether the user is on the section and subsection with the specified titles. @@ -203,13 +204,9 @@ class CourseNavPage(PageObject): """ return self.REMOVE_SPAN_TAG_RE.search(element.get_attribute('innerHTML')).groups()[0].strip() - def go_to_sequential_position(self, sequential_position): + @property + def active_subsection_url(self): """ - Within a section/subsection navigate to the sequential position specified by `sequential_position`. - - Arguments: - sequential_position (int): position in sequential bar - + return the url of the active subsection in the left nav """ - sequential_position_css = '#tab_{0}'.format(sequential_position - 1) - self.q(css=sequential_position_css).first.click() + return self.q(css='.chapter-content-container .menu-item.active a').attrs('href')[0] diff --git a/common/test/acceptance/pages/lms/courseware.py b/common/test/acceptance/pages/lms/courseware.py index b87b3c194e..819f79358a 100644 --- a/common/test/acceptance/pages/lms/courseware.py +++ b/common/test/acceptance/pages/lms/courseware.py @@ -20,6 +20,13 @@ class CoursewarePage(CoursePage): def is_browser_on_page(self): return self.q(css='body.courseware').present + @property + def chapter_count_in_navigation(self): + """ + Returns count of chapters available on LHS navigation. + """ + return len(self.q(css='nav.course-navigation a.chapter')) + @property def num_sections(self): """ @@ -101,11 +108,66 @@ class CoursewarePage(CoursePage): return element.text[0] return None - def get_active_subsection_url(self): + def go_to_sequential_position(self, sequential_position): """ - return the url of the active subsection in the left nav + Within a section/subsection navigate to the sequential position specified by `sequential_position`. + + Arguments: + sequential_position (int): position in sequential bar """ - return self.q(css='.chapter-content-container .menu-item.active a').attrs('href')[0] + sequential_position_css = '#sequence-list #tab_{0}'.format(sequential_position - 1) + self.q(css=sequential_position_css).first.click() + + @property + def sequential_position(self): + """ + Returns the position of the active tab in the sequence. + """ + tab_id = self._active_sequence_tab.attrs('id')[0] + return int(tab_id.split('_')[1]) + + @property + def _active_sequence_tab(self): # pylint: disable=missing-docstring + return self.q(css='#sequence-list .nav-item.active') + + @property + def is_next_button_enabled(self): # pylint: disable=missing-docstring + return not self.q(css='.sequence-nav > .sequence-nav-button.button-next.disabled').is_present() + + @property + def is_previous_button_enabled(self): # pylint: disable=missing-docstring + return not self.q(css='.sequence-nav > .sequence-nav-button.button-previous.disabled').is_present() + + def click_next_button_on_top(self): # pylint: disable=missing-docstring + self._click_navigation_button('sequence-nav', 'button-next') + + def click_next_button_on_bottom(self): # pylint: disable=missing-docstring + self._click_navigation_button('sequence-bottom', 'button-next') + + def click_previous_button_on_top(self): # pylint: disable=missing-docstring + self._click_navigation_button('sequence-nav', 'button-previous') + + def click_previous_button_on_bottom(self): # pylint: disable=missing-docstring + self._click_navigation_button('sequence-bottom', 'button-previous') + + def _click_navigation_button(self, top_or_bottom_class, next_or_previous_class): + """ + Clicks the navigation button, given the respective CSS classes. + """ + previous_tab_id = self._active_sequence_tab.attrs('data-id')[0] + + def is_at_new_tab_id(): + """ + Returns whether the active tab has changed. It is defensive + against the case where the page is still being loaded. + """ + active_tab = self._active_sequence_tab + return active_tab and previous_tab_id != active_tab.attrs('data-id')[0] + + self.q( + css='.{} > .sequence-nav-button.{}'.format(top_or_bottom_class, next_or_previous_class) + ).first.click() + EmptyPromise(is_at_new_tab_id, "Button navigation fulfilled").fulfill() @property def can_start_proctored_exam(self): @@ -160,13 +222,6 @@ class CoursewarePage(CoursePage): return self.entrance_exam_message_selector.is_present() \ and "You have passed the entrance exam" in self.entrance_exam_message_selector.text[0] - @property - def chapter_count_in_navigation(self): - """ - Returns count of chapters available on LHS navigation. - """ - return len(self.q(css='nav.course-navigation a.chapter')) - @property def is_timer_bar_present(self): """ @@ -178,7 +233,7 @@ class CoursewarePage(CoursePage): """ Returns the usage id of active sequence item """ get_active = lambda el: 'active' in el.get_attribute('class') attribute_value = lambda el: el.get_attribute('data-id') - return self.q(css='#sequence-list a').filter(get_active).map(attribute_value).results[0] + return self.q(css='#sequence-list .nav-item').filter(get_active).map(attribute_value).results[0] @property def breadcrumb(self): diff --git a/common/test/acceptance/tests/lms/test_lms_courseware.py b/common/test/acceptance/tests/lms/test_lms_courseware.py index c6391212be..0f1456fcbe 100644 --- a/common/test/acceptance/tests/lms/test_lms_courseware.py +++ b/common/test/acceptance/tests/lms/test_lms_courseware.py @@ -2,7 +2,7 @@ """ End-to-end tests for the LMS. """ -import time +from nose.plugins.attrib import attr from ..helpers import UniqueCourseTest from ...pages.studio.auto_auth import AutoAuthPage @@ -420,13 +420,20 @@ class CoursewareMultipleVerticalsTest(UniqueCourseTest): course_fix.add_children( XBlockFixtureDesc('chapter', 'Test Section 1').add_children( - XBlockFixtureDesc('sequential', 'Test Subsection 1').add_children( + XBlockFixtureDesc('sequential', 'Test Subsection 1,1').add_children( XBlockFixtureDesc('problem', 'Test Problem 1', data='problem 1 dummy body'), XBlockFixtureDesc('html', 'html 1', data="html 1 dummy body"), XBlockFixtureDesc('problem', 'Test Problem 2', data="problem 2 dummy body"), XBlockFixtureDesc('html', 'html 2', data="html 2 dummy body"), ), - XBlockFixtureDesc('sequential', 'Test Subsection 2'), + XBlockFixtureDesc('sequential', 'Test Subsection 1,2').add_children( + XBlockFixtureDesc('problem', 'Test Problem 3', data='problem 3 dummy body'), + ), + ), + XBlockFixtureDesc('chapter', 'Test Section 2').add_children( + XBlockFixtureDesc('sequential', 'Test Subsection 2,1').add_children( + XBlockFixtureDesc('problem', 'Test Problem 4', data='problem 4 dummy body'), + ), ), ).install() @@ -436,10 +443,53 @@ class CoursewareMultipleVerticalsTest(UniqueCourseTest): self.courseware_page.visit() self.course_nav = CourseNavPage(self.browser) + def test_navigation_buttons(self): + # start in first section + self.assert_navigation_state('Test Section 1', 'Test Subsection 1,1', 0, next_enabled=True, prev_enabled=False) + + # next takes us to next tab in sequential + self.courseware_page.click_next_button_on_top() + self.assert_navigation_state('Test Section 1', 'Test Subsection 1,1', 1, next_enabled=True, prev_enabled=True) + + # go to last sequential position + self.courseware_page.go_to_sequential_position(4) + self.assert_navigation_state('Test Section 1', 'Test Subsection 1,1', 3, next_enabled=True, prev_enabled=True) + + # next takes us to next sequential + self.courseware_page.click_next_button_on_bottom() + self.assert_navigation_state('Test Section 1', 'Test Subsection 1,2', 0, next_enabled=True, prev_enabled=True) + + # next takes us to next chapter + self.courseware_page.click_next_button_on_top() + self.assert_navigation_state('Test Section 2', 'Test Subsection 2,1', 0, next_enabled=False, prev_enabled=True) + + # previous takes us to previous chapter + self.courseware_page.click_previous_button_on_top() + self.assert_navigation_state('Test Section 1', 'Test Subsection 1,2', 0, next_enabled=True, prev_enabled=True) + + # previous takes us to last tab in previous sequential + self.courseware_page.click_previous_button_on_bottom() + self.assert_navigation_state('Test Section 1', 'Test Subsection 1,1', 3, next_enabled=True, prev_enabled=True) + + # previous takes us to previous tab in sequential + self.courseware_page.click_previous_button_on_bottom() + self.assert_navigation_state('Test Section 1', 'Test Subsection 1,1', 2, next_enabled=True, prev_enabled=True) + + def assert_navigation_state( + self, section_title, subsection_title, subsection_position, next_enabled, prev_enabled + ): + """ + Verifies that the navigation state is as expected. + """ + self.assertTrue(self.course_nav.is_on_section(section_title, subsection_title)) + self.assertEquals(self.courseware_page.sequential_position, subsection_position) + self.assertEquals(self.courseware_page.is_next_button_enabled, next_enabled) + self.assertEquals(self.courseware_page.is_previous_button_enabled, prev_enabled) + def test_tab_position(self): # test that using the position in the url direct to correct tab in courseware - self.course_nav.go_to_section('Test Section 1', 'Test Subsection 1') - subsection_url = self.courseware_page.get_active_subsection_url() + self.course_nav.go_to_section('Test Section 1', 'Test Subsection 1,1') + subsection_url = self.course_nav.active_subsection_url url_part_list = subsection_url.split('/') self.assertEqual(len(url_part_list), 9) @@ -481,3 +531,14 @@ class CoursewareMultipleVerticalsTest(UniqueCourseTest): position=4 ).visit() self.assertIn('html 2 dummy body', html2_page.get_selected_tab_content()) + + @attr('a11y') + def test_courseware_a11y(self): + """ + Run accessibility audit for the problem type. + """ + self.course_nav.go_to_section('Test Section 1', 'Test Subsection 1,1') + # Set the scope to the sequence navigation + self.courseware_page.a11y_audit.config.set_scope( + include=['div.sequence-nav']) + self.courseware_page.a11y_audit.check_for_accessibility_errors() diff --git a/common/test/acceptance/tests/lms/test_lms_edxnotes.py b/common/test/acceptance/tests/lms/test_lms_edxnotes.py index fe4314c7ba..b10c3d694c 100644 --- a/common/test/acceptance/tests/lms/test_lms_edxnotes.py +++ b/common/test/acceptance/tests/lms/test_lms_edxnotes.py @@ -194,14 +194,14 @@ class EdxNotesDefaultInteractionsTest(EdxNotesTestMixin): self.create_notes(components) self.assert_text_in_notes(self.note_unit_page.notes) - self.course_nav.go_to_sequential_position(2) + self.courseware_page.go_to_sequential_position(2) components = self.note_unit_page.components self.create_notes(components) components = self.note_unit_page.refresh() self.assert_text_in_notes(self.note_unit_page.notes) - self.course_nav.go_to_sequential_position(1) + self.courseware_page.go_to_sequential_position(1) components = self.note_unit_page.components self.assert_text_in_notes(self.note_unit_page.notes) @@ -227,7 +227,7 @@ class EdxNotesDefaultInteractionsTest(EdxNotesTestMixin): self.edit_notes(components) self.assert_text_in_notes(self.note_unit_page.notes) - self.course_nav.go_to_sequential_position(2) + self.courseware_page.go_to_sequential_position(2) components = self.note_unit_page.components self.edit_notes(components) self.assert_text_in_notes(self.note_unit_page.notes) @@ -235,7 +235,7 @@ class EdxNotesDefaultInteractionsTest(EdxNotesTestMixin): components = self.note_unit_page.refresh() self.assert_text_in_notes(self.note_unit_page.notes) - self.course_nav.go_to_sequential_position(1) + self.courseware_page.go_to_sequential_position(1) components = self.note_unit_page.components self.assert_text_in_notes(self.note_unit_page.notes) @@ -261,7 +261,7 @@ class EdxNotesDefaultInteractionsTest(EdxNotesTestMixin): self.remove_notes(components) self.assert_notes_are_removed(components) - self.course_nav.go_to_sequential_position(2) + self.courseware_page.go_to_sequential_position(2) components = self.note_unit_page.components self.remove_notes(components) self.assert_notes_are_removed(components) @@ -269,7 +269,7 @@ class EdxNotesDefaultInteractionsTest(EdxNotesTestMixin): components = self.note_unit_page.refresh() self.assert_notes_are_removed(components) - self.course_nav.go_to_sequential_position(1) + self.courseware_page.go_to_sequential_position(1) components = self.note_unit_page.components self.assert_notes_are_removed(components) @@ -1106,10 +1106,10 @@ class EdxNotesPageTest(EventsTestMixin, EdxNotesTestMixin): self.assertTrue(note.is_visible) note = self.note_unit_page.notes[1] self.assertFalse(note.is_visible) - self.course_nav.go_to_sequential_position(2) + self.courseware_page.go_to_sequential_position(2) note = self.note_unit_page.notes[0] self.assertFalse(note.is_visible) - self.course_nav.go_to_sequential_position(1) + self.courseware_page.go_to_sequential_position(1) note = self.note_unit_page.notes[0] self.assertFalse(note.is_visible) @@ -1494,7 +1494,7 @@ class EdxNotesToggleNotesTest(EdxNotesTestMixin): # Disable all notes self.note_unit_page.toggle_visibility() self.assertEqual(len(self.note_unit_page.notes), 0) - self.course_nav.go_to_sequential_position(2) + self.courseware_page.go_to_sequential_position(2) self.assertEqual(len(self.note_unit_page.notes), 0) self.course_nav.go_to_section(u"Test Section 1", u"Test Subsection 2") self.assertEqual(len(self.note_unit_page.notes), 0) @@ -1520,7 +1520,7 @@ class EdxNotesToggleNotesTest(EdxNotesTestMixin): # the page. self.note_unit_page.toggle_visibility() self.assertGreater(len(self.note_unit_page.notes), 0) - self.course_nav.go_to_sequential_position(2) + self.courseware_page.go_to_sequential_position(2) self.assertGreater(len(self.note_unit_page.notes), 0) self.course_nav.go_to_section(u"Test Section 1", u"Test Subsection 2") self.assertGreater(len(self.note_unit_page.notes), 0) diff --git a/common/test/acceptance/tests/studio/test_studio_outline.py b/common/test/acceptance/tests/studio/test_studio_outline.py index a72e0a0a4c..d7cd119d69 100644 --- a/common/test/acceptance/tests/studio/test_studio_outline.py +++ b/common/test/acceptance/tests/studio/test_studio_outline.py @@ -1546,7 +1546,7 @@ class PublishSectionTest(CourseOutlineTest): self.assertTrue(section.publish_action) self.courseware.visit() self.assertEqual(1, self.courseware.num_xblock_components) - self.course_nav.go_to_sequential_position(2) + self.courseware.go_to_sequential_position(2) self.assertEqual(1, self.courseware.num_xblock_components) def test_section_publishing(self): @@ -1571,7 +1571,7 @@ class PublishSectionTest(CourseOutlineTest): self.assertFalse(unit.publish_action) self.courseware.visit() self.assertEqual(1, self.courseware.num_xblock_components) - self.course_nav.go_to_sequential_position(2) + self.courseware.go_to_sequential_position(2) self.assertEqual(1, self.courseware.num_xblock_components) self.course_nav.go_to_section(SECTION_NAME, 'Test Subsection 2') self.assertEqual(1, self.courseware.num_xblock_components) diff --git a/common/test/acceptance/tests/video/test_video_module.py b/common/test/acceptance/tests/video/test_video_module.py index 91d09c447d..e3a7820b4c 100644 --- a/common/test/acceptance/tests/video/test_video_module.py +++ b/common/test/acceptance/tests/video/test_video_module.py @@ -11,6 +11,7 @@ from unittest import skipIf, skip from ..helpers import UniqueCourseTest, is_youtube_available, YouTubeStubConfig from ...pages.lms.video.video import VideoPage from ...pages.lms.tab_nav import TabNavPage +from ...pages.lms.courseware import CoursewarePage from ...pages.lms.course_nav import CourseNavPage from ...pages.lms.auto_auth import AutoAuthPage from ...pages.lms.course_info import CourseInfoPage @@ -49,6 +50,7 @@ class VideoBaseTest(UniqueCourseTest): self.video = VideoPage(self.browser) self.tab_nav = TabNavPage(self.browser) self.course_nav = CourseNavPage(self.browser) + self.courseware = CoursewarePage(self.browser, self.course_id) self.course_info_page = CourseInfoPage(self.browser, self.course_id) self.auth_page = AutoAuthPage(self.browser, course_id=self.course_id) @@ -190,7 +192,7 @@ class VideoBaseTest(UniqueCourseTest): """ Navigate to sequential specified by `video_display_name` """ - self.course_nav.go_to_sequential_position(position) + self.courseware.go_to_sequential_position(position) self.video.wait_for_video_player_render() @@ -917,13 +919,13 @@ class YouTubeVideoTest(VideoBaseTest): execute_video_steps(['A']) # go to second sequential position - self.course_nav.go_to_sequential_position(2) + self.courseware.go_to_sequential_position(2) execute_video_steps(tab2_video_names) # go back to first sequential position # we are again playing tab 1 videos to ensure that switching didn't broke some video functionality. - self.course_nav.go_to_sequential_position(1) + self.courseware.go_to_sequential_position(1) execute_video_steps(tab1_video_names) self.video.browser.refresh() diff --git a/lms/djangoapps/courseware/features/navigation.py b/lms/djangoapps/courseware/features/navigation.py index bcc7a04d2b..90c0517319 100644 --- a/lms/djangoapps/courseware/features/navigation.py +++ b/lms/djangoapps/courseware/features/navigation.py @@ -109,7 +109,7 @@ def when_i_navigate_to_a_subsection(step): @step(u'I navigate to an item in a sequence') def when_i_navigate_to_an_item_in_a_sequence(step): - sequence_css = 'a[data-element="2"]' + sequence_css = '.nav-item[data-element="2"]' world.css_click(sequence_css) diff --git a/lms/djangoapps/courseware/tests/test_split_module.py b/lms/djangoapps/courseware/tests/test_split_module.py index c5e6668ba3..61ed3816b6 100644 --- a/lms/djangoapps/courseware/tests/test_split_module.py +++ b/lms/djangoapps/courseware/tests/test_split_module.py @@ -123,7 +123,7 @@ class SplitTestBase(SharedModuleStoreTestCase): content = resp.content # Assert we see the proper icon in the top display - self.assertIn(' 0: - default_child = child_modules[0] + default_child = _get_child(child_modules) else: content_children = [child for child in child_modules if child.has_children_at_depth(min_depth - 1) and child.get_display_items()] - default_child = content_children[0] if content_children else None + default_child = _get_child(content_children) if content_children else None return default_child if not hasattr(xmodule, 'position'): return None - if xmodule.position is None: + if xmodule.position is None or requested_child: return _get_default_child_module(xmodule.get_display_items()) else: # position is 1-indexed. @@ -421,14 +434,6 @@ def _index_bulk_op(request, course_key, chapter, section, position): field_data_cache = FieldDataCache.cache_for_descriptor_descendents( course_key, user, course, depth=2) - course_module = get_module_for_descriptor( - user, request, course, field_data_cache, course_key, course=course - ) - if course_module is None: - log.warning(u'If you see this, something went wrong: if we got this' - u' far, should have gotten a course module for this user') - return redirect(reverse('about_course', args=[course_key.to_deprecated_string()])) - studio_url = get_studio_url(course, 'course') language_preference = get_user_preference(request.user, LANGUAGE_KEY) @@ -478,89 +483,89 @@ def _index_bulk_op(request, course_key, chapter, section, position): # passing CONTENT_DEPTH avoids returning 404 for a course with an # empty first section and a second section with content - return redirect_to_course_position(course_module, CONTENT_DEPTH) + return redirect_to_course_position(course, CONTENT_DEPTH) chapter_descriptor = course.get_child_by(lambda m: m.location.name == chapter) if chapter_descriptor is not None: - save_child_position(course_module, chapter) + save_child_position(course, chapter) else: - raise Http404('No chapter descriptor found with name {}'.format(chapter)) - - chapter_module = course_module.get_child_by(lambda m: m.location.name == chapter) - if chapter_module is None: # User may be trying to access a chapter that isn't live yet if masquerade and masquerade.role == 'student': # if staff is masquerading as student be kinder, don't 404 log.debug('staff masquerading as student: no chapter %s', chapter) return redirect(reverse('courseware', args=[course.id.to_deprecated_string()])) - raise Http404 + raise Http404('No chapter descriptor found with name {}'.format(chapter)) if course_has_entrance_exam(course): # Message should not appear outside the context of entrance exam subsection. # if section is none then we don't need to show message on welcome back screen also. - if getattr(chapter_module, 'is_entrance_exam', False) and section is not None: + if getattr(chapter_descriptor, 'is_entrance_exam', False) and section is not None: context['entrance_exam_current_score'] = get_entrance_exam_score(request, course) context['entrance_exam_passed'] = user_has_passed_entrance_exam(request, course) - if section is not None: - section_descriptor = chapter_descriptor.get_child_by(lambda m: m.location.name == section) - - if section_descriptor is None: - # Specifically asked-for section doesn't exist - if masquerade and masquerade.role == 'student': # don't 404 if staff is masquerading as student - log.debug('staff masquerading as student: no section %s', section) - return redirect(reverse('courseware', args=[course.id.to_deprecated_string()])) - raise Http404 - - # Allow chromeless operation - if section_descriptor.chrome: - chrome = [s.strip() for s in section_descriptor.chrome.lower().split(",")] - if 'accordion' not in chrome: - context['disable_accordion'] = True - if 'tabs' not in chrome: - context['disable_tabs'] = True - - if section_descriptor.default_tab: - context['default_tab'] = section_descriptor.default_tab - - # cdodge: this looks silly, but let's refetch the section_descriptor with depth=None - # which will prefetch the children more efficiently than doing a recursive load - section_descriptor = modulestore().get_item(section_descriptor.location, depth=None) - - # Load all descendants of the section, because we're going to display its - # html, which in general will need all of its children - field_data_cache.add_descriptor_descendents( - section_descriptor, depth=None - ) - - section_module = get_module_for_descriptor( - user, - request, - section_descriptor, - field_data_cache, - course_key, - position, - course=course - ) - - if section_module is None: - # User may be trying to be clever and access something - # they don't have access to. - raise Http404 - - # Save where we are in the chapter. - save_child_position(chapter_module, section) - section_render_context = {'activate_block_id': request.GET.get('activate_block_id')} - context['fragment'] = section_module.render(STUDENT_VIEW, section_render_context) - context['section_title'] = section_descriptor.display_name_with_default_escaped - else: - prev_section = get_current_child(chapter_module) - if prev_section is None: + if section is None: + section_descriptor = get_current_child(chapter_descriptor, requested_child=request.GET.get("child")) + if section_descriptor: + section = section_descriptor.url_name + else: # Something went wrong -- perhaps this chapter has no sections visible to the user. # Clearing out the last-visited state and showing "first-time" view by redirecting # to courseware. - course_module.position = None - course_module.save() + course.position = None + course.save() return redirect(reverse('courseware', args=[course.id.to_deprecated_string()])) + else: + section_descriptor = chapter_descriptor.get_child_by(lambda m: m.location.name == section) + + if section_descriptor is None: + # Specifically asked-for section doesn't exist + if masquerade and masquerade.role == 'student': # don't 404 if staff is masquerading as student + log.debug('staff masquerading as student: no section %s', section) + return redirect(reverse('courseware', args=[course.id.to_deprecated_string()])) + raise Http404 + + # Allow chromeless operation + if section_descriptor.chrome: + chrome = [s.strip() for s in section_descriptor.chrome.lower().split(",")] + if 'accordion' not in chrome: + context['disable_accordion'] = True + if 'tabs' not in chrome: + context['disable_tabs'] = True + + if section_descriptor.default_tab: + context['default_tab'] = section_descriptor.default_tab + + # cdodge: this looks silly, but let's refetch the section_descriptor with depth=None + # which will prefetch the children more efficiently than doing a recursive load + section_descriptor = modulestore().get_item(section_descriptor.location, depth=None) + + # Load all descendants of the section, because we're going to display its + # html, which in general will need all of its children + field_data_cache.add_descriptor_descendents( + section_descriptor, depth=None + ) + + section_module = get_module_for_descriptor( + user, + request, + section_descriptor, + field_data_cache, + course_key, + position, + course=course + ) + + # Save where we are in the chapter. + save_child_position(chapter_descriptor, section) + section_render_context = { + 'activate_block_id': request.GET.get('activate_block_id'), + 'redirect_url_func': ( + get_redirect_url if settings.FEATURES.get('ENABLE_NEXT_BUTTON_ACROSS_SECTIONS') else None + ), + 'requested_child': request.GET.get("child"), + } + context['accordion'] = render_accordion(user, request, course, chapter, section, field_data_cache) + context['fragment'] = section_module.render(STUDENT_VIEW, section_render_context) + context['section_title'] = section_descriptor.display_name_with_default_escaped result = render_to_response('courseware/courseware.html', context) except Exception as e: diff --git a/lms/envs/bok_choy.py b/lms/envs/bok_choy.py index d4d8ff2be4..1737836968 100644 --- a/lms/envs/bok_choy.py +++ b/lms/envs/bok_choy.py @@ -162,6 +162,9 @@ FEATURES['ENABLE_COURSEWARE_SEARCH'] = True # Enable dashboard search for tests FEATURES['ENABLE_DASHBOARD_SEARCH'] = True +# Enable cross-section Next button for tests +FEATURES['ENABLE_NEXT_BUTTON_ACROSS_SECTIONS'] = True + # Use MockSearchEngine as the search engine for test scenario SEARCH_ENGINE = "search.tests.mock_search_engine.MockSearchEngine" # Path at which to store the mock index diff --git a/lms/envs/common.py b/lms/envs/common.py index c7388c0f28..e7aa79ac59 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -371,7 +371,10 @@ FEATURES = { # This is the default, but can be disabled if all history # lives in the Extended table, saving the frontend from # making multiple queries. - 'ENABLE_READING_FROM_MULTIPLE_HISTORY_TABLES': True + 'ENABLE_READING_FROM_MULTIPLE_HISTORY_TABLES': True, + + # Enable Next Button to jump sequences in Sequence Navigation bar. + 'ENABLE_NEXT_BUTTON_ACROSS_SECTIONS': False, } # Ignore static asset files on import which match this pattern diff --git a/lms/templates/seq_module.html b/lms/templates/seq_module.html index f3ddc1938f..2bce3a658b 100644 --- a/lms/templates/seq_module.html +++ b/lms/templates/seq_module.html @@ -1,6 +1,7 @@ +<%page expression_filter="h"/> <%! from django.utils.translation import ugettext as _ %> -
+
% endfor @@ -43,7 +41,7 @@ aria-labelledby="tab_${idx}" aria-hidden="true" class="seq_contents tex2jax_ignore asciimath2jax_ignore"> - ${item['content'] | h} + ${item['content']}
% endfor