DATAUP-639 dynamic dropdown fixes #2911

briehl · 2022-04-27T18:33:21Z

Description of PR purpose/changes

This makes progress toward fixing some issues with the dynamic dropdown. It specifically addresses how values get displayed on load or on update.

There are a few issues here.
First, when the dropdown loads, if there's an initialValue given, it creates and selects an initial option. The problem is, there's no display info. For the taxonomy id case, it gets an id value, say, 12345. To fill out the display template, it needs a scientific_name and ncbi_taxon_id. Without those, the field would render in as "NCBI Tax ID undefined: undefined".

Second, when doing a search that reveals the same value, the actual answer would never get updated - a search that came back with tax id 12345 would always display that "undefined" stuff.

The main problem is that Select2 is a big jerk and doesn't like to change its Option data. There've been Select2 tickets existing since like 2014. (i.e.: select2/select2#2830) The only solutions are semi-horrible hacks, so that's what happen here. Fun!

The core fixes introduced here are:

The dynamic dropdown now emits a newDisplayValue along with newValue on the changed bus message. Not consumed anywhere yet.
The dropdown consumes a initialDisplayValue in its config, expected to be an object with the keys from the description_template config expected.
If an initialDisplayValue is not included, it defaults to the actual value. Not any worse than showing undefineds?

Jira Ticket / Issue

Related Jira ticket: https://kbase-jira.atlassian.net/browse/DATAUP-639

Added the Jira Ticket to the title of the PR (e.g. DATAUP-69 Adds a PR template)

Testing Instructions

Details for how to test the PR:

Tests pass locally and in GitHub Actions
Changes available by spinning up a local narrative and navigating to X to see Y

Dev Checklist:

My code follows the guidelines at https://sites.google.com/lbl.gov/trussresources/home?authuser=0
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
(JavaScript) I have run Prettier and ESLint on changed code manually or with a git precommit hook
Any dependent changes have been merged and published in downstream modules

Updating Version and Release Notes (if applicable)

[n/a] Version has been bumped for each release
Release notes have been updated for each release (and during the merge of feature branches)

briehl · 2022-04-27T18:35:03Z

src/biokbase/narrative/tests/test_jobmanager.py

@@ -543,7 +543,7 @@ def test_get_all_job_states__ignore_refresh_flag(self):
            states,
        )

-    ## get_job_states_by_cell_id
+    # get_job_states_by_cell_id


This made my flake8 freak out. Not sure how that happened.

briehl · 2022-04-27T18:36:43Z

kbase-extension/static/kbase/js/widgets/appWidgets2/input/dynamicDropdownInput.js

                value: undefined,
-            },
-            userId = runtime.userId();
+                descriptionFields: new Set(),


This will hold the fields used in the description template. So if the template is:

NCBI Taxon ID: {{ncbi_taxon_id}}: {{scientific_name}}

it will have the values ncbi_taxon_id and scientific_name.

briehl · 2022-04-27T18:37:49Z

kbase-extension/static/kbase/js/widgets/appWidgets2/input/dynamicDropdownInput.js

