(core) Fix hidden columns bug when editing data selection

Summary:
Editing data selection would sometimes cause columns to be hidden in the updated view. A
missing conditional was the culprit: generally, field visibility shouldn't be modified after the view is
updated, but we make an exception for charts to keep certain fields visible or hidden between
updates, so that chart configuration doesn't change too significantly and cause unexpected
data to be displayed. This special behavior for charts was erroneously being applied to non-charts
as well.

Also, when no columns were visible in a view, opening the row menu would cause an error to be
thrown. A loop was inadvertently using null control variables - an explicit check for non-null loop
variables was added, which skips the loop when no columns are visible.

Test Plan: Browser tests.

Reviewers: jarek

Reviewed By: jarek

Subscribers: jarek

Differential Revision: https://phab.getgrist.com/D3650

app/client/components/BaseView.js

@@ -339,7 +339,10 @@ BaseView.prototype.getAnchorLinkForSection = function(sectionId) {
       // Note that this case only happens in combination with the widget linking mentioned.
       // If the table is empty but the 'new record' row is selected, the `viewData.getRowId` line above works.
       || 'new';
-  const colRef = this.viewSection.viewFields().peek()[this.cursor.fieldIndex.peek()].colRef.peek();
+  // The `fieldIndex` will be null if there are no visible columns.
+  const fieldIndex = this.cursor.fieldIndex.peek();
+  const field = fieldIndex !== null ? this.viewSection.viewFields().peek()[fieldIndex] : null;
+  const colRef = field?.colRef.peek();
   return {hash: {sectionId, rowId, colRef}};
 }
 

app/client/components/GridView.js

@@ -539,12 +539,16 @@ GridView.prototype.getSelection = function() {
     rowEnd = this.getLastDataRowIndex();
   }
 
