From 3dc759594789a2026c2936d35d3416e077275e8e Mon Sep 17 00:00:00 2001 From: Boris Goldowsky Date: Thu, 12 Sep 2024 15:57:19 -0400 Subject: [PATCH 1/7] Track shared model ID in persistent state to avoid sync issues. Add ID update function to fix tile copy issue. --- .../bar-graph/bar-graph-content.test.ts | 2 +- src/plugins/bar-graph/bar-graph-content.ts | 73 +++++++------------ .../bar-graph/bar-graph-registration.ts | 4 +- src/plugins/bar-graph/bar-graph-utils.ts | 13 ++++ src/plugins/bar-graph/category-pulldown.tsx | 2 +- src/plugins/bar-graph/legend-area.tsx | 11 +-- 6 files changed, 50 insertions(+), 55 deletions(-) diff --git a/src/plugins/bar-graph/bar-graph-content.test.ts b/src/plugins/bar-graph/bar-graph-content.test.ts index 3765a8c43..71d3548cd 100644 --- a/src/plugins/bar-graph/bar-graph-content.test.ts +++ b/src/plugins/bar-graph/bar-graph-content.test.ts @@ -29,7 +29,7 @@ function sharedSampleDataSet() { const TestableBarGraphContentModel = BarGraphContentModel .actions(self => ({ setDataSet(ds: SharedDataSetType) { - self.dataSet = ds; + // self.dataSet = ds; } })); diff --git a/src/plugins/bar-graph/bar-graph-content.ts b/src/plugins/bar-graph/bar-graph-content.ts index 51c08ce15..14d6f4b20 100644 --- a/src/plugins/bar-graph/bar-graph-content.ts +++ b/src/plugins/bar-graph/bar-graph-content.ts @@ -1,14 +1,12 @@ -import { reaction } from "mobx"; -import { types, Instance, addDisposer } from "mobx-state-tree"; +import { types, Instance } from "mobx-state-tree"; import { isNumber } from "lodash"; -import { SharedModelType } from "../../models/shared/shared-model"; import { ITileContentModel, TileContentModel } from "../../models/tiles/tile-content"; import { kBarGraphTileType, kBarGraphContentType } from "./bar-graph-types"; import { getSharedModelManager } from "../../models/tiles/tile-environment"; -import { isSharedDataSet, SharedDataSet, SharedDataSetType } from "../../models/shared/shared-data-set"; -import { ISharedModelManager } from "../../models/shared/shared-model-manager"; +import { SharedDataSet, SharedDataSetType } from "../../models/shared/shared-data-set"; import { clueDataColorInfo } from "../../utilities/color-utils"; import { displayValue } from "./bar-graph-utils"; +import { SharedModelType } from "../../models/shared/shared-model"; export function defaultBarGraphContent(): BarGraphContentModelType { return BarGraphContentModel.create({yAxisLabel: "Counts"}); @@ -19,29 +17,26 @@ export const BarGraphContentModel = TileContentModel .props({ type: types.optional(types.literal(kBarGraphTileType), kBarGraphTileType), yAxisLabel: "", + // Generally we fetch the dataset from SharedModelManager, but we track the ID here + // in persisted state as well so that remote read-only tiles will notice when it changes. + dataSetId: types.maybe(types.string), primaryAttribute: types.maybe(types.string), secondaryAttribute: types.maybe(types.string) }) - .volatile(self => ({ - dataSet: undefined as SharedDataSetType|undefined - })) .views(self => ({ + get sharedModel() { + const sharedModelManager = self.tileEnv?.sharedModelManager; + const firstSharedModel = sharedModelManager?.getTileSharedModelsByType(self, SharedDataSet)?.[0]; + if (!firstSharedModel) return undefined; + return firstSharedModel as SharedDataSetType; + }, get isUserResizable() { return true; - }, + } + })) + .views(self => ({ get cases() { - return self.dataSet?.dataSet.cases; - }, - // Query the SharedModelManager to find a connected DataSet - // An argument is passed in to avoid the return value being cached. - sharedModelDataSet(smm: ISharedModelManager|undefined) { - if (!smm || !smm.isReady) return; - const sharedDataSets = smm.getTileSharedModelsByType(self, SharedDataSet); - if (sharedDataSets.length > 0 && isSharedDataSet(sharedDataSets[0])) { - return sharedDataSets[0]; - } else { - return undefined; - } + return self.sharedModel?.dataSet.cases; } })) .views(self => ({ @@ -66,7 +61,7 @@ export const BarGraphContentModel = TileContentModel * Any empty values of attributes are replaced with "(no value)". */ get dataArray() { - const dataSet = self.dataSet?.dataSet; + const dataSet = self.sharedModel?.dataSet; const primary = self.primaryAttribute; const secondary = self.secondaryAttribute; const cases = self.cases; @@ -139,9 +134,6 @@ export const BarGraphContentModel = TileContentModel }, setSecondaryAttribute(attrId: string|undefined) { self.secondaryAttribute = attrId; - }, - setSharedDataSet() { - self.dataSet = self.sharedModelDataSet(getSharedModelManager(self)); } })) .actions(self => ({ @@ -152,40 +144,27 @@ export const BarGraphContentModel = TileContentModel for (const sharedDataSet of sharedDataSets) { smm.removeTileSharedModel(self, sharedDataSet); } - self.dataSet = undefined; - }, - afterAttach() { - // After attached to document & SMM is ready, cache a reference to our dataset. - addDisposer(self, reaction( - () => { - return self.tileEnv?.sharedModelManager?.isReady; - }, - (ready) => { - if (!ready) return; - self.setSharedDataSet(); - }, { fireImmediately: true } - )); }, + updateAfterSharedModelChanges(sharedModel?: SharedModelType) { - // When new dataset is attached, cache a reference to it and pick a category attribute. - const dataSet = self.sharedModelDataSet(getSharedModelManager(self)); - if (self.dataSet !== dataSet) { + // When new dataset is attached, store its ID and pick a primary attribute to display. + const dataSetId = self.sharedModel?.dataSet?.id; + if (self.dataSetId !== dataSetId) { + self.dataSetId = dataSetId; self.setPrimaryAttribute(undefined); self.setSecondaryAttribute(undefined); - self.dataSet = dataSet; - if (dataSet) { - const atts = dataSet.dataSet.attributes; + if (dataSetId) { + const atts = self.sharedModel.dataSet.attributes; if (atts.length > 0) { self.setPrimaryAttribute(atts[0].id); } } - } } - })); + } +})); export interface BarGraphContentModelType extends Instance {} - export function isBarGraphModel(model?: ITileContentModel): model is BarGraphContentModelType { return model?.type === kBarGraphTileType; } diff --git a/src/plugins/bar-graph/bar-graph-registration.ts b/src/plugins/bar-graph/bar-graph-registration.ts index 3de8812d0..0ba5e2384 100644 --- a/src/plugins/bar-graph/bar-graph-registration.ts +++ b/src/plugins/bar-graph/bar-graph-registration.ts @@ -3,6 +3,7 @@ import { registerTileContentInfo } from "../../models/tiles/tile-content-info"; import { kBarGraphTileType, kBarGraphDefaultHeight } from "./bar-graph-types"; import { BarGraphComponent } from "./bar-graph-tile"; import { defaultBarGraphContent, BarGraphContentModel } from "./bar-graph-content"; +import { updateBarGraphContentWithNewSharedModelIds } from "./bar-graph-utils"; import Icon from "./assets/bar-graph-icon.svg"; @@ -11,7 +12,8 @@ registerTileContentInfo({ displayName: "Bar Graph", modelClass: BarGraphContentModel, defaultContent: defaultBarGraphContent, - defaultHeight: kBarGraphDefaultHeight + defaultHeight: kBarGraphDefaultHeight, + updateContentWithNewSharedModelIds: updateBarGraphContentWithNewSharedModelIds }); registerTileComponentInfo({ diff --git a/src/plugins/bar-graph/bar-graph-utils.ts b/src/plugins/bar-graph/bar-graph-utils.ts index bd7616abe..e0a5c02e1 100644 --- a/src/plugins/bar-graph/bar-graph-utils.ts +++ b/src/plugins/bar-graph/bar-graph-utils.ts @@ -1,3 +1,8 @@ +import { SnapshotOut } from "mobx-state-tree"; +import { SharedModelEntrySnapshotType } from "../../models/document/shared-model-entry"; +import { replaceJsonStringsWithUpdatedIds, UpdatedSharedDataSetIds } from "../../models/shared/shared-data-set"; +import { BarGraphContentModel } from "./bar-graph-content"; + const kMissingValueString = "(no value)"; // Substitute "(no value)" for missing data @@ -19,3 +24,11 @@ export function getBBox(element: SVGGraphicsElement): DOMRect { export function roundTo5(n: number): number { return Math.max(5, Math.ceil(n/5)*5); } + +export function updateBarGraphContentWithNewSharedModelIds( + content: SnapshotOut, + sharedDataSetEntries: SharedModelEntrySnapshotType[], + updatedSharedModelMap: Record +) { + return replaceJsonStringsWithUpdatedIds(content, '"', ...Object.values(updatedSharedModelMap)); +} diff --git a/src/plugins/bar-graph/category-pulldown.tsx b/src/plugins/bar-graph/category-pulldown.tsx index df6c2548a..67c5d1a04 100644 --- a/src/plugins/bar-graph/category-pulldown.tsx +++ b/src/plugins/bar-graph/category-pulldown.tsx @@ -18,7 +18,7 @@ export const CategoryPulldown = observer(function CategoryPulldown({setCategory, const readOnly = useReadOnlyContext(); const model = useBarGraphModelContext(); - const dataSet = model?.dataSet?.dataSet; + const dataSet = model?.sharedModel?.dataSet; const attributes = dataSet?.attributes || []; const current = (dataSet && model.primaryAttribute) ? dataSet.attrFromID(model.primaryAttribute)?.name diff --git a/src/plugins/bar-graph/legend-area.tsx b/src/plugins/bar-graph/legend-area.tsx index bd68d9eaf..ccb9654d7 100644 --- a/src/plugins/bar-graph/legend-area.tsx +++ b/src/plugins/bar-graph/legend-area.tsx @@ -26,11 +26,12 @@ export const LegendArea = observer(function LegendArea ({legendRef}: IProps) { } } - if (!model || !model.dataSet || !model.primaryAttribute) { + if (!model || !model.sharedModel || !model.primaryAttribute) { return null; } - const dataSet = model.dataSet.dataSet; + const dataSet = model.sharedModel.dataSet; + const dataSetName = model.sharedModel.name; const allAttributes = dataSet?.attributes || []; const availableAttributes = allAttributes.filter((a) => a.id !== model.primaryAttribute); const currentPrimary = dataSet?.attrFromID(model.primaryAttribute); @@ -44,13 +45,13 @@ export const LegendArea = observer(function LegendArea ({legendRef}: IProps) {
Data from: - {model.dataSet.name} + {dataSetName}
@@ -79,7 +80,7 @@ export const LegendArea = observer(function LegendArea ({legendRef}: IProps) {
{currentSecondary ? secondaryKeys.map((key) => ) - : } + : }
From c3f02fc94619083e1346ee1183c7ade9d5bf120f Mon Sep 17 00:00:00 2001 From: Boris Goldowsky Date: Thu, 12 Sep 2024 16:08:10 -0400 Subject: [PATCH 2/7] Update Jest tests --- .../bar-graph/bar-graph-content.test.ts | 60 +++++++++++++------ 1 file changed, 41 insertions(+), 19 deletions(-) diff --git a/src/plugins/bar-graph/bar-graph-content.test.ts b/src/plugins/bar-graph/bar-graph-content.test.ts index 71d3548cd..565a7f56f 100644 --- a/src/plugins/bar-graph/bar-graph-content.test.ts +++ b/src/plugins/bar-graph/bar-graph-content.test.ts @@ -26,10 +26,21 @@ function sharedSampleDataSet() { return SharedDataSet.create({ dataSet: sampleDataSet }); } -const TestableBarGraphContentModel = BarGraphContentModel +// This is a testable version of the BarGraphContentModel that doesn't rely on the shared model manager +// It just lets you set a SharedModel and returns that. +const TestingBarGraphContentModel = BarGraphContentModel + .volatile(() => ({ + storedSharedModel: undefined as SharedDataSetType | undefined + })) .actions(self => ({ - setDataSet(ds: SharedDataSetType) { - // self.dataSet = ds; + setSharedModel(sharedModel: SharedDataSetType) { + self.storedSharedModel = sharedModel; + self.updateAfterSharedModelChanges(sharedModel); + } + })) + .views(self => ({ + get sharedModel() { + return self.storedSharedModel; } })); @@ -44,6 +55,7 @@ describe("Bar Graph Content", () => { expect(content.yAxisLabel).toBe("Counts"); expect(getSnapshot(content)).toMatchInlineSnapshot(` Object { + "dataSetId": undefined, "primaryAttribute": undefined, "secondaryAttribute": undefined, "type": "BarGraph", @@ -78,20 +90,21 @@ Object { }); it("returns empty data array when there are no cases", () => { - const content = TestableBarGraphContentModel.create({ }); - content.setDataSet(sharedEmptyDataSet()); + const content = TestingBarGraphContentModel.create({ }); + content.setSharedModel(sharedEmptyDataSet()); expect(content.dataArray).toEqual([]); }); it("returns empty data array when there is no primary attribute", () => { - const content = TestableBarGraphContentModel.create({ }); - content.setDataSet(sharedSampleDataSet()); + const content = TestingBarGraphContentModel.create({ }); + content.setSharedModel(sharedSampleDataSet()); + content.setPrimaryAttribute(undefined); expect(content.dataArray).toEqual([]); }); it("returns expected data array with primary attribute", () => { - const content = TestableBarGraphContentModel.create({ }); - content.setDataSet(sharedSampleDataSet()); + const content = TestingBarGraphContentModel.create({ }); + content.setSharedModel(sharedSampleDataSet()); content.setPrimaryAttribute("att-s"); expect(content.dataArray).toEqual([ { "att-s": "cat", "value": 2 }, @@ -105,9 +118,18 @@ Object { ]); }); + it("sets first dataset attribute as the primary attribute by default", () => { + const content = TestingBarGraphContentModel.create({ }); + content.setSharedModel(sharedSampleDataSet()); + expect(content.dataArray).toEqual([ + { "att-s": "cat", "value": 2 }, + { "att-s": "owl","value": 2} + ]); + }); + it("returns expected data array with primary and secondary attributes", () => { - const content = TestableBarGraphContentModel.create({ }); - content.setDataSet(sharedSampleDataSet()); + const content = TestingBarGraphContentModel.create({ }); + content.setSharedModel(sharedSampleDataSet()); content.setPrimaryAttribute("att-s"); content.setSecondaryAttribute("att-l"); expect(content.dataArray).toEqual([ @@ -117,10 +139,10 @@ Object { }); it("fills in missing values with (no value)", () => { - const content = TestableBarGraphContentModel.create({ }); + const content = TestingBarGraphContentModel.create({ }); const dataSet = sharedSampleDataSet(); dataSet.dataSet?.attributes[1].setValue(3, undefined); // hide forest owl's location - content.setDataSet(dataSet); + content.setSharedModel(dataSet); content.setPrimaryAttribute("att-s"); content.setSecondaryAttribute("att-l"); expect(content.dataArray).toEqual([ @@ -138,23 +160,23 @@ Object { }); it("extracts primary keys", () => { - const content = TestableBarGraphContentModel.create({ }); - content.setDataSet(sharedSampleDataSet()); + const content = TestingBarGraphContentModel.create({ }); + content.setSharedModel(sharedSampleDataSet()); content.setPrimaryAttribute("att-s"); expect(content.primaryKeys).toEqual(["cat", "owl"]); }); it("extracts secondary keys", () => { - const content = TestableBarGraphContentModel.create({ }); - content.setDataSet(sharedSampleDataSet()); + const content = TestingBarGraphContentModel.create({ }); + content.setSharedModel(sharedSampleDataSet()); content.setPrimaryAttribute("att-s"); content.setSecondaryAttribute("att-l"); expect(content.secondaryKeys).toEqual(["yard", "forest"]); }); it("calculates the maximum data value", () => { - const content = TestableBarGraphContentModel.create({ }); - content.setDataSet(sharedSampleDataSet()); + const content = TestingBarGraphContentModel.create({ }); + content.setSharedModel(sharedSampleDataSet()); content.setPrimaryAttribute("att-s"); expect(content.maxDataValue).toBe(2); From 5f09a104fc1c2ade4d9ebe01fb6a48081a956b0e Mon Sep 17 00:00:00 2001 From: Boris Goldowsky Date: Fri, 13 Sep 2024 11:56:36 -0400 Subject: [PATCH 3/7] Read only & sync & test improvements. Prevent changes in read-only mode. When changing primary attribute, reset secondary as part of the same action to support undo. Make EditableAxisLabel handle things more locally to fix sync bugs. Test read-only views Test undo and redo of each operation. Add classes in DocEditorApp to support testing. --- .../tile_tests/bar_graph_tile_spec.js | 240 +++++++++++++----- cypress/support/elements/tile/BarGraphTile.js | 72 +++--- .../support/elements/tile/TableToolTile.js | 8 +- src/components/doc-editor/doc-editor-app.tsx | 8 +- src/plugins/bar-graph/bar-graph-content.ts | 6 +- src/plugins/bar-graph/bar-graph-tile.tsx | 2 +- src/plugins/bar-graph/chart-area.tsx | 3 - src/plugins/bar-graph/editable-axis-label.tsx | 27 +- src/plugins/bar-graph/legend-area.tsx | 10 +- 9 files changed, 253 insertions(+), 123 deletions(-) diff --git a/cypress/e2e/functional/tile_tests/bar_graph_tile_spec.js b/cypress/e2e/functional/tile_tests/bar_graph_tile_spec.js index 829558e4a..19711e72c 100644 --- a/cypress/e2e/functional/tile_tests/bar_graph_tile_spec.js +++ b/cypress/e2e/functional/tile_tests/bar_graph_tile_spec.js @@ -10,6 +10,8 @@ let clueCanvas = new ClueCanvas, // eslint-disable-next-line unused-imports/no-unused-vars const canvas = new Canvas; +const workspaces = ['.primary-workspace', '.read-only-local-workspace', '.read-only-remote-workspace']; + function textMatchesList(selector, expected) { selector.should('have.length', expected.length); selector.each(($el, index) => { @@ -28,59 +30,111 @@ context('Bar Graph Tile', function () { beforeTest(); clueCanvas.addTile('bargraph'); - barGraph.getTiles().should('have.length', 1); - barGraph.getTile() - .should('be.visible') - .and('have.class', 'bar-graph-tile') - .and('not.have.class', 'read-only'); + for (const workspace of workspaces) { + barGraph.getTiles(workspace).should('have.length', 1); + barGraph.getTile(workspace, 0) + .should('be.visible') + .and('have.class', 'bar-graph-tile'); + barGraph.getTileTitle(workspace).should("be.visible").and('have.text', 'Bar Graph 1'); + barGraph.getYAxisLabel(workspace).should('have.text', 'Counts'); + barGraph.getXAxisPulldownButton(workspace).should('have.text', 'Categories'); + } + barGraph.getTile(workspaces[0]).should('not.have.class', 'readonly'); + barGraph.getTile(workspaces[1]).should('have.class', 'readonly'); + barGraph.getTile(workspaces[2]).should('have.class', 'readonly'); - barGraph.getTileTitle().should("be.visible").and('have.text', 'Bar Graph 1'); - barGraph.getYAxisLabel().should('have.text', 'Counts'); - barGraph.getXAxisPulldownButton(0).should('have.text', 'Categories'); + // Undo/redo tile creation + clueCanvas.getUndoTool().click(); + for (const workspace of workspaces) { + barGraph.getTiles(workspace).should('have.length', 0); + } + clueCanvas.getRedoTool().click(); + for (const workspace of workspaces) { + barGraph.getTiles(workspace).should('have.length', 1); + } cy.log('Change Y axis label'); barGraph.getYAxisLabelEditor().should('not.exist'); barGraph.getYAxisLabelButton().click(); barGraph.getYAxisLabelEditor().should('be.visible').type(' of something{enter}'); barGraph.getYAxisLabelEditor().should('not.exist'); + for (const workspace of workspaces) { + barGraph.getYAxisLabel(workspace).should('have.text', 'Counts of something'); + } + + // Undo/redo label change + clueCanvas.getUndoTool().click(); + barGraph.getYAxisLabel().should('have.text', 'Counts'); + clueCanvas.getRedoTool().click(); barGraph.getYAxisLabel().should('have.text', 'Counts of something'); cy.log('Duplicate tile'); clueCanvas.getDuplicateTool().click(); - barGraph.getTiles().should('have.length', 2); - barGraph.getTile(0) - .should('be.visible') - .and('have.class', 'bar-graph-tile') - .and('not.have.class', 'read-only'); - barGraph.getTileTitle(0).should("be.visible").and('have.text', 'Bar Graph 1'); - barGraph.getYAxisLabel(0).should('have.text', 'Counts of something'); - barGraph.getXAxisPulldownButton(0).should('have.text', 'Categories'); - - barGraph.getTile(1) - .should('be.visible') - .and('have.class', 'bar-graph-tile') - .and('not.have.class', 'read-only'); - barGraph.getTileTitle(1).should("be.visible").and('have.text', 'Bar Graph 2'); - barGraph.getYAxisLabel(1).should('have.text', 'Counts of something'); - barGraph.getXAxisPulldownButton(1).should('have.text', 'Categories'); + for (const workspace of workspaces) { + barGraph.getTiles(workspace).should('have.length', 2); + barGraph.getTile(workspace, 0) + .should('be.visible') + .and('have.class', 'bar-graph-tile'); + barGraph.getTileTitle(workspace, 0).should("be.visible").and('have.text', 'Bar Graph 1'); + barGraph.getYAxisLabel(workspace, 0).should('have.text', 'Counts of something'); + barGraph.getXAxisPulldownButton(workspace, 0).should('have.text', 'Categories'); + + barGraph.getTile(workspace, 1) + .should('be.visible') + .and('have.class', 'bar-graph-tile'); + barGraph.getTileTitle(workspace, 1).should("be.visible").and('have.text', 'Bar Graph 2'); + barGraph.getYAxisLabel(workspace, 1).should('have.text', 'Counts of something'); + barGraph.getXAxisPulldownButton(workspace, 1).should('have.text', 'Categories'); + } + + // Undo/redo tile duplication + clueCanvas.getUndoTool().click(); + for (const workspace of workspaces) { + barGraph.getTiles(workspace).should('have.length', 1); + } + clueCanvas.getRedoTool().click(); + for (const workspace of workspaces) { + barGraph.getTiles(workspace).should('have.length', 2); + } cy.log('Delete tile'); clueCanvas.deleteTile('bargraph'); clueCanvas.deleteTile('bargraph'); - barGraph.getTiles().should('have.length', 0); + for (const workspace of workspaces) { + barGraph.getTiles(workspace).should('have.length', 0); + } + + // Undo/redo tile deletion + clueCanvas.getUndoTool().click(); + for (const workspace of workspaces) { + barGraph.getTiles(workspace).should('have.length', 1); + } + clueCanvas.getUndoTool().click(); + for (const workspace of workspaces) { + barGraph.getTiles(workspace).should('have.length', 2); + } + clueCanvas.getRedoTool().click(); + for (const workspace of workspaces) { + barGraph.getTiles(workspace).should('have.length', 1); + } + clueCanvas.getRedoTool().click(); + for (const workspace of workspaces) { + barGraph.getTiles(workspace).should('have.length', 0); + } }); it('Can link data ', function () { beforeTest(); clueCanvas.addTile('bargraph'); - barGraph.getTiles().click(); - barGraph.getYAxisLabel().should('have.text', 'Counts'); - barGraph.getXAxisPulldown().should('have.text', 'Categories'); - barGraph.getYAxisTickLabel().should('not.exist'); - barGraph.getXAxisTickLabel().should('not.exist'); - barGraph.getLegendArea().should('not.exist'); - barGraph.getBar().should('not.exist'); + for (const workspace of workspaces) { + barGraph.getYAxisLabel(workspace).should('have.text', 'Counts'); + barGraph.getXAxisPulldown(workspace).should('have.text', 'Categories'); + barGraph.getYAxisTickLabel(workspace).should('not.exist'); + barGraph.getXAxisTickLabel(workspace).should('not.exist'); + barGraph.getLegendArea(workspace).should('not.exist'); + barGraph.getBar(workspace).should('not.exist'); + } // Table dataset for testing: // 4 instances of X / Y / Z @@ -103,55 +157,121 @@ context('Bar Graph Tile', function () { cy.get('select').select('Table Data 1'); cy.get('.modal-button').contains("Graph It!").click(); - barGraph.getXAxisPulldown().should('have.text', 'x'); + for (const workspace of workspaces) { + barGraph.getXAxisPulldown(workspace).should('have.text', 'x'); + textMatchesList(barGraph.getXAxisTickLabel(workspace), ['X', 'XX']); + textMatchesList(barGraph.getYAxisTickLabel(workspace), ['0', '1', '2', '3', '4', '5']); + barGraph.getBar(workspace).should('have.length', 2); + barGraph.getDatasetLabel(workspace).should('have.text', 'Table Data 1'); + barGraph.getSortByMenuButton(workspace).should('have.text', 'None'); + barGraph.getSecondaryValueName(workspace).should('have.length', 1).and('have.text', 'x'); + } - textMatchesList(barGraph.getXAxisTickLabel(), ['X', 'XX']); - textMatchesList(barGraph.getYAxisTickLabel(), ['0', '1', '2', '3', '4', '5']); - barGraph.getBar().should('have.length', 2); - barGraph.getDatasetLabel().should('have.text', 'Table Data 1'); - barGraph.getSortByMenuButton().should('have.text', 'None'); - barGraph.getSecondaryValueName().should('have.length', 1).and('have.text', 'x'); + // Undo/redo linking + clueCanvas.getUndoTool().click(); + for (const workspace of workspaces) { + barGraph.getXAxisPulldown(workspace).should('have.text', 'Categories'); + barGraph.getLegendArea(workspace).should('not.exist'); + barGraph.getBar(workspace).should('not.exist'); + } + + clueCanvas.getRedoTool().click(); + for (const workspace of workspaces) { + barGraph.getDatasetLabel(workspace).should('have.text', 'Table Data 1'); + barGraph.getXAxisPulldown(workspace).should('have.text', 'x'); + barGraph.getBar(workspace).should('have.length', 2); + } cy.log('Legend should move to bottom when tile is narrow'); barGraph.getTileContent().should('have.class', 'horizontal').and('not.have.class', 'vertical'); clueCanvas.addTileByDrag('table', 'right'); clueCanvas.addTileByDrag('table', 'right'); - barGraph.getTileContent().should('have.class', 'vertical').and('not.have.class', 'horizontal'); + for (const workspace of workspaces) { + barGraph.getTileContent(workspace).should('have.class', 'vertical').and('not.have.class', 'horizontal'); + } clueCanvas.getUndoTool().click(); // undo add table clueCanvas.getUndoTool().click(); // undo add table - tableTile.getTableTile().should('have.length', 1); - barGraph.getTileContent().should('have.class', 'horizontal').and('not.have.class', 'vertical'); + for (const workspace of workspaces) { + tableTile.getTableTile(workspace).should('have.length', 1); + barGraph.getTileContent(workspace).should('have.class', 'horizontal').and('not.have.class', 'vertical'); + } cy.log('Change Sort By'); + barGraph.getSortByMenuButton().should('have.text', 'None'); barGraph.getSortByMenuButton().click(); barGraph.getChakraMenuItem().should('have.length', 3); barGraph.getChakraMenuItem().eq(1).should('have.text', 'y').click(); - textMatchesList(barGraph.getXAxisTickLabel(), ['X', 'XX']); - textMatchesList(barGraph.getYAxisTickLabel(), ['0', '1', '2', '3', '4', '5']); - barGraph.getBar().should('have.length', 3); - barGraph.getDatasetLabel().should('have.text', 'Table Data 1'); - barGraph.getSortByMenuButton().should('have.text', 'y'); - textMatchesList(barGraph.getSecondaryValueName(), ['Y', 'YY']); + for (const workspace of workspaces) { + textMatchesList(barGraph.getXAxisTickLabel(workspace), ['X', 'XX']); + textMatchesList(barGraph.getYAxisTickLabel(workspace), ['0', '1', '2', '3', '4', '5']); + barGraph.getBar(workspace).should('have.length', 3); + barGraph.getDatasetLabel(workspace).should('have.text', 'Table Data 1'); + barGraph.getSortByMenuButton(workspace).should('have.text', 'y'); + textMatchesList(barGraph.getSecondaryValueName(workspace), ['Y', 'YY']); + } + + // Undo-redo sort by + clueCanvas.getUndoTool().click(); + for (const workspace of workspaces) { + barGraph.getSortByMenuButton(workspace).should('have.text', 'None'); + barGraph.getBar(workspace).should('have.length', 2); + barGraph.getSecondaryValueName(workspace).should('have.text', 'x'); + } + clueCanvas.getRedoTool().click(); + for (const workspace of workspaces) { + barGraph.getSortByMenuButton(workspace).should('have.text', 'y'); + textMatchesList(barGraph.getSecondaryValueName(workspace), ['Y', 'YY']); + barGraph.getBar(workspace).should('have.length', 3); + } cy.log('Change Category'); barGraph.getXAxisPulldownButton().click(); barGraph.getChakraMenuItem().should('have.length', 3); barGraph.getChakraMenuItem().eq(1).should('have.text', 'y').click(); - barGraph.getXAxisPulldown().should('have.text', 'y'); - textMatchesList(barGraph.getXAxisTickLabel(), ['Y', 'YY']); - textMatchesList(barGraph.getYAxisTickLabel(), ['0', '2', '4', '6', '8', '10']); // there are 6 Ys in this view so scale expands. - barGraph.getBar().should('have.length', 2); - barGraph.getDatasetLabel().should('have.text', 'Table Data 1'); - barGraph.getSortByMenuButton().should('have.text', 'None'); - barGraph.getSecondaryValueName().should('have.length', 1).and('have.text', 'y'); + for (const workspace of workspaces) { + barGraph.getXAxisPulldown(workspace).should('have.text', 'y'); + textMatchesList(barGraph.getXAxisTickLabel(workspace), ['Y', 'YY']); + textMatchesList(barGraph.getYAxisTickLabel(workspace), ['0', '2', '4', '6', '8', '10']); // there are 6 Ys in this view so scale expands. + barGraph.getBar(workspace).should('have.length', 2); + barGraph.getDatasetLabel(workspace).should('have.text', 'Table Data 1'); + barGraph.getSortByMenuButton(workspace).should('have.text', 'None'); + barGraph.getSecondaryValueName(workspace).should('have.length', 1).and('have.text', 'y'); + } + + // Undo-redo category change + clueCanvas.getUndoTool().click(); + for (const workspace of workspaces) { + barGraph.getXAxisPulldown(workspace).should('have.text', 'x'); + textMatchesList(barGraph.getXAxisTickLabel(workspace), ['X', 'XX']); + } + clueCanvas.getRedoTool().click(); + for (const workspace of workspaces) { + barGraph.getXAxisPulldown(workspace).should('have.text', 'y'); + textMatchesList(barGraph.getXAxisTickLabel(workspace), ['Y', 'YY']); + } cy.log('Unlink data'); barGraph.getDatasetUnlinkButton().click(); - barGraph.getXAxisPulldown().should('have.text', 'Categories'); - barGraph.getYAxisTickLabel().should('not.exist'); - barGraph.getXAxisTickLabel().should('not.exist'); - barGraph.getLegendArea().should('not.exist'); - barGraph.getBar().should('not.exist'); + for (const workspace of workspaces) { + barGraph.getXAxisPulldown(workspace).should('have.text', 'Categories'); + barGraph.getYAxisTickLabel(workspace).should('not.exist'); + barGraph.getXAxisTickLabel(workspace).should('not.exist'); + barGraph.getLegendArea(workspace).should('not.exist'); + barGraph.getBar(workspace).should('not.exist'); + } + + // Undo-redo unlink + clueCanvas.getUndoTool().click(); + for (const workspace of workspaces) { + barGraph.getXAxisPulldown().should('have.text', 'y'); + textMatchesList(barGraph.getXAxisTickLabel(workspace), ['Y', 'YY']); + barGraph.getBar(workspace).should('have.length', 2); + } + clueCanvas.getRedoTool().click(); + for (const workspace of workspaces) { + barGraph.getXAxisPulldown(workspace).should('have.text', 'Categories'); + barGraph.getBar(workspace).should('not.exist'); + } }); }); diff --git a/cypress/support/elements/tile/BarGraphTile.js b/cypress/support/elements/tile/BarGraphTile.js index 5fe641923..d19a3b1d9 100644 --- a/cypress/support/elements/tile/BarGraphTile.js +++ b/cypress/support/elements/tile/BarGraphTile.js @@ -1,79 +1,79 @@ class BarGraphTile { getTiles(workspaceClass) { - return cy.get(`${workspaceClass || ".primary-workspace"} .canvas-area .bar-graph-tile`); + return cy.get(`${workspaceClass || ".primary-workspace"} .canvas .bar-graph-tile`); } - getTile(tileIndex = 0, workspaceClass) { - return this.getTiles().eq(tileIndex); + getTile(workspaceClass, tileIndex = 0) { + return this.getTiles(workspaceClass).eq(tileIndex); } - getTileTitle(tileIndex = 0, workspaceClass) { - return this.getTile(tileIndex, workspaceClass).find(`.editable-tile-title-text`); + getTileTitle(workspaceClass, tileIndex = 0) { + return this.getTile(workspaceClass, tileIndex).find(`.editable-tile-title-text`); } - getTileContent(tileIndex = 0, workspaceClass) { - return this.getTile(tileIndex, workspaceClass).find(`[data-testid="bar-graph-content"]`); + getTileContent(workspaceClass, tileIndex = 0) { + return this.getTile(workspaceClass, tileIndex).find(`[data-testid="bar-graph-content"]`); } - getChakraMenuItem(tileIndex = 0, workspaceClass) { + getChakraMenuItem(workspaceClass, tileIndex = 0) { return cy.get(`body .chakra-portal button`).filter(':visible'); } - getChartArea(tileIndex = 0, workspaceClass) { - return this.getTile(tileIndex, workspaceClass).find(`svg.bar-graph-svg`); + getChartArea(workspaceClass, tileIndex = 0) { + return this.getTile(workspaceClass, tileIndex).find(`svg.bar-graph-svg`); } - getYAxisLabel(tileIndex = 0, workspaceClass) { - return this.getChartArea(tileIndex, workspaceClass).find(`.editable-axis-label`); + getYAxisLabel(workspaceClass, tileIndex = 0) { + return this.getChartArea(workspaceClass, tileIndex).find(`.editable-axis-label`); } - getYAxisLabelButton(tileIndex = 0, workspaceClass) { - return this.getChartArea(tileIndex, workspaceClass).find(`[data-testid="axis-label-button"]`); + getYAxisLabelButton(workspaceClass, tileIndex = 0) { + return this.getChartArea(workspaceClass, tileIndex).find(`[data-testid="axis-label-button"]`); } - getYAxisLabelEditor(tileIndex = 0, workspaceClass) { - return this.getChartArea(tileIndex, workspaceClass).find(`[data-testid="axis-label-editor"] input`); + getYAxisLabelEditor(workspaceClass, tileIndex = 0) { + return this.getChartArea(workspaceClass, tileIndex).find(`[data-testid="axis-label-editor"] input`); } - getXAxisPulldown(tileIndex = 0, workspaceClass) { - return this.getChartArea(tileIndex, workspaceClass).find(`[data-testid="category-pulldown"]`); + getXAxisPulldown(workspaceClass, tileIndex = 0) { + return this.getChartArea(workspaceClass, tileIndex).find(`[data-testid="category-pulldown"]`); } - getXAxisPulldownButton(tileIndex = 0, workspaceClass) { - return this.getXAxisPulldown(tileIndex, workspaceClass).find(`button`); + getXAxisPulldownButton(workspaceClass, tileIndex = 0) { + return this.getXAxisPulldown(workspaceClass, tileIndex).find(`button`); } - getYAxisTickLabel(tileIndex = 0, workspaceClass) { - return this.getChartArea(tileIndex, workspaceClass).find(`.visx-axis-left text`); + getYAxisTickLabel(workspaceClass, tileIndex = 0) { + return this.getChartArea(workspaceClass, tileIndex).find(`.visx-axis-left text`); } - getXAxisTickLabel(tileIndex = 0, workspaceClass) { - return this.getChartArea(tileIndex, workspaceClass).find(`.visx-axis-bottom text`); + getXAxisTickLabel(workspaceClass, tileIndex = 0) { + return this.getChartArea(workspaceClass, tileIndex).find(`.visx-axis-bottom text`); } - getBar(tileIndex = 0, workspaceClass) { - return this.getChartArea(tileIndex, workspaceClass).find(`.visx-bar`); + getBar(workspaceClass, tileIndex = 0) { + return this.getChartArea(workspaceClass, tileIndex).find(`.visx-bar`); } - getLegendArea(tileIndex = 0, workspaceClass) { - return this.getTile(tileIndex, workspaceClass).find(`.bar-graph-legend`); + getLegendArea(workspaceClass, tileIndex = 0) { + return this.getTile(workspaceClass, tileIndex).find(`.bar-graph-legend`); } - getDatasetLabel(tileIndex = 0, workspaceClass) { - return this.getLegendArea(tileIndex, workspaceClass).find(`.dataset-header .dataset-name`); + getDatasetLabel(workspaceClass, tileIndex = 0) { + return this.getLegendArea(workspaceClass, tileIndex).find(`.dataset-header .dataset-name`); } - getDatasetUnlinkButton(tileIndex = 0, workspaceClass) { - return this.getLegendArea(tileIndex, workspaceClass).find(`.dataset-header .dataset-icon a`); + getDatasetUnlinkButton(workspaceClass, tileIndex = 0) { + return this.getLegendArea(workspaceClass, tileIndex).find(`.dataset-header .dataset-icon a`); } - getSortByMenuButton(tileIndex = 0, workspaceClass) { - return this.getLegendArea(tileIndex, workspaceClass).find(`.sort-by button.chakra-menu__menu-button`); + getSortByMenuButton(workspaceClass, tileIndex = 0) { + return this.getLegendArea(workspaceClass, tileIndex).find(`.sort-by button.chakra-menu__menu-button`); } - getSecondaryValueName(tileIndex = 0, workspaceClass) { - return this.getLegendArea(tileIndex, workspaceClass).find(`.secondary-values .secondary-value-name`); + getSecondaryValueName(workspaceClass, tileIndex = 0) { + return this.getLegendArea(workspaceClass, tileIndex).find(`.secondary-values .secondary-value-name`); } } diff --git a/cypress/support/elements/tile/TableToolTile.js b/cypress/support/elements/tile/TableToolTile.js index 5fa38207e..737f14604 100644 --- a/cypress/support/elements/tile/TableToolTile.js +++ b/cypress/support/elements/tile/TableToolTile.js @@ -4,10 +4,10 @@ function wsclass(workspaceClass) { class TableToolTile{ getTableTile(workspaceClass) { - return cy.get(`${wsclass(workspaceClass)} .canvas-area .table-tool`); + return cy.get(`${wsclass(workspaceClass)} .canvas .table-tool`); } getTableTitle(workspaceClass){ - return cy.get(`${wsclass(workspaceClass)} .canvas-area .table-title`); + return cy.get(`${wsclass(workspaceClass)} .canvas .table-title`); } getAddColumnButton(){ return cy.get('.add-column-button'); @@ -39,7 +39,7 @@ class TableToolTile{ this.getRemoveRowButton().click(); } getTableRow(){ - return cy.get('.canvas-area .rdg-row'); + return cy.get('.canvas .rdg-row'); } getColumnHeaderText(i){ return cy.get('.column-header-cell .editable-header-cell .header-name').text(); @@ -89,7 +89,7 @@ class TableToolTile{ this.getTableCell().eq(cell).type(num+'{enter}'); } getTableIndexColumnCell(){ - return cy.get('.canvas-area .rdg-cell.index-column'); + return cy.get('.canvas .rdg-cell.index-column'); } // Fill in a table tile with the given data (a list of lists) // Table tile should in the default state (2 columns, no rows) diff --git a/src/components/doc-editor/doc-editor-app.tsx b/src/components/doc-editor/doc-editor-app.tsx index 2edc3c5f6..0d47d2509 100644 --- a/src/components/doc-editor/doc-editor-app.tsx +++ b/src/components/doc-editor/doc-editor-app.tsx @@ -228,13 +228,13 @@ export const DocEditorApp = observer(function DocEditorApp() { { showLocalReadOnly && <>
Read Only Local
- + } { showRemoteReadOnly && <>
Read Only Remote (emulated)
- + } @@ -243,7 +243,7 @@ export const DocEditorApp = observer(function DocEditorApp() { ); }); -const ReadonlyCanvas = ({document}:{document: DocumentModelType}) => { +const ReadonlyCanvas = ({document, className}:{document: DocumentModelType, className: string}) => { const readOnlyScale = 0.5; const scaledStyle = { position: "absolute", @@ -254,7 +254,7 @@ const ReadonlyCanvas = ({document}:{document: DocumentModelType}) => { } as const; return ( -
+
= observer((props: ITilePro
diff --git a/src/plugins/bar-graph/chart-area.tsx b/src/plugins/bar-graph/chart-area.tsx index 5b20193d4..e0b37d3b1 100644 --- a/src/plugins/bar-graph/chart-area.tsx +++ b/src/plugins/bar-graph/chart-area.tsx @@ -38,7 +38,6 @@ export const ChartArea = observer(function BarGraphChart({ width, height }: IPro function setPrimaryAttribute(id: string) { model?.setPrimaryAttribute(id); - model?.setSecondaryAttribute(undefined); } function barColor(key: string) { @@ -179,8 +178,6 @@ export const ChartArea = observer(function BarGraphChart({ width, height }: IPro model?.setYAxisLabel(text)} /> void; } -const EditableAxisLabel: React.FC = ({text, x, y, setText}) => { - +const EditableAxisLabel: React.FC = observer(function EditableAxisLabel({x, y}) { + const model = useBarGraphModelContext(); const readOnly = useReadOnlyContext(); const textRef = React.useRef(null); const [boundingBox, setBoundingBox] = React.useState(null); const [editing, setEditing] = React.useState(false); - const [displayText, setDisplayText] = React.useState(text || "Y axis"); - const [editText, setEditText] = React.useState(text || "Y axis"); + const [editText, setEditText] = React.useState(""); + + const displayText = model?.yAxisLabel || ""; useEffect(() => { if (textRef.current) { @@ -28,12 +29,18 @@ const EditableAxisLabel: React.FC = ({text, x, y, setText}) => { } }, [x, y, displayText, textRef]); + const handleStartEdit = () => { + if (!readOnly) { + setEditText(displayText); + setEditing(true); + } + }; + const handleClose = (accept: boolean) => { setEditing(false); if (accept && editText) { const trimmed = editText.trim(); - setDisplayText(trimmed); - setText(trimmed); + model?.setYAxisLabel(trimmed); } }; @@ -81,7 +88,7 @@ const EditableAxisLabel: React.FC = ({text, x, y, setText}) => { strokeWidth={1.5} fill="none" pointerEvents={editing ? "none" : "all"} - onClick={() => { if (!readOnly) setEditing(true); }} + onClick={handleStartEdit} />} = ({text, x, y, setText}) => { ); -}; +}); export default EditableAxisLabel; diff --git a/src/plugins/bar-graph/legend-area.tsx b/src/plugins/bar-graph/legend-area.tsx index ccb9654d7..f0e32dab5 100644 --- a/src/plugins/bar-graph/legend-area.tsx +++ b/src/plugins/bar-graph/legend-area.tsx @@ -6,6 +6,7 @@ import { LegendSecondaryRow } from './legend-secondary-row'; import RemoveDataIcon from "../../assets/remove-data-icon.svg"; import DropdownCaretIcon from "../../assets/dropdown-caret.svg"; +import { useReadOnlyContext } from '../../components/document/read-only-context'; interface IProps { legendRef: React.RefObject; @@ -13,9 +14,10 @@ interface IProps { export const LegendArea = observer(function LegendArea ({legendRef}: IProps) { const model = useBarGraphModelContext(); + const readOnly = useReadOnlyContext(); function unlinkDataset() { - if (model) { + if (!readOnly && model) { model.unlinkDataSet(); } } @@ -68,9 +70,11 @@ export const LegendArea = observer(function LegendArea ({legendRef}: IProps) { - setSecondaryAttribute(undefined)}>None + setSecondaryAttribute(undefined)}>None {availableAttributes.map((a) => ( - setSecondaryAttribute(a.id)}>{a.name} + setSecondaryAttribute(a.id)}> + {a.name} + ))} From 9ec6bc0d0875ec664e1bdefc0dafd34a8d0098cc Mon Sep 17 00:00:00 2001 From: Boris Goldowsky Date: Fri, 13 Sep 2024 14:50:17 -0400 Subject: [PATCH 4/7] Logging and logging tests --- .../tile_tests/bar_graph_tile_spec.js | 31 +++++++++++++++++++ src/lib/logger-types.ts | 1 + src/plugins/bar-graph/bar-graph-utils.ts | 23 +++++++++++++- src/plugins/bar-graph/chart-area.tsx | 7 +++-- src/plugins/bar-graph/editable-axis-label.tsx | 15 ++++----- src/plugins/bar-graph/legend-area.tsx | 14 ++++++++- 6 files changed, 80 insertions(+), 11 deletions(-) diff --git a/cypress/e2e/functional/tile_tests/bar_graph_tile_spec.js b/cypress/e2e/functional/tile_tests/bar_graph_tile_spec.js index 19711e72c..6fe0db939 100644 --- a/cypress/e2e/functional/tile_tests/bar_graph_tile_spec.js +++ b/cypress/e2e/functional/tile_tests/bar_graph_tile_spec.js @@ -3,6 +3,9 @@ import Canvas from '../../../support/elements/common/Canvas'; import BarGraphTile from '../../../support/elements/tile/BarGraphTile'; import TableToolTile from '../../../support/elements/tile/TableToolTile'; +import { LogEventName } from "../../../../src/lib/logger-types"; + + let clueCanvas = new ClueCanvas, barGraph = new BarGraphTile, tableTile = new TableToolTile; @@ -22,6 +25,9 @@ function textMatchesList(selector, expected) { function beforeTest() { const url = "/editor/?appMode=qa&unit=./demo/units/qa/content.json"; cy.visit(url); + cy.window().then(win => { + cy.stub(win.ccLogger, "log").as("log"); + }); } context('Bar Graph Tile', function () { @@ -43,6 +49,10 @@ context('Bar Graph Tile', function () { barGraph.getTile(workspaces[1]).should('have.class', 'readonly'); barGraph.getTile(workspaces[2]).should('have.class', 'readonly'); + cy.get("@log") + .should("have.been.been.calledWith", LogEventName.CREATE_TILE, Cypress.sinon.match.object) + .its("firstCall.args.1").should("deep.include", { objectType: "BarGraph" }); + // Undo/redo tile creation clueCanvas.getUndoTool().click(); for (const workspace of workspaces) { @@ -62,6 +72,9 @@ context('Bar Graph Tile', function () { barGraph.getYAxisLabel(workspace).should('have.text', 'Counts of something'); } + cy.get("@log").its("lastCall.args.0").should("equal", LogEventName.BARGRAPH_TOOL_CHANGE); + cy.get("@log").its("lastCall.args.1").should("deep.include", { operation: "setYAxisLabel", text: "Counts of something" }); + // Undo/redo label change clueCanvas.getUndoTool().click(); barGraph.getYAxisLabel().should('have.text', 'Counts'); @@ -87,6 +100,9 @@ context('Bar Graph Tile', function () { barGraph.getXAxisPulldownButton(workspace, 1).should('have.text', 'Categories'); } + cy.get("@log").its("lastCall.args.0").should("equal", LogEventName.COPY_TILE); + cy.get("@log").its("lastCall.args.1").should("deep.include", { objectType: "BarGraph" }); + // Undo/redo tile duplication clueCanvas.getUndoTool().click(); for (const workspace of workspaces) { @@ -104,6 +120,9 @@ context('Bar Graph Tile', function () { barGraph.getTiles(workspace).should('have.length', 0); } + cy.get("@log").its("lastCall.args.0").should("equal", LogEventName.DELETE_TILE); + cy.get("@log").its("lastCall.args.1").should("deep.include", { objectType: "BarGraph" }); + // Undo/redo tile deletion clueCanvas.getUndoTool().click(); for (const workspace of workspaces) { @@ -167,6 +186,9 @@ context('Bar Graph Tile', function () { barGraph.getSecondaryValueName(workspace).should('have.length', 1).and('have.text', 'x'); } + cy.get("@log").its("lastCall.args.0").should("equal", LogEventName.TILE_LINK); + cy.get("@log").its("lastCall.args.1").should("nested.include", { "sourceTile.type": "BarGraph", "sharedModel.type": "SharedDataSet" }); + // Undo/redo linking clueCanvas.getUndoTool().click(); for (const workspace of workspaces) { @@ -210,6 +232,9 @@ context('Bar Graph Tile', function () { textMatchesList(barGraph.getSecondaryValueName(workspace), ['Y', 'YY']); } + cy.get("@log").its("lastCall.args.0").should("equal", LogEventName.BARGRAPH_TOOL_CHANGE); + cy.get("@log").its("lastCall.args.1").should("deep.include", { operation: "setSecondaryAttribute" }); + // Undo-redo sort by clueCanvas.getUndoTool().click(); for (const workspace of workspaces) { @@ -238,6 +263,9 @@ context('Bar Graph Tile', function () { barGraph.getSecondaryValueName(workspace).should('have.length', 1).and('have.text', 'y'); } + cy.get("@log").its("lastCall.args.0").should("equal", LogEventName.BARGRAPH_TOOL_CHANGE); + cy.get("@log").its("lastCall.args.1").should("deep.include", { operation: "setPrimaryAttribute" }); + // Undo-redo category change clueCanvas.getUndoTool().click(); for (const workspace of workspaces) { @@ -260,6 +288,9 @@ context('Bar Graph Tile', function () { barGraph.getBar(workspace).should('not.exist'); } + cy.get("@log").its("lastCall.args.0").should("equal", LogEventName.TILE_UNLINK); + cy.get("@log").its("lastCall.args.1").should("nested.include", { "sourceTile.type": "BarGraph", "sharedModel.type": "SharedDataSet" }); + // Undo-redo unlink clueCanvas.getUndoTool().click(); for (const workspace of workspaces) { diff --git a/src/lib/logger-types.ts b/src/lib/logger-types.ts index a566aa6b7..a0d77d3a0 100644 --- a/src/lib/logger-types.ts +++ b/src/lib/logger-types.ts @@ -32,6 +32,7 @@ export enum LogEventName { HIDE_SOLUTIONS, SHOW_SOLUTIONS, + BARGRAPH_TOOL_CHANGE, GEOMETRY_TOOL_CHANGE, DRAWING_TOOL_CHANGE, TABLE_TOOL_CHANGE, diff --git a/src/plugins/bar-graph/bar-graph-utils.ts b/src/plugins/bar-graph/bar-graph-utils.ts index e0a5c02e1..e5a1d8681 100644 --- a/src/plugins/bar-graph/bar-graph-utils.ts +++ b/src/plugins/bar-graph/bar-graph-utils.ts @@ -1,7 +1,10 @@ import { SnapshotOut } from "mobx-state-tree"; +import { LogEventName } from "../../lib/logger-types"; import { SharedModelEntrySnapshotType } from "../../models/document/shared-model-entry"; import { replaceJsonStringsWithUpdatedIds, UpdatedSharedDataSetIds } from "../../models/shared/shared-data-set"; -import { BarGraphContentModel } from "./bar-graph-content"; +import { logTileChangeEvent } from "../../models/tiles/log/log-tile-change-event"; +import { getTileIdFromContent } from "../../models/tiles/tile-model"; +import { BarGraphContentModel, BarGraphContentModelType } from "./bar-graph-content"; const kMissingValueString = "(no value)"; @@ -32,3 +35,21 @@ export function updateBarGraphContentWithNewSharedModelIds( ) { return replaceJsonStringsWithUpdatedIds(content, '"', ...Object.values(updatedSharedModelMap)); } + +// Define types here to document all possible values that this tile logs +type LoggableOperation = "setPrimaryAttribute" | "setSecondaryAttribute" | "setYAxisLabel"; +type LoggableChange = { + attributeId?: string; + text?: string; +}; + +export function logBarGraphEvent( + model: BarGraphContentModelType, operation: LoggableOperation, change: LoggableChange) { + const tileId = getTileIdFromContent(model) || ""; + + logTileChangeEvent(LogEventName.BARGRAPH_TOOL_CHANGE, { + tileId, + operation, + change + }); +} diff --git a/src/plugins/bar-graph/chart-area.tsx b/src/plugins/bar-graph/chart-area.tsx index e0b37d3b1..f25099718 100644 --- a/src/plugins/bar-graph/chart-area.tsx +++ b/src/plugins/bar-graph/chart-area.tsx @@ -8,7 +8,7 @@ import { Bar, BarGroup } from "@visx/shape"; import { useBarGraphModelContext } from "./bar-graph-content-context"; import { CategoryPulldown } from "./category-pulldown"; import EditableAxisLabel from "./editable-axis-label"; -import { roundTo5 } from "./bar-graph-utils"; +import { logBarGraphEvent, roundTo5 } from "./bar-graph-utils"; const margin = { top: 7, @@ -37,7 +37,10 @@ export const ChartArea = observer(function BarGraphChart({ width, height }: IPro const yMax = height - margin.top - margin.bottom; function setPrimaryAttribute(id: string) { - model?.setPrimaryAttribute(id); + if (model) { + model.setPrimaryAttribute(id); + logBarGraphEvent(model, "setPrimaryAttribute", { attributeId: id }); + } } function barColor(key: string) { diff --git a/src/plugins/bar-graph/editable-axis-label.tsx b/src/plugins/bar-graph/editable-axis-label.tsx index fe6d62a0e..6af8758e3 100644 --- a/src/plugins/bar-graph/editable-axis-label.tsx +++ b/src/plugins/bar-graph/editable-axis-label.tsx @@ -1,7 +1,7 @@ import React, { useEffect } from 'react'; import { observer } from 'mobx-react'; import { Text } from '@visx/text'; -import { getBBox } from './bar-graph-utils'; +import { getBBox, logBarGraphEvent } from './bar-graph-utils'; import { useReadOnlyContext } from '../../components/document/read-only-context'; import { useBarGraphModelContext } from './bar-graph-content-context'; @@ -36,11 +36,12 @@ const EditableAxisLabel: React.FC = observer(function EditableAxisLabel( } }; - const handleClose = (accept: boolean) => { + const handleEndEdit = (accept: boolean) => { setEditing(false); - if (accept && editText) { + if (model && accept && editText) { const trimmed = editText.trim(); - model?.setYAxisLabel(trimmed); + model.setYAxisLabel(trimmed); + logBarGraphEvent(model, "setYAxisLabel", { text: trimmed }); } }; @@ -48,11 +49,11 @@ const EditableAxisLabel: React.FC = observer(function EditableAxisLabel( const { key } = e; switch (key) { case "Escape": - handleClose(false); + handleEndEdit(false); break; case "Enter": case "Tab": - handleClose(true); + handleEndEdit(true); break; } }; @@ -66,7 +67,7 @@ const EditableAxisLabel: React.FC = observer(function EditableAxisLabel( value={editText} size={editText.length + 5} onKeyDown={handleKeyDown} - onBlur={() => handleClose(true)} + onBlur={() => handleEndEdit(true)} onChange={(e) => setEditText(e.target.value)} /> diff --git a/src/plugins/bar-graph/legend-area.tsx b/src/plugins/bar-graph/legend-area.tsx index f0e32dab5..7625c98e9 100644 --- a/src/plugins/bar-graph/legend-area.tsx +++ b/src/plugins/bar-graph/legend-area.tsx @@ -7,24 +7,36 @@ import { LegendSecondaryRow } from './legend-secondary-row'; import RemoveDataIcon from "../../assets/remove-data-icon.svg"; import DropdownCaretIcon from "../../assets/dropdown-caret.svg"; import { useReadOnlyContext } from '../../components/document/read-only-context'; +import { logBarGraphEvent } from './bar-graph-utils'; +import { logSharedModelDocEvent } from '../../models/document/log-shared-model-document-event'; +import { LogEventName } from '../../lib/logger-types'; +import { useTileModelContext } from '../../components/tiles/hooks/use-tile-model-context'; +import { getSharedModelManager } from '../../models/tiles/tile-environment'; interface IProps { legendRef: React.RefObject; } export const LegendArea = observer(function LegendArea ({legendRef}: IProps) { + const { tile } = useTileModelContext(); const model = useBarGraphModelContext(); const readOnly = useReadOnlyContext(); function unlinkDataset() { - if (!readOnly && model) { + const sharedModel = model?.sharedModel; + if (!readOnly && sharedModel) { model.unlinkDataSet(); + if (tile) { + const sharedTiles = getSharedModelManager()?.getSharedModelProviders(sharedModel) || []; + logSharedModelDocEvent(LogEventName.TILE_UNLINK, tile, sharedTiles, sharedModel); + } } } function setSecondaryAttribute(attributeId: string|undefined) { if (model) { model.setSecondaryAttribute(attributeId); + logBarGraphEvent(model, "setSecondaryAttribute", { attributeId }); } } From 9c22dc60c3b00eb6093744886cb1a04a3d4b9479 Mon Sep 17 00:00:00 2001 From: Boris Goldowsky Date: Fri, 13 Sep 2024 14:50:30 -0400 Subject: [PATCH 5/7] Manage deletion of attributes --- src/plugins/bar-graph/bar-graph-content.ts | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/plugins/bar-graph/bar-graph-content.ts b/src/plugins/bar-graph/bar-graph-content.ts index f69e75776..60a157198 100644 --- a/src/plugins/bar-graph/bar-graph-content.ts +++ b/src/plugins/bar-graph/bar-graph-content.ts @@ -161,9 +161,16 @@ export const BarGraphContentModel = TileContentModel self.setPrimaryAttribute(atts[0].id); } } + } + // Check if primary or secondary attribute has been deleted + if (self.primaryAttribute && !self.sharedModel?.dataSet.attrFromID(self.primaryAttribute)) { + self.setPrimaryAttribute(undefined); // this will also unset secondaryAttribute + } + if (self.secondaryAttribute && !self.sharedModel?.dataSet.attrFromID(self.secondaryAttribute)) { + self.setSecondaryAttribute(undefined); + } } - } -})); + })); export interface BarGraphContentModelType extends Instance {} From 858954bdb61388a94e66f62cc52da539db4ce439 Mon Sep 17 00:00:00 2001 From: Boris Goldowsky Date: Fri, 13 Sep 2024 15:21:47 -0400 Subject: [PATCH 6/7] Fix table test --- cypress/support/elements/tile/TableToolTile.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cypress/support/elements/tile/TableToolTile.js b/cypress/support/elements/tile/TableToolTile.js index 737f14604..68fb922ff 100644 --- a/cypress/support/elements/tile/TableToolTile.js +++ b/cypress/support/elements/tile/TableToolTile.js @@ -89,7 +89,7 @@ class TableToolTile{ this.getTableCell().eq(cell).type(num+'{enter}'); } getTableIndexColumnCell(){ - return cy.get('.canvas .rdg-cell.index-column'); + return this.getTableTile().find('.rdg-cell.index-column'); } // Fill in a table tile with the given data (a list of lists) // Table tile should in the default state (2 columns, no rows) From f32d8144a2f8b9a93ee84c78aae3f286f40adc92 Mon Sep 17 00:00:00 2001 From: Boris Goldowsky Date: Mon, 16 Sep 2024 11:02:13 -0400 Subject: [PATCH 7/7] More tests --- .../tile_tests/bar_graph_tile_spec.js | 30 +++++++++++++++++++ src/plugins/graph/graph-registration.ts | 2 +- 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/cypress/e2e/functional/tile_tests/bar_graph_tile_spec.js b/cypress/e2e/functional/tile_tests/bar_graph_tile_spec.js index 6fe0db939..57a15d2fe 100644 --- a/cypress/e2e/functional/tile_tests/bar_graph_tile_spec.js +++ b/cypress/e2e/functional/tile_tests/bar_graph_tile_spec.js @@ -81,6 +81,18 @@ context('Bar Graph Tile', function () { clueCanvas.getRedoTool().click(); barGraph.getYAxisLabel().should('have.text', 'Counts of something'); + // ESC key should cancel the edit + barGraph.getYAxisLabelButton().click(); + barGraph.getYAxisLabelEditor().should('be.visible').type(' abandon this{esc}'); + barGraph.getYAxisLabelEditor().should('not.exist'); + barGraph.getYAxisLabel().should('have.text', 'Counts of something'); + + // Should not be able to change Y axis label in read-only views + barGraph.getYAxisLabelButton(workspaces[1]).click(); + barGraph.getYAxisLabelEditor(workspaces[1]).should('not.exist'); + barGraph.getYAxisLabelButton(workspaces[2]).click(); + barGraph.getYAxisLabelEditor(workspaces[2]).should('not.exist'); + cy.log('Duplicate tile'); clueCanvas.getDuplicateTool().click(); for (const workspace of workspaces) { @@ -220,6 +232,16 @@ context('Bar Graph Tile', function () { cy.log('Change Sort By'); barGraph.getSortByMenuButton().should('have.text', 'None'); + + // Cannot change sort by in read-only views + for (const workspace of workspaces.slice(1)) { + barGraph.getSortByMenuButton(workspace).click(); + barGraph.getChakraMenuItem(workspace).should('have.length', 3); + barGraph.getChakraMenuItem(workspace).eq(1).should('have.text', 'y'); // menu exists + barGraph.getChakraMenuItem(workspace).should('be.disabled'); // all options disabled + barGraph.getSortByMenuButton(workspace).click(); // close menu + } + barGraph.getSortByMenuButton().click(); barGraph.getChakraMenuItem().should('have.length', 3); barGraph.getChakraMenuItem().eq(1).should('have.text', 'y').click(); @@ -250,6 +272,14 @@ context('Bar Graph Tile', function () { } cy.log('Change Category'); + + // Cannot change category in read-only views + for (const workspace of workspaces.slice(1)) { + barGraph.getXAxisPulldownButton(workspace).click(); + barGraph.getChakraMenuItem(workspace).should('have.length', 3).and('be.disabled'); + barGraph.getXAxisPulldownButton(workspace).click(); // close menu + } + barGraph.getXAxisPulldownButton().click(); barGraph.getChakraMenuItem().should('have.length', 3); barGraph.getChakraMenuItem().eq(1).should('have.text', 'y').click(); diff --git a/src/plugins/graph/graph-registration.ts b/src/plugins/graph/graph-registration.ts index b69a54df0..b09eb9529 100644 --- a/src/plugins/graph/graph-registration.ts +++ b/src/plugins/graph/graph-registration.ts @@ -5,7 +5,7 @@ import { GraphWrapperComponent } from "./components/graph-wrapper-component"; import { createGraphModel, GraphModel } from "./models/graph-model"; import { updateGraphContentWithNewSharedModelIds, updateGraphObjectWithNewSharedModelIds } from "./utilities/graph-utils"; - import { AppConfigModelType } from "../../models/stores/app-config-model"; +import { AppConfigModelType } from "../../models/stores/app-config-model"; import Icon from "./assets/graph-icon.svg"; import HeaderIcon from "./assets/graph-tile-id.svg";