Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions assets/js/dashboard/stats/csv-export/csv-export-body.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ import { Metric } from '../metrics'
import {
BREAKDOWN_REPORTS,
BreakdownReportConfig,
BreakdownReportKey
BreakdownReportKey,
getCustomPropsMetrics
} from '../reports/reports-config'

const BREAKDOWN_CSV_REPORTS = {
Expand All @@ -41,9 +42,13 @@ const BREAKDOWN_CSV_REPORTS = {
'utm_terms.csv': BreakdownReportKey.utmTerms,
'countries.csv': BreakdownReportKey.countries,
'regions.csv': BreakdownReportKey.regions,
'cities.csv': BreakdownReportKey.cities
'cities.csv': BreakdownReportKey.cities,
'conversions.csv': BreakdownReportKey.goals
}
type CsvFilename = 'visitors.csv' | keyof typeof BREAKDOWN_CSV_REPORTS
type CsvFilename =
| 'visitors.csv'
| 'custom_props.csv'
| keyof typeof BREAKDOWN_CSV_REPORTS

type CsvReportParams = {
dimensions: [TimeDimension] | BreakdownReportConfig['dimensions']
Expand Down Expand Up @@ -114,6 +119,10 @@ export function createCsvExportRequestBody(
? PAGE_FILTERED_VISITORS_CSV_METRICS
: DEFAULT_VISITORS_CSV_METRICS
},
'custom_props.csv': {
dimensions: ['event:props:*'],
metrics: getCustomPropsMetrics({ hasConversionGoalFilter: isGoalFilter })

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

suggestion, non-blocking: The function getCustomPropsMetrics depends on isRevenueAvailable, which isn't passed.

It seems we actually want to say that in the CSV report, revenue is not available (isRevenueAvailable: false), or do we? With MetricContext allowing undefined, I can't actually tell what the intent is for this CSV report.

I think we shouldn't allow undefined in MetricContext.

Shorter args list at call sites is not worth losing explicitly declared intent at call sites.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It seems we actually want to say that in the CSV report, revenue is not available (isRevenueAvailable: false), or do we? With MetricContext allowing undefined, I can't actually tell what the intent is for this CSV report.

Yeah, the intent is that revenue is not available. No specific reason other than sticking to legacy behaviour.

I think we shouldn't allow undefined in MetricContext.

IMHO it doesn't (currently) hurt readability because the behaviour between undefined and false is equivalent. Everything in the MetricContext that is irrelevant from the caller's perspective can simply be left unspecified and get interpreted as false, which, at least to me, is quite intuitive.

This would change with the addition of a context variable that would have a default value of true, but that's not the case, and I doubt it ever will be?

That said though, I've written out isRevenueAvailable: false to make this specific case explicit. 1632261.

},
...Object.entries(BREAKDOWN_CSV_REPORTS).reduce(
(acc, [filename, reportKey]) => {
const config = BREAKDOWN_REPORTS[reportKey]
Expand All @@ -128,7 +137,7 @@ export function createCsvExportRequestBody(
}
return acc
},
{} as Omit<CsvReportsConfig, 'visitors.csv'>
{} as Omit<CsvReportsConfig, 'visitors.csv' | 'custom_props.csv'>
)
}

Expand Down
35 changes: 20 additions & 15 deletions assets/js/dashboard/stats/reports/reports-config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -335,6 +335,9 @@ export const BREAKDOWN_REPORTS: Record<
[BreakdownReportKey.goals]: {
dimensions: ['event:goal'],
getMetrics: (ctx: MetricContext) => {
if (ctx.isCsv) {
return ['visitors', 'events']
}
if (ctx.isRevenueAvailable) {
return [
'visitors',
Expand All @@ -357,23 +360,25 @@ export function customPropsReportConfig(
): BreakdownReportConfig {
return {
dimensions: [`event:props:${propKey}` as NonTimeDimension],
getMetrics: (ctx: MetricContext) => {
if (ctx.hasConversionGoalFilter && ctx.isRevenueAvailable) {
return [
'visitors',
'events',
'conversion_rate',
'total_revenue',
'average_revenue'
]
}
if (ctx.hasConversionGoalFilter) {
return ['visitors', 'events', 'conversion_rate']
}
return ['visitors', 'events', 'percentage']
},
getMetrics: getCustomPropsMetrics,
detailsTitle: 'Custom property breakdown',
detailsPath: `custom-prop-values/${propKey}`,
dimensionLabel: propKey
}
}

export function getCustomPropsMetrics(ctx: MetricContext): Metric[] {
if (ctx.hasConversionGoalFilter && ctx.isRevenueAvailable) {
return [
'visitors',
'events',
'conversion_rate',
'total_revenue',
'average_revenue'
]
}
if (ctx.hasConversionGoalFilter) {
return ['visitors', 'events', 'conversion_rate']
}
return ['visitors', 'events', 'percentage']
}
69 changes: 62 additions & 7 deletions e2e/tests/dashboard/csv-export.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,21 @@ const EXPECTED_HEADERS = {
commonGoalFilteredBreakdown: ['name', 'conversions', 'conversion_rate'],
nameAndVisitors: ['name', 'visitors'],
nameVersionAndVisitors: ['name', 'version', 'visitors'],
nameVersionGoalFiltered: ['name', 'version', 'conversions', 'conversion_rate']
nameVersionGoalFiltered: [
'name',
'version',
'conversions',
'conversion_rate'
],
customPropsDefault: ['property', 'value', 'visitors', 'events', 'percentage'],
customPropsGoalFiltered: [
'property',
'value',
'visitors',
'events',
'conversion_rate'
],
conversionsBreakdown: ['name', 'unique_conversions', 'total_conversions']
}

const UTM_AND_SOURCE_REPORTS = [
Expand Down Expand Up @@ -119,11 +133,14 @@ test('csv export column headers match expected metrics for each report', async (
await populateStats({
request,
domain,
events: [{ name: 'pageview', timestamp: '2021-01-01 12:00:00' }]
events: [
{ name: 'pageview', 'meta.key': ['author'], 'meta.value': ['john'] },
{ name: 'Signup', 'meta.key': ['author'], 'meta.value': ['john'] }
]
})

await test.step('without any filters', async () => {
await page.goto(`/${domain}`, {
await page.goto(`/${domain}?period=all`, {
waitUntil: 'commit'
})

Expand Down Expand Up @@ -157,13 +174,23 @@ test('csv export column headers match expected metrics for each report', async (
EXPECTED_HEADERS.nameVersionAndVisitors
)
}
expect(getCsv(csvs, 'custom_props.csv')[0]).toEqual(
EXPECTED_HEADERS.customPropsDefault
)
expect(getCsv(csvs, 'conversions.csv')[0]).toEqual(
EXPECTED_HEADERS.conversionsBreakdown
)
})

await test.step('with a page filter', async () => {
await page.goto(`/${domain}?f=is,page,/blog`, {
await page.goto(`/${domain}?period=all&f=is,page,/`, {
waitUntil: 'commit'
})

await expect(
page.getByRole('button', { name: 'Remove filter: Page is /' })
).toBeVisible()

const csvs = parseAllCsvs(await triggerExportAndAwaitDownload(page))

expect(getCsv(csvs, 'visitors.csv')[0]).toEqual(
Expand Down Expand Up @@ -194,15 +221,25 @@ test('csv export column headers match expected metrics for each report', async (
EXPECTED_HEADERS.nameVersionAndVisitors
)
}
expect(getCsv(csvs, 'custom_props.csv')[0]).toEqual(
EXPECTED_HEADERS.customPropsDefault
)
expect(getCsv(csvs, 'conversions.csv')[0]).toEqual(
EXPECTED_HEADERS.conversionsBreakdown
)
})

await test.step('with a goal filter', async () => {
await addGoal({ request, domain, params: { event_name: 'Signup' } })

await page.goto(`/${domain}?f=is,goal,Signup`, {
await page.goto(`/${domain}?period=all&f=is,goal,Signup`, {
waitUntil: 'commit'
})

await expect(
page.getByRole('button', { name: 'Remove filter: Goal is Signup' })
).toBeVisible()

const csvs = parseAllCsvs(await triggerExportAndAwaitDownload(page))

expect(getCsv(csvs, 'visitors.csv')[0]).toEqual(
Expand All @@ -225,13 +262,25 @@ test('csv export column headers match expected metrics for each report', async (
EXPECTED_HEADERS.nameVersionGoalFiltered
)
}
expect(getCsv(csvs, 'custom_props.csv')[0]).toEqual(
EXPECTED_HEADERS.customPropsGoalFiltered
)
expect(getCsv(csvs, 'conversions.csv')[0]).toEqual(
EXPECTED_HEADERS.conversionsBreakdown
)
})

await test.step('with a custom prop filter', async () => {
await page.goto(`/${domain}?f=is,props:author,john`, {
await page.goto(`/${domain}?period=all&f=is,props:author,john`, {
waitUntil: 'commit'
})

await expect(
page.getByRole('button', {
name: 'Remove filter: Property author is john'
})
).toBeVisible()

const csvs = parseAllCsvs(await triggerExportAndAwaitDownload(page))

expect(getCsv(csvs, 'visitors.csv')[0]).toEqual(
Expand Down Expand Up @@ -262,6 +311,12 @@ test('csv export column headers match expected metrics for each report', async (
EXPECTED_HEADERS.nameVersionAndVisitors
)
}
expect(getCsv(csvs, 'custom_props.csv')[0]).toEqual(
EXPECTED_HEADERS.customPropsDefault
)
expect(getCsv(csvs, 'conversions.csv')[0]).toEqual(
EXPECTED_HEADERS.conversionsBreakdown
)
})
})

Expand Down Expand Up @@ -309,7 +364,7 @@ test('filters out empty visit:* dimension values', async ({
]
})

await page.goto(`/${domain}?period=day&date=2021-01-01`, {
await page.goto(`/${domain}?period=all`, {
waitUntil: 'commit'
})

Expand Down
40 changes: 25 additions & 15 deletions lib/plausible/stats/api_query_parser.ex
Original file line number Diff line number Diff line change
Expand Up @@ -250,8 +250,8 @@ defmodule Plausible.Stats.ApiQueryParser do

defp parse_dimension_entry(key, error_message) do
case {
parse_time(key),
parse_filter_dimension_string(key)
parse_time_dimension(key),
parse_non_time_dimension(key)
} do
{{:ok, time}, _} -> {:ok, time}
{_, {:ok, dimension}} -> {:ok, dimension}
Expand All @@ -261,32 +261,42 @@ defmodule Plausible.Stats.ApiQueryParser do

defp parse_metric_or_dimension([value, _] = entry) do
case {
parse_time(value),
parse_time_dimension(value),
parse_metric(value),
parse_filter_dimension_string(value)
parse_non_time_dimension(value)
} do
{{:ok, time}, _, _} ->
{:ok, time}
{{:ok, time_dimension}, _, _} ->
{:ok, time_dimension}

{_, {:ok, metric}, _} ->
{:ok, metric}

{_, _, {:ok, dimension}} ->
{:ok, dimension}
{_, _, {:ok, non_time_dimension}} ->
{:ok, non_time_dimension}

_ ->
{:error,
%QueryError{code: :invalid_order_by, message: "Invalid order_by entry '#{i(entry)}'."}}
end
end

defp parse_time("time"), do: {:ok, "time"}
defp parse_time("time:minute"), do: {:ok, "time:minute"}
defp parse_time("time:hour"), do: {:ok, "time:hour"}
defp parse_time("time:day"), do: {:ok, "time:day"}
defp parse_time("time:week"), do: {:ok, "time:week"}
defp parse_time("time:month"), do: {:ok, "time:month"}
defp parse_time(_), do: :error
defp parse_time_dimension("time"), do: {:ok, "time"}
defp parse_time_dimension("time:minute"), do: {:ok, "time:minute"}
defp parse_time_dimension("time:hour"), do: {:ok, "time:hour"}
defp parse_time_dimension("time:day"), do: {:ok, "time:day"}
defp parse_time_dimension("time:week"), do: {:ok, "time:week"}
defp parse_time_dimension("time:month"), do: {:ok, "time:month"}
defp parse_time_dimension(_), do: :error

@breakdown_only_non_time_dimensions ["event:prop_key"]

defp parse_non_time_dimension(dimension) do
if dimension in @breakdown_only_non_time_dimensions do
{:ok, dimension}
else
parse_filter_dimension_string(dimension)
end
end

defp parse_order_direction([_, "asc"]), do: {:ok, :asc}
defp parse_order_direction([_, "desc"]), do: {:ok, :desc}
Expand Down
Loading
Loading