Skip to content

Commit

Permalink
Merge pull request #5419 from gooddata/SHA_master
Browse files Browse the repository at this point in the history 
fix: alert decrease is not calculated correctly
  • Loading branch information
@hackerstanislav
hackerstanislav authored Oct 2, 2024
2 parents fc1287d + d7a5e6a commit fb06cc1
Show file tree
Hide file tree
Showing 3 changed files with 169 additions and 54 deletions.
72 changes: 72 additions & 0 deletions ...d/src/presentation/widget/insight/configuration/InsightAlertConfig/test/transform.test.ts
View file
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
149 changes: 95 additions & 54 deletions ...dk-ui-dashboard/src/presentation/widget/insight/configuration/InsightAlertConfig/utils.ts
View file
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
2 changes: 2 additions & 0 deletions libs/sdk-ui-dashboard/src/presentation/widget/insight/types.ts
View file
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[];
};

0 comments on commit fb06cc1

Please sign in to comment.