Refactor out controlUtils.js module + unit tests #7350

mistercrunch · 2019-04-23T06:54:14Z

SUMMARY

Controls are a breed of complex overrides, mutations and a patchwork of twisted logic. This effort breaks down large functions into smaller ones, make things DRYer, and add a covering set of tests. The refactor is a bit risky, but much less risky than having the current logic in place.

I got pulled in this rabbit hole while trying to debug an issue around the "Polygon" example, where it would fail when missing the metric (related to color scaler and bounds). I then realized that the validator overrides weren't applied when using setControlValue, post load. So I started digging into this patchwork in store.js the really evolved to be messy over time.

DiggidyDave · 2019-04-23T20:51:41Z

This is fantastic 👏 , it will also be much more testable! are the unit tests coming in this PR?

mistercrunch · 2019-04-23T23:15:03Z

Yes, still work in progress. Hoping to get to high coverage on that new file and the previous one

mistercrunch · 2019-04-24T17:33:49Z

@DiggidyDave I added unit tests, this is ready for review

codecov-io · 2019-04-24T18:02:26Z

Codecov Report

Merging #7350 into master will increase coverage by 0.16%.
The diff coverage is 96.51%.

@@            Coverage Diff             @@
##           master    #7350      +/-   ##
==========================================
+ Coverage   64.89%   65.05%   +0.16%     
==========================================
  Files         428      429       +1     
  Lines       20979    20993      +14     
  Branches     2337     2331       -6     
==========================================
+ Hits        13615    13658      +43     
+ Misses       7240     7219      -21     
+ Partials      124      116       -8

Impacted Files	Coverage Δ
...set/assets/src/explore/reducers/getInitialState.js	`90.9% <ø> (ø)`	⬆️
...ts/src/explore/components/ExploreViewContainer.jsx	`31.91% <ø> (ø)`	⬆️
...rset/assets/src/explore/reducers/exploreReducer.js	`31.57% <100%> (ø)`	⬆️
superset/assets/src/explore/store.js	`93.75% <100%> (+29.38%)`	⬆️
superset/assets/src/visualizations/deckgl/utils.js	`92.85% <50%> (-1.59%)`	⬇️
superset/assets/src/explore/controlUtils.js	`97.01% <97.01%> (ø)`
superset/assets/src/explore/controls.jsx	`42.85% <0%> (+0.79%)`	⬆️
superset/assets/src/explore/validators.js	`68.75% <0%> (+6.25%)`	⬆️
superset/assets/src/featureFlags.ts	`87.5% <0%> (+12.5%)`	⬆️

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 4fe152d...ae4a5c1. Read the comment docs.

michellethomas · 2019-04-26T00:23:09Z

superset/assets/src/explore/store.js

-      } else {
-        formData[k] = control.default;
-      }
+    const controlState = getControlState(k, vizType, null, inputFormData[k]);


It looks like getControlState does more than what was previously going on in this function. It seems like it wouldn't be a problem to have extra functionality added like applyMapStateToPropsToControl, validateControl, and handleMissingChoice but I'm not super familiar with where this is used. Do you think they'll be any changes around adding this additional logic (or maybe it just wasn't needed but is not harmful)? From codecov it looks like this is one of the sections of the diff that doesn't have any added tests, is there anything that would be useful to test here?

It repeated some of the logic (like applying defaults), but skipped important part that should be consistent like mapStateToProps. I'm guessing this was mostly fine because in the end the logic here cares only about the value part of the controlState.

Let me see if I can throw in some useful unit tests here.

I added some tests to validate assumptions around applyDefaultFormData.

