Skip to content

Commit 53eb6cd

Browse files
committed
[FIX] CorePlugins: Prevent dispatch during adaptRange
The method `adaptRange` was originally designed for each plugin to update their own ranges, whithout having to manually react to the different commands that alter the sheets structure. We have abused the system when we introduced the spreadsheet pivots to allow the modification of a pivot dataset (which is stored in PivotCorePlugin) from another entry point `SpreadsheetPivotCorePlugin`) by dispatching an UPDATE_PIVOT command but there are some side effects that we did not account for, the dispatched command is forwarded to EVERY other plugin. Meaning that during an adaptRange, which should only concern a specific plugin, we end up notifying a change to the whole core/coreview stack. This revision removes the access to 'dispatch' during the handling of `adaptRange` so we ensure that the changes are kept locally. Task: 5380747 X-original-commit: d17a974
1 parent 6cbf13e commit 53eb6cd

File tree

5 files changed

+383
-61
lines changed

5 files changed

+383
-61
lines changed

src/helpers/pivot/pivot_registry.ts

Lines changed: 42 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { ModelConfig } from "../../model";
22
import { Registry } from "../../registries/registry";
3-
import { CoreGetters, Getters } from "../../types";
3+
import { ApplyRangeChange, CoreGetters, Getters, Range } from "../../types";
44
import { PivotCoreDefinition, PivotField, PivotFields } from "../../types/pivot";
55
import { Pivot } from "../../types/pivot_runtime";
66
import { PivotRuntimeDefinition } from "./pivot_runtime_definition";
@@ -32,6 +32,11 @@ export interface PivotRegistryItem {
3232
datetimeGranularities: string[];
3333
isMeasureCandidate: (field: PivotField) => boolean;
3434
isGroupable: (field: PivotField) => boolean;
35+
adaptRanges?: (
36+
getters: CoreGetters,
37+
definition: PivotCoreDefinition,
38+
applyChange: ApplyRangeChange
39+
) => PivotCoreDefinition;
3540
}
3641

3742
export const pivotRegistry = new Registry<PivotRegistryItem>();
@@ -54,4 +59,40 @@ pivotRegistry.add("SPREADSHEET", {
5459
datetimeGranularities: [...dateGranularities, "hour_number", "minute_number", "second_number"],
5560
isMeasureCandidate: (field: PivotField) => field.type !== "boolean",
5661
isGroupable: () => true,
62+
adaptRanges: (getters, definition, applyChange) => {
63+
if (definition.type !== "SPREADSHEET" || !definition.dataSet) {
64+
return definition;
65+
}
66+
const { sheetId, zone } = definition.dataSet;
67+
const range = getters.getRangeFromZone(sheetId, zone);
68+
const adaptedRange = adaptPivotRange(range, applyChange);
69+
70+
if (adaptedRange === range) {
71+
return definition;
72+
}
73+
74+
const dataSet = adaptedRange && {
75+
sheetId: adaptedRange.sheetId,
76+
zone: adaptedRange.zone,
77+
};
78+
return { ...definition, dataSet };
79+
},
5780
});
81+
82+
function adaptPivotRange(
83+
range: Range | undefined,
84+
applyChange: ApplyRangeChange
85+
): Range | undefined {
86+
if (!range) {
87+
return undefined;
88+
}
89+
const change = applyChange(range);
90+
switch (change.changeType) {
91+
case "NONE":
92+
return range;
93+
case "REMOVE":
94+
return undefined;
95+
default:
96+
return change.range;
97+
}
98+
}

src/plugins/core/pivot.ts

Lines changed: 24 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { compile } from "../../formulas";
22
import { deepCopy, deepEquals } from "../../helpers";
33
import { createPivotFormula, getMaxObjectId } from "../../helpers/pivot/pivot_helpers";
4+
import { pivotRegistry } from "../../helpers/pivot/pivot_registry";
45
import { SpreadsheetPivotTable } from "../../helpers/pivot/table_spreadsheet_pivot";
56
import {
67
ApplyRangeChange,
@@ -145,6 +146,18 @@ export class PivotCorePlugin extends CorePlugin<CoreState> implements CoreState
145146
}
146147

147148
adaptRanges(applyChange: ApplyRangeChange) {
149+
for (const pivotId in this.pivots) {
150+
const definition = deepCopy(this.pivots[pivotId]?.definition);
151+
if (!definition) {
152+
continue;
153+
}
154+
const newDefinition = pivotRegistry
155+
.get(definition.type)
156+
?.adaptRanges?.(this.getters, definition, applyChange);
157+
if (newDefinition && !deepEquals(definition, newDefinition)) {
158+
this.history.update("pivots", pivotId, "definition", newDefinition);
159+
}
160+
}
148161
for (const sheetId in this.compiledMeasureFormulas) {
149162
for (const formulaString in this.compiledMeasureFormulas[sheetId]) {
150163
const compiledFormula = this.compiledMeasureFormulas[sheetId][formulaString];
@@ -332,19 +345,19 @@ export class PivotCorePlugin extends CorePlugin<CoreState> implements CoreState
332345
if (!pivot) {
333346
continue;
334347
}
335-
const def = deepCopy(pivot.definition);
336348

337-
for (const measure of def.measures) {
349+
for (const measure of pivot.definition.measures) {
338350
if (measure.computedBy?.formula === formulaString) {
339-
const measureIndex = def.measures.indexOf(measure);
340-
if (measureIndex !== -1) {
341-
def.measures[measureIndex].computedBy = {
342-
formula: newFormulaString,
343-
sheetId,
344-
};
345-
}
346-
347-
this.dispatch("UPDATE_PIVOT", { pivotId, pivot: def });
351+
const measureIndex = pivot.definition.measures.indexOf(measure);
352+
this.history.update(
353+
"pivots",
354+
pivotId,
355+
"definition",
356+
"measures",
357+
measureIndex,
358+
"computedBy",
359+
{ formula: newFormulaString, sheetId }
360+
);
348361
}
349362
}
350363
}

0 commit comments

Comments
 (0)