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(native-filters): Don't send unnecessary PUT request on dashboard render #15146

Merged
merged 5 commits into from
Jun 14, 2021

Conversation

kgabryje
Copy link
Member

@kgabryje kgabryje commented Jun 14, 2021

SUMMARY

When dashboard first renders, we calculate tabs and charts in scope of each native filter. Before, we used to send a PUT request with the results, which also modified dashboard's metadata. This PR fixes that behaviour by saving the results only in redux, without sending them to the backend. Also, now we run the calculations only if the native filters feature flag is enabled
This PR is related to an issue raised by @graceguo-supercat in comments #14933 (comment) and #14933 (comment). The problem with setting properties show_native_filters and native_filter_configuration was not caused directly by PR #14933, but the problem was elevated by that PR because of sending unnecessary PUT request. The root of the issue will be handled in a separate PR by @villebro.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Notice that in "AFTER" screenshot there's no request to /dashboard endpoint which modifies metadata.
Before:
image

After:
image

TESTING INSTRUCTIONS

  1. Set DASHBOARD_NATIVE_FILTERS feature flag to True
  2. Add some native filters
  3. Reload the dashboard
  4. Verify that SET_FILTER_SCOPES action was dispatched and no request to /dashboard endpoint was made
  5. Verify that highlighting tabs and charts in scope of filters works as before
  6. Unset DASHBOARD_NATIVE_FILTERS feature flag
  7. Verify that SET_FILTER_SCOPES action was NOT dispatched

ADDITIONAL INFORMATION

CC: @villebro @graceguo-supercat

@codecov
Copy link

codecov bot commented Jun 14, 2021

Codecov Report

Merging #15146 (759ff08) into master (57035c1) will decrease coverage by 0.07%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #15146      +/-   ##
==========================================
- Coverage   77.49%   77.41%   -0.08%     
==========================================
  Files         969      969              
  Lines       49905    49918      +13     
  Branches     6386     6393       +7     
==========================================
- Hits        38672    38645      -27     
- Misses      11030    11070      +40     
  Partials      203      203              
Flag Coverage Δ
javascript 72.21% <50.00%> (-0.16%) ⬇️

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

Impacted Files Coverage Δ
...et-frontend/src/dashboard/actions/nativeFilters.ts 50.84% <28.57%> (-8.77%) ⬇️
...components/DashboardBuilder/DashboardContainer.tsx 82.97% <57.14%> (-17.03%) ⬇️
...t-frontend/src/dashboard/reducers/nativeFilters.ts 60.00% <100.00%> (+1.02%) ⬆️
...nd/src/dashboard/components/nativeFilters/utils.ts 56.25% <0.00%> (-30.00%) ⬇️

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 57035c1...759ff08. Read the comment docs.

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix! Works like expected, but one minor non-blocking comment regarding naming.

superset-frontend/src/dashboard/actions/nativeFilters.ts Outdated Show resolved Hide resolved
@villebro villebro added the #bug:blocking! Blocking issues with high priority label Jun 14, 2021
@kgabryje kgabryje merged commit 3866044 into apache:master Jun 14, 2021
henryyeh pushed a commit to preset-io/superset that referenced this pull request Jun 15, 2021
…render (apache#15146)

* fix(native-filters): Don't send unnecessary PUT request on dashboard render

* Run native filters scopes only if feature flag is enabled

* Change action name

* Run native filters scopes only if at least 1 filter added

* Fix lint

(cherry picked from commit 3866044)
cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
…render (apache#15146)

* fix(native-filters): Don't send unnecessary PUT request on dashboard render

* Run native filters scopes only if feature flag is enabled

* Change action name

* Run native filters scopes only if at least 1 filter added

* Fix lint
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
…render (apache#15146)

* fix(native-filters): Don't send unnecessary PUT request on dashboard render

* Run native filters scopes only if feature flag is enabled

* Change action name

* Run native filters scopes only if at least 1 filter added

* Fix lint
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
…render (apache#15146)

* fix(native-filters): Don't send unnecessary PUT request on dashboard render

* Run native filters scopes only if feature flag is enabled

* Change action name

* Run native filters scopes only if at least 1 filter added

* Fix lint
@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 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 #bug:blocking! Blocking issues with high priority preset:2021.23 size/M 🚢 1.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[native-filters] interacting w native filter sidebar should not change dashboard ownership
4 participants