PXD-1514 fix(explorer): use sqon to calculate actual number of selected items #346
Conversation
src/components/charts/helper.js
Outdated
| if (!sqon || !sqon.content) return undefined; | ||
| const sqonItems = sqon.content.filter(item => item.content.field === field); | ||
| if (!sqonItems || sqonItems.length !== 1) return undefined; | ||
| const sqonValues = sqonItems[0].content.value; |
There was a problem hiding this comment.
whats does grabbing the first index give us?
There was a problem hiding this comment.
It seems arranger use sqon for filtering queries, sqon.content stores all selected fields, and since this function only need one specific field, the sqonItems should be always in length of 1 or zero: 1 means there are some values selected on this field, and zero means no values selected on this field (which means every values should be counted for this field in the visualizations)
src/components/charts/helper.js
Outdated
| return sqonValues; | ||
| }; | ||
|
|
||
| const getSQONCount = (sqon, field) => { |
There was a problem hiding this comment.
this function is pretty much the same as the one above it, I don't think we need two
There was a problem hiding this comment.
getSQONCount gets the count of selected values for given field, and getSQONValues gets the selected values for given field
src/components/charts/helper.js
Outdated
| label, | ||
| value: field.buckets.length, | ||
| value: Math.min(field.buckets.length, sqonCount), |
There was a problem hiding this comment.
does grabbing the min of these two always give us the value we want?
There was a problem hiding this comment.
When there are no selected values in a given field, this means all values should be counted, so here I think it would be easy to understand that we let sqonCount = Inifinity. That's why here I use Math.min. I can change it to a if...else statement if you want.
src/components/charts/helper.js
Outdated
| }); | ||
|
|
||
| const getCharts = (data, arrangerConfig) => { | ||
| const getSQONValues = (sqon, field) => { | ||
| if (!sqon || !sqon.content) return undefined; |
There was a problem hiding this comment.
can we return something other than undefined?
There was a problem hiding this comment.
Sure. We could discuss about return value for sqon=null case. Do you think null would be better?
src/components/charts/helper.test.js
Outdated
|
|
||
| it('return selecetd values from SQON as expected', () => { | ||
| expect(helper.getSQONValues(selectWhiteSqon, 'ethnicity')).toEqual(selectWhiteSqonValues); | ||
| }); |
There was a problem hiding this comment.
I think we need some more tests. what if there is a filter with no values selected? does it return the length of the buckets or the sqon count? things like that.
| * If no value selected, return null | ||
| */ | ||
| const getSQONValues = (sqon, field) => { | ||
| if (!sqon || !sqon.content) return null; |
There was a problem hiding this comment.
you could also return an empty array right? that might be easier so you wouldn't have to check for null values, but I'll leave that up to you, just a suggestion
There was a problem hiding this comment.
Thanks. I think returning an empty array will just be similar to returning null so I still want to keep it 😛
sqonValues === null || sqonValues.includes(bucket.key).... =>
sqonValues.length === 0 ? || sqonValues.includes(bucket.key)...
|
More test cases added~ Please have a check again~ Thanks! @abgeorge7 |
Current counts and charts in visualizations only shows bucket numbers in the filters, but we want the visualization result shows actual number of selected items in the filters. To solve this, we use
sqonto calculate the correct numbers.