Part of the goal here is to be more consistent and dry across the board. Before this PR, the dashboard app used applyDefaultFormData to try and mimic a subset of what is going on in getControlState, mostly applying overrides and defaults. This makes it such the logic is closer/DRYer between explore and dashboard. There's still divergence around edge cases, mostly related to the fact that mapStateToProps doesn't currently work on the dashboard side since the state is different there. Now the role of mapStateToProps seems to be mostly to configure the controls themselves (think columns' choices attribute based on datasource for instance), and should not affect the value property. Ideally we should pass the same context on both sides, but this PR fails at fully bringing this back together. Another PR could push further in that direction.

That makes sense, thanks for the context.

superset/assets/src/explore/controlUtils.js

mistercrunch · 2019-04-30T00:04:52Z

@michellethomas thank you for the review, I addressed your comments!

michellethomas · 2019-04-30T01:21:01Z

this lgtm thanks for doing this! I did a little testing with production data and didn't see any issues. You might want to sync up with @kristw before merging because this conflicts with #6827

kristw · 2019-04-30T13:00:40Z

This LGTM @mistercrunch I will take care of resolving conflicts in my PR after you merge. Best regards, Krist

…

-- Krist Wongsuphasawat http://kristw.yellowpigz.com

On Apr 30, 2019, 08:22 +0700, michellethomas ***@***.***>, wrote: this lgtm thanks for doing this! I did a little testing with production data and didn't see any issues. You might want to sync up with @kristw before merging because this conflicts with #6827 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

* filter out all nan series (#7313) * improve not rich tooltip (#7345) * Create issue_label_bot.yaml (#7341) * fix: do not save colors without a color scheme (#7347) * [wtforms] Strip leading/trailing whitespace (#7084) * [schema] Updating the datasources schema (#5451) * limit tables/views returned if schema is not provided (#7358) * limit tables/views returned if schema is not provided * fix typo * improve code performance * handle the case when table name or view name does not present a schema * Add type anno (#7342) * Updated local dev instructions to include missing step * First pass at type annotations * [schema] Updating the base column schema (#5452) * Update 937d04c16b64_update_datasources.py (#7361) * Feature flag for client cache (#7348) * Feature flag for client cache * Fix integration test * Revert "Fix integration test" This reverts commit 58434ab. * Feature flag for client cache * Fix integration tests * Add feature flag to config.py * Add another feature check * Fix more integration tests * Fix raw HTML in SliceAdder (#7338) * remove backendSync.json (#7331) * [bubbles] issue when using duplicated metrics (#7087) * SUPERSET-7: Docker compose config version breaks on Ubuntu 16.04 (#7359) * SUPERSET-8: Update text in docs copyright footer (#7360) * SUPERSET-7: Docker compose config version breaks on Ubuntu 16.04 * SUPERSET-8: Extra text in docs copyright footer * [schema] Adding commits and removing unnecessary foreign-key definitions (#7371) * Store last selected dashboard in sessionStorage (#7181) * Store last selected dashboard in sessionStorage * Fix tests * [schema] Updating the base metric schema (#5453) * Fix NoneType bug & fill the test recipients with original recipients if empty (#7365) * Added living goods as among the users of Superset (#7407) * Added living goods as among the users of Superset Living Goods is a non profit organisation with operation in africa and the middle east. We work in community health use data heavily on day to day. Superset is our platform of choice for dashboards. * Update README.md * [dashboard] allow user re-order top-level tabs (#7390) * [SQL Lab] Increase timeout threshold for offline check (#7411) * Bump FAB to 2.0.0 (#7323) * Bump FAB to 2.0.0 * [tests] whitelist SecurityApi login and refresh endpoints * [style] Fix, C812 missing trailing commas * [security] Remove SUPERSET_UPDATE_PERMS flag Registering sources needs to be performed after the views are initialized on UPDATE_PERMS=False configuration * [docs] New, FAB_UPDATE_PERMS and flask fab cli * [docs] Fix, db upgrade needs to come first, create-admin needs a db * [cli] New, superset init bootstraps all permissions for FAB and Superset * [style] Fix, flakes * [annotations] Improves UX on annotation validation, start_dttm, end_dttm (#7326) * Setting renderTrigger on label_colors (#7410) * Refactor out controlUtils.js module + unit tests (#7350) * [WiP]refactor out a controlUtils.js file * unit tests * add missing license * Addressing comments * feature: see Presto row and array data types (#7413) * Merge lastest from master into lyft-release-sp8 (#7405) * filter out all nan series (#7313) * improve not rich tooltip (#7345) * Create issue_label_bot.yaml (#7341) * fix: do not save colors without a color scheme (#7347) * [wtforms] Strip leading/trailing whitespace (#7084) * [schema] Updating the datasources schema (#5451) * limit tables/views returned if schema is not provided (#7358) * limit tables/views returned if schema is not provided * fix typo * improve code performance * handle the case when table name or view name does not present a schema * Add type anno (#7342) * Updated local dev instructions to include missing step * First pass at type annotations * [schema] Updating the base column schema (#5452) * Update 937d04c16b64_update_datasources.py (#7361) * Feature flag for client cache (#7348) * Feature flag for client cache * Fix integration test * Revert "Fix integration test" This reverts commit 58434ab. * Feature flag for client cache * Fix integration tests * Add feature flag to config.py * Add another feature check * Fix more integration tests * Fix raw HTML in SliceAdder (#7338) * remove backendSync.json (#7331) * [bubbles] issue when using duplicated metrics (#7087) * SUPERSET-7: Docker compose config version breaks on Ubuntu 16.04 (#7359) * SUPERSET-8: Update text in docs copyright footer (#7360) * SUPERSET-7: Docker compose config version breaks on Ubuntu 16.04 * SUPERSET-8: Extra text in docs copyright footer * [schema] Adding commits and removing unnecessary foreign-key definitions (#7371) * Store last selected dashboard in sessionStorage (#7181) * Store last selected dashboard in sessionStorage * Fix tests * [schema] Updating the base metric schema (#5453) * Fix NoneType bug & fill the test recipients with original recipients if empty (#7365) * feat: see Presto row and array data types (#7391) * feat: see Presto row and array data types * fix: address PR comments * fix: lint and build issues * fix: add types * add stronger type hints where possible * fix: lint issues and add select_star func in Hive * add missing pkg init * fix: build issues * fix: pylint issues * fix: use logging instead of print * Removed --console-log and superset runserver (#7421) * Fixes dashboard export button missing download and #7353 (#7427) * Added additional German translations to string file (#6604) * Added additional German translations to string file Updates to German translation files as per directions * Removed messages.json * [fix] Fixing SQL parsing issue (#7374) * add chinese translate (#7402) * Quick fix to address deadlock issue (#7434) * feat: view presto row objects in data grid (#7445) * Merge lastest from master into lyft-release-sp8 (#7405) * filter out all nan series (#7313) * improve not rich tooltip (#7345) * Create issue_label_bot.yaml (#7341) * fix: do not save colors without a color scheme (#7347) * [wtforms] Strip leading/trailing whitespace (#7084) * [schema] Updating the datasources schema (#5451) * limit tables/views returned if schema is not provided (#7358) * limit tables/views returned if schema is not provided * fix typo * improve code performance * handle the case when table name or view name does not present a schema * Add type anno (#7342) * Updated local dev instructions to include missing step * First pass at type annotations * [schema] Updating the base column schema (#5452) * Update 937d04c16b64_update_datasources.py (#7361) * Feature flag for client cache (#7348) * Feature flag for client cache * Fix integration test * Revert "Fix integration test" This reverts commit 58434ab. * Feature flag for client cache * Fix integration tests * Add feature flag to config.py * Add another feature check * Fix more integration tests * Fix raw HTML in SliceAdder (#7338) * remove backendSync.json (#7331) * [bubbles] issue when using duplicated metrics (#7087) * SUPERSET-7: Docker compose config version breaks on Ubuntu 16.04 (#7359) * SUPERSET-8: Update text in docs copyright footer (#7360) * SUPERSET-7: Docker compose config version breaks on Ubuntu 16.04 * SUPERSET-8: Extra text in docs copyright footer * [schema] Adding commits and removing unnecessary foreign-key definitions (#7371) * Store last selected dashboard in sessionStorage (#7181) * Store last selected dashboard in sessionStorage * Fix tests * [schema] Updating the base metric schema (#5453) * Fix NoneType bug & fill the test recipients with original recipients if empty (#7365) * feat: see Presto row and array data types (#7391) * feat: see Presto row and array data types * fix: address PR comments * fix: lint and build issues * fix: add types * Incorporate feedback from initial PR (prematurely merged to lyft-release-sp8) (#7415) * add stronger type hints where possible * fix: lint issues and add select_star func in Hive * add missing pkg init * fix: build issues * fix: pylint issues * fix: use logging instead of print * feat: view presto row objects in data grid * fix: address feedback * fix: spacing * feat: Scheduling queries from SQL Lab (#7416) (#7446) * Merge lastest from master into lyft-release-sp8 (#7405) * filter out all nan series (#7313) * improve not rich tooltip (#7345) * Create issue_label_bot.yaml (#7341) * fix: do not save colors without a color scheme (#7347) * [wtforms] Strip leading/trailing whitespace (#7084) * [schema] Updating the datasources schema (#5451) * limit tables/views returned if schema is not provided (#7358) * limit tables/views returned if schema is not provided * fix typo * improve code performance * handle the case when table name or view name does not present a schema * Add type anno (#7342) * Updated local dev instructions to include missing step * First pass at type annotations * [schema] Updating the base column schema (#5452) * Update 937d04c16b64_update_datasources.py (#7361) * Feature flag for client cache (#7348) * Feature flag for client cache * Fix integration test * Revert "Fix integration test" This reverts commit 58434ab. * Feature flag for client cache * Fix integration tests * Add feature flag to config.py * Add another feature check * Fix more integration tests * Fix raw HTML in SliceAdder (#7338) * remove backendSync.json (#7331) * [bubbles] issue when using duplicated metrics (#7087) * SUPERSET-7: Docker compose config version breaks on Ubuntu 16.04 (#7359) * SUPERSET-8: Update text in docs copyright footer (#7360) * SUPERSET-7: Docker compose config version breaks on Ubuntu 16.04 * SUPERSET-8: Extra text in docs copyright footer * [schema] Adding commits and removing unnecessary foreign-key definitions (#7371) * Store last selected dashboard in sessionStorage (#7181) * Store last selected dashboard in sessionStorage * Fix tests * [schema] Updating the base metric schema (#5453) * Fix NoneType bug & fill the test recipients with original recipients if empty (#7365) * feat: see Presto row and array data types (#7391) * feat: see Presto row and array data types * fix: address PR comments * fix: lint and build issues * fix: add types * Incorporate feedback from initial PR (prematurely merged to lyft-release-sp8) (#7415) * add stronger type hints where possible * fix: lint issues and add select_star func in Hive * add missing pkg init * fix: build issues * fix: pylint issues * fix: use logging instead of print * feat: view presto row objects in data grid * fix: address feedback * fix: spacing * Workaround for no results returned (#7442) * feat: view presto row objects in data grid (#7436) * feat: view presto row objects in data grid * fix: address feedback * fix: spacing * feat: Scheduling queries from SQL Lab (#7416) * Lightweight pipelines POC * Add docs * Minor fixes * Remove Lyft URL * Use enum * Minor fix * Fix unit tests * Mark props as required * feat: Add `validate_sql_json` endpoint for checking that a given sql query is valid for the chosen database (#7422) (#7462) merge from lyft-release-sp8 to master * Adds missing metric sum__SP_RUR_TOTL (#7452) * Late import for optional lib pyhive (#7471) * Late import for optional lib pyhive * fix * fix: calendar heatmap examples (#7375) Fixing a set of examples that trip on ValueError vs TypeError * bugfix: Improve support for special characters in schema and table names (#7297) * Bugfix to SQL Lab to support tables and schemas with characters that require quoting * Remove debugging prints * Add uri encoding to secondary tables call * Quote schema names for presto * Quote selected_schema on Snowflake, MySQL and Hive * Remove redundant parens * Add python unit tests * Add js unit test * Fix flake8 linting error * [dashboard] After update filter, trigger new queries when charts are visible (#7233) * trigger query when chart is visible * add integration test * fix: alter sql columns to long text #7463 (#7476) Merge lyft-release-sp8@7bfe7bc to master * Refactor ConsoleLog (#7428) * Revised Chinese translation (#7464) * add chinese translate * edit chinese translation * druid connector: avoid using 'dimensions' for scan queries (#7377) After the following PyDruid change (contained in version 0.5.2) the Superset Histogram charts rendered with Druid data are broken: druid-io/pydruid@0a59a70 Bump the pydruid requirements accordingly in setup.py Issue: #7368

pull-request-size bot added the size/L label Apr 23, 2019

mistercrunch mentioned this pull request Apr 23, 2019

Fetch charts with GET to benefit from browser cache and conditional requests #7032

Merged

mistercrunch force-pushed the fix_validate branch 2 times, most recently from c859ea1 to 55c2736 Compare April 24, 2019 05:55

mistercrunch added the #code-quality label Apr 24, 2019

mistercrunch changed the title ~~[WiP] refactor out a controlUtils.js file~~ Refactor out a controlUtils.js file + unit tests Apr 24, 2019

mistercrunch requested review from kristw and betodealmeida and removed request for kristw April 24, 2019 17:32

mistercrunch added the review label Apr 24, 2019

mistercrunch changed the title ~~Refactor out a controlUtils.js file + unit tests~~ Refactor out controlUtils.js module + unit tests Apr 24, 2019

mistercrunch closed this Apr 24, 2019

mistercrunch reopened this Apr 24, 2019

mistercrunch force-pushed the fix_validate branch from 234b424 to cb9101e Compare April 24, 2019 17:47

kristw mentioned this pull request Apr 25, 2019

Separate vis-specific controls from centralized controls #6827

Closed

michellethomas reviewed Apr 26, 2019

View reviewed changes

superset/assets/src/explore/controlUtils.js Outdated Show resolved Hide resolved

mistercrunch added 3 commits April 29, 2019 16:48

[WiP]refactor out a controlUtils.js file

6a7c0bc

unit tests

f4de383

add missing license

1fea495

Addressing comments

ae4a5c1

mistercrunch force-pushed the fix_validate branch from 4d0c93f to ae4a5c1 Compare April 30, 2019 00:05

michellethomas approved these changes Apr 30, 2019

View reviewed changes

mistercrunch merged commit 46579b1 into apache:master Apr 30, 2019

mistercrunch deleted the fix_validate branch April 30, 2019 18:45

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.34.0 labels Feb 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor out controlUtils.js module + unit tests #7350

Refactor out controlUtils.js module + unit tests #7350

mistercrunch commented Apr 23, 2019 •

edited

Loading

DiggidyDave commented Apr 23, 2019

mistercrunch commented Apr 23, 2019

mistercrunch commented Apr 24, 2019

codecov-io commented Apr 24, 2019 •

edited

Loading

michellethomas Apr 26, 2019

mistercrunch Apr 29, 2019

mistercrunch Apr 30, 2019

michellethomas Apr 30, 2019

mistercrunch commented Apr 30, 2019

michellethomas commented Apr 30, 2019

kristw commented Apr 30, 2019 via email

Refactor out controlUtils.js module + unit tests #7350

Refactor out controlUtils.js module + unit tests #7350

Conversation

mistercrunch commented Apr 23, 2019 • edited Loading

CATEGORY

SUMMARY

DiggidyDave commented Apr 23, 2019

mistercrunch commented Apr 23, 2019

mistercrunch commented Apr 24, 2019

codecov-io commented Apr 24, 2019 • edited Loading

Codecov Report

michellethomas Apr 26, 2019

Choose a reason for hiding this comment

mistercrunch Apr 29, 2019

Choose a reason for hiding this comment

mistercrunch Apr 30, 2019

Choose a reason for hiding this comment

michellethomas Apr 30, 2019

Choose a reason for hiding this comment

mistercrunch commented Apr 30, 2019

michellethomas commented Apr 30, 2019

kristw commented Apr 30, 2019 via email

mistercrunch commented Apr 23, 2019 •

edited

Loading

codecov-io commented Apr 24, 2019 •

edited

Loading