Skip to content

Commit

Permalink
chore(query-nodes): camel case decimal places, aggregation axis, show…
Browse files Browse the repository at this point in the history 
… label/percent stack view (#19759)
  • Loading branch information
@thmsobrmlr
thmsobrmlr authored Jan 16, 2024
1 parent b603565 commit 9700a2b
Show file tree
Hide file tree
Showing 21 changed files with 128 additions and 84 deletions.
4 changes: 2 additions & 2 deletions frontend/src/lib/components/UnitPicker/CustomUnitModal.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,11 @@ function chooseFormativeElementValue(
trendsFilter: TrendsFilter | null | undefined
): string {
if (formativeElement === 'prefix') {
return trendsFilter?.aggregation_axis_prefix || ''
return trendsFilter?.aggregationAxisPrefix || ''
}

if (formativeElement === 'postfix') {
return trendsFilter?.aggregation_axis_postfix || ''
return trendsFilter?.aggregationAxisPostfix || ''
}

return ''
Expand Down
28 changes: 14 additions & 14 deletions frontend/src/lib/components/UnitPicker/UnitPicker.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export function UnitPicker(): JSX.Element {
const { reportAxisUnitsChanged } = useActions(eventUsageLogic)

const [isVisible, setIsVisible] = useState(false)
const [localAxisFormat, setLocalAxisFormat] = useState(trendsFilter?.aggregation_axis_format || undefined)
const [localAxisFormat, setLocalAxisFormat] = useState(trendsFilter?.aggregationAxisFormat || undefined)
const [customUnitModal, setCustomUnitModal] = useState<'prefix' | 'postfix' | null>(null)

const customUnitModalRef = useRef<HTMLDivElement | null>(null)
Expand All @@ -50,9 +50,9 @@ export function UnitPicker(): JSX.Element {
setLocalAxisFormat(format)

updateInsightFilter({
aggregation_axis_format: format,
aggregation_axis_prefix: prefix,
aggregation_axis_postfix: postfix,
aggregationAxisFormat: format,
aggregationAxisPrefix: prefix,
aggregationAxisPostfix: postfix,
})

reportAxisUnitsChanged({
Expand All @@ -72,11 +72,11 @@ export function UnitPicker(): JSX.Element {
if (localAxisFormat) {
displayValue = aggregationDisplayMap[localAxisFormat]
}
if (trendsFilter?.aggregation_axis_prefix?.length) {
displayValue = `Prefix: ${trendsFilter?.aggregation_axis_prefix}`
if (trendsFilter?.aggregationAxisPrefix?.length) {
displayValue = `Prefix: ${trendsFilter?.aggregationAxisPrefix}`
}
if (trendsFilter?.aggregation_axis_postfix?.length) {
displayValue = `Postfix: ${trendsFilter?.aggregation_axis_postfix}`
if (trendsFilter?.aggregationAxisPostfix?.length) {
displayValue = `Postfix: ${trendsFilter?.aggregationAxisPostfix}`
}
return displayValue
}, [localAxisFormat, trendsFilter])
Expand Down Expand Up @@ -118,22 +118,22 @@ export function UnitPicker(): JSX.Element {
<LemonDivider />
<LemonButton
onClick={() => setCustomUnitModal('prefix')}
active={!!trendsFilter?.aggregation_axis_prefix}
active={!!trendsFilter?.aggregationAxisPrefix}
fullWidth
>
Custom prefix
{trendsFilter?.aggregation_axis_prefix
? `: ${trendsFilter?.aggregation_axis_prefix}...`
{trendsFilter?.aggregationAxisPrefix
? `: ${trendsFilter?.aggregationAxisPrefix}...`
: '...'}
</LemonButton>
<LemonButton
onClick={() => setCustomUnitModal('postfix')}
active={!!trendsFilter?.aggregation_axis_postfix}
active={!!trendsFilter?.aggregationAxisPostfix}
fullWidth
>
Custom postfix
{trendsFilter?.aggregation_axis_postfix
? `: ${trendsFilter?.aggregation_axis_postfix}...`
{trendsFilter?.aggregationAxisPostfix
? `: ${trendsFilter?.aggregationAxisPostfix}...`
: '...'}
</LemonButton>
</>
Expand Down
10 changes: 6 additions & 4 deletions frontend/src/queries/nodes/InsightQuery/utils/filtersToQueryNode.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,7 @@ describe('filtersToQueryNode', () => {
formula: 'A+B',
shown_as: ShownAsValue.VOLUME,
display: ChartDisplayType.ActionsAreaGraph,
show_percent_stack_view: true,
}

const result = filtersToQueryNode(filters)
Expand All @@ -327,12 +328,13 @@ describe('filtersToQueryNode', () => {
show_legend: true,
hidden_legend_indexes: [0, 10],
compare: true,
aggregation_axis_format: 'numeric',
aggregation_axis_prefix: '£',
aggregation_axis_postfix: '%',
decimal_places: 8,
aggregationAxisFormat: 'numeric',
aggregationAxisPrefix: '£',
aggregationAxisPostfix: '%',
decimalPlaces: 8,
formula: 'A+B',
display: ChartDisplayType.ActionsAreaGraph,
showPercentStackView: true,
},
breakdownFilter: {
breakdown_histogram_bin_count: 1,
Expand Down
11 changes: 6 additions & 5 deletions frontend/src/queries/nodes/InsightQuery/utils/filtersToQueryNode.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -254,14 +254,15 @@ export const filtersToQueryNode = (filters: Partial<FilterType>): InsightQueryNo
show_legend: filters.show_legend,
hidden_legend_indexes: cleanHiddenLegendIndexes(filters.hidden_legend_keys),
compare: filters.compare,
aggregation_axis_format: filters.aggregation_axis_format,
aggregation_axis_prefix: filters.aggregation_axis_prefix,
aggregation_axis_postfix: filters.aggregation_axis_postfix,
decimal_places: filters.decimal_places,
aggregationAxisFormat: filters.aggregation_axis_format,
aggregationAxisPrefix: filters.aggregation_axis_prefix,
aggregationAxisPostfix: filters.aggregation_axis_postfix,
decimalPlaces: filters.decimal_places,
formula: filters.formula,
display: filters.display,
show_values_on_series: filters.show_values_on_series,
show_percent_stack_view: filters.show_percent_stack_view,
showPercentStackView: filters.show_percent_stack_view,
showLabelsOnSeries: filters.show_labels_on_series,
})
}

Expand Down
18 changes: 12 additions & 6 deletions frontend/src/queries/nodes/InsightQuery/utils/queryNodeToFilter.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,13 @@ describe('queryNodeToFilter', () => {
display: ChartDisplayType.ActionsBar,
// breakdown_histogram_bin_count?: TrendsFilterLegacy['breakdown_histogram_bin_count']
// show_legend?: TrendsFilterLegacy['show_legend']
// aggregation_axis_format?: TrendsFilterLegacy['aggregation_axis_format']
// aggregation_axis_prefix?: TrendsFilterLegacy['aggregation_axis_prefix']
// aggregation_axis_postfix?: TrendsFilterLegacy['aggregation_axis_postfix']
// decimal_places?: TrendsFilterLegacy['decimal_places']
aggregationAxisFormat: 'numeric',
aggregationAxisPrefix: 'M',
aggregationAxisPostfix: '$',
decimalPlaces: 5,
// show_values_on_series?: TrendsFilterLegacy['show_values_on_series']
// show_labels_on_series?: TrendsFilterLegacy['show_labels_on_series']
// show_percent_stack_view?: TrendsFilterLegacy['show_percent_stack_view']
showLabelsOnSeries: true,
showPercentStackView: true,
// hidden_legend_indexes?: TrendsFilterLegacy['hidden_legend_indexes']
},
}
Expand All @@ -78,6 +78,12 @@ describe('queryNodeToFilter', () => {
display: ChartDisplayType.ActionsBar,
formula: 'A + B',
compare: true,
decimal_places: 5,
aggregation_axis_format: 'numeric',
aggregation_axis_prefix: 'M',
aggregation_axis_postfix: '$',
show_labels_on_series: true,
show_percent_stack_view: true,
}
expect(result).toEqual(filters)
})
Expand Down
12 changes: 12 additions & 0 deletions frontend/src/queries/nodes/InsightQuery/utils/queryNodeToFilter.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,19 @@ export const queryNodeToFilter = (query: InsightQueryNode): Partial<FilterType>
const legacyProps: TrendsFilterLegacy = {}
if (isTrendsQuery(query)) {
legacyProps.smoothing_intervals = insightFilter.smoothingIntervals
legacyProps.decimal_places = insightFilter.decimalPlaces
legacyProps.aggregation_axis_format = insightFilter.aggregationAxisFormat
legacyProps.aggregation_axis_postfix = insightFilter.aggregationAxisPostfix
legacyProps.aggregation_axis_prefix = insightFilter.aggregationAxisPrefix
legacyProps.show_labels_on_series = insightFilter.showLabelsOnSeries
legacyProps.show_percent_stack_view = insightFilter.showPercentStackView
delete insightFilter.smoothingIntervals
delete insightFilter.decimalPlaces
delete insightFilter.aggregationAxisFormat
delete insightFilter.aggregationAxisPostfix
delete insightFilter.aggregationAxisPrefix
delete insightFilter.showLabelsOnSeries
delete insightFilter.showPercentStackView
}
Object.assign(filters, insightFilter)
Object.assign(filters, legacyProps)
Expand Down
10 changes: 5 additions & 5 deletions frontend/src/queries/nodes/InsightViz/InsightDisplayConfig.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,8 @@ export function InsightDisplayConfig(): JSX.Element {
(showPercentStackView && isPercentStackViewOn ? 1 : 0) +
(!isPercentStackViewOn &&
isTrends &&
trendsFilter?.aggregation_axis_format &&
trendsFilter.aggregation_axis_format !== 'numeric'
trendsFilter?.aggregationAxisFormat &&
trendsFilter.aggregationAxisFormat !== 'numeric'
? 1
: 0) +
(hasLegend && showLegend ? 1 : 0)
Expand Down Expand Up @@ -199,7 +199,7 @@ function DecimalPrecisionInput(): JSX.Element {

const reportChange = useDebouncedCallback(() => {
posthog.capture('decimal places changed', {
decimal_places: trendsFilter?.decimal_places,
decimal_places: trendsFilter?.decimalPlaces,
})
}, 500)

Expand All @@ -211,10 +211,10 @@ function DecimalPrecisionInput(): JSX.Element {
min={0}
max={9}
defaultValue={DEFAULT_DECIMAL_PLACES}
value={trendsFilter?.decimal_places}
value={trendsFilter?.decimalPlaces}
onChange={(value) => {
updateInsightFilter({
decimal_places: value,
decimalPlaces: value,
})
reportChange()
}}
Expand Down
4 changes: 2 additions & 2 deletions frontend/src/queries/nodes/InsightViz/utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,15 +107,15 @@ export const getShowValueOnSeries = (query: InsightQueryNode): boolean | undefin

export const getShowLabelsOnSeries = (query: InsightQueryNode): boolean | undefined => {
if (isTrendsQuery(query)) {
return query.trendsFilter?.show_labels_on_series
return query.trendsFilter?.showLabelsOnSeries
} else {
return undefined
}
}

export const getShowPercentStackView = (query: InsightQueryNode): boolean | undefined => {
if (isTrendsQuery(query)) {
return query.trendsFilter?.show_percent_stack_view
return query.trendsFilter?.showPercentStackView
} else {
return undefined
}
Expand Down
14 changes: 7 additions & 7 deletions frontend/src/queries/schema.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3741,13 +3741,13 @@
"TrendsFilter": {
"additionalProperties": false,
"properties": {
"aggregation_axis_format": {
"aggregationAxisFormat": {
"$ref": "#/definitions/AggregationAxisFormat"
},
"aggregation_axis_postfix": {
"aggregationAxisPostfix": {
"type": "string"
},
"aggregation_axis_prefix": {
"aggregationAxisPrefix": {
"type": "string"
},
"breakdown_histogram_bin_count": {
Expand All @@ -3756,7 +3756,7 @@
"compare": {
"type": "boolean"
},
"decimal_places": {
"decimalPlaces": {
"type": "number"
},
"display": {
Expand All @@ -3771,13 +3771,13 @@
},
"type": "array"
},
"show_labels_on_series": {
"showLabelsOnSeries": {
"type": "boolean"
},
"show_legend": {
"showPercentStackView": {
"type": "boolean"
},
"show_percent_stack_view": {
"show_legend": {
"type": "boolean"
},
"show_values_on_series": {
Expand Down
14 changes: 7 additions & 7 deletions frontend/src/queries/schema.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -513,15 +513,15 @@ export type TrendsFilter = {
compare?: TrendsFilterLegacy['compare']
formula?: TrendsFilterLegacy['formula']
display?: TrendsFilterLegacy['display']
breakdown_histogram_bin_count?: TrendsFilterLegacy['breakdown_histogram_bin_count']
show_legend?: TrendsFilterLegacy['show_legend']
aggregation_axis_format?: TrendsFilterLegacy['aggregation_axis_format']
aggregation_axis_prefix?: TrendsFilterLegacy['aggregation_axis_prefix']
aggregation_axis_postfix?: TrendsFilterLegacy['aggregation_axis_postfix']
decimal_places?: TrendsFilterLegacy['decimal_places']
breakdown_histogram_bin_count?: TrendsFilterLegacy['breakdown_histogram_bin_count'] // TODO: fully move into BreakdownFilter
aggregationAxisFormat?: TrendsFilterLegacy['aggregation_axis_format']
aggregationAxisPrefix?: TrendsFilterLegacy['aggregation_axis_prefix']
aggregationAxisPostfix?: TrendsFilterLegacy['aggregation_axis_postfix']
decimalPlaces?: TrendsFilterLegacy['decimal_places']
show_values_on_series?: TrendsFilterLegacy['show_values_on_series']
show_labels_on_series?: TrendsFilterLegacy['show_labels_on_series']
show_percent_stack_view?: TrendsFilterLegacy['show_percent_stack_view']
showLabelsOnSeries?: TrendsFilterLegacy['show_labels_on_series']
showPercentStackView?: TrendsFilterLegacy['show_percent_stack_view']
hidden_legend_indexes?: TrendsFilterLegacy['hidden_legend_indexes']
}

Expand Down
2 changes: 1 addition & 1 deletion frontend/src/scenes/funnels/FunnelLineGraph.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ export function FunnelLineGraph({
return `${count}%`
},
}}
trendsFilter={{ aggregation_axis_format: 'percentage' } as TrendsFilter}
trendsFilter={{ aggregationAxisFormat: 'percentage' } as TrendsFilter}
labelGroupType={aggregationGroupTypeIndex ?? 'people'}
incompletenessOffsetFromEnd={incompletenessOffsetFromEnd}
onClick={
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/scenes/insights/EditorFilters/PercentStackViewFilter.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export function PercentStackViewFilter(): JSX.Element {
className="p-1 px-2"
checked={!!showPercentStackView}
onChange={(checked) => {
updateInsightFilter({ show_percent_stack_view: checked })
updateInsightFilter({ showPercentStackView: checked })
}}
label={<span className="font-normal">Show as % of total</span>}
size="small"
Expand Down
23 changes: 17 additions & 6 deletions frontend/src/scenes/insights/aggregationAxisFormat.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,27 @@ export const INSIGHT_UNIT_OPTIONS: LemonSelectOptionLeaf<AggregationAxisFormat>[
{ value: 'percentage_scaled', label: 'Percent (0-1)' },
]

// this function needs to support a trendsFilter as part of an insight query and
// legacy trend filters, as we still return these as part of a data response
export const formatAggregationAxisValue = (
trendsFilter: TrendsFilter | null | undefined | Partial<TrendsFilterType>,
value: number | string
): string => {
value = Number(value)
let formattedValue = humanFriendlyNumber(value, trendsFilter?.decimal_places)
if (trendsFilter?.aggregation_axis_format) {
switch (trendsFilter?.aggregation_axis_format) {
const decimalPlaces =
(trendsFilter as TrendsFilter)?.decimalPlaces || (trendsFilter as Partial<TrendsFilterType>)?.decimal_places
const aggregationAxisFormat =
(trendsFilter as TrendsFilter)?.aggregationAxisFormat ||
(trendsFilter as Partial<TrendsFilterType>)?.aggregation_axis_format
const aggregationAxisPrefix =
(trendsFilter as TrendsFilter)?.aggregationAxisPrefix ||
(trendsFilter as Partial<TrendsFilterType>)?.aggregation_axis_prefix
const aggregationAxisPostfix =
(trendsFilter as TrendsFilter)?.aggregationAxisPostfix ||
(trendsFilter as Partial<TrendsFilterType>)?.aggregation_axis_postfix
let formattedValue = humanFriendlyNumber(value, decimalPlaces)
if (aggregationAxisFormat) {
switch (aggregationAxisFormat) {
case 'duration':
formattedValue = humanFriendlyDuration(value)
break
Expand All @@ -40,9 +53,7 @@ export const formatAggregationAxisValue = (
break
}
}
return `${trendsFilter?.aggregation_axis_prefix || ''}${formattedValue}${
trendsFilter?.aggregation_axis_postfix || ''
}`
return `${aggregationAxisPrefix || ''}${formattedValue}${aggregationAxisPostfix || ''}`
}

export const formatPercentStackAxisValue = (
Expand Down
10 changes: 10 additions & 0 deletions frontend/src/scenes/insights/aggregationAxisFormats.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,19 @@ describe('formatAggregationAxisValue', () => {
},
expected: '£3,940💖',
},
{
candidate: 3940,
filters: {
aggregationAxisFormat: 'numeric',
aggregationAxisPrefix: '£',
aggregationAxisPostfix: '💖',
},
expected: '£3,940💖',
},
{ candidate: 0.8709423, filters: {}, expected: '0.87' },
{ candidate: 0.8709423, filters: { decimal_places: 2 }, expected: '0.87' },
{ candidate: 0.8709423, filters: { decimal_places: 3 }, expected: '0.871' },
{ candidate: 0.8709423, filters: { decimalPlaces: 3 }, expected: '0.871' },
{ candidate: 0.8709423, filters: { decimal_places: 9 }, expected: '0.8709423' },
{ candidate: 0.8709423, filters: { decimal_places: -1 }, expected: '0.87' }, // Fall back to default for unsupported values
]
Expand Down
4 changes: 2 additions & 2 deletions frontend/src/scenes/insights/utils.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -181,15 +181,15 @@ describe('formatAggregationValue', () => {

it('uses render count when there is a value and property format is a no-op', () => {
const fakeRenderCount = (x: number): string =>
formatAggregationAxisValue({ aggregation_axis_format: 'duration' }, x)
formatAggregationAxisValue({ aggregationAxisFormat: 'duration' }, x)
const noOpFormatProperty = jest.fn((_, y) => y)
const actual = formatAggregationValue('some name', 500, fakeRenderCount, noOpFormatProperty)
expect(actual).toEqual('8m 20s')
})

it('uses render count when there is a value and property format converts number to string', () => {
const fakeRenderCount = (x: number): string =>
formatAggregationAxisValue({ aggregation_axis_format: 'duration' }, x)
formatAggregationAxisValue({ aggregationAxisFormat: 'duration' }, x)
const noOpFormatProperty = jest.fn((_, y) => String(y))
const actual = formatAggregationValue('some name', 500, fakeRenderCount, noOpFormatProperty)
expect(actual).toEqual('8m 20s')
Expand Down
Loading

0 comments on commit 9700a2b

Please sign in to comment.