Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: alert decrease is not calculated correctly #5419

Merged
merged 1 commit into from
Oct 2, 2024
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
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ describe("alert transforms", () => {
},
},
},
isPrimary: true,
comparators: [],
};

Expand All @@ -128,9 +129,49 @@ describe("alert transforms", () => {
},
},
},
isPrimary: true,
comparators: [
{
comparator: AlertMetricComparatorType.PreviousPeriod,
isPrimary: false,
measure: {
measure: {
localIdentifier: "localMetric_pp_1",
title: "metric_pp_1",
definition: {
previousPeriodMeasure: {
measureIdentifier: "localMetric2",
dateDataSets: [{ dataSet: { uri: "dateDataSetUri" }, periodsAgo: 1 }],
},
},
},
},
},
],
};

const previousPeriodMetric1: AlertMetric = {
measure: {
measure: {
localIdentifier: "localMetric2",
title: "metric2",
format: "#,##0.00",
definition: {
measureDefinition: {
filters: [],
item: {
type: "measure",
identifier: "simple_metric_2",
},
},
},
},
},
isPrimary: false,
comparators: [
{
comparator: AlertMetricComparatorType.PreviousPeriod,
isPrimary: true,
measure: {
measure: {
localIdentifier: "localMetric_pp_1",
Expand Down Expand Up @@ -248,6 +289,37 @@ describe("alert transforms", () => {
},
});
});

it("transformAlertByMetric, relative and provide comparison metric, relative isPrimary", () => {
const res = transformAlertByMetric(baseRelative, previousPeriodMetric1);
expect(res).toEqual({
...baseRelative,
title: "metric2",
alert: {
...baseRelative.alert,
condition: {
...baseRelative.alert.condition,
measure: {
operator: "CHANGE",
left: {
format: "#,##0.00",
id: "localMetric2",
title: "metric2",
},
right: {
format: "#,##0.00",
id: "localMetric_pp_1",
title: "metric_pp_1",
},
},
},
execution: {
...baseRelative.alert.execution,
measures: [previousPeriodMetric.comparators[0].measure, previousPeriodMetric.measure],
},
},
});
});
});

describe("transformAlertByValue", () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -164,63 +164,87 @@ type InsightType =
export const getSupportedInsightMeasuresByInsight = (insight: IInsight | null | undefined): AlertMetric[] => {
const insightType = insight ? (insightVisualizationType(insight) as InsightType) : null;

const allMetrics = collectAllMetric(insight);

const simpleMetrics = allMetrics
.map<AlertMetric | undefined>((measure) => {
if (isSimpleMeasure(measure) || isArithmeticMeasure(measure)) {
return {
measure,
comparators: [],
};
}
return undefined;
})
.filter(Boolean) as AlertMetric[];
const { primaries, others } = collectAllMetric(insight);

const simpleMetrics = [
...(primaries.map<AlertMetric | undefined>(transformMeasure(true)).filter(Boolean) as AlertMetric[]),
...(others.map<AlertMetric | undefined>(transformMeasure(false)).filter(Boolean) as AlertMetric[]),
];

//NOTE: For now only headline insight support previous period and same period previous year,
// if we want to support other insight types, just add the logic here or remove the condition at
// all to support all insight types
if (insightType === "headline") {
const previousPeriodMetrics = allMetrics.filter((measure) =>
isPreviousPeriodMeasure(measure),
) as IMeasure<IPreviousPeriodMeasureDefinition>[];
previousPeriodMetrics.forEach((measure) => {
const found = simpleMetrics.find(
(simpleMetric) =>
simpleMetric.measure.measure.localIdentifier ===
measure.measure.definition.previousPeriodMeasure.measureIdentifier,
);
if (found) {
found.comparators.push({
measure,
comparator: AlertMetricComparatorType.PreviousPeriod,
});
}
});

const popMetrics = allMetrics.filter((measure) =>
isPoPMeasure(measure),
) as IMeasure<IPoPMeasureDefinition>[];
popMetrics.forEach((measure) => {
const found = simpleMetrics.find(
(simpleMetric) =>
simpleMetric.measure.measure.localIdentifier ===
measure.measure.definition.popMeasureDefinition.measureIdentifier,
);
if (found) {
found.comparators.push({
measure,
comparator: AlertMetricComparatorType.SamePeriodPreviousYear,
});
}
});
transformPreviousPeriodMeasure(primaries, simpleMetrics, true);
transformPreviousPeriodMeasure(others, simpleMetrics, false);
transformPoPMeasure(primaries, simpleMetrics, true);
transformPoPMeasure(others, simpleMetrics, false);
}

return simpleMetrics;
};

function collectAllMetric(insight: IInsight | null | undefined) {
function transformMeasure(isPrimary: boolean) {
return (measure: IMeasure) => {
if (isSimpleMeasure(measure) || isArithmeticMeasure(measure)) {
return {
measure,
isPrimary,
comparators: [],
};
}
return undefined;
};
}

function transformPreviousPeriodMeasure(
metrics: IMeasure[],
simpleMetrics: AlertMetric[],
isPrimary: boolean,
) {
const previousPeriodMetrics = metrics.filter((measure) =>
isPreviousPeriodMeasure(measure),
) as IMeasure<IPreviousPeriodMeasureDefinition>[];
previousPeriodMetrics.forEach((measure) => {
const found = simpleMetrics.find(
(simpleMetric) =>
simpleMetric.measure.measure.localIdentifier ===
measure.measure.definition.previousPeriodMeasure.measureIdentifier,
);
if (found) {
found.comparators.push({
measure,
isPrimary,
comparator: AlertMetricComparatorType.PreviousPeriod,
});
}
});
}

function transformPoPMeasure(metrics: IMeasure[], simpleMetrics: AlertMetric[], isPrimary: boolean) {
const popMetrics = metrics.filter((measure) =>
isPoPMeasure(measure),
) as IMeasure<IPoPMeasureDefinition>[];
popMetrics.forEach((measure) => {
const found = simpleMetrics.find(
(simpleMetric) =>
simpleMetric.measure.measure.localIdentifier ===
measure.measure.definition.popMeasureDefinition.measureIdentifier,
);
if (found) {
found.comparators.push({
measure,
isPrimary,
comparator: AlertMetricComparatorType.SamePeriodPreviousYear,
});
}
});
}

function collectAllMetric(insight: IInsight | null | undefined): {
primaries: IMeasure[];
others: IMeasure[];
} {
const insightType = insight ? (insightVisualizationType(insight) as InsightType) : null;

switch (insightType) {
Expand All @@ -241,18 +265,24 @@ function collectAllMetric(insight: IInsight | null | undefined) {
const insightTertiaryMeasuresBucket: IBucket | undefined = insight
? insightBucket(insight, BucketNames.TERTIARY_MEASURES)
: undefined;
return [
...(insightMeasuresBucket ? bucketMeasures(insightMeasuresBucket) : []),
...(insightSecondaryMeasuresBucket ? bucketMeasures(insightSecondaryMeasuresBucket) : []),
...(insightTertiaryMeasuresBucket ? bucketMeasures(insightTertiaryMeasuresBucket) : []),
];

return {
primaries: insightMeasuresBucket ? bucketMeasures(insightMeasuresBucket) : [],
others: [
...(insightSecondaryMeasuresBucket ? bucketMeasures(insightSecondaryMeasuresBucket) : []),
...(insightTertiaryMeasuresBucket ? bucketMeasures(insightTertiaryMeasuresBucket) : []),
],
};
}
case "repeater": {
const insightColumnsBucket: IBucket | undefined = insight
? insightBucket(insight, BucketNames.COLUMNS)
: undefined;

return insightColumnsBucket ? bucketMeasures(insightColumnsBucket) : [];
return {
primaries: insightColumnsBucket ? bucketMeasures(insightColumnsBucket) : [],
others: [],
};
}
case "donut":
case "treemap":
Expand All @@ -267,7 +297,10 @@ function collectAllMetric(insight: IInsight | null | undefined) {
case "pyramid":
case "waterfall":
default: {
return [];
return {
primaries: [],
others: [],
};
}
}
}
Expand Down Expand Up @@ -552,6 +585,14 @@ function transformAlertExecutionByMetric(
);

if (condition.type === "relative" && periodMeasure) {
// if the period measure is primary, it should be the first measure in the execution
if (periodMeasure.isPrimary) {
return {
...execution,
measures: [periodMeasure.measure, measure.measure],
};
}

return {
...execution,
measures: [measure.measure, periodMeasure.measure],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,7 @@ export enum AlertMetricComparatorType {
*/
export type AlertMetricComparator = {
measure: IMeasure;
isPrimary: boolean;
comparator: AlertMetricComparatorType;
};

Expand All @@ -267,5 +268,6 @@ export type AlertMetricComparator = {
*/
export type AlertMetric = {
measure: IMeasure;
isPrimary: boolean;
comparators: AlertMetricComparator[];
};