(core) Fixing layout errors after deleting widgets

Summary: Deleting widget from a page layout was creating errors in the json definition, it wasn't updated so it had ids of invalid sections. Normally those errors, are handled by ViewLayout, but when the page is duplicated ViewLayout can't fix those properly, as they are sometimes duplicated during mapping. Test Plan: Added test Reviewers: georgegevoian Reviewed By: georgegevoian Differential Revision: https://phab.getgrist.com/D4388
gristlabs · Oct 28, 2024 · 6a6968b · 6a6968b
1 parent 599110f
commit 6a6968b
Show file tree

Hide file tree

Showing 11 changed files with 168 additions and 87 deletions.
diff --git a/app/client/components/Layout.ts b/app/client/components/Layout.ts
@@ -53,6 +53,7 @@
  */
 
 
+import {BoxSpec} from 'app/client/lib/BoxSpec';
 import dom, {detachNode, findAncestor} from 'app/client/lib/dom';
 import koArray, {isKoArray, KoArray} from 'app/client/lib/koArray';
 import {cssClass, domData, foreach, scope, style, toggleClass} from 'app/client/lib/koDom';
@@ -69,13 +70,6 @@ export interface ContentBox {
   dom: HTMLElement|null;
 }
 
-export interface BoxSpec {
-  leaf?: string|number;
-  size?: number;
-  children?: BoxSpec[];
-  collapsed?: BoxSpec[];
-}
-
 /**
  * A LayoutBox is the node in the hierarchy of boxes comprising the layout. This class is used for
  * rendering as well as for the code editor. Since it may be rendered many times on a page, it's

diff --git a/app/client/components/LayoutEditor.ts b/app/client/components/LayoutEditor.ts
@@ -838,8 +838,11 @@ function resizeLayoutBoxSmoothly(layoutBox: LayoutBox, startRect: string|DOMRect
     // usable and not cause errors elsewhere.
   })
   .finally(function() {
-    layoutBox.dom!.classList.remove('layout_editor_resize_transition');
-    layoutBox.dom!.style.flexGrow = prevFlexGrow;
+    if (!layoutBox.dom) {
+      return;
+    }
+    layoutBox.dom.classList.remove('layout_editor_resize_transition');
+    layoutBox.dom.style.flexGrow = prevFlexGrow;
   });
 }
 

diff --git a/app/client/components/ViewLayout.ts b/app/client/components/ViewLayout.ts
@@ -8,10 +8,11 @@ import * as DetailView from 'app/client/components/DetailView';
 import {FormView} from 'app/client/components/Forms/FormView';
 import * as GridView from 'app/client/components/GridView';
 import {GristDoc} from 'app/client/components/GristDoc';
-import {BoxSpec, Layout} from 'app/client/components/Layout';
+import {Layout} from 'app/client/components/Layout';
 import {LayoutEditor} from 'app/client/components/LayoutEditor';
 import {LayoutTray} from 'app/client/components/LayoutTray';
 import {printViewSection} from 'app/client/components/Printing';
+import {BoxSpec, purgeBoxSpec} from 'app/client/lib/BoxSpec';
 import {Delay} from 'app/client/lib/Delay';
 import {createObsArray} from 'app/client/lib/koArrayWrap';
 import {logTelemetryEvent} from 'app/client/lib/telemetry';
@@ -42,7 +43,6 @@ import {
 } from 'grainjs';
 import * as ko from 'knockout';
 import debounce from 'lodash/debounce';
-import * as _ from 'underscore';
 
 const t = makeT('ViewLayout');
 
@@ -385,53 +385,10 @@ export class ViewLayout extends DisposableWithEvents implements IDomComponent {
    * this view section. By default we just arrange them into a list of rows, two fields per row.
    */
   private _updateLayoutSpecWithSections(spec: BoxSpec) {
-    // We use tmpLayout as a way to manipulate the layout before we get a final spec from it.
-    const tmpLayout = Layout.create(spec, () => dom('div'), true);
-
-    const specFieldIds = tmpLayout.getAllLeafIds();
     const viewSectionIds = this.viewModel.viewSections().all().map(function(f) { return f.getRowId(); });
-
-    function addToSpec(leafId: number) {
-      const newBox = tmpLayout.buildLayoutBox({ leaf: leafId });
-      const root = tmpLayout.rootBox();
-      if (!root || root.isDisposed()) {
-        tmpLayout.setRoot(newBox);
-        return newBox;
-      }
-      const rows = root.childBoxes.peek();
-      const lastRow = rows[rows.length - 1];
-      if (rows.length >= 1 && lastRow.isLeaf()) {
-        // Add a new child to the last row.
-        lastRow.addChild(newBox, true);
-      } else {
-        // Add a new row.
-        tmpLayout.rootBox()!.addChild(newBox, true);
-      }
-      return newBox;
-    }
-
-    // For any stale fields (no longer among viewFields), remove them from tmpLayout.
-    _.difference(specFieldIds, viewSectionIds).forEach(function(leafId: string|number) {
-      tmpLayout.getLeafBox(leafId)?.dispose();
-    });
-
-    // For all fields that should be in the spec but aren't, add them to tmpLayout. We maintain a
-    // two-column layout, so add a new row, or a second box to the last row if it's a leaf.
-    const missingLeafs = _.difference(viewSectionIds, specFieldIds);
-    const collapsedLeafs = new Set((spec.collapsed || []).map(c => c.leaf));
-    missingLeafs.forEach(function(leafId: any) {
-      if (!collapsedLeafs.has(leafId)) {
-        addToSpec(leafId);
-      }
-    });
-
-    spec = tmpLayout.getLayoutSpec();
-    tmpLayout.dispose();
-    return spec;
+    return purgeBoxSpec({spec, validLeafIds: viewSectionIds});
   }
 