-  var rowId;
+  // Start or end will be null if no fields are visible.
+  if (colStart !== null && colEnd !== null) {
     for(var i = colStart; i <= colEnd; i++) {
       let field = this.viewSection.viewFields().at(i);
       fields.push(field);
       colStyle[field.colId()] = this._getColStyle(i);
     }
+  }
+
+  var rowId;
   for(var j = rowStart; j <= rowEnd; j++) {
     rowId = this.viewData.getRowId(j);
     rowIds.push(rowId);

app/client/components/GristDoc.ts

@@ -666,7 +666,7 @@ export class GristDoc extends DisposableWithEvents {
         // if table changes or a table is made a summary table, let's replace the view section by a
         // new one, and return.
         if (oldVal.table !== newVal.table || oldVal.summarize !== newVal.summarize) {
-          return await this._replaceViewSection(section, newVal);
+          return await this._replaceViewSection(section, oldVal, newVal);
         }
 
         // if type changes, let's save it.
@@ -1072,7 +1072,11 @@ export class GristDoc extends DisposableWithEvents {
     return await invokePrompt("Table name", "Create", '', "Default table name");
   }
 
-  private async _replaceViewSection(section: ViewSectionRec, newVal: IPageWidget) {
+  private async _replaceViewSection(
+    section: ViewSectionRec,
+    oldVal: IPageWidget,
+    newVal: IPageWidget
+  ) {
 
     const docModel = this.docModel;
     const viewModel = section.view();
@@ -1108,8 +1112,10 @@ export class GristDoc extends DisposableWithEvents {
     // persist options
     await newSection.options.saveOnly(options);
 
-    // persist view fields if possible
+    // charts needs to keep view fields consistent across updates
+    if (oldVal.type === 'chart' && newVal.type === 'chart') {
       await this.setSectionViewFieldsFromArray(newSection, colIds);
+    }
 
     // update theme, and chart type
     await newSection.theme.saveOnly(theme);

test/nbrowser/RowMenu.ts

@@ -0,0 +1,82 @@
+import * as gu from 'test/nbrowser/gristUtils';
+import { setupTestSuite } from 'test/nbrowser/testUtils';
+import { assert, driver, Key, WebElement } from 'mocha-webdriver';
+
+describe('RowMenu', function() {
+  this.timeout(20000);
+  const cleanup = setupTestSuite();
+
+  async function rightClick(el: WebElement) {
+    await driver.withActions((actions) => actions.contextClick(el));
+  }
+
+  async function assertRowMenuOpensAndCloses() {
+    const firstRow = await gu.getRow(1);
+    // make sure that toggle is there
+    assert.isTrue(await firstRow.find(".test-row-menu-trigger").isPresent());
+    // but is hidden
+    assert.isFalse(await firstRow.find(".test-row-menu-trigger").isDisplayed());
+    // hover the row
+    await firstRow.mouseMove();
+    // make sure toggle is visible
+    assert.isTrue(await firstRow.find(".test-row-menu-trigger").isDisplayed());
+    // make sure that clicking on it opens up the menu
+    await firstRow.find(".test-row-menu-trigger").click();
+    assert.isTrue(await driver.findWait('.grist-floating-menu', 1000).isDisplayed());
+    // close the menu
+    await driver.sendKeys(Key.ESCAPE);
+    // make sure the menu is closed
+    assert.lengthOf(await driver.findAll('.grist-floating-menu'), 0);
+  }
+
+  async function assertRowMenuOpensWithRightClick() {
+    const firstRow = await gu.getRow(1);
+    // make sure right click opens up the menu
+    const toggle = await firstRow.find(".test-row-menu-trigger");
+    await rightClick(toggle);
+    assert.isTrue(await driver.findWait('.grist-floating-menu', 1000).isDisplayed());
+    // close the menu by clicking the toggle
+    await toggle.click();
+    // make sure the menu is closed
+    assert.lengthOf(await driver.findAll('.grist-floating-menu'), 0);
+  }
+
+  before(async () => {
+    const session = await gu.session().login();
+    await session.tempDoc(cleanup, "CardView.grist");
+  });
+
+  it('should show row toggle', async function() {
+    await assertRowMenuOpensAndCloses();
+    await assertRowMenuOpensWithRightClick();
+  });
+
+  it('should hide row toggle when mouse moves away', async function() {
+    const [firstRow, secondRow] = [await gu.getRow(1), await gu.getRow(2)];
+    await secondRow.mouseMove();
+    assert.isTrue(await firstRow.find(".test-row-menu-trigger").isPresent());
+    assert.isFalse(await firstRow.find(".test-row-menu-trigger").isDisplayed());
+  });
+
+  it('should support right click anywhere on the row', async function() {
+    // rigth click a cell in a row
+    await rightClick(await gu.getCell(0, 1));
+
+    // check that the context menu shows
+    assert.isTrue(await driver.findWait('.grist-floating-menu', 1000).isDisplayed());
+
+    // send ESC to close the menu
+    await driver.sendKeys(Key.ESCAPE);
+
+    // check that the context menu is gone
+    assert.isFalse(await driver.find('.grist-floating-menu').isPresent());
+  });
+
+  it('should work even when no columns are visible', async function() {
+    // Previously, a bug would cause an error to be thrown instead.
+    await gu.openColumnMenu('A', 'Hide column');
+    await gu.openColumnMenu('B', 'Hide column');
+    await assertRowMenuOpensAndCloses();
+    await assertRowMenuOpensWithRightClick();
+  });
+});

test/nbrowser/gristUtils.ts

@@ -383,6 +383,13 @@ export function getActiveCell(): WebElementPromise {
 }
 
 
+/**
+ * Returns a visible GridView row from the active section.
+ */
+export function getRow(rowNum: number): WebElementPromise {
+  return driver.findContent('.active_section .gridview_data_row_num', String(rowNum));
+}
+
 /**
  * Get the numeric value from the row header of the first selected row. This would correspond to
  * the row with the cursor when a single rows is selected.

test/nbrowser/saveViewSection.ts

@@ -0,0 +1,162 @@
+import { assert, driver, Key } from 'mocha-webdriver';
+import * as gu from 'test/nbrowser/gristUtils';
+import { server, setupTestSuite } from 'test/nbrowser/testUtils';
+
+describe("saveViewSection", function() {
+  this.timeout(20000);
+  setupTestSuite();
+
+  it("should work correctly when turning a table to 'summary'", async () => {
+    // create a new document
+    const docId = await gu.createNewDoc('Chimpy', 'nasa', 'Horizon', 'test-updateViewSection');
+
+    // Login and open document
+    await server.simulateLogin('Chimpy', 'chimpy@getgrist.com', 'nasa');
+    await driver.get(`${server.getHost()}/o/nasa/doc/${docId}`);
+
+    // add new section
+    await gu.addNewSection(/Table/, /Table1/);
+
+    // change name and edit data of the 1st section (first found - both have the same name)
+    await gu.renameSection('TABLE1', 'Foo');
+
+    // open right panel
+    await gu.toggleSidePanel('right');
+    await driver.find('.test-config-data').click();
+
+    // check there is no groupedBy
+    assert.equal(await driver.find('.test-pwc-groupedBy').isDisplayed(), false);
+
+    // click edit table data
+    await driver.find('.test-pwc-editDataSelection').doClick();
+
+    // summarize table by 'A' and save
+    await driver.findContent('.test-wselect-table', /Table1/).find('.test-wselect-pivot').doClick();
+    await driver.findContent('.test-wselect-column', /A/).doClick();
+    await driver.find('.test-wselect-addBtn').doClick();
+
+    // wait for server
+    await gu.waitForServer();
+
+    // check that new table is summarized
+    assert.equal(await driver.findWait('.test-pwc-table', 2000).getText(), 'Table1');
+    assert.deepEqual(await driver.findAll('.test-pwc-groupedBy-col', (e) => e.getText()), ['A']);
+
+    // check sections name did not change
+    assert.deepEqual(await gu.getSectionTitles(), ['Foo', 'TABLE1']);
+
+    // check 1st section is active
+    assert(await driver.find('.viewsection_content').matches('.active_section'));
+  });
+
+  it('should work correctly when changing table', async () => {
+    // click edit table data
+    await driver.find('.test-pwc-editDataSelection').doClick();
+
+    // create a new table
+    await driver.findContent('.test-wselect-table', /New Table/).doClick();
+    await driver.find('.test-wselect-addBtn').doClick();
+
+    // wait for server
+    await gu.waitForServer();
+
+    // check that first section shows table2 with no grouped by cols
+    assert.equal(await driver.findWait('.test-pwc-table', 2000).getText(), 'Table2');
+    assert.equal(await driver.find('.test-pwc-groupedBy').isDisplayed(), false);
+
+    // check sections name did not change
+    assert.deepEqual(await gu.getSectionTitles(), ['Foo', 'TABLE1']);
+
+    // check 1st section is active
+    assert(await driver.find('.viewsection_content').matches('.active_section'));
+
+    // revert to what it was
+    await gu.undo();
+  });
+
+  it("should work correctly when changing type", async () => {
+
+    async function switchTypeAndAssert(t: string) {
+      // open page widget picker
+      await driver.find('.test-pwc-editDataSelection').doClick();
+
+      // select type t and save
+      await driver.findContent('.test-wselect-type', gu.exactMatch(t)).doClick();
+      await driver.find('.test-wselect-addBtn').doClick();
+      await gu.waitForServer();
+
+      // check section's type
+      await driver.find('.test-pwc-editDataSelection').doClick();
+      assert.equal(await driver.find('.test-wselect-type[class*=-selected]').getText(), t);
+
+      // close page widget picker
+      await driver.sendKeys(Key.ESCAPE);
+      await gu.checkForErrors();
+    }
+
+    // TODO: check what's shown by asserting data for each type
+    await switchTypeAndAssert('Card');
+    await switchTypeAndAssert('Table');
+    await switchTypeAndAssert('Chart');
+
+  });
+
+  it("should work correctly when changing grouped by column", async () => {
+
+    // open page widget picker
+    await driver.find('.test-pwc-editDataSelection').doClick();
+
+    // Select column B
+    await driver.findContent('.test-wselect-column', /B/).doClick();
+    await driver.find('.test-wselect-addBtn').doClick();
+    await gu.waitForServer();
+
+    // check grouped by is now A, B
+    assert.deepEqual(await driver.findAll('.test-pwc-groupedBy-col', (e) => e.getText()), ['A', 'B']);
+
+    await gu.undo();
+  });
+
+  it("should not hide any columns when changing to a summary table", async () => {
+    // Previously, a bug when changing data selection would sometimes cause columns to be hidden.
+    // This test replicates a scenario that was used to reproduce the bug, and checks that it no
+    // longer occurs.
+
+    async function assertActiveSectionColumns(...expected: string[]) {
+      const activeSection = await driver.find('.active_section');
+      const actual = (await activeSection.findAll('.column_name', el => el.getText()))
+        .filter(name => name !== '+');
+      assert.deepEqual(actual, expected);
+    }
+
+    // Create a Places table with a single Place column.
+    await gu.addNewTable('Places');
+    await gu.renameColumn({col: 0}, 'Place');
+    await gu.sendKeys(Key.ARROW_RIGHT);
+    await gu.sendKeys(Key.chord(Key.ALT, '-'));
+    await gu.waitForServer();
+    await gu.sendKeys(Key.chord(Key.ALT, '-'));
+    await gu.waitForServer();
+
+    // Create an Orders table, and rename the last column to Test.
+    await gu.addNewTable('Orders');
+    await gu.renameColumn({col: 2}, 'Test');
+
+    // Duplicate the Places page.
+    await gu.openPageMenu('Places');
+    await driver.find('.test-docpage-duplicate').click();
+    await driver.find('.test-modal-confirm').click();
+    await driver.findContentWait('.test-docpage-label', /copy/, 1000);
+    await gu.waitForServer();
+
+    // Change the duplicated page's data to summarize Orders, grouping by column Test.
+    await driver.find('.test-pwc-editDataSelection').doClick();
+    await driver.findContent('.test-wselect-table', /Orders/).find('.test-wselect-pivot').doClick();
+    await driver.findContent('.test-wselect-column', /Test/).doClick();
+    await driver.find('.test-wselect-addBtn').doClick();
+    await gu.waitForServer();
+
+    // Check all columns are visible.
+    await assertActiveSectionColumns('Test', 'count');
+  });
+});