fix(dashboard): Fill form with the latest values when undo in native filters #16851

geido · 2021-09-27T08:09:18Z

SUMMARY

This PR fixes several issues related to the undo functionality in native filters. It stores the latest form data and adds it back to the form when the user removes a filter and then undoes the removal.

BEFORE

See issue #16821

AFTER

Video.Game.Sales.mp4

TESTING INSTRUCTIONS

Open a Dashboard with native filters enabled
Add a filter
Remove the filter
Undo your action and make sure the latest data was preserved

ADDITIONAL INFORMATION

Has associated issue: Fixes [native filter] dataset/column/advanced element disappears from the "Filters configuration and scoping" modal when user undo deleting native filter #16821
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

…issue_16821_filters_undo-s1

geido · 2021-09-27T08:09:57Z

@jinghua-qa @junlincc

codecov · 2021-09-27T08:40:35Z

Codecov Report

Merging #16851 (32a16a2) into master (100760c) will increase coverage by 0.00%.
The diff coverage is 90.90%.

❗ Current head 32a16a2 differs from pull request most recent head 3e9c827. Consider uploading reports for the commit 3e9c827 to get more accurate results

@@           Coverage Diff           @@
##           master   #16851   +/-   ##
=======================================
  Coverage   76.93%   76.94%           
=======================================
  Files        1021     1021           
  Lines       54708    54724   +16     
  Branches     7459     7468    +9     
=======================================
+ Hits        42092    42106   +14     
- Misses      12372    12374    +2     
  Partials      244      244

Flag	Coverage Δ
javascript	`71.14% <90.90%> (+0.01%)`	⬆️

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

Impacted Files	Coverage Δ
...onfigModal/FiltersConfigForm/FiltersConfigForm.tsx	`73.87% <90.90%> (+0.60%)`	⬆️

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 100760c...3e9c827. Read the comment docs.

geido · 2021-09-27T09:46:25Z

/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-27T09:48:27Z

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

villebro

Nice work! LGTM with one small question/comment.

villebro · 2021-09-27T10:12:13Z

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

-    !!filterToEdit?.adhoc_filters || !!filterToEdit?.time_range;
+    !!formFilter?.adhoc_filters ||
+    !!formFilter?.time_range ||
+    !!filterToEdit?.adhoc_filters?.length ||
+    !!filterToEdit?.time_range;


should we also change it to formFilter?.adhoc_filters?.length || like it is later for filterToEdit, just in case?

Thanks for your feedback! It appears that when the form doesn't have any filters it will just return undefined, reason why I am only checking for that here

michael-s-molina · 2021-09-27T11:21:00Z

@geido If I'm not mistaken when the user removes a filter, it only marks that filter as deleted but the form values are preserved. If that's the case, why do we need the undoFormValues state? Can't we just use the original form?

geido · 2021-09-27T11:23:26Z

@geido If I'm not mistaken when the user removes a filter, it only marks that filter as deleted but the form values are preserved. If that's the case, why do we need the undoFormValues state? Can't we just use the original form?

@michael-s-molina I checked that and it appears that when the filter is removed the form is also cleaned up. You won't have the most recent state of the form, hence I am storing it on removal and reapplying the values on undo.

michael-s-molina · 2021-09-27T11:30:17Z

@geido If I'm not mistaken when the user removes a filter, it only marks that filter as deleted but the form values are preserved. If that's the case, why do we need the undoFormValues state? Can't we just use the original form?

@michael-s-molina I checked that and it appears that when the filter is removed the form is also cleaned up. You won't have the most recent state of the form, hence I am storing it on removal and reapplying the values on undo.

Do you know where the form is being cleared? This is the handler for the delete action and it's only marking it as removed.

export const createHandleTabEdit = (
  setRemovedFilters: (
    value:
      | ((
          prevState: Record<string, FilterRemoval>,
        ) => Record<string, FilterRemoval>)
      | Record<string, FilterRemoval>,
  ) => void,
  setSaveAlertVisible: Function,
  addFilter: Function,
) => (filterId: string, action: 'add' | 'remove') => {
  const completeFilterRemoval = (filterId: string) => {
    // the filter state will actually stick around in the form,
    // and the filterConfig/newFilterIds, but we use removedFilters
    // to mark it as removed.
    setRemovedFilters(removedFilters => ({
      ...removedFilters,
      [filterId]: { isPending: false },
    }));
  };

  if (action === 'remove') {
    // first set up the timer to completely remove it
    const timerId = window.setTimeout(
      () => completeFilterRemoval(filterId),
      REMOVAL_DELAY_SECS * 1000,
    );
    // mark the filter state as "removal in progress"
    setRemovedFilters(removedFilters => ({
      ...removedFilters,
      [filterId]: { isPending: true, timerId },
    }));
    setSaveAlertVisible(false);
  } else if (action === 'add') {
    addFilter();
  }
};

