[dashboard] pass dashboard filters to share chart url in dropdown

[graceguo-supercat]

CATEGORY

Choose one

• Bug Fix
• Enhancement (new features, refinement)
• Refactor
• Add tests
• Build / Development Environment
• Documentation

SUMMARY

In dashboard, we allow user to share chart, which is a shorten url which carried current dashboard filters state, and chart's parent tab information (if it has one). But currently, dashboard's selected filters values are not included in the shorten url.

I found in dashboard's component, we used filters props for dashboard's selected filters state. But for Chart component, we used filters props to carry the values for the current filter_box, so that current filter_box will pre-populate selected values.

Solution:
filter prop should be assigned at the right place.

TEST PLAN

CI and manual test

REVIEWERS

@michellethomas @etr2460 @kristw @williaster

[codecov-io]

Codecov Report

Merging #7642 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master    #7642   +/-   ##
=======================================
  Coverage   65.57%   65.57%           
=======================================
  Files         435      435           
  Lines       21754    21754           
  Branches     2394     2394           
=======================================
  Hits        14266    14266           
  Misses       7367     7367           
  Partials      121      121

Impacted Files	Coverage Δ
superset/assets/src/chart/Chart.jsx	10.86% <ø> (ø)	⬆️
superset/assets/src/chart/ChartRenderer.jsx	7.57% <ø> (ø)	⬆️
...t/assets/src/dashboard/reducers/getInitialState.js	0% <ø> (ø)	⬆️
.../src/dashboard/components/gridComponents/Chart.jsx	70.88% <ø> (ø)	⬆️
superset/assets/src/dashboard/containers/Chart.jsx	90.9% <100%> (ø)	⬆️

---

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 d62c37b...2f79a85. Read the comment docs.

[mistercrunch]

So is the filters prop really more of a extraFilters prop, to add filter on top of the chart's self-defined filters? Should we rename it to reflect that more clearly?

[graceguo-supercat]

So is the filters prop really more of a extraFilters prop, to add filter on top of the chart's self-defined filters?

Not exactly. For Dashboard components, filters prop is the current filters state in redux. extraFilters has merged extra dashboard filter's value with chart's own formData params. It is not stored in redux state, but will be passed to backend as query parameter.

For FilterBox viz, filters prop is just original selected values:
https://github.com/apache/incubator-superset/blob/1dc17f357cf5c94f663f7a338a7814d5f79452e9/superset/assets/src/visualizations/FilterBox/transformProps.js#L51
So I prefer to re-name Chart component's filters props to be origSelectedValues. But origSelectedValues is not in the standard chart props list:
https://github.com/apache-superset/superset-ui/blob/7b3ab5f71398c7180b5a24f69e67016802025183/packages/superset-ui-chart/src/models/ChartProps.ts#L19

Besides origSelectedValues for FilterBox, is there any other viz_type using filters prop? @kristw

[mistercrunch]

All this is somewhat confusing (not because of your PR! was confusing before). Ideally we clarify the prop names / split them into many, or at least adding some comments to the code with the things you just mentioned.

[kristw]

typo interactive => interacting

[kristw]

How about initialFilters so we know at least it is related to filters?
On a side note, is this field only used by filter_box?

[kristw]

I verified and only table and filter_box use this filters field from ChartProps.

In a follow up thought, initialValues might be a better name (use values from your initial idea to make it sounds more open to other vis andinitialto shorten theorigSelected` part)

Hmm. or initialState.

[graceguo-supercat]

vote for initialValues

[kristw]

LGTM. Have a few small comments.

[kristw]

Once this PR is in I can work on updating the ChartProps to change the filters name to match the name in this PR and less confusing. (It will be a breaking change on the @superset-ui, which can go along with a few breaking changes I am planning.)