-
-
   // Resizes the scrolly windows of all viewSection classes with a 'scrolly' property.
   private _onResize() {
     this.viewModel.viewSections().all().forEach(vs => {

diff --git a/app/client/components/duplicatePage.ts b/app/client/components/duplicatePage.ts
@@ -1,31 +1,32 @@
-import { GristDoc } from 'app/client/components/GristDoc';
-import { logTelemetryEvent } from 'app/client/lib/telemetry';
-import { ViewFieldRec, ViewSectionRec } from 'app/client/models/DocModel';
-import { cssInput } from 'app/client/ui/cssInput';
-import { cssField, cssLabel } from 'app/client/ui/MakeCopyMenu';
-import { IPageWidget, toPageWidget } from 'app/client/ui/PageWidgetPicker';
-import { confirmModal } from 'app/client/ui2018/modals';
-import { BulkColValues, getColValues, RowRecord, UserAction } from 'app/common/DocActions';
-import { arrayRepeat } from 'app/common/gutil';
-import { schema } from 'app/common/schema';
-import { dom } from 'grainjs';
-import cloneDeepWith = require('lodash/cloneDeepWith');
-import flatten = require('lodash/flatten');
-import forEach = require('lodash/forEach');
-import zip = require('lodash/zip');
-import zipObject = require('lodash/zipObject');
+import {GristDoc} from 'app/client/components/GristDoc';
+import {BoxSpec, purgeBoxSpec} from 'app/client/lib/BoxSpec';
 import {makeT} from 'app/client/lib/localization';
+import {logTelemetryEvent} from 'app/client/lib/telemetry';
+import {ViewFieldRec, ViewSectionRec} from 'app/client/models/DocModel';
+import {cssInput} from 'app/client/ui/cssInput';
+import {cssField, cssLabel} from 'app/client/ui/MakeCopyMenu';
+import {IPageWidget, toPageWidget} from 'app/client/ui/PageWidgetPicker';
+import {confirmModal} from 'app/client/ui2018/modals';
+import {BulkColValues, getColValues, RowRecord, UserAction} from 'app/common/DocActions';
+import {arrayRepeat} from 'app/common/gutil';
+import {schema} from 'app/common/schema';
+import {dom} from 'grainjs';
+import cloneDeepWith from 'lodash/cloneDeepWith';
+import flatten from 'lodash/flatten';
+import forEach from 'lodash/forEach';
+import zip from 'lodash/zip';
+import zipObject from 'lodash/zipObject';
 
 const t = makeT('duplicatePage');
 
 // Duplicate page with pageId. Starts by prompting user for a new name.
-export async function duplicatePage(gristDoc: GristDoc, pageId: number) {
+export async function buildDuplicatePageDialog(gristDoc: GristDoc, pageId: number) {
   const pagesTable = gristDoc.docModel.pages;
   const pageName = pagesTable.rowModels[pageId].view.peek().name.peek();
   let inputEl: HTMLInputElement;
   setTimeout(() => { inputEl.focus(); inputEl.select(); }, 100);
 
-  confirmModal('Duplicate page', 'Save', () => makeDuplicate(gristDoc, pageId, inputEl.value), {
+  confirmModal('Duplicate page', 'Save', () => duplicatePage(gristDoc, pageId, inputEl.value), {
     explanation: dom('div', [
       cssField(
         cssLabel("Name"),
@@ -36,7 +37,10 @@ export async function duplicatePage(gristDoc: GristDoc, pageId: number) {
   });
 }
 
-async function makeDuplicate(gristDoc: GristDoc, pageId: number, pageName: string = '') {
+/**
+ * Duplicates page recreating all sections that are on it.
+ */
+async function duplicatePage(gristDoc: GristDoc, pageId: number, pageName: string = '') {
   const sourceView = gristDoc.docModel.pages.rowModels[pageId].view.peek();
   pageName = pageName || `${sourceView.name.peek()} (copy)`;
   const viewSections = sourceView.viewSections.peek().peek();
@@ -208,7 +212,14 @@ function newViewSectionAction(widget: IPageWidget, viewId: number) {
 *      collapsed: [{leaf: 2}]
  *   }, {1: 10, 2: 20})
  */
-export function patchLayoutSpec(layoutSpec: any, mapIds: {[id: number]: number}) {
+export function patchLayoutSpec(layoutSpec: BoxSpec, mapIds: {[id: number]: number}) {
+  // First remove any invalid ids from the layoutSpec. We are doing the same thing what
+  // `ViewLayout` does when it load itself.
+  layoutSpec = purgeBoxSpec({
+    spec: layoutSpec,
+    validLeafIds: Object.keys(mapIds).map(Number),
+    restoreCollapsed: true
+  });
   const cloned = cloneDeepWith(layoutSpec, (val, key) => {
     if (key === 'leaf' && mapIds[val]) {
       return mapIds[val];

diff --git a/app/client/lib/BoxSpec.ts b/app/client/lib/BoxSpec.ts
@@ -0,0 +1,66 @@
+import {Layout} from 'app/client/components/Layout';
+import {dom} from 'grainjs';
+import * as _ from 'underscore';
+
+export interface BoxSpec {
+  leaf?: string|number;
+  size?: number;
+  children?: BoxSpec[];
+  collapsed?: BoxSpec[];
+}
+
+export function purgeBoxSpec(options: {
+  spec: BoxSpec;
+  validLeafIds: number[];
+  restoreCollapsed?: boolean;
+}): BoxSpec {
+  const {spec, validLeafIds, restoreCollapsed} = options;
+  // We use tmpLayout as a way to manipulate the layout before we get a final spec from it.
+  const tmpLayout = Layout.create(spec, () => dom('div'), true);
+  const specFieldIds = tmpLayout.getAllLeafIds();
+
+  // For any stale fields (no longer among validLeafIds), remove them from tmpLayout.
+  _.difference(specFieldIds, validLeafIds).forEach(function(leafId: string | number) {
+    tmpLayout.getLeafBox(leafId)?.dispose();
+  });
+
+  // For all fields that should be in the spec but aren't, add them to tmpLayout. We maintain a
+  // two-column layout, so add a new row, or a second box to the last row if it's a leaf.
+  const missingLeafs = _.difference(validLeafIds, specFieldIds);
+  const collapsedLeafs = new Set((spec.collapsed || []).map(c => c.leaf));
+  missingLeafs.forEach(function(leafId: any) {
+    // Omit collapsed leafs from the spec.
+    if (!collapsedLeafs.has(leafId)) {
+      addToSpec(tmpLayout, leafId);
+    }
+  });
+
+  const newSpec = tmpLayout.getLayoutSpec();
+
+  // Restore collapsed state, omitting any leafs that are no longer valid.
+  if (spec.collapsed && restoreCollapsed) {
+    newSpec.collapsed = spec.collapsed.filter(c => c.leaf && validLeafIds.includes(c.leaf as number));
+  }
+
+  tmpLayout.dispose();
+  return newSpec;
+}
+
+function addToSpec(tmpLayout: Layout, leafId: number) {
+  const newBox = tmpLayout.buildLayoutBox({leaf: leafId});
+  const root = tmpLayout.rootBox();
+  if (!root || root.isDisposed()) {
+    tmpLayout.setRoot(newBox);
+    return newBox;
+  }
+  const rows = root.childBoxes.peek();
+  const lastRow = rows[rows.length - 1];
+  if (rows.length >= 1 && lastRow.isLeaf()) {
+    // Add a new child to the last row.
+    lastRow.addChild(newBox, true);
+  } else {
+    // Add a new row.
+    tmpLayout.rootBox()!.addChild(newBox, true);
+  }
+  return newBox;
+}
diff --git a/app/client/models/entities/ViewRec.ts b/app/client/models/entities/ViewRec.ts
@@ -1,4 +1,4 @@
-import {BoxSpec} from 'app/client/components/Layout';
+import {BoxSpec} from 'app/client/lib/BoxSpec';
 import {KoArray} from 'app/client/lib/koArray';
 import * as koUtil from 'app/client/lib/koUtil';
 import {DocModel, IRowModel, PageRec, recordSet, refRecord} from 'app/client/models/DocModel';

diff --git a/app/client/ui/Pages.ts b/app/client/ui/Pages.ts
@@ -1,5 +1,5 @@
 import {createGroup} from 'app/client/components/commands';
-import {duplicatePage} from 'app/client/components/duplicatePage';
+import {buildDuplicatePageDialog} from 'app/client/components/duplicatePage';
 import {GristDoc} from 'app/client/components/GristDoc';
 import {makeT} from 'app/client/lib/localization';
 import {logTelemetryEvent} from 'app/client/lib/telemetry';
@@ -73,8 +73,7 @@ function buildDomFromTable(pagesTable: MetaTableModel<PageRec>, activeDoc: Grist
   const actions: PageActions = {
     onRename: (newName: string) => newName.length && pageName.saveOnly(newName),
     onRemove: () => removeView(activeDoc, viewId, pageName.peek()),
-    // TODO: duplicate should prompt user for confirmation
-    onDuplicate: () => duplicatePage(activeDoc, pageId),
+    onDuplicate: () => buildDuplicatePageDialog(activeDoc, pageId),
     // Can't remove last visible page
     isRemoveDisabled: () => activeDoc.docModel.visibleDocPages.peek().length <= 1,
     isReadonly

diff --git a/test/nbrowser/SelectBySummary.ts b/test/nbrowser/SelectBySummary.ts
@@ -178,7 +178,7 @@ describe('SelectBySummary', function() {
 
   it('should filter a summary table selected by a less detailed summary table', async function() {
     // Delete the Table1 widget so that we can hide the table in ACL without hiding the whole page.
-    await gu.deleteWidget('TABLE1');
+    await gu.deleteWidgetWithData('TABLE1');
 
     // Open the ACL UI
     await driver.find('.test-tools-access-rules').click();

diff --git a/test/nbrowser/ViewLayoutCollapse.ts b/test/nbrowser/ViewLayoutCollapse.ts
@@ -15,7 +15,7 @@ describe("ViewLayoutCollapse", function() {
   let session: gu.Session;
 
   before(async () => {
-    session = await gu.session().login();
+    session = await gu.session().teamSite.login();
     await session.tempNewDoc(cleanup);
   });
 
@@ -58,6 +58,8 @@ describe("ViewLayoutCollapse", function() {
 
     // Now on this page we saw two uncollapsed sections (make sure this is not the case).
     assert.deepEqual(await gu.getSectionTitles(), ['NotCollapsed']);
+    // And one collapsed.
+    assert.deepEqual(await collapsedSectionTitles(), ['Collapsed']);
   });
 
   it('fix:can delete root section', async function() {
@@ -395,7 +397,7 @@ describe("ViewLayoutCollapse", function() {
     assert.equal(cursor.col, 1);
     assert.equal(await gu.getActiveCell().getText(), 'angel');
     assert.equal(await gu.getActiveSectionTitle(), 'INVESTMENTS');
-    assert.match(await driver.getCurrentUrl(), /\/o\/docs\/[^/]+\/Investment-Research\/p\/1$/);
+    assert.match(await driver.getCurrentUrl(), /Investment-Research\/p\/1$/);
     await revert();
   });
 

diff --git a/test/nbrowser/Views.ntest.js b/test/nbrowser/Views.ntest.js
@@ -113,7 +113,7 @@ describe('Views.ntest', function() {
     await gu.waitForServer();
     await gu.actions.viewSection('TABLE4').selectSection();
     // Delete the section
-    await gu.deleteWidget('TABLE4');
+    await gu.deleteWidgetWithData('TABLE4');
     // Assert that the default section (Table1 record) is now active.
     assert.equal(await $('.active_section > .viewsection_title').text(), 'TABLE1');
     // Assert that focus is returned to the deleted section on undo.

diff --git a/test/nbrowser/gristUtils.ts b/test/nbrowser/gristUtils.ts
@@ -263,6 +263,50 @@ export function getSection(sectionOrTitle: string|WebElement): WebElement|WebEle
     .findClosest('.viewsection_content');
 }
 
+/**
+ * Detaches section from the layout. Used for manipulating sections in the layout.
+ */
+export async function detachFromLayout(section?: string) {
+  section ??= await getActiveSectionTitle();
+    const handle = getSection(section).find('.viewsection_drag_indicator');
+    await driver.withActions((actions) => actions
+      .move({origin: handle}));
+    await driver.withActions((actions) => actions
+      .move({origin: handle, x : 1}) // This is needed to show the drag element.
+      .press());
+  await driver.withActions(actions => actions.move({origin: handle, ...{x : 10, y: 10}}));
+  return {
+    /** Moves this leave over another section + offset. */
+    async moveTo(otherSection: string, offset?: {x?: number, y?: number}) {
+      const otherSectionElement = await getSection(otherSection).find('.viewsection_drag_indicator');
+      await driver.withActions(actions => actions.move({
+        origin: otherSectionElement,
+        ...offset
+      }));
+      return this;
+    },
+    /** Releases the dragged section. */
+    async release() {
+      await driver.withActions(actions => actions.release());
+      return this;
+    },
+    /**
+     * Waits for Grist to save this section. The save is debounced, so we need to wait
+     * for couple of events.
+     */
+    async waitForSave() {
+      // Wait for the test class that indicates we have pending save.
+      await driver.findWait(".test-viewLayout-save-pending", 100);
+      // Then wait for that class to be removed (which means Grist has started saving editor).
+      await waitToPass(async () => {
+        assert.isFalse(await driver.find(".test-viewLayout-save-pending").isPresent());
+      });
+      // And wait for the server to process.
+      await waitForServer();
+    }
+  };
+}
+
 /**
  * Click into a section without disrupting cursor positions.
  */
@@ -3916,10 +3960,15 @@ export async function waitForAccessDenied() {
 export async function deleteWidget(title: string) {
   const menu = await openSectionMenu('viewLayout', title);
   await menu.findContent('.test-cmd-name', 'Delete widget').click();
-  if (await driver.findWait('.test-option-deleteOnlyWidget', 100).isPresent()) {
-    await driver.find('.test-option-deleteOnlyWidget').click();
-    await driver.find('.test-modal-confirm').click();
-  }
+  await waitForServer();
+}
+
+export async function deleteWidgetWithData(title?: string) {
+  title ??= await getActiveSectionTitle();
+  const menu = await openSectionMenu('viewLayout', title);
+  await menu.findContent('.test-cmd-name', 'Delete widget').click();
+  await driver.findWait('.test-option-deleteOnlyWidget', 100).click();
+  await driver.find('.test-modal-confirm').click();
   await waitForServer();
 }