geido · 2021-09-27T12:35:38Z

@michael-s-molina as stated above, I confirm that the form is wiped out as soon as the form rerenders after the remove action. Any data you already have about the filter (such as the one in the filterConfigMap ) might be outdated as it does not contain the most recent state of the form. Thus, the only solution that seems to solve both scenarios is to store the current state of the form. I am happy to consider a different solution but it does not appear to be obvious at the moment. Let me know if anything different comes to your mind. Thank you!

michael-s-molina · 2021-09-27T12:37:57Z

@michael-s-molina as stated above, I confirm that the form is wiped out as soon as the form rerenders after the remove action. Any data you already have about the filter (such as the one in the filterConfigMap ) might be outdated as it does not contain the most recent state of the form. Thus, the only solution that seems to solve both scenarios is to store the current state of the form. I am happy to consider a different solution but it does not appear to be obvious at the moment. Let me know if anything different comes to your mind. Thank you!

I was reviewing and found this:

This is the code that is clearing the form. preserve={false} is forcing the values to be cleared. The default value of Ant Design is true. If we change it to true, the values are preserved after restoring. The only thing is that I don’t know why this was set to false.

michael-s-molina · 2021-09-27T12:43:13Z

@geido We can investigate this in a follow-up since this is a P1. For now, let's merge it.

github-actions · 2021-09-27T16:39:51Z

Ephemeral environment shutdown and build artifacts deleted.

sadpandajoe · 2021-09-27T18:06:05Z

🏷️ 2021.38

…filters (apache#16851) * Set undoFormValues * Reorganize * Revert check * Fix and clean up * Fix pre-filter and sort values (cherry picked from commit d3f6145)

…filters (apache#16851) * Set undoFormValues * Reorganize * Revert check * Fix and clean up * Fix pre-filter and sort values

geido added 6 commits September 25, 2021 17:29

Set undoFormValues

74984dc

Reorganize

c67f0b6

Revert check

fe5c420

Fix and clean up

169508e

Fix pre-filter and sort values

f6151c6

Merge branch 'master' of https://github.com/apache/superset into fix/…

3e9c827

…issue_16821_filters_undo-s1

superset-github-bot bot added the Flexiana label Sep 27, 2021

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

geido requested review from michael-s-molina, kgabryje and villebro and removed request for michael-s-molina and kgabryje September 27, 2021 08:10

villebro approved these changes Sep 27, 2021

View reviewed changes

michael-s-molina approved these changes Sep 27, 2021

View reviewed changes

junlincc merged commit d3f6145 into apache:master Sep 27, 2021

junlincc added the rush! Requires immediate attention label Sep 27, 2021

superset-github-bot bot added the preset:2021.38 label Sep 27, 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(dashboard): Fill form with the latest values when undo in native filters #16851

fix(dashboard): Fill form with the latest values when undo in native filters #16851

geido commented Sep 27, 2021

geido commented Sep 27, 2021

codecov bot commented Sep 27, 2021

geido commented Sep 27, 2021

github-actions bot commented Sep 27, 2021

villebro left a comment

villebro Sep 27, 2021

geido Sep 27, 2021

michael-s-molina commented Sep 27, 2021

geido commented Sep 27, 2021

michael-s-molina commented Sep 27, 2021

geido commented Sep 27, 2021

michael-s-molina commented Sep 27, 2021 •

edited

Loading

michael-s-molina commented Sep 27, 2021 •

edited

Loading

github-actions bot commented Sep 27, 2021

sadpandajoe commented Sep 27, 2021

fix(dashboard): Fill form with the latest values when undo in native filters #16851

fix(dashboard): Fill form with the latest values when undo in native filters #16851

Conversation

geido commented Sep 27, 2021

SUMMARY

BEFORE

AFTER

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

geido commented Sep 27, 2021

codecov bot commented Sep 27, 2021

Codecov Report

geido commented Sep 27, 2021

github-actions bot commented Sep 27, 2021

villebro left a comment

Choose a reason for hiding this comment

villebro Sep 27, 2021

Choose a reason for hiding this comment

geido Sep 27, 2021

Choose a reason for hiding this comment

michael-s-molina commented Sep 27, 2021

geido commented Sep 27, 2021

michael-s-molina commented Sep 27, 2021

geido commented Sep 27, 2021

michael-s-molina commented Sep 27, 2021 • edited Loading

michael-s-molina commented Sep 27, 2021 • edited Loading

github-actions bot commented Sep 27, 2021

sadpandajoe commented Sep 27, 2021

michael-s-molina commented Sep 27, 2021 •

edited

Loading

michael-s-molina commented Sep 27, 2021 •

edited

Loading