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 not loading with default first value in filter #23512

Merged
merged 1 commit into from
Mar 29, 2023

Conversation

geido
Copy link
Member

@geido geido commented Mar 28, 2023

SUMMARY

Users are able to configure a dashboard filter to automatically select the first value by default. However, when this function was used the dashboard didn't load properly.

The culprit was a redundant useEffect that caused the filter value to reset to undefined. By removing the redundant useEffect the issue is resolved.

AFTER

DEV.Unicode.Test.mp4

TESTING INSTRUCTIONS

  1. Open a Dashboard
  2. Create a filter with first default value
  3. The Dashboard should load successfully

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • 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

@codecov
Copy link

codecov bot commented Mar 28, 2023

Codecov Report

Merging #23512 (e6feadf) into master (8db5cb1) will decrease coverage by 11.21%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master   #23512       +/-   ##
===========================================
- Coverage   67.65%   56.44%   -11.21%     
===========================================
  Files        1910     1910               
  Lines       73745    73743        -2     
  Branches     7987     7987               
===========================================
- Hits        49891    41625     -8266     
- Misses      21813    30077     +8264     
  Partials     2041     2041               
Flag Coverage Δ
hive 52.73% <ø> (ø)
mysql ?
postgres ?
presto 52.66% <ø> (ø)
python 59.21% <ø> (-23.16%) ⬇️
sqlite ?
unit 52.58% <ø> (ø)

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

Impacted Files Coverage Δ
...c/filters/components/Select/SelectFilterPlugin.tsx 64.63% <ø> (-0.85%) ⬇️

... and 302 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@kgabryje
Copy link
Member

kgabryje commented Mar 28, 2023

I'm almost sure that that useEffect wasn't redundant and something broke when I tried to remove it some time ago. The problem is, I don't remember what it was... Could you try to test some less common scenarios such as opening a dashboard with filters In URL?
Maybe @jinghua-qa could help testing this solution?
@villebro any chance you remember what that useEffect was for?

@jinghua-qa
Copy link
Member

I'm almost sure that that useEffect wasn't redundant and something broke when I tried to remove it some time ago. The problem is, I don't remember what it was... Could you try to test some less common scenarios such as opening a dashboard with filters In URL? Maybe @jinghua-qa could help testing this solution? @villebro any chance you remember what that useEffect was for?

sure i can test

@geido
Copy link
Member Author

geido commented Mar 28, 2023

I'm almost sure that that useEffect wasn't redundant and something broke when I tried to remove it some time ago. The problem is, I don't remember what it was... Could you try to test some less common scenarios such as opening a dashboard with filters In URL? Maybe @jinghua-qa could help testing this solution? @villebro any chance you remember what that useEffect was for?

@kgabryje if you look at line https://github.com/apache/superset/pull/23512/files#diff-b4d2de68c8a3b04879772237085ede1b49a6908b6f2890a9ab1aaeae9e18b854R265 there is another useEffect that is doing the updates to the datamask but with a more in-depth logic around it. Unless I am missing anything, I don't see how that useEffect would do anything different other than re-running updates to the datamask without caring about the required logics. Do you have any insights?

@jinghua-qa
Copy link
Member

/testenv up

@github-actions
Copy link
Contributor

@jinghua-qa Ephemeral environment spinning up at http://54.201.26.107:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@jinghua-qa
Copy link
Member

test PR locally have tried select with first default value by UI and dashboard properties, also have tried opening a dashboard with filters In URL, did not find any issue. LGTM

@geido geido merged commit 4220d32 into master Mar 29, 2023
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

jinghua-qa pushed a commit to preset-io/superset that referenced this pull request Mar 29, 2023
jinghua-qa pushed a commit to preset-io/superset that referenced this pull request Apr 10, 2023
@mistercrunch mistercrunch added 🍒 2.1.1 🍒 2.1.2 🍒 2.1.3 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.0.0 labels Mar 13, 2024
@mistercrunch mistercrunch deleted the fix/dashboard-filter-first-value branch March 26, 2024 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.1.1 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels preset:2023.11 size/XS v2.1 🍒 2.1.1 🍒 2.1.2 🍒 2.1.3 🚢 3.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants