From dceace62b6ed14e757190d20fc6499baf4b32c65 Mon Sep 17 00:00:00 2001
From: Taka <taka@vm-linux-dev>
Date: Wed, 15 Aug 2018 03:26:01 +0000
Subject: [PATCH] Check for a dirty scenario before destructive operations.

---
 conftest.py                                   |  10 +-
 vasl_templates/main_window.py                 |  44 ++++-
 vasl_templates/webapp/static/css/main.css     |   3 +
 vasl_templates/webapp/static/main.js          |  18 ++-
 vasl_templates/webapp/static/snippets.js      | 130 ++++++++++-----
 vasl_templates/webapp/static/utils.js         |   1 +
 .../tests/test_dirty_scenario_checks.py       | 153 ++++++++++++++++++
 .../webapp/tests/test_scenario_persistence.py |  82 +++++++---
 .../webapp/tests/test_template_packs.py       |   6 +-
 .../webapp/tests/test_vo_reports.py           |   4 +-
 vasl_templates/webapp/tests/utils.py          |  23 ++-
 11 files changed, 387 insertions(+), 87 deletions(-)
 create mode 100644 vasl_templates/webapp/tests/test_dirty_scenario_checks.py

diff --git a/conftest.py b/conftest.py
index a75404e..72039ee 100644
--- a/conftest.py
+++ b/conftest.py
@@ -39,12 +39,12 @@ def pytest_addoption( parser ):
 
     # add test options
     parser.addoption(
-        "--no-clipboard", action="store_true", dest="no_clipboard", default=False,
-        help="Don't use the clipboard to get snippets."
+        "--short-tests", action="store_true", dest="short_tests", default=False,
+        help="Run a shorter version of the test suite."
     )
     parser.addoption(
-        "--vo-reports", action="store_true", dest="check_vo_reports", default=False,
-        help="Check the vehicle/ordnance reports."
+        "--no-clipboard", action="store_true", dest="no_clipboard", default=False,
+        help="Don't use the clipboard to get snippets."
     )
 
 # ---------------------------------------------------------------------
@@ -62,6 +62,8 @@ def webapp():
             # very quickly, causing problems by obscuring other elements and making them non-clickable :-/
             # We used to explicitly dismiss them, but it's simpler to just always disable them.
             kwargs["store_msgs"] = 1
+            # stop the browser from checking for a dirty scenario when leaving the page
+            kwargs["disable_close_window_check"] = 1
             # check if the tests are being run headless
             if pytest.config.option.headless: #pylint: disable=no-member
                 # yup - there is no clipboard support :-/
diff --git a/vasl_templates/main_window.py b/vasl_templates/main_window.py
index 44deab3..6c3dea4 100644
--- a/vasl_templates/main_window.py
+++ b/vasl_templates/main_window.py
@@ -2,7 +2,7 @@
 
 import os
 
-from PyQt5.QtWidgets import QWidget, QVBoxLayout, QLabel, QFileDialog
+from PyQt5.QtWidgets import QWidget, QVBoxLayout, QLabel, QFileDialog, QMessageBox
 from PyQt5.QtWebEngineWidgets import QWebEngineView, QWebEngineProfile, QWebEnginePage
 from PyQt5.QtGui import QDesktopServices
 from PyQt5.QtCore import QUrl
@@ -23,6 +23,8 @@ class MainWindow( QWidget ):
         # initialize
         assert MainWindow._main_window is None
         MainWindow._main_window = self
+        self.view = None
+        self._is_closing = False
 
         # initialize
         super().__init__()
@@ -38,19 +40,47 @@ class MainWindow( QWidget ):
         if not webapp.config.get( "DISABLE_WEBENGINEVIEW" ):
             # load the webapp
             # NOTE: We create an off-the-record profile to stop the view from using cached JS files :-/
-            view = QWebEngineView()
-            layout.addWidget( view )
-            profile = QWebEngineProfile( None, view )
+            self.view = QWebEngineView()
+            layout.addWidget( self.view )
+            profile = QWebEngineProfile( None, self.view )
             profile.downloadRequested.connect( self.onDownloadRequested )
-            page = QWebEnginePage( profile, view )
-            view.setPage( page )
-            view.load( QUrl(url) )
+            page = QWebEnginePage( profile, self.view )
+            self.view.setPage( page )
+            self.view.load( QUrl(url) )
         else:
             label = QLabel()
             label.setText( "Running the {} application.\n\nClose this window when you're done.".format( APP_NAME ) )
             layout.addWidget( label )
             QDesktopServices.openUrl( QUrl(url) )
 
