diff --git a/common/lib/xmodule/xmodule/js/src/sequence/display.js b/common/lib/xmodule/xmodule/js/src/sequence/display.js index b3fb21600c..aa756088a5 100644 --- a/common/lib/xmodule/xmodule/js/src/sequence/display.js +++ b/common/lib/xmodule/xmodule/js/src/sequence/display.js @@ -339,7 +339,7 @@ // `direction` can be 'previous' or 'next' Sequence.prototype._change_sequential = function(direction, event) { - var analyticsEventName, isBottomNav, newPosition, offset, widgetPlacement; + var analyticsEventName, isBottomNav, newPosition, offset, targetUrl, widgetPlacement; // silently abort if direction is invalid. if (direction !== 'previous' && direction !== 'next') { @@ -355,19 +355,27 @@ widgetPlacement = 'top'; } + if ((direction === 'next') && (this.position >= this.contents.length)) { + targetUrl = this.nextUrl; + } else if ((direction === 'previous') && (this.position === 1)) { + targetUrl = this.prevUrl; + } + // Formerly known as seq_next and seq_prev Logger.log(analyticsEventName, { id: this.id, current_tab: this.position, tab_count: this.num_contents, widget_placement: widgetPlacement + }).always(function() { + if (targetUrl) { + // Wait to load the new page until we've attempted to log the event + window.location.href = targetUrl; + } }); - if ((direction === 'next') && (this.position >= this.contents.length)) { - window.location.href = this.nextUrl; - } else if ((direction === 'previous') && (this.position === 1)) { - window.location.href = this.prevUrl; - } else { + // If we're staying on the page, no need to wait for the event logging to finish + if (!targetUrl) { // If the bottom nav is used, scroll to the top of the page on change. if (isBottomNav) { $.scrollTo(0, 150); diff --git a/common/test/acceptance/pages/lms/courseware.py b/common/test/acceptance/pages/lms/courseware.py index 0712b0c7c1..bec5c14c5c 100644 --- a/common/test/acceptance/pages/lms/courseware.py +++ b/common/test/acceptance/pages/lms/courseware.py @@ -131,7 +131,10 @@ class CoursewarePage(CoursePage): against the case where the page is still being loaded. """ active_tab = self._active_sequence_tab - return active_tab and int(active_tab.attrs('data-element')[0]) == sequential_position + try: + return active_tab and int(active_tab.attrs('data-element')[0]) == sequential_position + except IndexError: + return False sequential_position_css = '#sequence-list #tab_{0}'.format(sequential_position - 1) self.q(css=sequential_position_css).first.click() @@ -181,7 +184,10 @@ class CoursewarePage(CoursePage): 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] + try: + return active_tab and previous_tab_id != active_tab.attrs('data-id')[0] + except IndexError: + return False self.q( css='.{} > .sequence-nav-button.{}'.format(top_or_bottom_class, next_or_previous_class) diff --git a/common/test/acceptance/tests/lms/test_lms_courseware.py b/common/test/acceptance/tests/lms/test_lms_courseware.py index 70d04c8bb0..e5b2e4b52b 100644 --- a/common/test/acceptance/tests/lms/test_lms_courseware.py +++ b/common/test/acceptance/tests/lms/test_lms_courseware.py @@ -5,7 +5,6 @@ End-to-end tests for the LMS. import json from datetime import datetime, timedelta -from unittest import skip import ddt from flaky import flaky @@ -441,7 +440,7 @@ class CoursewareMultipleVerticalsTest(CoursewareMultipleVerticalsTestBase): Test courseware with multiple verticals """ - @skip('Disable temporarily to get course bookmarks out') + @flaky # PLAT-1198; should be fixed, but verify that failures stop before removing def test_navigation_buttons(self): self.courseware_page.visit()