Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
13 changes: 13 additions & 0 deletions migrations/1772117945260_data_source_null_is_zero.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
/* eslint-disable @typescript-eslint/no-explicit-any */
import type { Kysely } from "kysely";

export async function up(db: Kysely<any>): Promise<void> {
await db.schema
.alterTable("dataSource")
.addColumn("nullIsZero", "boolean", (col) => col.defaultTo(false))
.execute();
}

export async function down(db: Kysely<any>): Promise<void> {
await db.schema.alterTable("dataSource").dropColumn("nullIsZero").execute();
}
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ export default function ConfigurationForm({
const [isPublic, setIsPublic] = useState(dataSource.public);

const [naIsNull, setNaIsNull] = useState(dataSource.naIsNull || false);
const [nullIsZero, setNullIsZero] = useState(dataSource.nullIsZero || false);

// Form state
const [loading, setLoading] = useState(false);
Expand All @@ -69,12 +70,26 @@ export default function ConfigurationForm({
// Notify parent of autoImport status change
onAutoImportChange?.(autoImport);

// Invalidate webhook status to refetch and show/hide errors
await queryClient.invalidateQueries({
queryKey: trpc.dataSource.checkWebhookStatus.queryKey({
dataSourceId: dataSource.id,
// Invalidate cached data source queries and derived stats so
// map rendering and other parts of the app reflect the new config
await Promise.all([
queryClient.invalidateQueries({
queryKey: trpc.dataSource.checkWebhookStatus.queryKey({
dataSourceId: dataSource.id,
}),
}),
});
queryClient.invalidateQueries({
queryKey: trpc.dataSource.byId.queryKey({
dataSourceId: dataSource.id,
}),
}),
queryClient.invalidateQueries({
queryKey: trpc.dataSource.listReadable.queryKey(),
}),
queryClient.invalidateQueries({
queryKey: trpc.area.stats.queryKey(),
}),
]);

if (redirectToParent) {
router.push(`/data-sources/${dataSource.id}`);
Expand Down Expand Up @@ -111,6 +126,7 @@ export default function ConfigurationForm({
dateFormat,
public: isPublic,
naIsNull,
nullIsZero,
});
};

Expand Down Expand Up @@ -193,6 +209,17 @@ export default function ConfigurationForm({
<Switch checked={naIsNull} onCheckedChange={(v) => setNaIsNull(v)} />
</FormFieldWrapper>

<FormFieldWrapper
label="Treat empty values as zero"
hint="Switch this on if numeric columns in your data source are blank when the value is zero."
isHorizontal
>
<Switch
checked={nullIsZero}
onCheckedChange={(v) => setNullIsZero(v)}
/>
</FormFieldWrapper>

<FormFieldWrapper
label="Share this data with the Movement Data Library"
hint="Warning: this will make the data publicly available."
Expand Down
50 changes: 38 additions & 12 deletions src/app/map/[id]/components/Legend.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,11 @@ export default function Legend() {
.filter((v) => v && v !== "null" && v !== "undefined"),
);

const valueLabels =
dataSource?.columnMetadata.find(
(c) => c.name === viewConfig.areaDataColumn,
)?.valueLabels || {};

return (
<div className="flex flex-col gap-1.5 w-full py-1">
{Object.keys(colorScheme.colorMap)
Expand All @@ -155,15 +160,25 @@ export default function Legend() {
}
return a < b ? -1 : 1;
})
.map((key) => (
<div className="flex items-center gap-2 text-xs" key={key}>
<div
className="w-3 h-3 flex-shrink-0 border border-neutral-300"
style={{ backgroundColor: colorScheme.colorMap[key] }}
/>
<span>{key}</span>
</div>
))}
.map((key) => {
let label = valueLabels[key];
if (
!label &&
areaStats?.primary?.columnType === ColumnType.Number &&
Number(key) === 0
) {
label = valueLabels[""];
}
return (
<div className="flex items-center gap-2 text-xs" key={key}>
<div
className="w-3 h-3 flex-shrink-0 border border-neutral-300"
style={{ backgroundColor: colorScheme.colorMap[key] }}
/>
<span>{label || key}</span>
</div>
);
})}
<div className="flex items-center gap-2 text-xs">
<div
className="w-3 h-3 flex-shrink-0 border border-neutral-300"
Expand Down Expand Up @@ -293,9 +308,20 @@ export default function Legend() {
const hasValueLabels = Object.keys(valueLabels).length > 0;

if (hasValueLabels) {
numTicks = Object.keys(valueLabels).length;
denom = Math.max(numTicks - 1, 1);
values = Object.keys(valueLabels).map(Number).toSorted();
const numericKeys = Object.keys(valueLabels)
.map((key) => Number(key))
.filter(
(v) =>
Number.isFinite(v) &&
v >= colorScheme.minValue &&
v <= colorScheme.maxValue,
)
.toSorted((a, b) => a - b);
if (numericKeys.length) {
values = numericKeys;
numTicks = values.length;
denom = Math.max(numTicks - 1, 1);
}
}
Comment on lines 310 to 325
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

When hasValueLabels is true, keys are filtered to those within [minValue, maxValue]. If none of the configured labels fall in-range, this sets numTicks to 0 and renders no tick labels (but still reserves the taller h-10 layout). Consider falling back to the default tick generation when the filtered list is empty (or at least keeping a minimum of 2 ticks).

Copilot uses AI. Check for mistakes.

return (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -168,8 +168,6 @@ export default function VisualisationPanel({
dataSource?.geocodingConfig,
viewConfig.areaSetGroupCode,
);
const showEmptyZeroSwitch = !isCount && columnOneIsNumber;

const showStyle = !viewConfig.areaDataSecondaryColumn;
const canSelectColorScale = isCount || columnOneIsNumber;
const canSelectColorScheme = canSelectColorScale && !isCategorical;
Expand Down Expand Up @@ -443,25 +441,6 @@ export default function VisualisationPanel({
</Select>
</>
)}

{showEmptyZeroSwitch && (
<div className="col-span-2 flex items-center gap-2">
<Label
htmlFor="choropleth-empty-zero-switch"
className="text-sm text-muted-foreground font-normal"
>
Treat empty values as zero
</Label>

<Switch
id="choropleth-empty-zero-switch"
checked={Boolean(viewConfig.areaDataNullIsZero)}
onCheckedChange={(v) =>
updateViewConfig({ areaDataNullIsZero: v })
}
/>
</div>
)}
</div>
{!viewConfig.areaDataSourceId && (
<div className="flex items-center gap-2">
Expand Down Expand Up @@ -532,7 +511,7 @@ export default function VisualisationPanel({
}
>
<SelectTrigger
className="w-full min-w-0"
className={cn("w-full min-w-0", SELECT_TO_BUTTON_CLASSES)}
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

The constant SELECT_TO_BUTTON_CLASSES is referenced but not visible in the diff. Ensure this constant is properly imported or defined in this file.

Copilot uses AI. Check for mistakes.
id="color-scale-type-select"
>
<SelectValue placeholder="Choose color scale...">
Expand Down
3 changes: 0 additions & 3 deletions src/app/map/[id]/data.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ export const useAreaStats = () => {
areaDataColumn: column,
areaDataSecondaryColumn: secondaryColumn,
areaDataSourceId: dataSourceId,
areaDataNullIsZero: nullIsZero,
areaSetGroupCode,
} = viewConfig;

Expand Down Expand Up @@ -118,7 +117,6 @@ export const useAreaStats = () => {
dataSourceId,
column: columnOrCount,
secondaryColumn: secondaryColumn,
nullIsZero,
includeColumns,
boundingBox: requiresBoundingBox ? boundingBox : null,
},
Expand All @@ -134,7 +132,6 @@ export const useAreaStats = () => {
dataSourceId,
column,
secondaryColumn,
nullIsZero,
includeColumns,
]);

Expand Down
1 change: 0 additions & 1 deletion src/app/map/[id]/utils/mapView.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ export const createNewViewConfig = (): MapViewConfig => {
return {
areaDataSourceId: "",
areaDataColumn: "",
areaDataNullIsZero: true,
areaSetGroupCode: undefined,
mapStyleName: MapStyleName.Light,
showLabels: true,
Expand Down
1 change: 1 addition & 0 deletions src/server/models/DataSource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,7 @@ export const dataSourceSchema = z.object({
createdAt: z.date(),
dateFormat: z.string(),
naIsNull: z.boolean().optional(),
nullIsZero: z.boolean().optional(),
});

export type DataSource = z.infer<typeof dataSourceSchema>;
Expand Down
1 change: 0 additions & 1 deletion src/server/models/MapView.ts
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,6 @@ export const mapViewConfigSchema = z.object({
areaDataSourceId: z.string(),
areaDataColumn: z.string(),
areaDataSecondaryColumn: z.string().optional(),
areaDataNullIsZero: z.boolean().optional(),
areaSetGroupCode: areaSetGroupCode.nullish(),
secondaryAreaSetCode: areaSetCode.nullish(),
choroplethOpacityPct: z.number().optional(),
Expand Down
21 changes: 12 additions & 9 deletions src/server/stats/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ export const getAreaStats = async ({
calculationType,
column,
secondaryColumn,
nullIsZero,
includeColumns,
boundingBox = null,
}: {
Expand All @@ -54,7 +53,6 @@ export const getAreaStats = async ({
calculationType: CalculationType;
column: string;
secondaryColumn?: string;
nullIsZero?: boolean;
includeColumns?: string[] | null;
boundingBox?: BoundingBox | null;
}): Promise<AreaStats> => {
Expand Down Expand Up @@ -110,7 +108,6 @@ export const getAreaStats = async ({
areaSetCode,
calculationType,
column,
nullIsZero,
boundingBox,
});
const valueRange = getValueRange(primaryStats.stats);
Expand All @@ -128,7 +125,6 @@ export const getAreaStats = async ({
areaSetCode,
calculationType,
column: secondaryColumn,
nullIsZero,
boundingBox,
});
const secondaryValueRange = getValueRange(secondaryStats.stats);
Expand Down Expand Up @@ -225,6 +221,7 @@ export const getMaxColumnByArea = async (
${caseWhen.end()} AS max_column
FROM data_record
WHERE data_source_id = ${dataSourceId}
AND geocode_result->'areas'->>${areaSetCode} IS NOT NULL
AND ${getBoundingBoxSQL(boundingBox)}
AND ${hasNonNullValueCondition}
AND ${caseWhen.end()} IS NOT NULL
Expand Down Expand Up @@ -254,32 +251,36 @@ const getColumnValueByArea = async ({
areaSetCode,
calculationType,
column,
nullIsZero,
boundingBox,
}: {
dataSource: DataSource;
areaSetCode: AreaSetCode;
calculationType: CalculationType;
column: string;
nullIsZero: boolean | undefined;
boundingBox: BoundingBox | null;
}) => {
const columnDef = dataSource.columnDefs.find((c) => c.name === column);
if (!columnDef) {
throw new Error(`Data source column not found: ${column}`);
}

// Coalesce empty values to 0 if set
const numberSelect = nullIsZero
// Coalesce numeric empty values to 0 if set
const numberSelect = dataSource.nullIsZero
? sql`(COALESCE(NULLIF(json->>${column}, ''), '0'))::float`
: sql`(NULLIF(json->>${column}, ''))::float`;
Comment on lines +267 to 270
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

getColumnValueByArea now changes numeric aggregation semantics based on dataSource.nullIsZero (coalescing empty strings to 0). There are existing Vitest feature tests that exercise getAreaStats, but none cover the new nullIsZero behavior; please add a test case that imports a CSV with blank numeric values and asserts that Sum/Avg differ when nullIsZero is enabled vs disabled.

Copilot uses AI. Check for mistakes.

// Coalesce mode empty values to 0 if the column is numeric and nullIsZero is set
const modeSelect =
columnDef.type === ColumnType.Number && dataSource.nullIsZero
? sql`MODE () WITHIN GROUP (ORDER BY COALESCE(NULLIF(json->>${column}, ''), '0'))`
: sql`MODE () WITHIN GROUP (ORDER BY NULLIF(json->>${column}, ''))`;
Comment on lines +272 to +276
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

The new MODE query path also changes behavior when nullIsZero is enabled (and now treats empty strings as NULL when disabled). Please add a focused test for CalculationType.Mode on a numeric column with blank values to lock in the intended behavior for both nullIsZero=true and nullIsZero=false.

Copilot uses AI. Check for mistakes.

// Select is always MODE for ColumnType !== Number
const isMode =
calculationType === CalculationType.Mode ||
columnDef.type !== ColumnType.Number;
const valueSelect = isMode
? sql`MODE () WITHIN GROUP (ORDER BY json->>${column})`.as("value")
? modeSelect.as("value")
: db.fn(calculationType, [numberSelect]).as("value");

const query = db
Expand All @@ -289,6 +290,7 @@ const getColumnValueByArea = async ({
valueSelect,
])
.where("dataRecord.dataSourceId", "=", dataSource.id)
.where(sql<boolean>`geocode_result->'areas'->>${areaSetCode} IS NOT NULL`)
.where(getBoundingBoxSQL(boundingBox))
.groupBy("areaCode");

Expand All @@ -310,6 +312,7 @@ export const getRecordCountByArea = async (
({ fn }) => fn.countAll().as("value"),
])
.where("dataRecord.dataSourceId", "=", dataSourceId)
.where(sql<boolean>`geocode_result->'areas'->>${areaSetCode} IS NOT NULL`)
.where(getBoundingBoxSQL(boundingBox))
.groupBy("areaCode");

Expand Down
1 change: 0 additions & 1 deletion src/server/trpc/routers/area.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ export const areaRouter = router({
calculationType: z.nativeEnum(CalculationType),
column: z.string(),
secondaryColumn: z.string().optional(),
nullIsZero: z.boolean().optional(),
includeColumns: z.array(z.string()).optional().nullable(),
boundingBox: boundingBoxSchema.nullish(),
}),
Expand Down
1 change: 1 addition & 0 deletions src/server/trpc/routers/dataSource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,7 @@ export const dataSourceRouter = router({
dateFormat: input.dateFormat,
public: input.public,
naIsNull: input.naIsNull,
nullIsZero: input.nullIsZero,
} as DataSourceUpdate;
Comment on lines 239 to 243
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

The new nullIsZero flag is persisted via updateConfig, but the mutation handler only enqueues an importDataSource job when certain config fields change. If naIsNull (and possibly nullIsZero, depending on intent) should take effect on existing imported records, it likely needs to be included in the “configFieldsChanged” check; otherwise the UI’s “Save and import” flow won’t actually import when toggling these flags.

Copilot uses AI. Check for mistakes.

logger.info(
Expand Down
18 changes: 13 additions & 5 deletions tests/feature/importDataSource.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -222,8 +222,12 @@ describe("importDataSource tests", () => {

expect(columnDefs).toEqual([
{ name: "Code", type: ColumnType.String },
{ name: "Con %", type: ColumnType.Number },
{ name: "Electorate", type: ColumnType.Number },
{ name: "Lab %", type: ColumnType.Number },
{ name: "Name", type: ColumnType.String },
{ name: "Segment", type: ColumnType.Number },
{ name: "Segment name", type: ColumnType.String },
]);

const areaStats = await getAreaStats({
Expand All @@ -247,17 +251,21 @@ describe("importDataSource tests", () => {
});

expect(stats).toEqual([
{
areaCode: "E14001070",
value: 71002,
},
{
areaCode: "E14001071",
value: 79169,
},
{
areaCode: "E14001088",
value: 72198,
value: 72199,
},
{
areaCode: "E14001092",
value: 71002,
},
{
areaCode: "E14001225",
value: 72481,
},
]);
});
Expand Down
Loading