Backport js fixes (#1674)

* backport many javascript bug fixes

* update changelog

CHANGELOG.md

@@ -7,6 +7,15 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 
 ## [Unreleased]
 
+## [2.0.21] - 2021-12-20
+
+### Fixed
+
+- Dynamic javascript now correctly clamps values correcting [1649](https://github.com/OSC/ondemand/issues/1649).
+- Dynamic javascript can hide multiple elements correcting [1666](https://github.com/OSC/ondemand/issues/1666).
+- Dynamic javascript now correctly handles options with numbers, hyphens and underscores
+  back-porting [1656](https://github.com/OSC/ondemand/pull/1656).
+
 ## [2.0.20] - 2021-12-01
 
 ### Security
@@ -935,7 +944,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - From 1.3.7 - 1.4.2 updated app versions
 
 
-[Unreleased]: https://github.com/OSC/ondemand/compare/v2.0.20...release_2.0
+[Unreleased]: https://github.com/OSC/ondemand/compare/v2.0.21...release_2.0
+[2.0.21]: https://github.com/OSC/ondemand/compare/v2.0.20...v2.0.21
 [2.0.20]: https://github.com/OSC/ondemand/compare/v2.0.19...v2.0.20
 [2.0.19]: https://github.com/OSC/ondemand/compare/v2.0.18...v2.0.19
 [2.0.18]: https://github.com/OSC/ondemand/compare/v2.0.17...v2.0.18

apps/dashboard/app/javascript/packs/batchConnect.js

@@ -16,14 +16,18 @@ const optionForHandlerCache = {};
 // simples array of string ids for elements that have a handler
 const minMaxHandlerCache = [];
 const setHandlerCache = [];
-const hideHandlerCache = [];
+// hide handler cache is a map in the form '{ from: [hideThing1, hideThing2] }'
+const hideHandlerCache = {};
 
 // Lookup tables for setting min & max values
 // for different directives.
 const minMaxLookup = {};
 const setValueLookup = {};
 const hideLookup = {};
 
+// the regular expression for mountain casing
+const mcRex = /[[-_]([a-z])|([_-][0-9])/g;
+
 function bcElement(name) {
   return `${bcPrefix}_${name.toLowerCase()}`;
 };
@@ -42,29 +46,26 @@ function shortId(elementId) {
 
 /**
  * Mountain case the words from a string, by tokenizing on [-_].  In the
- * simplest case it simple capitalizes.
+ * simplest case it just capitalizes.
+ *
+ * There is a special case where seperators are followed numbers. In this case
+ * The seperator is kept as a hyphen because that's how jQuery expects it.
  *
- * @param      {string}  str     The word string to capitalize
+ * @param      {string}  str     The word string to mountain case
  *
  * @example  given 'foo' this returns 'Foo'
  * @example  given 'foo-bar' this returns 'FooBar'
+ * @example  given 'physics_1234' this returns 'Physics-1234'
  */
- function mountainCaseWords(str) {
-  let camelCase = "";
-  let capitalize = true;
-
-  str.split('').forEach((c) => {
-    if (capitalize) {
-      camelCase += c.toUpperCase();
-      capitalize = false;
-    } else if(c == '-' || c == '_') {
-      capitalize = true;
-    } else {
-      camelCase += c;
-    }
+// Convert dashed to camelCase
+function mountainCaseWords(str) {
+  const lower = str.toLowerCase();
+  const first = lower.charAt(0).toUpperCase();
+  const rest = lower.slice(1).replace(mcRex, function(_all, letter, prefixedNumber) {
+    return letter ? letter.toUpperCase() : prefixedNumber.replace('_','-');
   });
 
-  return camelCase;
+  return  `${first}${rest}`;
 }
 
 function snakeCaseWords(str) {
@@ -136,18 +137,20 @@ function makeChangeHandlers(){
 function addHideHandler(optionId, option, key,  configValue) {
   const changeId = idFromToken(key.replace(/^hide/,''));
 
-  if(hideLookup[optionId] === undefined) hideLookup[optionId] = new Table(changeId, undefined);
+  if(hideLookup[optionId] === undefined) hideLookup[optionId] = new Table(changeId, 'option_value');
   const table = hideLookup[optionId];
-  table.put(option, undefined, configValue);
+  table.put(changeId, option, configValue);
 
-  if(!hideHandlerCache.includes(optionId)) {
+  if(hideHandlerCache[optionId] === undefined) hideHandlerCache[optionId] = [];
+
+  if(!hideHandlerCache[optionId].includes(changeId)) {
     const changeElement = $(`#${optionId}`);
 
     changeElement.on('change', (event) => {
       updateVisibility(event, changeId);
     });
 
-    hideHandlerCache.push(optionId);
+    hideHandlerCache[optionId].push(changeId);
   }
 
   updateVisibility({ target: document.querySelector(`#${optionId}`) }, changeId);
@@ -348,7 +351,7 @@ function updateVisibility(event, changeId) {
   if (changeElement.size() <= 0) return;
 
   // safe to access directly?
-  const hide = hideLookup[id].get(val, undefined);
+  const hide = hideLookup[id].get(changeId, val);
   if(hide === undefined) {
     changeElement.show();
   }else if(hide === true) {
@@ -374,30 +377,36 @@ function toggleMinMax(event, changeId, otherId) {
   const prev = {
     min: changeElement.attr('min'),
     max: changeElement.attr('max'),
-    val: changeElement.val()
   };
 
   [ 'max', 'min' ].forEach((dim) => {
     if(mm && mm[dim] !== undefined) {
       changeElement.attr(dim, mm[dim]);
-
-      let val = clamp(prev['val'], prev[dim], mm[dim], dim)
-      if (val !== undefined) {
-        changeElement.attr('value', val);
-        changeElement.val(val);
-      }
     }
   });
+
+  const val = clamp(changeElement.val(), prev, mm)
+  if (val !== undefined) {
+    changeElement.attr('value', val);
+    changeElement.val(val);
+  }
 }
 
-function clamp(previous, previousBoundary, currentBoundary, comparator){
-  const pb = parseInt(previousBoundary) || undefined;
-  const p  = parseInt(previous) || undefined;
+function clamp(currentValue, previous, next) {
+  if(next === undefined){
+    return undefined;
 
-  if(comparator == 'min' && p <= pb) {
-    return currentBoundary;
-  } else if(comparator == 'max' && p >= pb) {
-    return currentBoundary;
+  // you've set the boundary, so when you go to the next value - keep it at the next's boundary
+  } else if(previous && previous['max'] && currentValue == previous['max']) {
+    return next['max'];
+  } else if(previous && previous['min'] && currentValue == previous['min']) {
+    return next['min'];
+
+  // otherwise you could be up or down shifting to fit within the next's boundaries
+  } else if(next['max'] && currentValue >= next['max']) {
+    return next['max'];
+  } else if(next['min'] && currentValue <= next['min']) {
+    return next['min'];
   } else {
     return undefined;
   }

apps/dashboard/test/fixtures/sys_with_gateway_apps/bc_jupyter/form.yml

@@ -26,6 +26,7 @@ attributes:
       - [
           "any",
           data-hide-cuda-version: true,
+          data-hide-advanced-options: true,
           data-minnn-bc-not-found-for-cluster-mistype: 30,
           data-max-bc-not-found-for-cluster-mistype: 30,
           data-maximum-bc-not-found-for-cluster-mistype: 30
@@ -44,6 +45,7 @@ attributes:
           "hugemem",
           data-option-for-cluster-oakley: false,
           data-hide-cuda-version: true,
+          data-hide-advanced-options: true,
           data-max-bc-num-slots-for-cluster-owens: 42,
           data-min-bc-num-slots-for-cluster-owens: 42,
         ]
@@ -58,11 +60,13 @@ attributes:
       - [
           "same",
           data-hide-cuda-version: true,
+          data-hide-advanced-options: true,
           data-min-bc-num-slots: 100,
           data-max-bc-num-slots: 200
         ]
       - [
           "other-40ish-option",
+          data-hide-advanced-options: true,
           data-max-bc-num-slots-for-cluster-owens: 40,
           data-max-bc-num-slots-for-cluster-oakley: 48,
         ]
@@ -103,6 +107,23 @@ attributes:
       - "4"
       - "7"
       - "38"
+  classroom:
+    widget: select
+    options:
+      - [ 'Physics 1234', 'physics_1234' ]
+      - [ 'Astronomy 5678', 'astronomy_5678' ]
+  classroom_size:
+    widget: select
+    options:
+      - [ 'small', 'small' ]
+      - [
+          'medium', 'medium',
+          data-option-for-classroom-astronomy-5678: false,
+        ]
+      - [
+          'large', 'large',
+          data-option-for-classroom-astronomy-5678: false,
+        ]
 form:
   - bc_num_hours
   - bc_num_slots
@@ -113,3 +134,6 @@ form:
   - python_version
   - cuda_version
   - hidden_change_thing
+  - classroom
+  - classroom_size
+  - advanced_options

apps/dashboard/test/system/batch_connect_test.rb

@@ -4,8 +4,7 @@
 
 class BatchConnectTest < ApplicationSystemTestCase
   def setup
-    OodAppkit.stubs(:clusters).returns(OodCore::Clusters.load_file('test/fixtures/config/clusters.d'))
-    SysRouter.stubs(:base_path).returns(Rails.root.join('test/fixtures/sys_with_gateway_apps'))
+    stub_sys_apps
     Configuration.stubs(:bc_dynamic_js?).returns(true)
   end
 
@@ -61,6 +60,74 @@ def setup
     assert_equal '42', find_value('bc_num_slots')
   end
 
+  test 'clamping works on maxes' do
+    visit new_batch_connect_session_context_url('sys/bc_jupyter')
+
+    # defaults
+    assert_equal 20, find_max('bc_num_slots')
+    assert_equal 1, find_min('bc_num_slots')
+    assert_equal 'any', find_value('node_type')
+
+    # put the max for 'any'
+    fill_in bc_ele_id('bc_num_slots'), with: 20
+
+    # now toggle to gpu. Max is 28 and the value is 28
+    select('gpu', from: bc_ele_id('node_type'))
+    assert_equal 28, find_max('bc_num_slots')
+    assert_equal '28', find_value('bc_num_slots')
+  end
+
+  test 'clamping works on mins' do
+    visit new_batch_connect_session_context_url('sys/bc_jupyter')
+
+    # defaults
+    assert_equal 20, find_max('bc_num_slots')
+    assert_equal 1, find_min('bc_num_slots')
+    assert_equal 'any', find_value('node_type')
+
+    # put the max for 'any'
+    fill_in bc_ele_id('bc_num_slots'), with: 1
+
+    # now toggle to gpu. min is 2 and the value is 2
+    select('gpu', from: bc_ele_id('node_type'))
+    assert_equal 2, find_min('bc_num_slots')
+    assert_equal '2', find_value('bc_num_slots')
+  end
+
+  test 'clamping shifts left' do
+    visit new_batch_connect_session_context_url('sys/bc_jupyter')
+
+    # setup to start with same
+    select('same', from: bc_ele_id('node_type'))
+    assert_equal 200, find_max('bc_num_slots')
+    assert_equal 100, find_min('bc_num_slots')
+
+    # less than current max, but greater than the next choices'
+    fill_in bc_ele_id('bc_num_slots'), with: 150
+
+    # toggle back to 'gpu' and it should clamp to 150 down to 28
+    select('gpu', from: bc_ele_id('node_type'))
+    assert_equal 28, find_max('bc_num_slots')
+    assert_equal '28', find_value('bc_num_slots')
+  end
+
+  test 'clamping shifts right' do
+    visit new_batch_connect_session_context_url('sys/bc_jupyter')
+
+    # start with defaults
+    assert_equal 1, find_min('bc_num_slots')
+    assert_equal 20, find_max('bc_num_slots')
+    assert_equal 'any', find_value('node_type')
+
+    # not the max, but less than the next choices'
+    fill_in bc_ele_id('bc_num_slots'), with: 18
+
+    # toggle back to 'oakley' and it should clamp 18 up to 100 (same's minimum)
+    select('same', from: bc_ele_id('node_type'))
+    assert_equal 100, find_min('bc_num_slots')
+    assert_equal '100', find_value('bc_num_slots')
+  end
+
   test 'changing the cluster changes max' do
     # max starts out at 20
     visit new_batch_connect_session_context_url('sys/bc_jupyter')
@@ -257,6 +324,31 @@ def setup
     assert !find("##{bc_ele_id('cuda_version')}", visible: false).visible?
   end
 
+  # similar to the use case above, but for https://github.com/OSC/ondemand/issues/1666
+  test 'hiding a second option' do
+    visit new_batch_connect_session_context_url('sys/bc_jupyter')
+
+    # default is any, so we can't see cuda_version or advanced_options
+    assert_equal 'any', find_value('node_type')
+    assert !find("##{bc_ele_id('cuda_version')}", visible: false).visible?
+    assert !find("##{bc_ele_id('advanced_options')}", visible: false).visible?
+
+    # gpu shows both cuda_version and advanced_options
+    select('gpu', from: bc_ele_id('node_type'))
+    assert find("##{bc_ele_id('cuda_version')}").visible?
+    assert find("##{bc_ele_id('advanced_options')}").visible?
+
+    # toggle back to 'same' and both are gone
+    select('same', from: bc_ele_id('node_type'))
+    assert !find("##{bc_ele_id('cuda_version')}", visible: false).visible?
+    assert !find("##{bc_ele_id('advanced_options')}", visible: false).visible?
+
+    # now select advanced and cuda is hidden, but advanced is shown
+    select('advanced', from: bc_ele_id('node_type'))
+    assert !find("##{bc_ele_id('cuda_version')}", visible: false).visible?
+    assert find("##{bc_ele_id('advanced_options')}").visible?
+  end
+
   test 'options with hyphens' do
     visit new_batch_connect_session_context_url('sys/bc_jupyter')
 
@@ -272,4 +364,19 @@ def setup
     select('oakley', from: bc_ele_id('cluster'))
     assert_equal 48, find_max('bc_num_slots')
   end
+
+  test 'options with numbers' do
+    visit new_batch_connect_session_context_url('sys/bc_jupyter')
+
+    # defaults
+    assert_equal 'physics_1234', find_value('classroom')
+    assert_equal 'small', find_value('classroom_size')
+    assert_equal '', find_option_style('classroom_size', 'medium')
+    assert_equal '', find_option_style('classroom_size', 'large')
+
+    # now change the classroom and see the other sizes disappear
+    select('Astronomy 5678', from: bc_ele_id('classroom'))
+    assert_equal 'display: none;', find_option_style('classroom_size', 'medium')
+    assert_equal 'display: none;', find_option_style('classroom_size', 'large')
+  end
 end