Skip to content

Conversation

@villebro
Copy link
Member

@villebro villebro commented Jul 17, 2020

SUMMARY

Currently Filter Box events aren't being properly reflected in the new chart data endpoint requests. This was because extra_filters weren't being properly merged with filters from the chart data payload. The bulk of the fix was done in superset-ui ( apache-superset/superset-ui#688 ), and updates @superset-ui/chart to the most recent released version.

TEST PLAN

CI + new tests.

SCREENSHOTS

Below are screenshots of a Filter Box with changing legacy charts (Name Line, Multiformat Line) and one new chart (Name Cloud, ECharts Multiformat).

BEFORE

Previously Filter Box changes (date range, time grain, gender selector) were NOT reflected in the viz' calling the new chart data endpoint (Name Cloud, ECharts Multiformat).
captured (13)
captured (14)

AFTER

As can be seen, changes in the Filter Box are reflected properly in all charts.
captured (12)
captured (11)

ADDITIONAL INFORMATION

Copy link
Member Author

Choose a reason for hiding this comment

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

The current logic generates an invalid filter (picked up by TS linter in superset-ui).

@villebro villebro added the v0.37 label Jul 17, 2020

Choose a reason for hiding this comment

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

should be 2020 :)

Copy link
Member Author

Choose a reason for hiding this comment

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

image

@villebro villebro force-pushed the villebro/extra-filters branch from fc5df9a to 01473ce Compare July 20, 2020 08:51
op: Array.isArray(values) ? 'IN' : '=',
val: values,
}))
.filter(filter => filter.val !== null);

Choose a reason for hiding this comment

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

when user clear out a filter, it's value will be null, and we need to send out this to backend.

Copy link
Member Author

@villebro villebro Jul 21, 2020

Choose a reason for hiding this comment

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

@graceguo-supercat when the filter is cleared, the filter will be removed, thus triggering a filter event (tested that it works properly on both legacy and new charts). Having an undefined value for a binary or set filter is also invalid according to the type declarations on superset-ui, so if this is the case we probably need to make changes there. See https://github.com/apache-superset/superset-ui/blob/97e7ba99d0efc363805a415f5f1a646f781ba1fb/packages/superset-ui-query/src/types/Query.ts#L8-L22

Choose a reason for hiding this comment

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

agree!

@villebro villebro changed the title [WIP] fix: extra filters for chart data endpoint fix: extra filters for chart data endpoint Jul 21, 2020
Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link

@graceguo-supercat graceguo-supercat left a comment

Choose a reason for hiding this comment

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

LGTM!

@villebro villebro merged commit 3c39b26 into apache:master Jul 21, 2020
@villebro villebro deleted the villebro/extra-filters branch July 21, 2020 05:25
villebro added a commit that referenced this pull request Jul 21, 2020
* fix: extra filters

* fix old test and add new test

* add test for null filter value

* leave lowercase until all operators are fully uppercased

* bump packages

* rename test
michellethomas pushed a commit to airbnb/superset-fork that referenced this pull request Jul 23, 2020
@john-bodley john-bodley mentioned this pull request Aug 27, 2020
6 tasks
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
* fix: extra filters

* fix old test and add new test

* add test for null filter value

* leave lowercase until all operators are fully uppercased

* bump packages

* rename test
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
* fix: extra filters

* fix old test and add new test

* add test for null filter value

* leave lowercase until all operators are fully uppercased

* bump packages

* rename test
@mistercrunch mistercrunch added 🍒 0.37.0 Cherry-picked to 0.37.0 🍒 0.37.1 Cherry-picked to 0.37.1 🍒 0.37.2 Cherry-picked to 0.37.2 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.38.0 First shipped in 0.38.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 size/M v0.37 🍒 0.37.0 Cherry-picked to 0.37.0 🍒 0.37.1 Cherry-picked to 0.37.1 🍒 0.37.2 Cherry-picked to 0.37.2 🚢 0.38.0 First shipped in 0.38.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Filter isn't applied to word cloud chart on a dashboard

4 participants