Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(dashboard): FilterBox JS error when datasets API is slow #15993

Merged
merged 3 commits into from
Jul 31, 2021

Conversation

ktmud
Copy link
Member

@ktmud ktmud commented Jul 30, 2021

SUMMARY

This fixes a bug introduced by #15699: when the /dashboard/<dashboardId>/datasets API is slow and the FilterBox is rendered before this API is returned, it will fail and show a JS error:

filterbox-fix-before.mp4

This PR makes the async re-rendering process more robust by:

  1. Fixing the code in FilterBox that is causing the JS error
  2. Introduce a PLACEHOLDER_DATASOURCE for all chart renderers so similar JS errors will not happen for other charts which forgot to handle the case when datasource is not loaded.
  3. Don't render JS errors when chart's datasource is PLACEHOLDER_DATASOURCE. After the API is successfully returned, chart reducer will trigger a re-render.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

See above

After

filterbox-name-fix-after.mp4

TESTING INSTRUCTIONS

  1. Insert
            import time

            time.sleep(3)

here to mock a slow response for the /datasets API.
2. Create a dashboard with Filterbox, select a couple of filter columns with verbose names (change column Label in datasource editor).

ADDITIONAL INFORMATION

  • Has associated issue: perf(dashboard): make loading datasets non-blocking #15699
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@ktmud ktmud added the v1.3 label Jul 30, 2021
@ktmud ktmud force-pushed the filterbox-datasource-lazyload-fix branch from 327fb60 to 7d2e478 Compare July 30, 2021 22:26
@@ -93,7 +94,7 @@ class ChartRenderer extends React.Component {
nextProps.queriesResponse !== this.props.queriesResponse;
return (
this.hasQueryResponseChange ||
nextProps.datasource !== this.props.datasource ||
!isEqual(nextProps.datasource, this.props.datasource) ||

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@ktmud ktmud force-pushed the filterbox-datasource-lazyload-fix branch from 7d2e478 to 9c3a169 Compare July 30, 2021 22:39
@@ -373,7 +373,7 @@ max-locals=15
max-returns=10

# Maximum number of branch for function / method body
max-branches=12
max-branches=15
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allow more if branches instead of suppressing the too-many-branches error inline.

cc @villebro @john-bodley @etr2460

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yikes! This is an anti-pattern in my opinion. The check is there for a reason to prevent non-readable code. If one must override this, it should be done locally and not globally.

@codecov
Copy link

codecov bot commented Jul 30, 2021

Codecov Report

Merging #15993 (f94cfc4) into master (c37c56c) will decrease coverage by 0.08%.
The diff coverage is 53.84%.

❗ Current head f94cfc4 differs from pull request most recent head 32e6cc8. Consider uploading reports for the commit 32e6cc8 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #15993      +/-   ##
==========================================
- Coverage   77.04%   76.95%   -0.09%     
==========================================
  Files         988      989       +1     
  Lines       52389    52400      +11     
  Branches     6626     6634       +8     
==========================================
- Hits        40365    40327      -38     
- Misses      11800    11850      +50     
+ Partials      224      223       -1     
Flag Coverage Δ
javascript 71.53% <47.82%> (-0.02%) ⬇️
presto ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
superset-frontend/src/chart/Chart.jsx 44.64% <0.00%> (-1.66%) ⬇️
superset-frontend/src/chart/ChartRenderer.jsx 37.17% <0.00%> (-0.80%) ⬇️
superset-frontend/src/components/Icons/index.tsx 100.00% <ø> (ø)
...set-frontend/src/components/ListViewCard/index.tsx 100.00% <ø> (ø)
...re/components/controls/DatasourceControl/index.jsx 79.06% <ø> (ø)
...perset-frontend/src/views/CRUD/chart/ChartCard.tsx 75.60% <ø> (ø)
...t-frontend/src/views/CRUD/welcome/SavedQueries.tsx 59.59% <ø> (ø)
...end/src/visualizations/FilterBox/transformProps.ts 0.00% <0.00%> (ø)
superset-frontend/src/chart/chartReducer.ts 33.75% <33.33%> (-0.04%) ⬇️
...perset-frontend/src/dashboard/containers/Chart.jsx 92.30% <50.00%> (-7.70%) ⬇️
... and 10 more

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 c37c56c...32e6cc8. Read the comment docs.

for filter_ in form_data.get("adhoc_filters") or []:
if filter_["clause"] == "WHERE" and filter_.get("subject"):
column_names.add(filter_.get("subject"))

# columns used by Filter Box
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ONE LINER CHALLENGE!

column_names.update(filter_config["column"] for filter_config in form_data.get("filter_configs", []) if filter_config and "column" in filter_config)

Copy link
Contributor

@serenajiang serenajiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥳

@serenajiang serenajiang merged commit b73d7ba into apache:master Jul 31, 2021
serenajiang pushed a commit to airbnb/superset-fork that referenced this pull request Jul 31, 2021
opus-42 pushed a commit to opus-42/incubator-superset that referenced this pull request Nov 14, 2021
cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
@john-bodley john-bodley deleted the filterbox-datasource-lazyload-fix branch June 8, 2022 05:26
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.3.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L v1.3 🚢 1.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants