fix: dashboard filter scope bug #13695
Conversation
| // - chart_103 | ||
| // - chart_104 | ||
|
|
||
| const nodes3 = [ |
There was a problem hiding this comment.
this const can live in the test case right? since it's not being used by any other tests
| @@ -22,11 +22,29 @@ import { flatMap, isEmpty } from 'lodash'; | |||
| import { CHART_TYPE, TAB_TYPE } from './componentTypes'; | |||
| import { getChartIdAndColumnFromFilterKey } from './getDashboardFilterKey'; | |||
|
|
|||
| function getChartsFromTabsNotInScope({ tabs = [], tabsInScope = [] }) { | |||
There was a problem hiding this comment.
maybe getChartIdsFromTabsNotInScope? To clarify that this isn't returning chart objects
| const chartsNotInScope = []; | ||
| tabs.forEach(({ value: tab, children: tabChildren }) => { | ||
| if (tabChildren && !tabsInScope.includes(tab)) { | ||
| tabChildren.forEach(({ value: subTab, children: subTabChildren }) => { | ||
| if (subTabChildren && !tabsInScope.includes(subTab)) { | ||
| chartsNotInScope.push( | ||
| ...subTabChildren.filter(({ type }) => type === CHART_TYPE), | ||
| ); | ||
| } | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| // return chartId only | ||
| return chartsNotInScope.map(({ value }) => value); |
There was a problem hiding this comment.
I feel like this could probably be done more succinctly with a reduce? I can try to work my way through it as a suggestion if you'd like
There was a problem hiding this comment.
I would use reduce a few month ago...Recently @ktmud gave me a code review comment and says reduce is hard to read (for many ppl)...like this thread 🤣
https://twitter.com/jaffathecake/status/1213077702300852224?lang=en
So now i try to avoid using reduce. Sorry i can't find original PR :)
There was a problem hiding this comment.
lol TIL!
I do wonder what would happen if we all used reduce more often; maybe it would be as easy to read as map! But this works :D
Codecov Report
@@ Coverage Diff @@
## master #13695 +/- ##
==========================================
- Coverage 77.39% 73.10% -4.29%
==========================================
Files 927 615 -312
Lines 47003 21866 -25137
Branches 5806 5719 -87
==========================================
- Hits 36377 15986 -20391
+ Misses 10483 5737 -4746
Partials 143 143
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| tabs: tabChildren, | ||
| tabsInScope: flatMap(tabScopes, ({ scope }) => scope), | ||
| }); | ||
| const immuneChartsFromTabsInScope = flatMap( |
There was a problem hiding this comment.
Should this be called immuneChartIdsFromTabsInScope, too?
| @@ -22,11 +22,29 @@ import { flatMap, isEmpty } from 'lodash'; | |||
| import { CHART_TYPE, TAB_TYPE } from './componentTypes'; | |||
| import { getChartIdAndColumnFromFilterKey } from './getDashboardFilterKey'; | |||
|
|
|||
| function getChartIdsFromTabsNotInScope({ tabs = [], tabsInScope = [] }) { | |||
There was a problem hiding this comment.
Hate to make function names even longer, but do you think getImmuneChartIdsFromTabsNotInScope would make this easier to understand?
* fix: dashboard filter scope bug * fix comments * fix function/variables name (cherry picked from commit fa072cd)
|
LGTM |
* fix: dashboard filter scope bug * fix comments * fix function/variables name
mistercrunch
added
🏷️ bot
SUMMARY
This PR is to fix a logic bug in dashboard filter scope.
When calculate filter's scope, if a filter applicable to multiple child nodes (like sub-tab(s), and a chart), we aggregate the filter scope up to their common parent node level, and mark those chart type children that not applicable to the filter as immune. The bug was in the aggregate function, which didn't collect all the immune charts.
TEST PLAN
added new unit test.