-                const newOption = new Option(value, value, true, true);
-                $(control).append(newOption).trigger('change');
+            if (model.value) {
+                const selectorValue = value.replace(/"/g, '\\"').replace(/'/g, "\\'");


This is from an issue with the FTP staging version of the dropdown - if there are quotes in the value (say, a file name with quotes in it), it goes all kinds of wonky. Need to be escaped.

briehl · 2022-04-27T18:39:22Z

kbase-extension/static/kbase/js/widgets/appWidgets2/input/dynamicDropdownInput.js

+                    const dataAdapter = $(control).data('select2').dataAdapter;
+                    const newOption = dataAdapter.option(
+                        Object.assign({ id: model.value, value: model.value }, displayValue)
+                    );
+                    dataAdapter.addOptions(newOption);
+                    $(control).val(value).trigger('change');


From some deep diving in Select2 code and docs, this creates a new option with all of the data that the Select2 option would otherwise see from a successful search, and lets Select2 do the usual rendering.

briehl · 2022-04-27T18:39:51Z

kbase-extension/static/kbase/js/widgets/appWidgets2/input/dynamicDropdownInput.js

+         * @param {string} searchTerm
+         * @returns Array
+         */
+        async function fetchFtpStagingData(searchTerm) {


Popped this out of the fetchData function, effectively unchanged otherwise.

briehl · 2022-04-27T18:40:36Z

kbase-extension/static/kbase/js/widgets/appWidgets2/input/dynamicDropdownInput.js

@@ -272,7 +300,7 @@ define([
                        // could check here that each item is a map? YAGNI
                        obj = flattenObject(obj);
                        if (!('id' in obj)) {
-                            obj.id = _index; // what the fuck
+                            obj.id = _index;


Every object passed to Select2 needs a unique id field.

if only the original author of that line had read the docs!

briehl · 2022-04-27T18:41:59Z

kbase-extension/static/kbase/js/widgets/appWidgets2/input/dynamicDropdownInput.js

+         * @returns {JQueryElement} a jQuery-wrapped div element
+         */
+        function formatFtpStagingDisplay(fileData) {
+            return $(


Took this out of the formatObjectDisplay function.
Really, the StagingService dropdown could be its own input, or a subclass of this one if we were doing object-oriented things properly.

briehl · 2022-04-27T18:43:48Z

kbase-extension/static/kbase/js/widgets/appWidgets2/input/dynamicDropdownInput.js

+                else {
+                    formattedString = retObj.text || retObj.id;
+                }
+                // and squash the result in a jQuery object with a div.


Select2 wants either a string (which will be put in as a textContent field to avoid XSS things) or a jquery element. We do our own escaping with util/string, so there should be less danger.

lgtm-com · 2022-04-27T18:43:49Z

This pull request introduces 2 alerts when merging 61e252f into 3cb1d2a - view on LGTM.com

new alerts:

2 for Incomplete string escaping or encoding

briehl · 2022-04-27T18:45:25Z

kbase-extension/static/kbase/js/widgets/appWidgets2/input/dynamicDropdownInput.js

+                        delay: 250,
+                        processResults: (data) => {
+                            // update the currently selected one if applicable
+                            const currentData = dropdown.select2('data')[0];


This fixes the issue where display data doesn't get updated. So if you start with undefined stuff in the currently selected option, then do a search that brings in a matching value, this will update that option with data from the service.

briehl · 2022-04-27T18:46:26Z

kbase-extension/static/kbase/js/widgets/appWidgets2/input/dynamicDropdownInput.js

                    .on('change', () => {
-                        doChange();
+                        const data = dropdown.select2('data');
+                        return doChange(data ? data[0] : {});


Send the data object along when doing a change, so we save the display value as well.

briehl · 2022-04-27T18:47:28Z

kbase-extension/static/kbase/js/widgets/appWidgets2/input/dynamicDropdownInput.js

+                const control = ui.getElement('input-container.input');
+                if (control) {
+                    $(control).select2('destroy').html('');
+                }


Select2 puts all kinds of things in all kinds of places in the DOM. We need to specifically call destroy to make sure it all goes away.

codecov · 2022-04-27T18:48:00Z

Codecov Report

Merging #2911 (63e0348) into develop (8599a3f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #2911   +/-   ##
========================================
  Coverage    73.20%   73.20%           
========================================
  Files           36       36           
  Lines         3896     3896           
========================================
  Hits          2852     2852           
  Misses        1044     1044

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dfe7696...63e0348. Read the comment docs.

ialarmedalien

LGTM

lgtm-com · 2022-04-27T22:25:14Z

This pull request introduces 2 alerts when merging a666982 into dd9200d - view on LGTM.com

new alerts:

2 for Incomplete string escaping or encoding

lgtm-com · 2022-04-28T17:14:10Z

This pull request introduces 2 alerts when merging 63e0348 into dfe7696 - view on LGTM.com

new alerts:

2 for Incomplete string escaping or encoding

sonarcloud · 2022-04-28T17:21:13Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
2 Security Hotspots
1 Code Smell

No Coverage information
0.0% Duplication

briehl added 5 commits April 27, 2022 10:37

more work on dynamic dropdown, bug fixing and tests

0e4596b

more testing

db732be

fdescribe -> describe

98531bd

Merge branch 'develop' into DATAUP-639-dyndrop-2

8dd323d

remove orig files

61e252f

briehl commented Apr 27, 2022

View reviewed changes

ialarmedalien approved these changes Apr 27, 2022

View reviewed changes

Merge branch 'develop' into DATAUP-639-dyndrop-2

a666982

Merge branch 'develop' into DATAUP-639-dyndrop-2

63e0348

ialarmedalien merged commit 5ced514 into develop Apr 29, 2022

ialarmedalien deleted the DATAUP-639-dyndrop-2 branch April 29, 2022 14:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DATAUP-639 dynamic dropdown fixes #2911

DATAUP-639 dynamic dropdown fixes #2911

briehl commented Apr 27, 2022

briehl Apr 27, 2022

briehl Apr 27, 2022

briehl Apr 27, 2022

briehl Apr 27, 2022

briehl Apr 27, 2022 •

edited

briehl Apr 27, 2022

ialarmedalien Apr 27, 2022

briehl Apr 27, 2022

briehl Apr 27, 2022

lgtm-com bot commented Apr 27, 2022

briehl Apr 27, 2022

briehl Apr 27, 2022

briehl Apr 27, 2022

codecov bot commented Apr 27, 2022 •

edited

ialarmedalien left a comment

lgtm-com bot commented Apr 27, 2022

lgtm-com bot commented Apr 28, 2022

sonarcloud bot commented Apr 28, 2022

DATAUP-639 dynamic dropdown fixes #2911

DATAUP-639 dynamic dropdown fixes #2911

Conversation

briehl commented Apr 27, 2022

Description of PR purpose/changes

Jira Ticket / Issue

Testing Instructions

Dev Checklist:

Updating Version and Release Notes (if applicable)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briehl Apr 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Apr 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 27, 2022 • edited

Codecov Report

ialarmedalien left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Apr 27, 2022

lgtm-com bot commented Apr 28, 2022

sonarcloud bot commented Apr 28, 2022

briehl Apr 27, 2022 •

edited

codecov bot commented Apr 27, 2022 •

edited