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] pivot: add suggestion message to broken pivot formulas #4956

Closed
Closed
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 @@ -2,6 +2,7 @@ import { deepCopy, deepEquals } from "../../../../helpers";
import { isDateField } from "../../../../helpers/pivot/pivot_helpers";
import { pivotRegistry } from "../../../../helpers/pivot/pivot_registry";
import { Get } from "../../../../store_engine";
import { NotificationStore } from "../../../../stores/notification_store";
import { SpreadsheetStore } from "../../../../stores/spreadsheet_store";
import { _t } from "../../../../translation";
import { Command, UID } from "../../../../types";
Expand All @@ -18,6 +19,9 @@ export class PivotSidePanelStore extends SpreadsheetStore {

private updatesAreDeferred: boolean = false;
private draft: PivotCoreDefinition | null = null;
private notification = this.get(NotificationStore);
private alreadyNotified = false;

constructor(get: Get, private pivotId: UID) {
super(get);
}
Expand Down Expand Up @@ -140,6 +144,19 @@ export class PivotSidePanelStore extends SpreadsheetStore {
pivot: this.draft,
});
this.draft = null;
if (!this.alreadyNotified && !this.isDynamicPivotInViewport()) {
const formulaId = this.getters.getPivotFormulaId(this.pivotId);
const pivotExample = `=PIVOT(${formulaId})`;
this.alreadyNotified = true;
this.notification.notifyUser({
type: "info",
text: _t(
"Pivot updates only work with dynamic pivot tables. Use %s or re-insert the static pivot from the Data menu.",
pivotExample
),
sticky: false,
});
}
}
}

Expand Down Expand Up @@ -175,14 +192,23 @@ export class PivotSidePanelStore extends SpreadsheetStore {
this.fields,
cleanedDefinition
);
if (this.updatesAreDeferred) {
this.draft = cleanedWithGranularity;
} else {
this.model.dispatch("UPDATE_PIVOT", {
pivotId: this.pivotId,
pivot: cleanedWithGranularity,
});
this.draft = cleanedWithGranularity;
if (!this.updatesAreDeferred) {
this.applyUpdate();
}
}

private isDynamicPivotInViewport() {
const sheetId = this.getters.getActiveSheetId();
for (const col of this.getters.getSheetViewVisibleCols()) {
for (const row of this.getters.getSheetViewVisibleRows()) {
const isDynamicPivot = this.getters.isSpillPivotFormula({ sheetId, col, row });
if (isDynamicPivot) {
return true;
}
}
}
return false;
}

private addDefaultDateTimeGranularity(fields: PivotFields, definition: PivotCoreDefinition) {
Expand Down
12 changes: 10 additions & 2 deletions src/functions/module_lookup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -728,9 +728,13 @@ export const PIVOT_VALUE = {
}

if (!pivot.areDomainArgsFieldsValid(domainArgs)) {
const suggestion = _t(
"Consider using a dynamic pivot formula: %s. Or re-insert the static pivot from the Data menu.",
`=PIVOT(${_pivotFormulaId})`
);
return {
value: CellErrorType.GenericError,
message: _t("Dimensions don't match the pivot definition"),
message: _t("Dimensions don't match the pivot definition") + ". " + suggestion,
};
}
const domain = pivot.parseArgsToPivotDomain(domainArgs);
Expand Down Expand Up @@ -761,9 +765,13 @@ export const PIVOT_HEADER = {
return error;
}
if (!pivot.areDomainArgsFieldsValid(domainArgs)) {
const suggestion = _t(
"Consider using a dynamic pivot formula: %s. Or re-insert the static pivot from the Data menu.",
`=PIVOT(${_pivotFormulaId})`
);
return {
value: CellErrorType.GenericError,
message: _t("Dimensions don't match the pivot definition"),
message: _t("Dimensions don't match the pivot definition") + ". " + suggestion,
};
}
const domain = pivot.parseArgsToPivotDomain(domainArgs);
Expand Down
14 changes: 7 additions & 7 deletions tests/pivots/spreadsheet_pivot/spreadsheet_pivot.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -932,7 +932,7 @@ describe("Spreadsheet Pivot", () => {
});
setCellContent(model, "A27", '=PIVOT.HEADER(1, "Date:not_a_granularity", 2024)');
expect(getEvaluatedCell(model, "A27").message).toBe(
"Dimensions don't match the pivot definition"
"Dimensions don't match the pivot definition. Consider using a dynamic pivot formula: =PIVOT(1). Or re-insert the static pivot from the Data menu."
);
});

