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 extra filters #10692

Merged

Conversation

john-bodley
Copy link
Member

@john-bodley john-bodley commented Aug 27, 2020

SUMMARY

This PR fixes an issue which may have been introduced in #10359 which allowed for the == operator for extra filters however the logic for building the filters from a warming perspective were not updated resulting in a cache miss.

The fix is to fetch the filter metadata and use the == operator (and a corresponding scalar value) for non-multiple selections.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

Updated the unit tests.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@john-bodley john-bodley force-pushed the john-bodley--fix-build-extra-filters branch 2 times, most recently from b6d6453 to a0447ca Compare August 27, 2020 03:10
@john-bodley john-bodley force-pushed the john-bodley--fix-build-extra-filters branch from a0447ca to d88caa3 Compare August 27, 2020 17:59
@codecov-commenter
Copy link

Codecov Report

Merging #10692 into master will decrease coverage by 2.96%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10692      +/-   ##
==========================================
- Coverage   64.31%   61.35%   -2.97%     
==========================================
  Files         786      426     -360     
  Lines       36924    13788   -23136     
  Branches     3514     3533      +19     
==========================================
- Hits        23746     8459   -15287     
+ Misses      13069     5142    -7927     
- Partials      109      187      +78     
Flag Coverage Δ
#cypress ?
#javascript 61.35% <ø> (+0.52%) ⬆️
#python ?

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

Impacted Files Coverage Δ
superset-frontend/src/SqlLab/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/dashboard/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/index.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/dashboard/index.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/setup/setupColors.js 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/chart/ChartContainer.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/setup/setupFormatters.js 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/reducers/index.js 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/setup/setupPluginsExtra.js 0.00% <0.00%> (-100.00%) ⬇️
... and 503 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 6ff96cf...d88caa3. Read the comment docs.

@john-bodley john-bodley force-pushed the john-bodley--fix-build-extra-filters branch from d88caa3 to c29b4f3 Compare August 27, 2020 19:10
Copy link

@graceguo-supercat graceguo-supercat left a comment

Choose a reason for hiding this comment

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

LGTM!

@graceguo-supercat
Copy link

graceguo-supercat commented Aug 27, 2020

Add a little context: this function build_extra_filters is mostly used by warm_up job.
It pulls default_filters and filter_scopes settings from dashboard metadata, and generate extra_filters request parameter which will be used by slices in the dashboard. This simulates front-end JS logic, where user opens dashboard, dashboard will apply default_filters to all the slices.

@john-bodley john-bodley merged commit 1ee87cc into apache:master Sep 2, 2020
amitmiran137 pushed a commit to ofekisr/incubator-superset that referenced this pull request Sep 7, 2020
…boards_permissions

* upstream/master: (32 commits)
  docs: Add a note to contributing.md on reporting security vulnerabilities (apache#10796)
  Fix: Include RLS filters for cache keys (apache#10805)
  feat: filters for database list view (apache#10772)
  fix: MVC show saved query (apache#10781)
  added creator column and adjusted order columns (apache#10789)
  security: disallow uuid package on jinja2 (apache#10794)
  feat: CRUD REST API for saved queries (apache#10777)
  fix: disable domain sharding on explore view (apache#10787)
  fix: can not type `0.05` in `TextControl` (apache#10778)
  fix: pivot table timestamp grouping (apache#10774)
  fix: add validator information to email/slack alerts (apache#10762)
  More Label touchups (margins) (apache#10722)
  fix: dashboard extra filters (apache#10692)
  fix: re-installing local superset in cache image (apache#10766)
  feat: SIP-34 table list view for databases (apache#10705)
  refactor: convert DatasetList schema filter to use new distinct api (apache#10746)
  chore: removing fsevents dependency (apache#10751)
  Fix precommit hook for docs/installation.rst (apache#10759)
  feat(database): POST, PUT, DELETE API endpoints (apache#10741)
  docs: Update OAuth configuration in installation.rst (apache#10748)
  ...
@villebro villebro added the v0.38 label Sep 10, 2020
villebro pushed a commit to preset-io/superset that referenced this pull request Sep 11, 2020
villebro pushed a commit that referenced this pull request Sep 11, 2020
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.38.0 labels Mar 12, 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 v0.38 🚢 0.38.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants