Fix Filter component to handle invalid range filter #162

cyrielo · 2017-10-10T21:43:59Z

This updated PR implement manual range filtering, disable the range slider when range values are invalid example when the min and max are equal or min is less than max.

coveralls · 2017-10-10T21:50:38Z

Coverage remained the same at 41.53% when pulling 12aa065bd6efba5d105dd177ffd7c997470fb861 on cyril/reports_on_the_fly into c0ba78b on master.

jameshowardwang · 2017-10-10T23:05:12Z

app/assets/javascripts/components/ReportFilter.jsx

@@ -73,7 +75,7 @@ class ReportFilter extends React.Component {
      behaviour: 'tap-drag',
      range: {
        'min': 0,
-        'max': this.props.highest_tax_counts.highest_nt_zscore
+        'max':this.highest_nt_zscore


missing a space

cdebourcy · 2017-10-10T22:26:29Z

app/assets/javascripts/components/ReportFilter.jsx

-    const default_nt_zscore_threshold = `0-${this.props.highest_tax_counts.highest_nt_zscore}`;
-    const default_nt_rpm_threshold = `0-${this.props.highest_tax_counts.highest_nt_rpm}`;
+    this.highest_nt_zscore = this.props.highest_tax_counts.highest_nt_zscore || 100;
+    this.highest_nt_rpm = this.props.highest_tax_counts.highest_nt_rpm || 100;


Why 100? RPM is maximum 1e6 (and there is no limit to how large z-score can be)

Valid points, @cdebourcy

@cyrielo @eggerdesigns and I had a chat about filter ranges, and we decided that 1) the default range should be the min ... max from the report data 2) if min=max, the sliders will be turned off in UI. This will address the issue that prompted this pull request, namely that a javascript exception was being thrown by the slider UI component when min=max. 3) as per Joe's original design, we'll use 100 and not 1e6 to indicate that there are not enough samples to fit a normal distribution and compute z score; as you said zscores are often over 100, but a slider going to 1e6 is not useful in practice, and Joe is already using 100 in his tool...

boris-dimitrov

as explained by @cdebourcy and the discussion with @eggerdesigns

coveralls · 2017-10-11T02:25:30Z

Coverage remained the same at 36.709% when pulling e674bca86092d403196a2936eb33587b2b902e4b on cyril/reports_on_the_fly into c816930 on master.

coveralls · 2017-10-11T02:29:47Z

Coverage remained the same at 36.709% when pulling e674bca86092d403196a2936eb33587b2b902e4b on cyril/reports_on_the_fly into c816930 on master.

boris-dimitrov · 2017-10-16T16:51:50Z

app/assets/javascripts/components/ReportFilter.jsx

    const nt_rpm_end =
      (nt_rpm_threshold.split(',').length > 1) ? parseInt(nt_rpm_threshold.split(',')[1], 10) :
-        this.props.highest_tax_counts.highest_nt_rpm;
-
+      this.highest_nt_rpm;


There is a lot of repeated code here, when we go to 8 columns with 2 bounds each, it will likely be unmanageable. Probably should generalize/parameterize so you don't have to repeat each line 16 times: for each {highest, lowest} x {genus, species} x {nt, nr} x {zscore, rpm}.

boris-dimitrov · 2017-10-16T16:58:31Z

app/assets/javascripts/components/ReportFilter.jsx

+      nt_zscore.noUiSlider.on('update', (values, handle) => {
+        if (handle === 0) {
+          this.setState({
+            nt_zscore_start: Math.round(values[handle])


start and end shouldn't be rounded, because data points may end up outside the rounded range

to convert start and end to integers, use the function Math.floor for start, and Math.ceil for end
(i am assuming JS has those, and I am also assuming that Math.floor(-1.1) is -2.0 and not -1.0 -- could you please confirm that).

But to the broader question, why are you converting to integers? If the actual data range is [-4.35 to -1.21], shouldn't users be permitted to select values in that range?

My suggestion is for now if you need to convert to integers, to use floor and ceil, and also research whether converting to integers is wise.

boris-dimitrov · 2017-10-16T17:00:18Z

app/assets/javascripts/components/ReportFilter.jsx

+          });
+        }
+      });
+    }


Is it typical to duplicate code like that in JS? For example, when you figure out the correct rounding math, if the code is duplicated, you will need to fix in two places.

boris-dimitrov · 2017-10-16T17:08:22Z

app/assets/javascripts/components/ReportFilter.jsx

-                    </div>
+                    ) : ''}
+                     { (!this.has_valid_nt_rpm_range && !this.has_valid_nt_zscore_range) ?
+                    <div className="center"><small>Cannot set thresholds</small></div> : '' }


Let's show this to Rebecca when it's pushed, she may prefer some different way to indicate the min=max condition. Personally I don't think of that as an error condition.

@jshoe

Bump loofah from 2.2.3 to 2.3.1Bumps [loofah](https://github.com/flavorjones/loofah) from 2.2.3 to 2.3.1. <details> <summary>Release notes</summary> *Sourced from [loofah's releases](https://github.com/flavorjones/loofah/releases).* > ## 2.3.1 / 2019-10-22 > > ### Security > > Address CVE-2019-15587: Unsanitized JavaScript may occur in sanitized output when a crafted SVG element is republished. > > This CVE's public notice is at [flavorjones/loofah#171](https://github-redirect.dependabot.com/flavorjones/loofah/issues/171) > > ## 2.3.0 / 2019-09-28 > > ### Features > > * Expand set of allowed protocols to include `tel:` and `line:`. [#104, [#147](https://github-redirect.dependabot.com/flavorjones/loofah/issues/147)] > * Expand set of allowed CSS functions. [related to [#122](https://github-redirect.dependabot.com/flavorjones/loofah/issues/122)] > * Allow greater precision in shorthand CSS values. [#149](https://github-redirect.dependabot.com/flavorjones/loofah/issues/149) (Thanks, [@danfstucky](https://github.com/danfstucky)!) > * Allow CSS property `list-style` [#162](https://github-redirect.dependabot.com/flavorjones/loofah/issues/162) (Thanks, [@jaredbeck](https://github.com/jaredbeck)!) > * Allow CSS keywords `thick` and `thin` [#168](https://github-redirect.dependabot.com/flavorjones/loofah/issues/168) (Thanks, [@georgeclaghorn](https://github.com/georgeclaghorn)!) > * Allow HTML property `contenteditable` [#167](https://github-redirect.dependabot.com/flavorjones/loofah/issues/167) (Thanks, [@andreynering](https://github.com/andreynering)!) > > > ### Bug fixes > > * CSS hex values are no longer limited to lowercase hex. Previously uppercase hex were scrubbed. [#165](https://github-redirect.dependabot.com/flavorjones/loofah/issues/165) (Thanks, [@asok](https://github.com/asok)!) > > > ### Deprecations / Name Changes > > The following method and constants are hereby deprecated, and will be completely removed in a future release: > > * Deprecate `Loofah::Helpers::ActionView.white_list_sanitizer`, please use `Loofah::Helpers::ActionView.safe_list_sanitizer` instead. > * Deprecate `Loofah::Helpers::ActionView::WhiteListSanitizer`, please use `Loofah::Helpers::ActionView::SafeListSanitizer` instead. > * Deprecate `Loofah::HTML5::WhiteList`, please use `Loofah::HTML5::SafeList` instead. > > Thanks to [@JuanitoFatas](https://github.com/JuanitoFatas) for submitting these changes in [#164](https://github-redirect.dependabot.com/flavorjones/loofah/issues/164) and for making the language used in Loofah more inclusive. > > </details> <details> <summary>Changelog</summary> *Sourced from [loofah's changelog](https://github.com/flavorjones/loofah/blob/master/CHANGELOG.md).* > ## 2.3.1 / 2019-10-22 > > ### Security > > Address CVE-2019-15587: Unsanitized JavaScript may occur in sanitized output when a crafted SVG element is republished. > > This CVE's public notice is at [flavorjones/loofah#171](https://github-redirect.dependabot.com/flavorjones/loofah/issues/171) > > > ## 2.3.0 / 2019-09-28 > > ### Features > > * Expand set of allowed protocols to include `tel:` and `line:`. [#104, [#147](https://github-redirect.dependabot.com/flavorjones/loofah/issues/147)] > * Expand set of allowed CSS functions. [related to [#122](https://github-redirect.dependabot.com/flavorjones/loofah/issues/122)] > * Allow greater precision in shorthand CSS values. [#149](https://github-redirect.dependabot.com/flavorjones/loofah/issues/149) (Thanks, [@danfstucky](https://github.com/danfstucky)!) > * Allow CSS property `list-style` [#162](https://github-redirect.dependabot.com/flavorjones/loofah/issues/162) (Thanks, [@jaredbeck](https://github.com/jaredbeck)!) > * Allow CSS keywords `thick` and `thin` [#168](https://github-redirect.dependabot.com/flavorjones/loofah/issues/168) (Thanks, [@georgeclaghorn](https://github.com/georgeclaghorn)!) > * Allow HTML property `contenteditable` [#167](https://github-redirect.dependabot.com/flavorjones/loofah/issues/167) (Thanks, [@andreynering](https://github.com/andreynering)!) > > > ### Bug fixes > > * CSS hex values are no longer limited to lowercase hex. Previously uppercase hex were scrubbed. [#165](https://github-redirect.dependabot.com/flavorjones/loofah/issues/165) (Thanks, [@asok](https://github.com/asok)!) > > > ### Deprecations / Name Changes > > The following method and constants are hereby deprecated, and will be completely removed in a future release: > > * Deprecate `Loofah::Helpers::ActionView.white_list_sanitizer`, please use `Loofah::Helpers::ActionView.safe_list_sanitizer` instead. > * Deprecate `Loofah::Helpers::ActionView::WhiteListSanitizer`, please use `Loofah::Helpers::ActionView::SafeListSanitizer` instead. > * Deprecate `Loofah::HTML5::WhiteList`, please use `Loofah::HTML5::SafeList` instead. > > Thanks to [@JuanitoFatas](https://github.com/JuanitoFatas) for submitting these changes in [#164](https://github-redirect.dependabot.com/flavorjones/loofah/issues/164) and for making the language used in Loofah more inclusive. </details> <details> <summary>Commits</summary> - [`83df303`](flavorjones/loofah@83df303) version bump to v2.3.1 - [`e323a77`](flavorjones/loofah@e323a77) Merge pull request [#172](https://github-redirect.dependabot.com/flavorjones/loofah/issues/172) from flavorjones/171-xss-vulnerability - [`1d81f91`](flavorjones/loofah@1d81f91) update CHANGELOG - [`0c6617a`](flavorjones/loofah@0c6617a) mitigate XSS vulnerability in SVG animate attributes - [`a5bd819`](flavorjones/loofah@a5bd819) rufo formatting - [`1bdf276`](flavorjones/loofah@1bdf276) formatting in README - [`1908dc2`](flavorjones/loofah@1908dc2) update CHANGELOG with release date - [`bcbd7b3`](flavorjones/loofah@bcbd7b3) update dev gemspec - [`f6d4c2d`](flavorjones/loofah@f6d4c2d) version bump to v2.3.0 - [`08fee8c`](flavorjones/loofah@08fee8c) update dev deps - Additional commits viewable in [compare view](flavorjones/loofah@v2.2.3...v2.3.1) </details> <br /> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=loofah&package-manager=bundler&previous-version=2.2.3&new-version=2.3.1)](https://help.github.com/articles/configuring-automated-security-fixes) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) Dependabot will merge this PR once CI passes on it, as requested by @jshoe. [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot ignore this [patch|minor|major] version` will close this PR and stop Dependabot creating any more for this minor/major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) - `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language - `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language - `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language - `@dependabot use this milestone` will set the current milestone as the default for future PRs for this repo and language You can disable automated security fix PRs for this repo from the [Security Alerts page](https://github.com/chanzuckerberg/idseq-web/network/alerts). </details>

* Parameterize SFN call for consensus genome workflow * Add Jira tickets to TODOs

cyrielo requested a review from boris-dimitrov October 10, 2017 21:44

jameshowardwang requested a review from a team October 10, 2017 23:04

jameshowardwang approved these changes Oct 10, 2017

View reviewed changes

cdebourcy suggested changes Oct 11, 2017

View reviewed changes

cdebourcy approved these changes Oct 11, 2017

View reviewed changes

boris-dimitrov suggested changes Oct 11, 2017

View reviewed changes

cyrielo force-pushed the cyril/reports_on_the_fly branch 2 times, most recently from f958169 to e674bca Compare October 11, 2017 02:21

cyrielo force-pushed the cyril/reports_on_the_fly branch 2 times, most recently from b24526b to 36a3080 Compare October 16, 2017 05:29

boris-dimitrov reviewed Oct 16, 2017

View reviewed changes

boris-dimitrov approved these changes Oct 16, 2017

View reviewed changes

Cyril Ologho added 5 commits October 16, 2017 14:49

Fix Filter component to handle invalid range filter

48b3e34

remove report filter

86eca93

add ability to manually enter range values

92632c2

hide slider bubble and style manual input

775180b

update report filter to use float number to capture all values

53f53b4

cyrielo force-pushed the cyril/reports_on_the_fly branch from 3423848 to 53f53b4 Compare October 16, 2017 21:50

merge changes with view levels update

854df87

cyrielo merged commit 2f48f2a into master Oct 16, 2017

jameshowardwang deleted the cyril/reports_on_the_fly branch September 12, 2018 17:57

kislyuk pushed a commit that referenced this pull request Jul 27, 2020

Parameterize SFN call for consensus genome workflow (#162)

59cd492

* Parameterize SFN call for consensus genome workflow * Add Jira tickets to TODOs

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Filter component to handle invalid range filter #162

Fix Filter component to handle invalid range filter #162

cyrielo commented Oct 10, 2017 •

edited

Loading

coveralls commented Oct 10, 2017

jameshowardwang Oct 10, 2017

cdebourcy Oct 10, 2017

boris-dimitrov Oct 11, 2017 •

edited

Loading

boris-dimitrov left a comment

coveralls commented Oct 11, 2017

coveralls commented Oct 11, 2017

boris-dimitrov Oct 16, 2017

boris-dimitrov Oct 16, 2017

boris-dimitrov Oct 16, 2017

boris-dimitrov Oct 16, 2017

Fix Filter component to handle invalid range filter #162

Fix Filter component to handle invalid range filter #162

Conversation

cyrielo commented Oct 10, 2017 • edited Loading

coveralls commented Oct 10, 2017

jameshowardwang Oct 10, 2017

Choose a reason for hiding this comment

cdebourcy Oct 10, 2017

Choose a reason for hiding this comment

boris-dimitrov Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

boris-dimitrov left a comment

Choose a reason for hiding this comment

coveralls commented Oct 11, 2017

coveralls commented Oct 11, 2017

boris-dimitrov Oct 16, 2017

Choose a reason for hiding this comment

boris-dimitrov Oct 16, 2017

Choose a reason for hiding this comment

boris-dimitrov Oct 16, 2017

Choose a reason for hiding this comment

boris-dimitrov Oct 16, 2017

Choose a reason for hiding this comment

cyrielo commented Oct 10, 2017 •

edited

Loading

boris-dimitrov Oct 11, 2017 •

edited

Loading