Merge pull request #14757 from edx/jmbowman/PLAT-1198

PLAT-1198 Reduce risk of losing navigation events

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);

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
+            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
+            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)

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()