fix: Removing parent filter causes incorrect state of child filter #16876

michael-s-molina · 2021-09-28T18:03:03Z

SUMMARY

Fixes #16825

It also fixes another issue where the advanced section was being collapsed when a user unchecked all the options.

@junlincc @jinghua-qa It would be great to have your approval.

AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2021-09-28.at.2.55.39.PM.mov

TESTING INSTRUCTIONS

Check the original issue for instructions.

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 · 2021-09-28T18:56:39Z

Codecov Report

Merging #16876 (a2df466) into master (27a40d2) will decrease coverage by 0.02%.
The diff coverage is 67.35%.

❗ Current head a2df466 differs from pull request most recent head de0bc2d. Consider uploading reports for the commit de0bc2d to get more accurate results

@@            Coverage Diff             @@
##           master   #16876      +/-   ##
==========================================
- Coverage   77.05%   77.02%   -0.03%     
==========================================
  Files        1021     1022       +1     
  Lines       54693    54859     +166     
  Branches     7457     7482      +25     
==========================================
+ Hits        42141    42256     +115     
- Misses      12307    12356      +49     
- Partials      245      247       +2

Flag	Coverage Δ
javascript	`71.13% <42.40%> (-0.06%)`	⬇️

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

Impacted Files	Coverage Δ
...et-frontend/src/dashboard/components/Dashboard.jsx	`78.09% <0.00%> (-0.76%)`	⬇️
...set-frontend/src/dashboard/containers/Dashboard.ts	`0.00% <0.00%> (ø)`
superset-frontend/src/dataMask/reducer.ts	`68.51% <0.00%> (-2.64%)`	⬇️
superset-frontend/src/setup/setupColors.ts	`100.00% <ø> (ø)`
...set-frontend/src/views/CRUD/data/database/types.ts	`100.00% <ø> (ø)`
superset/databases/api.py	`92.95% <ø> (ø)`
...c/views/CRUD/data/database/DatabaseModal/index.tsx	`44.05% <4.34%> (-0.09%)`	⬇️
.../database/DatabaseModal/DatabaseConnectionForm.tsx	`48.71% <5.88%> (-5.21%)`	⬇️
superset-frontend/src/dataMask/actions.ts	`66.66% <33.33%> (-5.56%)`	⬇️
superset/db_engine_specs/bigquery.py	`84.71% <37.50%> (-2.13%)`	⬇️
... and 18 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 27a40d2...de0bc2d. Read the comment docs.

villebro

Thanks for fixing this + making the code much more readable 👍

villebro · 2021-09-29T05:47:31Z

...ashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx

    restoreFilter,
    form,
    parentFilters,
  }: FiltersConfigFormProps,
  ref: React.RefObject<any>,
 ) => {
+  const removed = !!removedFilters[filterId];


nit: isRemoved

geido · 2021-09-29T09:26:43Z

/testenv up FEATURE_DASHBOARD_NATIVE_FILTERS=true FEATURE_DASHBOARD_CROSS_FILTERS=true FEATURE_DASHBOARD_NATIVE_FILTERS_SET=true FEATURE_DASHBOARD_FILTERS_EXPERIMENTAL=true

github-actions · 2021-09-29T09:28:31Z

@geido Ephemeral environment spinning up at http://52.13.99.32:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

geido · 2021-09-29T09:53:37Z

Hi @michael-s-molina. I found a problem with deleting and undoing a deletion on the fly without saving. As you can see in the video the child filter won't show as hierarchical after the parent filter has been removed and then added back again.

Video.Game.Sales.1.mp4

michael-s-molina · 2021-09-29T10:29:35Z

Hi @michael-s-molina. I found a problem with deleting and undoing a deletion on the fly without saving. As you can see in the video the child filter won't show as hierarchical after the parent filter has been removed and then added back again.

Video.Game.Sales.1.mp4

@geido I noticed that in my tests. It's related to the form preserve={false}. I didn't change it because it's a minor problem and I know that you're investigating the preserve={false} in the undo PR follow-up so I preferred to wait for your conclusions. If in the end, if you remove the preserve={false}, this bug will be fixed. If not, I can fix it in a follow-up using a different strategy.

geido · 2021-09-29T10:34:56Z

Hi @michael-s-molina. I found a problem with deleting and undoing a deletion on the fly without saving. As you can see in the video the child filter won't show as hierarchical after the parent filter has been removed and then added back again.
Video.Game.Sales.1.mp4

@geido I noticed that in my tests. It's related to the form preserve={false}. I didn't change it because it's a minor problem and I know that you're investigating the preserve={false} in the undo PR follow-up so I preferred to wait for your conclusions. If in the end, if you remove the preserve={false}, this bug will be fixed. If not, I can fix it in a follow-up using a different strategy.

I was able to reproduce the correct behavior on master. It appears that the hierarchical setting is properly restored when undoing. One difference that I see is that you have to re-open the "Advanced" section in master currently. Not sure if that is related.

Video.Game.Sales.3.mp4

michael-s-molina · 2021-09-29T14:33:41Z

@geido I changed the strategy to present "(deleted)" when a parent is removed. This works for saved and non-saved filters.

Screen.Recording.2021-09-29.at.11.30.13.AM.mov

geido

LGTM! Thanks for the fix!

github-actions · 2021-09-29T16:04:33Z

Ephemeral environment shutdown and build artifacts deleted.

…pache#16876) * fix: Removing parent filter causes incorrect state of child filter * removed to isRemoved * Shows (deleted) when the parent is removed * Removes unnecessary code

fix: Removing parent filter causes incorrect state of child filter

60c1ada

michael-s-molina requested a review from villebro September 28, 2021 18:03

pull-request-size bot added the size/L label Sep 28, 2021

michael-s-molina added this to In progress in Native dashboard filters via automation Sep 28, 2021

junlincc moved this from In progress to In review in Native dashboard filters Sep 28, 2021

villebro approved these changes Sep 29, 2021

View reviewed changes

removed to isRemoved

ca3bf96

Shows (deleted) when the parent is removed

6fb7c53

geido approved these changes Sep 29, 2021

View reviewed changes

Removes unnecessary code

de0bc2d

michael-s-molina merged commit 4da4fe4 into apache:master Sep 29, 2021

Native dashboard filters automation moved this from In review to Done Sep 29, 2021

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Removing parent filter causes incorrect state of child filter #16876

fix: Removing parent filter causes incorrect state of child filter #16876

michael-s-molina commented Sep 28, 2021 •

edited

codecov bot commented Sep 28, 2021 •

edited

villebro left a comment

villebro Sep 29, 2021

geido commented Sep 29, 2021

github-actions bot commented Sep 29, 2021

geido commented Sep 29, 2021

michael-s-molina commented Sep 29, 2021 •

edited

geido commented Sep 29, 2021 •

edited

michael-s-molina commented Sep 29, 2021

geido left a comment

github-actions bot commented Sep 29, 2021

fix: Removing parent filter causes incorrect state of child filter #16876

fix: Removing parent filter causes incorrect state of child filter #16876

Conversation

michael-s-molina commented Sep 28, 2021 • edited

SUMMARY

AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Sep 28, 2021 • edited

Codecov Report

villebro left a comment

Choose a reason for hiding this comment

villebro Sep 29, 2021

Choose a reason for hiding this comment

geido commented Sep 29, 2021

github-actions bot commented Sep 29, 2021

geido commented Sep 29, 2021

michael-s-molina commented Sep 29, 2021 • edited

geido commented Sep 29, 2021 • edited

michael-s-molina commented Sep 29, 2021

geido left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 29, 2021

michael-s-molina commented Sep 28, 2021 •

edited

codecov bot commented Sep 28, 2021 •

edited

michael-s-molina commented Sep 29, 2021 •

edited

geido commented Sep 29, 2021 •

edited