Expand Down Expand Up @@ -966,7 +966,7 @@ describe("Spreadsheet Pivot", () => {
// without granularity
setCellContent(model, "A31", '=PIVOT.HEADER(1, "Date", )');
expect(getEvaluatedCell(model, "A31").message).toBe(
"Dimensions don't match the pivot definition"
"Dimensions don't match the pivot definition. Consider using a dynamic pivot formula: =PIVOT(1). Or re-insert the static pivot from the Data menu."
);

// no a number
Expand Down Expand Up @@ -1009,7 +1009,7 @@ describe("Spreadsheet Pivot", () => {
// without granularity
setCellContent(model, "A31", '=PIVOT.HEADER(1, "Date", )');
expect(getEvaluatedCell(model, "A31").message).toBe(
"Dimensions don't match the pivot definition"
"Dimensions don't match the pivot definition. Consider using a dynamic pivot formula: =PIVOT(1). Or re-insert the static pivot from the Data menu."
);
setCellContent(model, "A32", '=PIVOT.HEADER(1, "Date:quarter_number", "not a number")');
expect(getEvaluatedCell(model, "A32").message).toBe(
Expand Down Expand Up @@ -1060,7 +1060,7 @@ describe("Spreadsheet Pivot", () => {
// without granularity
setCellContent(model, "A32", '=PIVOT.HEADER(1, "Date", )');
expect(getEvaluatedCell(model, "A32").message).toBe(
"Dimensions don't match the pivot definition"
"Dimensions don't match the pivot definition. Consider using a dynamic pivot formula: =PIVOT(1). Or re-insert the static pivot from the Data menu."
);

// not a number
Expand Down Expand Up @@ -1106,7 +1106,7 @@ describe("Spreadsheet Pivot", () => {
// without granularity
setCellContent(model, "A31", '=PIVOT.HEADER(1, "Date", )');
expect(getEvaluatedCell(model, "A31").message).toBe(
"Dimensions don't match the pivot definition"
"Dimensions don't match the pivot definition. Consider using a dynamic pivot formula: =PIVOT(1). Or re-insert the static pivot from the Data menu."
);

// not a number
Expand Down Expand Up @@ -1159,7 +1159,7 @@ describe("Spreadsheet Pivot", () => {
// without granularity
setCellContent(model, "A31", '=PIVOT.HEADER(1, "Date", )');
expect(getEvaluatedCell(model, "A31").message).toBe(
"Dimensions don't match the pivot definition"
"Dimensions don't match the pivot definition. Consider using a dynamic pivot formula: =PIVOT(1). Or re-insert the static pivot from the Data menu."
);

// not a number
Expand Down Expand Up @@ -1209,7 +1209,7 @@ describe("Spreadsheet Pivot", () => {
// without granularity
setCellContent(model, "A31", '=PIVOT.HEADER(1, "Date", )');
expect(getEvaluatedCell(model, "A31").message).toBe(
"Dimensions don't match the pivot definition"
"Dimensions don't match the pivot definition. Consider using a dynamic pivot formula: =PIVOT(1). Or re-insert the static pivot from the Data menu."
);

// not a number
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
import { Model, SpreadsheetChildEnv } from "../../../src";
import { toZone } from "../../../src/helpers";
import { SpreadsheetPivot } from "../../../src/helpers/pivot/spreadsheet_pivot/spreadsheet_pivot";
import { createSheet, setCellContent, undo } from "../../test_helpers/commands_helpers";
import { NotificationStore } from "../../../src/stores/notification_store";
import {
createSheet,
setCellContent,
setViewportOffset,
undo,
} from "../../test_helpers/commands_helpers";
import { click, dragElement, setInputValueAndTrigger } from "../../test_helpers/dom_helper";
import { getCellText } from "../../test_helpers/getters_helpers";
import { mountSpreadsheet, nextTick } from "../../test_helpers/helpers";
Expand Down Expand Up @@ -235,6 +241,35 @@ describe("Spreadsheet pivot side panel", () => {
]);
});

test("notify when no dynamic pivot is visible", async () => {
setCellContent(model, "A4", "=PIVOT(1)");
const mockNotify = jest.fn();
const notificationStore = env.getStore(NotificationStore);
notificationStore.updateNotificationCallbacks({
notifyUser: mockNotify,
});

await click(fixture.querySelector(".o-pivot-measure .add-dimension")!);
await click(fixture.querySelectorAll(".o-autocomplete-value")[1]);
// don't notify when dynamic pivot is visible
expect(mockNotify).toHaveBeenCalledTimes(0);

// scroll beyond the =PIVOT formula
setViewportOffset(model, 0, 1000);
await click(fixture.querySelector(".o-pivot-measure .add-dimension")!);
await click(fixture.querySelectorAll(".o-autocomplete-value")[1]);
expect(mockNotify).toHaveBeenCalledWith({
text: "Pivot updates only work with dynamic pivot tables. Use =PIVOT(1) or re-insert the static pivot from the Data menu.",
sticky: false,
type: "info",
});

// don't notify a second time
await click(fixture.querySelector(".o-pivot-measure .add-dimension")!);
await click(fixture.querySelectorAll(".o-autocomplete-value")[1]);
expect(mockNotify).toHaveBeenCalledTimes(1);
});

test("Invalid pivot dimensions are displayed as such in the side panel", async () => {
setCellContent(model, "A1", "ValidDimension");
setCellContent(model, "A2", "10");
Expand Down