From bcca65243fdfae911e533348d1495c189dc1aac4 Mon Sep 17 00:00:00 2001 From: Jeff Ohrstrom Date: Mon, 20 Dec 2021 16:35:05 -0500 Subject: [PATCH] Backport js fixes (#1674) * backport many javascript bug fixes * update changelog --- CHANGELOG.md | 12 +- .../app/javascript/packs/batchConnect.js | 81 +++++++------ .../sys_with_gateway_apps/bc_jupyter/form.yml | 24 ++++ .../bc_jupyter/template/.keep | 0 .../test/system/batch_connect_test.rb | 111 +++++++++++++++++- 5 files changed, 189 insertions(+), 39 deletions(-) create mode 100644 apps/dashboard/test/fixtures/sys_with_gateway_apps/bc_jupyter/template/.keep diff --git a/CHANGELOG.md b/CHANGELOG.md index d792ee5c90..523e17a0b2 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/apps/dashboard/app/javascript/packs/batchConnect.js b/apps/dashboard/app/javascript/packs/batchConnect.js index d731a60700..aaafb00787 100644 --- a/apps/dashboard/app/javascript/packs/batchConnect.js +++ b/apps/dashboard/app/javascript/packs/batchConnect.js @@ -16,7 +16,8 @@ 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. @@ -24,6 +25,9 @@ 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; } diff --git a/apps/dashboard/test/fixtures/sys_with_gateway_apps/bc_jupyter/form.yml b/apps/dashboard/test/fixtures/sys_with_gateway_apps/bc_jupyter/form.yml index 11fd632ac5..ccc68b1063 100644 --- a/apps/dashboard/test/fixtures/sys_with_gateway_apps/bc_jupyter/form.yml +++ b/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 diff --git a/apps/dashboard/test/fixtures/sys_with_gateway_apps/bc_jupyter/template/.keep b/apps/dashboard/test/fixtures/sys_with_gateway_apps/bc_jupyter/template/.keep new file mode 100644 index 0000000000..e69de29bb2 diff --git a/apps/dashboard/test/system/batch_connect_test.rb b/apps/dashboard/test/system/batch_connect_test.rb index a603a250c5..2568fdd86e 100644 --- a/apps/dashboard/test/system/batch_connect_test.rb +++ b/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