+    def closeEvent( self, evt ) :
+        """Handle requests to close the window (i.e. exit the application)."""
+
+        # check if we need to check for a dirty scenario
+        if self.view is None or self._is_closing:
+            return
+
+        # check if the scenario is dirty
+        def callback( is_dirty ):
+            """Callback for PyQt to return the result of running the Javascript."""
+            if not is_dirty:
+                # nope - just close the window
+                self._is_closing = True
+                self.close()
+                return
+            # yup - ask the user to confirm the close
+            rc = QMessageBox.question( self, "Close program",
+                "This scenario has been changed\n\nDo you want to close the program, and lose your changes?",
+                QMessageBox.Yes | QMessageBox.No,
+                QMessageBox.No
+            )
+            if rc == QMessageBox.Yes:
+                # confirmed - close the window
+                self._is_closing = True
+                self.close()
+        self.view.page().runJavaScript( "is_scenario_dirty()", callback )
+        evt.ignore() # nb: we wait until the Javascript finishes to process the event
+
     @staticmethod
     def onDownloadRequested( item ):
         """Handle download requests."""
diff --git a/vasl_templates/webapp/static/css/main.css b/vasl_templates/webapp/static/css/main.css
index 9fb2242..efa0b63 100644
--- a/vasl_templates/webapp/static/css/main.css
+++ b/vasl_templates/webapp/static/css/main.css
@@ -109,6 +109,7 @@ button.edit-template img { height: 18px ; vertical-align: middle ; margin-right:
 
 .ui-dialog-titlebar { padding: 0.2em 0.5em 0.2em 0.5em !important ; }
 .ui-dialog-titlebar-close { margin-top: -10px !important ; }
+.ui-dialog-content p { margin-bottom: 0.5em ; }
 
 #edit-template { overflow: hidden ; padding: 2px ; }
 .ui-dialog.edit-template .ui-dialog-titlebar { background: #f8d868 ; }
@@ -122,6 +123,8 @@ button.edit-template img { height: 18px ; vertical-align: middle ; margin-right:
 .ui-dialog.edit-simple_note .ui-dialog-buttonpane { border: none ; padding: 0 ; font-size: 75% ; }
 .ui-dialog.edit-simple_note button { margin: 0 0 0 5px ; padding: 0.1em 0.2em ; }
 
+.ui-dialog.ask .ui-dialog-titlebar { background: #ffa878 ; }
+
 #select-vo { overflow: hidden ; padding: 2px ; }
 #select-vo .header { height: 1.75em ; margin-top: 0.25em ; font-size: 80% ; }
 #select-vo select { width: 100% ; top: 2em ; height: calc(100% - 1.5em) ; }
diff --git a/vasl_templates/webapp/static/main.js b/vasl_templates/webapp/static/main.js
index be25c4e..4982461 100644
--- a/vasl_templates/webapp/static/main.js
+++ b/vasl_templates/webapp/static/main.js
@@ -1,7 +1,7 @@
-var gTemplatePack = {} ;
-var gDefaultNationalities = {} ;
-var gValidTemplateIds = [] ;
-var gVehicleOrdnanceListings = {} ;
+gTemplatePack = {} ;
+gDefaultNationalities = {} ;
+gValidTemplateIds = [] ;
+gVehicleOrdnanceListings = {} ;
 
 var _NATIONALITY_SPECIFIC_BUTTONS = {
     "russian": [ "mol", "mol-p" ],
@@ -288,6 +288,16 @@ $(document).ready( function () {
     // initialize hotkeys
     init_hotkeys() ;
 
+    // check for a dirty scenario before leaving the page
+    if ( ! getUrlParam( "disable_close_window_check" ) ) {
+        window.addEventListener( "beforeunload", function(evt) {
+            if ( is_scenario_dirty() ) {
+                evt.returnValue = "This scenario has been changed. Do you want to leave the page, and lose your changes?" ;
+                return evt.returnValue ;
+            }
+        } ) ;
+    }
+
     // add some dummy links for the test suite to edit templates
     if ( getUrlParam( "edit_template_links" ) ) {
         $("button.generate").each( function() {
diff --git a/vasl_templates/webapp/static/snippets.js b/vasl_templates/webapp/static/snippets.js
index 3557b17..33d107d 100644
--- a/vasl_templates/webapp/static/snippets.js
+++ b/vasl_templates/webapp/static/snippets.js
@@ -15,6 +15,7 @@ var _DAY_OF_MONTH_POSTFIXES = { // nb: we assume English :-/
 } ;
 
 var gDefaultScenario = null ;
+var gLastSavedScenario = null ;
 
 // --------------------------------------------------------------------
 
@@ -38,7 +39,7 @@ function generate_snippet( $btn, extra_params )
 
     // unload the template parameters
     var template_id = $btn.data( "id" ) ;
-    unload_params( params, true ) ;
+    unload_snippet_params( params, true ) ;
 
     // set player-specific parameters
     var nationalities = gTemplatePack.nationalities ;
@@ -191,7 +192,7 @@ function generate_snippet( $btn, extra_params )
 
 // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
 
-function unload_params( params, check_date_capabilities )
+function unload_snippet_params( params, check_date_capabilities )
 {
     // collect all the template parameters
     add_param = function($elem) { params[ $elem.attr("name") ] = $elem.val() ; } ;
@@ -439,19 +440,30 @@ function edit_template( template_id )
 
 function on_load_scenario()
 {
-    // FIXME! confirm this operation if the scenario is dirty
-
-    // FOR TESTING PORPOISES! We can't control a file upload from Selenium (since
-    // the browser will use native controls), so we store the result in a <div>).
-    if ( getUrlParam( "scenario_persistence" ) ) {
-        var $elem = $( "#_scenario-persistence_" ) ;
-        do_load_scenario( JSON.parse( $elem.val() ) ) ;
-        showInfoMsg( "The scenario was loaded." ) ; // nb: the tests are looking for this
-        return ;
+    // check if the scenario is dirty
+    if ( ! is_scenario_dirty() )
+        do_on_load_scenario() ;
+    else {
+        // yup - confirm the operation
+        ask( "Load scenario", "<p>This scenario has been changed.<p>Do you want load another scenario, and lose your changes?", {
+            ok: do_on_load_scenario,
+            cancel: function() {},
+        } ) ;
     }
 
-    // ask the user to upload the scenario file
-    $("#load-scenario").trigger( "click" ) ;
+    function do_on_load_scenario() {
+        // FOR TESTING PORPOISES! We can't control a file upload from Selenium (since
+        // the browser will use native controls), so we get the data from a <textarea>).
+        if ( getUrlParam( "scenario_persistence" ) ) {
+            var $elem = $( "#_scenario-persistence_" ) ;
+            do_load_scenario( JSON.parse( $elem.val() ) ) ;
+            showInfoMsg( "The scenario was loaded." ) ; // nb: the tests are looking for this
+            return ;
+        }
+
+        // ask the user to upload the scenario file
+        $("#load-scenario").trigger( "click" ) ;
+    }
 }
 
 function on_load_scenario_file_selected()
@@ -576,14 +588,37 @@ function do_load_scenario( params )
         ) ) ;
     }
 
+    // remember the state of this scenario
+    gLastSavedScenario = unload_params_for_save() ;
+
     // update the UI
     $("#tabs").tabs( "option", "active", 0 ) ;
     on_scenario_date_change() ;
 }
 
-// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+// --------------------------------------------------------------------
 
 function on_save_scenario()
+{
+    // unload the template parameters
+    var params = unload_params_for_save() ;
+    var data = JSON.stringify( params ) ;
+
+    // remember this as the last saved scenario
+    gLastSavedScenario = params ;
+
+    // FOR TESTING PORPOISES! We can't control a file download from Selenium (since
+    // the browser will use native controls), so we store the result in a <textarea>).
+    if ( getUrlParam( "scenario_persistence" ) ) {
+        $("#_scenario-persistence_").val( data ) ;
+        return ;
+    }
+
+    // return the parameters to the user as a downloadable file
+    download( data, "scenario.json", "application/json" ) ;
+}
+
+function unload_params_for_save()
 {
     // unload the template parameters
     function extract_vo_names( key ) { // nb: we only need to save the vehicle/ordnance name
@@ -595,7 +630,7 @@ function on_save_scenario()
         params[key] = names ;
     }
     var params = {} ;
-    unload_params( params, false ) ;
+    unload_snippet_params( params, false ) ;
     params.SCENARIO_NOTES = $("#scenario_notes-sortable").sortable2( "get-entry-data" ) ;
     params.OB_SETUPS_1 = $("#ob_setups-sortable_1").sortable2( "get-entry-data" ) ;
     params.OB_SETUPS_2 = $("#ob_setups-sortable_2").sortable2( "get-entry-data" ) ;
@@ -605,42 +640,44 @@ function on_save_scenario()
     extract_vo_names( "ORDNANCE_1" ) ;
     extract_vo_names( "VEHICLES_2" ) ;
     extract_vo_names( "ORDNANCE_2" ) ;
-    var data = JSON.stringify( params ) ;
-
-    // FOR TESTING PORPOISES! We can't control a file download from Selenium (since
-    // the browser will use native controls), so we store the result in a <div>).
-    if ( getUrlParam( "scenario_persistence" ) ) {
-        $("#_scenario-persistence_").val( data ) ;
-        return ;
-    }
 
-    // return the parameters to the user as a downloadable file
-    download( data, "scenario.json", "application/json" ) ;
+    return params ;
 }
 
-// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+// --------------------------------------------------------------------
 
 function on_new_scenario( verbose )
 {
-    // FIXME! confirm this operation if the scenario is dirty
-
-    // load the default scenario
-    if ( gDefaultScenario )
-        do_load_scenario( gDefaultScenario ) ;
+    // check if the scenario is dirty
+    if ( ! is_scenario_dirty() )
+        do_on_new_scenario() ;
     else {
-        $.getJSON( gGetDefaultScenarioUrl, function(data) {
-            gDefaultScenario = data ;
-            do_load_scenario( data ) ;
-            update_page_load_status( "default-scenario" ) ;
-        } ).fail( function( xhr, status, errorMsg ) {
-            showErrorMsg( "Can't get the default scenario:<div class='pre'>" + escapeHTML(errorMsg) + "</div>" ) ;
-            return ;
+        // yup - confirm the operation
+        ask( "New scenario", "<p>This scenario has been changed.<p>Do you want to reset it, and lose your changes?", {
+            ok: do_on_new_scenario,
+            cancel: function() {},
         } ) ;
     }
 
-    // provide some feedback to the user
-    if ( verbose )
-        showInfoMsg( "The scenario was reset." ) ;
+    function do_on_new_scenario() {
+        // load the default scenario
+        if ( gDefaultScenario )
+            do_load_scenario( gDefaultScenario ) ;
+        else {
+            $.getJSON( gGetDefaultScenarioUrl, function(data) {
+                gDefaultScenario = data ;
+                do_load_scenario( data ) ;
+                update_page_load_status( "default-scenario" ) ;
+            } ).fail( function( xhr, status, errorMsg ) {
+                showErrorMsg( "Can't get the default scenario:<div class='pre'>" + escapeHTML(errorMsg) + "</div>" ) ;
+                return ;
+            } ) ;
+        }
+
+        // provide some feedback to the user
+        if ( verbose )
+            showInfoMsg( "The scenario was reset." ) ;
+    }
 }
 
 function reset_scenario()
@@ -671,6 +708,17 @@ function reset_scenario()
 
 // --------------------------------------------------------------------
 
+function is_scenario_dirty()
+{
+    // check if the scenario has been changed since it was loaded, or last saved
+    if ( gLastSavedScenario === null )
+        return false ;
+    var params = unload_params_for_save() ;
+    return (JSON.stringify(params) != JSON.stringify(gLastSavedScenario) ) ;
+}
+
+// --------------------------------------------------------------------
+
 function on_template_pack()
 {
     // FOR TESTING PORPOISES! We can't control a file upload from Selenium (since
diff --git a/vasl_templates/webapp/static/utils.js b/vasl_templates/webapp/static/utils.js
index 824c7ba..102a3e6 100644
--- a/vasl_templates/webapp/static/utils.js
+++ b/vasl_templates/webapp/static/utils.js
@@ -128,6 +128,7 @@ function ask( title, msg, args )
     var $dlg = $("#ask") ;
     $dlg.html( msg ) ;
     $dlg.dialog( {
+        dialogClass: "ask",
         modal: true,
         title: title,
         open: function() {
diff --git a/vasl_templates/webapp/tests/test_dirty_scenario_checks.py b/vasl_templates/webapp/tests/test_dirty_scenario_checks.py
new file mode 100644
index 0000000..2a7b6e4
--- /dev/null
+++ b/vasl_templates/webapp/tests/test_dirty_scenario_checks.py
@@ -0,0 +1,153 @@
+""" Test checks for a dirty scenario. """
+
+import re
+
+import pytest
+from selenium.webdriver.support.ui import Select
+
+from vasl_templates.webapp.tests.test_scenario_persistence import ALL_SCENARIO_PARAMS
+from vasl_templates.webapp.tests.test_vehicles_ordnance import add_vo
+from vasl_templates.webapp.tests.utils import \
+    init_webapp, select_tab, select_menu_option, add_simple_note, select_droplist_val, select_droplist_index, \
+    drag_sortable_entry_to_trash, get_sortable_entry_count, \
+    get_stored_msg, set_stored_msg_marker, find_child, find_children, wait_for
+
+# ---------------------------------------------------------------------
+
+def test_dirty_scenario_checks( webapp, webdriver ):
+    """Test checking for a dirty scenario."""
+
+    # initialize
+    init_webapp( webapp, webdriver )
+
+    # initialize
+    SIMPLE_NOTES = {
+        "SCENARIO_NOTES": "#scenario_notes-sortable",
+        "SSR": "#ssr-sortable",
+        "OB_SETUPS_1": "#ob_setups-sortable_1",
+        "OB_NOTES_1": "#ob_notes-sortable_1",
+        "OB_SETUPS_2": "#ob_setups-sortable_2",
+        "OB_NOTES_2": "#ob_notes-sortable_2",
+    }
+    VEHICLE_ORDNANCE = {
+        "VEHICLES_1": ( "#vehicles-sortable_1", 1, "a german vehicle" ),
+        "ORDNANCE_1": ( "#ordnance-sortable_1", 1, "a german ordnance" ),
+        "VEHICLES_2": ( "#vehicles-sortable_2", 2, "a russian vehicle" ),
+        "ORDNANCE_2": ( "#ordnance-sortable_2", 2, "a russian ordnance" ),
+    }
+
+    def change_field( param ):
+        """Make a change to a field."""
+        # make a change to the specified field
+        if param in SIMPLE_NOTES:
+            target = find_child( SIMPLE_NOTES[param] )
+            add_simple_note( target, "changed value", None )
+            return target
+        if param in VEHICLE_ORDNANCE:
+            info = VEHICLE_ORDNANCE[param]
+            target = find_child( info[0] )
+            mo = re.search( r"([a-z]+)-", info[0] )
+            add_vo( mo.group(1), info[1], info[2] )
+            return target
+        target = next( e for e in [
+            find_child( "{}[name='{}']".format( ctype, param ) )
+            for ctype in ["input","select","textarea"]
+        ] if e  )
+        if target.tag_name in ("input","textarea"):
+            new_val = "01/01/2000" if param == "SCENARIO_DATE" else "changed value"
+            target.send_keys( new_val )
+            return target, "", new_val
+        elif target.tag_name == "select":
+            sel = Select( target )
+            prev_val = sel.first_selected_option.get_attribute( "value" )
+            select_droplist_index( sel, 2 )
+            new_val = sel.first_selected_option.get_attribute( "value" )
+            return target, prev_val, new_val
+        assert False
+        return None
+
+    def check_field( param, state ):
+        """Check that a change we made to a field is still there."""
+        if param in SIMPLE_NOTES:
+            assert get_sortable_entry_count( state ) == 1
+        elif param in VEHICLE_ORDNANCE:
+            assert get_sortable_entry_count( state ) == 1
+        elif state[0].tag_name in ("input","textarea"):
+            assert state[0].get_attribute("value") == state[2]
+        elif state[0].tag_name == "select":
+            assert Select(state[0]).first_selected_option.get_attribute("value") == state[2]
+        else:
+            assert False
+
+    def revert_field( param, state ):
+        """Revert a change we made to a field."""
+        if param in SIMPLE_NOTES:
+            drag_sortable_entry_to_trash( state, 0 )
+        elif param in VEHICLE_ORDNANCE:
+            drag_sortable_entry_to_trash( state, 0 )
+        elif state[0].tag_name in ("input","textarea"):
+            state[0].clear()
+        elif state[0].tag_name == "select":
+            select_droplist_val( Select(state[0]), state[1] )
+        else:
+            assert False
+
+    def cancel_confirmation():
+        """Cancel the confirmation dialog."""
+        btns = find_children( ".ui-dialog-buttonset button" )
+        btn = next( b for b in btns if b.text == "Cancel" )
+        btn.click()
+
+    def do_test( tab_id, param ):
+        """Test checking for a dirty scenario."""
+
+        # change the specified field
+        select_tab( tab_id )
+        state = change_field( param )
+
+        # make sure we get asked to confirm a "new scenario" operation
+        select_menu_option( "new_scenario" )
+        wait_for( 2, lambda: find_child("#ask") is not None )
+        elem = find_child( "#ask" )
+        assert "This scenario has been changed" in elem.text
+
+        # cancel the confirmation requiest, make sure the change we made is still there
+        cancel_confirmation()
+        select_tab( tab_id )
+        check_field( param, state )
+
+        # revert the change
+        revert_field( param, state )
+
+        # we should now be able to reset the scenario without a confirmation
+        _ = set_stored_msg_marker( "_last-info_" )
+        select_menu_option( "new_scenario" )
+        wait_for( 2, lambda: get_stored_msg("_last-info_") == "The scenario was reset." )
+
+        # change the field again
+        select_tab( tab_id )
+        state = change_field( param )
+
+        # make sure we get asked to confirm a "load scenario" operation
+        select_menu_option( "load_scenario" )
+        wait_for( 2, lambda: find_child("#ask") is not None )
+        elem = find_child( "#ask" )
+        assert "This scenario has been changed" in elem.text
+
+        # cancel the confirmation request, make sure the change we made is still there
+        cancel_confirmation()
+        select_tab( tab_id )
+        check_field( param, state )
+
+        # revert the change
+        revert_field( param, state )
+
+        # we should be able to load a scenario without a confirmation
+        # NOTE: We don't do this, since it will cause the OPEN FILE dialog to come up :-/
+
+    # change each parameter, then try to reset/load the scenario
+    for tab_id,params in ALL_SCENARIO_PARAMS.items():
+        for param in params:
+            do_test( tab_id, param )
+            if pytest.config.option.short_tests: #pylint: disable=no-member
+                break # nb: it's a bit excessive to check *every* parameter :-/
diff --git a/vasl_templates/webapp/tests/test_scenario_persistence.py b/vasl_templates/webapp/tests/test_scenario_persistence.py
index a86cedf..c7530f3 100644
--- a/vasl_templates/webapp/tests/test_scenario_persistence.py
+++ b/vasl_templates/webapp/tests/test_scenario_persistence.py
@@ -1,6 +1,7 @@
 """ Test loading/saving scenarios. """
 
 import json
+import itertools
 
 from selenium.webdriver.support.ui import Select
 
@@ -8,6 +9,28 @@ from vasl_templates.webapp.tests.utils import \
     init_webapp, get_nationalities, set_template_params, select_tab, select_menu_option, \
     get_sortable_entry_text, get_stored_msg, set_stored_msg, set_stored_msg_marker, find_child, find_children, wait_for
 
+# this table lists all parameters stored in a scenario
+ALL_SCENARIO_PARAMS = {
+    "scenario": [
+        "SCENARIO_NAME", "SCENARIO_LOCATION", "SCENARIO_DATE", "SCENARIO_WIDTH",
+        "PLAYER_1", "PLAYER_1_ELR", "PLAYER_1_SAN",
+        "PLAYER_2", "PLAYER_2_ELR", "PLAYER_2_SAN",
+        "VICTORY_CONDITIONS", "VICTORY_CONDITIONS_WIDTH",
+        "SCENARIO_NOTES",
+        "SSR", "SSR_WIDTH",
+    ],
+    "ob1": [
+        "OB_SETUPS_1", "OB_NOTES_1",
+        "VEHICLES_1", "VEHICLES_WIDTH_1",
+        "ORDNANCE_1", "ORDNANCE_WIDTH_1",
+    ],
+    "ob2": [
+        "OB_SETUPS_2", "OB_NOTES_2",
+        "VEHICLES_2", "VEHICLES_WIDTH_2",
+        "ORDNANCE_2", "ORDNANCE_WIDTH_2",
+    ],
+}
+
 # ---------------------------------------------------------------------
 
 def test_scenario_persistence( webapp, webdriver ): #pylint: disable=too-many-statements,too-many-locals
@@ -25,14 +48,19 @@ def test_scenario_persistence( webapp, webdriver ): #pylint: disable=too-many-st
             assert elem.text.strip() == "{} OB".format( nationalities[nat]["display_name"] )
 
     # load the scenario fields
-    scenario_params = {
+    SCENARIO_PARAMS = {
         "scenario": {
-            "SCENARIO_NAME": "my test scenario", "SCENARIO_LOCATION": "right here", "SCENARIO_DATE": "12/31/1945",
+            "SCENARIO_NAME": "my test scenario",
+            "SCENARIO_LOCATION": "right here",
+            "SCENARIO_DATE": "12/31/1945",
             "SCENARIO_WIDTH": "101",
             "PLAYER_1": "russian", "PLAYER_1_ELR": "1", "PLAYER_1_SAN": "2",
             "PLAYER_2": "german", "PLAYER_2_ELR": "3", "PLAYER_2_SAN": "4",
             "VICTORY_CONDITIONS": "just do it!", "VICTORY_CONDITIONS_WIDTH": "102",
-            "SCENARIO_NOTES": [ { "caption": "note #1", "width": "" }, { "caption": "note #2", "width": "100px" } ],
+            "SCENARIO_NOTES": [
+                { "caption": "note #1", "width": "" },
+                { "caption": "note #2", "width": "100px" }
+            ],
             "SSR": [ "This is an SSR.", "This is another SSR." ],
             "SSR_WIDTH": "103",
         },
@@ -59,19 +87,30 @@ def test_scenario_persistence( webapp, webdriver ): #pylint: disable=too-many-st
             "ORDNANCE_WIDTH_2": "303",
         },
     }
-    for tab_id,fields in scenario_params.items():
+    for tab_id,fields in SCENARIO_PARAMS.items():
         select_tab( tab_id )
         set_template_params( fields )
     check_ob_tabs( "russian", "german" )
 
+    # make sure that our test scenario includes everything
+    lhs = { k: set(v) for k,v in SCENARIO_PARAMS.items() }
+    rhs = { k: set(v) for k,v in ALL_SCENARIO_PARAMS.items() }
+    assert lhs == rhs
+
     # save the scenario and check the results
     saved_scenario = _save_scenario()
     expected = {
-        k.upper(): v for tab in scenario_params.values() for k,v in tab.items()
+        k.upper(): v for tab in SCENARIO_PARAMS.values() for k,v in tab.items()
     }
     assert saved_scenario == expected
 
+    # make sure that our list of scenario parameters is correct
+    lhs = set( saved_scenario.keys() )
+    rhs = set( itertools.chain( *ALL_SCENARIO_PARAMS.values() ) )
+    assert lhs == rhs
+
     # reset the scenario and check the save results
+    # nb: we just saved the scenario, so we shouldn't get asked to confirm the "new scenario" operation
     _ = set_stored_msg_marker( "_last-info_" )
     select_menu_option( "new_scenario" )
     wait_for( 2, lambda: get_stored_msg("_last-info_") == "The scenario was reset." )
@@ -95,11 +134,12 @@ def test_scenario_persistence( webapp, webdriver ): #pylint: disable=too-many-st
     }
 
     # load a scenario and make sure it was loaded into the UI correctly
+    # nb: we just reset the scenario, so we shouldn't get asked to confirm the "load scenario" operation
     _load_scenario( saved_scenario )
     check_ob_tabs( "russian", "german" )
-    for tab_id in scenario_params:
+    for tab_id in SCENARIO_PARAMS:
         select_tab( tab_id )
-        for field,val in scenario_params[tab_id].items():
+        for field,val in SCENARIO_PARAMS[tab_id].items():
             if field in ("SCENARIO_NOTES","SSR"):
                 continue # nb: these require special handling, we do it below
             if field in ("OB_SETUPS_1","OB_SETUPS_2","OB_NOTES_1","OB_NOTES_2"):
@@ -113,18 +153,18 @@ def test_scenario_persistence( webapp, webdriver ): #pylint: disable=too-many-st
                 assert elem.get_attribute("value") == val
     select_tab( "scenario" )
     scenario_notes = [ c.text for c in find_children("#scenario_notes-sortable li") ]
-    assert scenario_notes == [ sn["caption"] for sn in scenario_params["scenario"]["SCENARIO_NOTES"] ]
-    assert get_sortable_entry_text(ssrs) == scenario_params["scenario"]["SSR"]
+    assert scenario_notes == [ sn["caption"] for sn in SCENARIO_PARAMS["scenario"]["SCENARIO_NOTES"] ]
+    assert get_sortable_entry_text(ssrs) == SCENARIO_PARAMS["scenario"]["SSR"]
     select_tab( "ob1" )
-    assert get_sortable_entry_text(ob_setups1) == [ obs["caption"] for obs in scenario_params["ob1"]["OB_SETUPS_1"] ]
-    assert get_sortable_entry_text(ob_notes1) == [ obs["caption"] for obs in scenario_params["ob1"]["OB_NOTES_1"] ]
-    assert get_sortable_entry_text(vehicles1) == scenario_params["ob1"]["VEHICLES_1"]
-    assert get_sortable_entry_text(ordnance1) == scenario_params["ob1"]["ORDNANCE_1"]
+    assert get_sortable_entry_text(ob_setups1) == [ obs["caption"] for obs in SCENARIO_PARAMS["ob1"]["OB_SETUPS_1"] ]
+    assert get_sortable_entry_text(ob_notes1) == [ obs["caption"] for obs in SCENARIO_PARAMS["ob1"]["OB_NOTES_1"] ]
+    assert get_sortable_entry_text(vehicles1) == SCENARIO_PARAMS["ob1"]["VEHICLES_1"]
+    assert get_sortable_entry_text(ordnance1) == SCENARIO_PARAMS["ob1"]["ORDNANCE_1"]
     select_tab( "ob2" )
-    assert get_sortable_entry_text(ob_setups2) == [ obs["caption"] for obs in scenario_params["ob2"]["OB_SETUPS_2"] ]
-    assert get_sortable_entry_text(ob_notes2) == [ obs["caption"] for obs in scenario_params["ob2"]["OB_NOTES_2"] ]
-    assert get_sortable_entry_text(vehicles2) == scenario_params["ob2"]["VEHICLES_2"]
-    assert get_sortable_entry_text(ordnance2) == scenario_params["ob2"]["ORDNANCE_2"]
+    assert get_sortable_entry_text(ob_setups2) == [ obs["caption"] for obs in SCENARIO_PARAMS["ob2"]["OB_SETUPS_2"] ]
+    assert get_sortable_entry_text(ob_notes2) == [ obs["caption"] for obs in SCENARIO_PARAMS["ob2"]["OB_NOTES_2"] ]
+    assert get_sortable_entry_text(vehicles2) == SCENARIO_PARAMS["ob2"]["VEHICLES_2"]
+    assert get_sortable_entry_text(ordnance2) == SCENARIO_PARAMS["ob2"]["ORDNANCE_2"]
 
 # ---------------------------------------------------------------------
 
@@ -135,6 +175,7 @@ def test_loading_ssrs( webapp, webdriver ):
     init_webapp( webapp, webdriver, scenario_persistence=1 )
 
     # initialize
+    # nb: we only load scenarios in this test, so we should never get asked to confirm the "load scenario" operation
     select_tab( "scenario" )
     sortable = find_child( "#ssr-sortable" )
     def do_test( ssrs ): # pylint: disable=missing-docstring
@@ -162,7 +203,7 @@ def test_unknown_vo( webapp, webdriver ):
     init_webapp( webapp, webdriver, scenario_persistence=1 )
 
     # load a scenario that has unknown vehicles/ordnance
-    scenario_params = {
+    SCENARIO_PARAMS = {
         "PLAYER_1": "german",
         "VEHICLES_1": [ "unknown vehicle 1a", "unknown vehicle 1b" ],
         "ORDNANCE_1":  [ "unknown ordnance 1a", "unknown ordnance 1b" ],
@@ -171,10 +212,11 @@ def test_unknown_vo( webapp, webdriver ):
         "ORDNANCE_2":  [ "unknown ordnance 2" ],
     }
     _ = set_stored_msg_marker( "_last-warning_" )
-    _load_scenario( scenario_params )
+    # nb: we haven't made any changes, so we shouldn't get asked to confirm the "load scenario" operation
+    _load_scenario( SCENARIO_PARAMS )
     last_warning = get_stored_msg( "_last-warning_" )
     assert last_warning.startswith( "Unknown vehicles/ordnance:" )
-    for key,vals in scenario_params.items():
+    for key,vals in SCENARIO_PARAMS.items():
         if not key.startswith( ("VEHICLES_","ORDNANCE_") ):
             continue
         assert all( v in last_warning for v in vals )
diff --git a/vasl_templates/webapp/tests/test_template_packs.py b/vasl_templates/webapp/tests/test_template_packs.py
index a39d569..9d1a402 100644
--- a/vasl_templates/webapp/tests/test_template_packs.py
+++ b/vasl_templates/webapp/tests/test_template_packs.py
@@ -11,7 +11,7 @@ from vasl_templates.webapp import snippets
 from vasl_templates.webapp.tests.utils import \
     select_tab, select_menu_option, get_clipboard, get_stored_msg, set_stored_msg, set_stored_msg_marker,\
     add_simple_note, for_each_template, find_child, find_children, wait_for, \
-    select_droplist_val, get_droplist_vals, init_webapp
+    select_droplist_val, get_droplist_vals_index, init_webapp
 
 # ---------------------------------------------------------------------
 
@@ -106,7 +106,7 @@ def test_nationality_data( webapp, webdriver ):
     # FUDGE!  player1_sel.first_selected_option.text doesn't contain the right value
     # if we're using jQuery selectmenu's :-/
     assert player1_sel.first_selected_option.get_attribute( "value" ) == "british"
-    droplist_vals = get_droplist_vals( player1_sel )
+    droplist_vals = get_droplist_vals_index( player1_sel )
     assert droplist_vals["british"] == "British"
 
     # upload a template pack that contains nationality data
@@ -117,7 +117,7 @@ def test_nationality_data( webapp, webdriver ):
     # check that the UI was updated correctly
     assert tab_ob1.text.strip() == "Poms! OB"
     assert player1_sel.first_selected_option.get_attribute( "value" ) == "british"
-    droplist_vals2 = get_droplist_vals( player1_sel )
+    droplist_vals2 = get_droplist_vals_index( player1_sel )
     assert droplist_vals2["british"] == "Poms!"
 
     # check that there is a new Korean player
diff --git a/vasl_templates/webapp/tests/test_vo_reports.py b/vasl_templates/webapp/tests/test_vo_reports.py
index 86b9a1f..b383a7a 100644
--- a/vasl_templates/webapp/tests/test_vo_reports.py
+++ b/vasl_templates/webapp/tests/test_vo_reports.py
@@ -15,8 +15,8 @@ from vasl_templates.webapp.tests.utils import find_child, find_children, wait_fo
 # NOTE: Running these checks is fairly slow, and once done, don't provide a great deal of value
 # in the day-to-day development process, so we make them optional.
 @pytest.mark.skipif(
-    not pytest.config.option.check_vo_reports, #pylint: disable=no-member
-    reason = "--no-reports not specified"
+    pytest.config.option.short_tests, #pylint: disable=no-member
+    reason = "--short-tests specified"
 )
 def test_vo_reports( webapp, webdriver ):
     """Check the vehicle/ordnance reports."""
diff --git a/vasl_templates/webapp/tests/utils.py b/vasl_templates/webapp/tests/utils.py
index 2523f85..ae7d428 100644
--- a/vasl_templates/webapp/tests/utils.py
+++ b/vasl_templates/webapp/tests/utils.py
@@ -274,26 +274,37 @@ def find_children( sel, parent=None ):
 
 def select_droplist_val( sel, val ):
     """Select a droplist option by value."""
+    options = get_droplist_vals_index( sel )
+    _do_select_droplist( sel, options[val] )
 
-    # get the options from the original <select>
-    sel_id = sel._el.get_attribute( "id" ) #pylint: disable=protected-access
+def select_droplist_index( sel, index ):
+    """Select a droplist option by index."""
     options = get_droplist_vals( sel )
+    _do_select_droplist( sel, options[index][1] )
+
+def _do_select_droplist( sel, val ):
+    """Select a droplist option."""
 
     # open the jQuery droplist
+    sel_id = sel._el.get_attribute( "id" ) #pylint: disable=protected-access
     elem = find_child( "#{}-button .ui-selectmenu-icon".format( sel_id ) )
     elem.click()
 
     # select the requested option (nb: clicking on the child option doesn't work :shrug:)
     elem = find_child( "#{}-button".format( sel_id ) )
-    elem.send_keys( options[val] )
+    elem.send_keys( val )
     elem.send_keys( Keys.RETURN )
 
+def get_droplist_vals_index( sel ):
+    """Get the value/text for each option in a droplist."""
+    return { k: v for k,v in get_droplist_vals(sel) }
+
 def get_droplist_vals( sel ):
     """Get the value/text for each option in a droplist."""
-    return {
-        opt.get_attribute("value"): opt.get_attribute("text")
+    return [
+        ( opt.get_attribute("value"), opt.get_attribute("text") )
         for opt in sel.options
-    }
+    ]
 
 # - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -