(core) Revealing hidden pages with visible children.

Summary:
When a page is hidden, all its nested pages are shown as children of
a different page that happens to be before (as in pagePos) that page.

This diff shows those pages as CENSORED.

Test Plan: Updated

Reviewers: alexmojaki

Reviewed By: alexmojaki

Subscribers: alexmojaki

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

app/client/models/DocModel.ts

@@ -13,6 +13,7 @@ import {KoArray} from 'app/client/lib/koArray';
 import {KoSaveableObservable} from 'app/client/models/modelUtil';
 
 import * as ko from 'knockout';
+import memoize from 'lodash/memoize';
 
 import * as koArray from 'app/client/lib/koArray';
 import * as koUtil from 'app/client/lib/koUtil';
@@ -142,6 +143,8 @@ export class DocModel {
 
   public allTabs: KoArray<TabBarRec> = this.tabBar.createAllRowsModel('tabPos');
 
+  /** Pages that are shown in the menu. These can include censored pages if they have children. */
+  public menuPages: ko.Computed<PageRec[]>;
   // Excludes pages hidden by ACL rules or other reasons (e.g. doc-tour)
   public visibleDocPages: ko.Computed<PageRec[]>;
 
@@ -189,6 +192,19 @@ export class DocModel {
 
     // Get a list of only the visible pages.
     const allPages = this.pages.createAllRowsModel('pagePos');
+    this.menuPages = ko.computed(() => {
+      const pagesToShow = allPages.all().filter(p => !p.isSpecial()).sort((a, b) => a.pagePos() - b.pagePos());
+      // Helper to find all children of a page.
+      const children = memoize((page: PageRec) => {
+        const following = pagesToShow.slice(pagesToShow.indexOf(page) + 1);
+        const firstOutside = following.findIndex(p => p.indentation() <= page.indentation());
+        return firstOutside >= 0 ? following.slice(0, firstOutside) : following;
+      });
+      // Helper to test if the page is hidden and all its children are hidden.
+      // In that case, we won't show it at all.
+      const hide = memoize((page: PageRec): boolean => page.isCensored() && children(page).every(p => hide(p)));
+      return pagesToShow.filter(p => !hide(p));
+    });
     this.visibleDocPages = ko.computed(() => allPages.all().filter(p => !p.isHidden()));
   }
 

app/client/models/TreeModel.ts

@@ -24,6 +24,7 @@ import reverse = require("lodash/reverse");
  * `TreeModel` and any item in it implements `TreeItem`.
  */
 export interface TreeNode {
+  hidden?: boolean;
   // Returns an observable array of children. Or null if the node does not accept children.
   children(): ObsArray<TreeItem>|null;
 
@@ -61,7 +62,7 @@ export interface TreeTableData {
 }
 
 // describes a function that builds dom for a particular record
-type DomBuilder = (id: number) => HTMLElement;
+type DomBuilder = (id: number, hidden: boolean) => HTMLElement;
 
 
 // Returns a list of the records from table that is suitable to build the tree model, ie: records
@@ -108,8 +109,9 @@ export function fromTableData(table: TreeTableData, buildDom: DomBuilder, oldMod
     const children = indentations[rec.indentation + 1] || [];
     delete indentations[rec.indentation + 1];
     const item = oldItems[rec.id] || new TreeItemRecord();
+    item.hidden = rec.hidden;
     item.init(storage, index, reverse(children));
-    item.buildDom = () => buildDom(rec.id);
+    item.buildDom = () => buildDom(rec.id, rec.hidden);
     siblings.push(item);
   });
   return new TreeModelRecord(storage, reverse(indentations[0] || []));
@@ -123,7 +125,7 @@ interface Storage {
 
 // TreeNode implementation that uses a grist table.
 export class TreeNodeRecord implements TreeNode {
-
+  public hidden: boolean = false;
   public storage: Storage;
   public index: number|"root";
   public children: () => ObsArray<TreeItemRecord>;
@@ -187,6 +189,7 @@ export class TreeNodeRecord implements TreeNode {
       forEach(update[0], (val, key) => values[key] = update.map(rec => rec[key]));
       const rowIds = values.id;
       delete values.id;
+      delete values.hidden;
       userActions.push(["BulkUpdateRecord", rowIds, values]);
     }
 

app/client/models/entities/PageRec.ts

@@ -5,11 +5,20 @@ import * as ko from 'knockout';
 export interface PageRec extends IRowModel<"_grist_Pages"> {
   view: ko.Computed<ViewRec>;
   isHidden: ko.Computed<boolean>;
+  isCensored: ko.Computed<boolean>;
+  isSpecial: ko.Computed<boolean>;
 }
 
 export function createPageRec(this: PageRec, docModel: DocModel): void {
   this.view = refRecord(docModel.views, this.viewRef);
-  this.isHidden = ko.pureComputed(() => {
+  // Page is hidden when any of this is true:
+  // - It has an empty name (or no name at all)
+  // - It is GristDocTour (unless user wants to see it)
+  // - It is a page generated for a hidden table TODO: Follow up - don't create
+  //   pages for hidden tables.
+  // This is used currently only the left panel, to hide pages from the user.
+  this.isCensored = ko.pureComputed(() => !this.view().name());
+  this.isSpecial = ko.pureComputed(() => {
     const name = this.view().name();
     const isTableHidden = () => {
       const viewId = this.view().id();
@@ -17,12 +26,9 @@ export function createPageRec(this: PageRec, docModel: DocModel): void {
       const primaryTable = tables.find(t => t.primaryViewId() === viewId);
       return !!primaryTable && primaryTable.tableId()?.startsWith("GristHidden_");
     };
-    // Page is hidden when any of this is true:
+    return (name === 'GristDocTour' && !docModel.showDocTourTable) || isTableHidden();
-    // - It has an empty name (or no name at all)
+  });
-    // - It is GristDocTour (unless user wants to see it)
+  this.isHidden = ko.pureComputed(() => {
-    // - It is a page generated for a hidden table TODO: Follow up - don't create
+    return this.isCensored() || this.isSpecial();
-    //   pages for hidden tables.
-    // This is used currently only the left panel, to hide pages from the user.
-    return !name || (name === 'GristDocTour' && !docModel.showDocTourTable) || isTableHidden();
   });
 }

app/client/ui/Pages.ts

@@ -12,7 +12,7 @@ import {labeledCircleCheckbox} from 'app/client/ui2018/checkbox';
 import {theme} from 'app/client/ui2018/cssVars';
 import {cssLink} from 'app/client/ui2018/links';
 import {ISaveModalOptions, saveModal} from 'app/client/ui2018/modals';
-import {buildPageDom, PageActions} from 'app/client/ui2018/pages';
+import {buildCensoredPage, buildPageDom, PageActions} from 'app/client/ui2018/pages';
 import {mod} from 'app/common/gutil';
 import {Computed, Disposable, dom, DomContents, fromKo, makeTestId, observable, Observable, styled} from 'grainjs';
 
@@ -24,11 +24,12 @@ export function buildPagesDom(owner: Disposable, activeDoc: GristDoc, isOpen: Ob
   const buildDom = buildDomFromTable.bind(null, pagesTable, activeDoc);
 
   const records = Computed.create<TreeRecord[]>(owner, (use) =>
-    use(activeDoc.docModel.visibleDocPages).map(page => ({
+    use(activeDoc.docModel.menuPages).map(page => ({
       id: page.getRowId(),
       indentation: use(page.indentation),
       pagePos: use(page.pagePos),
       viewRef: use(page.viewRef),
+      hidden: use(page.isCensored),
     }))
   );
   const getTreeTableData = (): TreeTableData => ({
@@ -58,7 +59,12 @@ export function buildPagesDom(owner: Disposable, activeDoc: GristDoc, isOpen: Ob
 
 const testId = makeTestId('test-removepage-');
 
-function buildDomFromTable(pagesTable: MetaTableModel<PageRec>, activeDoc: GristDoc, pageId: number) {
+function buildDomFromTable(pagesTable: MetaTableModel<PageRec>, activeDoc: GristDoc, pageId: number, hidden: boolean) {
+
+  if (hidden) {
+    return buildCensoredPage();
+  }
+
   const {isReadonly} = activeDoc;
   const pageName = pagesTable.rowModels[pageId].view.peek().name;
   const viewId = pagesTable.rowModels[pageId].view.peek().id.peek();

app/client/ui/TreeViewComponent.ts

@@ -298,7 +298,7 @@ export class TreeViewComponent extends Disposable {
 
     let headerElement: HTMLElement;
     let labelElement: HTMLElement;
-    let handleElement: HTMLElement;
+    let handleElement: HTMLElement|null = null;
     let offsetElement: HTMLElement;
     let arrowElement: HTMLElement;
 
@@ -329,13 +329,14 @@ export class TreeViewComponent extends Disposable {
           ),
           delayedMouseDrag(this._startDrag.bind(this), this._options.dragStartDelay),
         ),
-        css.itemLabelRight(
+        treeItem.hidden ? null : css.itemLabelRight(
           handleElement = css.centeredIcon('DragDrop',
             dom.style('top', (use) => use(deltaY) + 'px'),
             testId('handle'),
             dom.hide(this._options.isReadonly),
           ),
-          mouseDrag((startEvent, elem) => this._startDrag(startEvent))),
+          mouseDrag((startEvent, elem) => this._startDrag(startEvent))
+        ),
       ),
       ...args
     );

app/client/ui2018/pages.ts

@@ -4,6 +4,7 @@ import { cssEditorInput } from "app/client/ui/HomeLeftPane";
 import { itemHeader, itemHeaderWrapper, treeViewContainer } from "app/client/ui/TreeViewComponentCss";
 import { theme } from "app/client/ui2018/cssVars";
 import { icon } from "app/client/ui2018/icons";
+import { hoverTooltip } from 'app/client/ui/tooltips';
 import { menu, menuItem, menuText } from "app/client/ui2018/menus";
 import { dom, domComputed, DomElementArg, makeTestId, observable, Observable, styled } from "grainjs";
 
@@ -106,6 +107,20 @@ export function buildPageDom(name: Observable<string>, actions: PageActions, ...
     ));
 }
 
+export function buildCensoredPage() {
+  return cssPageItem(
+    cssPageInitial(
+      testId('initial'),
+      dom.text('C'),
+    ),
+    cssCensoredPageName(
+      dom.text('CENSORED'),
+      testId('label'),
+    ),
+    hoverTooltip('This page is censored due to access rules.'),
+  );
+}
+
 const cssPageItem = styled('a', `
   display: flex;
   flex-direction: row;
@@ -143,6 +158,10 @@ const cssPageName = styled('div', `
   }
 `);
 
+const cssCensoredPageName = styled(cssPageName, `
+  color: ${theme.disabledPageFg};
+`);
+
 function onHoverSupport(yesNo: boolean) {
   // On desktop, we show page menu button on hover over page link. This isn't usable on mobile,
   // and interferes with clicks on iOS; so instead we show the button when the page is selected.

app/common/themes/GristLight.ts

@@ -202,7 +202,7 @@ export const GristLight: ThemeColors = {
   'left-panel-page-hover-bg': 'rgba(217,217,217,0.6)',
   'left-panel-active-page-fg': '#FFFFFF',
   'left-panel-active-page-bg': '#262633',
-  'left-panel-disabled-page-fg': '#D9D9D9',
+  'left-panel-disabled-page-fg': '#BDBDBD',
   'left-panel-page-options-fg': '#929299',
   'left-panel-page-options-hover-fg': 'white',
   'left-panel-page-options-hover-bg': '#D9D9D9',

test/nbrowser/Pages.ts

@@ -7,7 +7,7 @@ import {server, setupTestSuite} from 'test/nbrowser/testUtils';
 import values = require('lodash/values');
 
 describe('Pages', function() {
-  this.timeout(20000);
+  this.timeout(30000);
   setupTestSuite();
   let doc: DocCreationInfo;
   let api: UserAPI;
@@ -20,6 +20,124 @@ describe('Pages', function() {
     api = session.createHomeApi();
   });
 
+  it('should show censor pages', async () => {
+    // Make a 3 level hierarchy.
+    assert.deepEqual(await gu.getPageTree(), [
+      {
+        label: 'Interactions', children: [
+          { label: 'Documents' },
+        ]
+      },
+      {
+        label: 'People', children: [
+          { label: 'User & Leads' },
+          { label: 'Overview' },
+        ]
+      },
+    ]);
+    await insertPage(/Overview/, /User & Leads/);
+    assert.deepEqual(await gu.getPageTree(), [
+      {
+        label: 'Interactions', children: [
+          { label: 'Documents' },
+        ]
+      },
+      {
+        label: 'People', children: [
+          { label: 'User & Leads', children: [{ label: 'Overview' }] },
+        ]
+      },
+    ]);
+    const revertAcl = await gu.beginAclTran(api, doc.id);
+    // Update ACL, hide People table from all users.
+    await hideTable("People");
+    // We will be reloaded, but it's not easy to wait for it, so do the refresh manually.
+    await gu.reloadDoc();
+    assert.deepEqual(await gu.getPageTree(), [
+      {
+        label: 'Interactions', children: [
+          { label: 'Documents'},
+        ]
+      },
+      {
+        label: 'CENSORED', children: [
+          { label: 'User & Leads', children: [{ label: 'Overview' }] },
+        ]
+      },
+    ]);
+
+    // Test that we can't click this page.
+    await driver.findContent('.test-treeview-itemHeader', /CENSORED/).click();
+    await gu.waitForServer();
+    assert.equal(await gu.getSectionTitle(), 'INTERACTIONS');
+
+    // Test that we don't have move handler.
+    assert.isFalse(
+      await driver.findContent('.test-treeview-itemHeaderWrapper', /CENSORED/)
+                  .find('.test-treeview-handle').isPresent()
+    );
+
+    // Now hide User_Leads
+    await hideTable("User_Leads");
+    await gu.reloadDoc();
+    assert.deepEqual(await gu.getPageTree(), [
+      {
+        label: 'Interactions', children: [
+          { label: 'Documents'},
+        ]
+      },
+      {
+        label: 'CENSORED', children: [
+          { label: 'CENSORED', children: [{ label: 'Overview' }] },
+        ]
+      },
+    ]);
+
+    // Now hide Overview, and test that whole node is hidden.
+    await hideTable("Overview");
+    await gu.reloadDoc();
+    assert.deepEqual(await gu.getPageTree(), [
+      {
+        label: 'Interactions', children: [
+          { label: 'Documents'},
+        ]
+      }
+    ]);
+
+    // Now hide Documents, this is a leaf, so it should be hidden from the start
+    await hideTable("Documents");
+    await gu.reloadDoc();
+    assert.deepEqual(await gu.getPageTree(), [
+      {
+        label: 'Interactions'
+      }
+    ]);
+
+    // Now hide Interactions, we should have a blank treeview
+    await hideTable("Interactions");
+    // We can wait for doc to load, because it waits for section.
+    await driver.findWait(".test-treeview-container", 1000);
+    assert.deepEqual(await gu.getPageTree(), []);
+
+    // Rollback
+    await revertAcl();
+    await gu.reloadDoc();
+    assert.deepEqual(await gu.getPageTree(), [
+      {
+        label: 'Interactions', children: [
+          { label: 'Documents' },
+        ]
+      },
+      {
+        label: 'People', children: [
+          { label: 'User & Leads', children: [{ label: 'Overview' }] },
+        ]
+      },
+    ]);
+    await gu.undo();
+  });
+
+
   it('should list all pages in document', async () => {
 
     // check content of _girst_Pages and _grist_Views
@@ -122,8 +240,7 @@ describe('Pages', function() {
     // revert changes
     await gu.undo(2);
     assert.deepEqual(await gu.getPageNames(), ['Interactions', 'Documents', 'People', 'User & Leads', 'Overview']);
-
+  });
-  })
 
   it('should not allow blank page name', async () => {
     // Begin renaming of People page
@@ -403,7 +520,7 @@ describe('Pages', function() {
 
   it('should not throw JS errors when removing the current page without a slug', async () => {
     // Create and open new document
-    const docId = await session.tempNewDoc(cleanup, "test-page-removal-js-error")
+    const docId = await session.tempNewDoc(cleanup, "test-page-removal-js-error");
     await driver.get(`${server.getHost()}/o/test-grist/doc/${docId}`);
     await gu.waitForUrl('test-page-removal-js-error');
 
@@ -442,7 +559,7 @@ describe('Pages', function() {
 
   it('should offer a way to delete last tables', async () => {
     // Create and open new document
-    const docId = await session.tempNewDoc(cleanup, "prompts")
+    const docId = await session.tempNewDoc(cleanup, "prompts");
     await driver.get(`${server.getHost()}/o/test-grist/doc/${docId}`);
     await gu.waitForUrl('prompts');
 
@@ -482,9 +599,18 @@ describe('Pages', function() {
     assert.deepEqual(await gu.getSectionTitles(), ['TABLE C', 'TABLE D', 'TABLE1' ]);
   });
 
+
+  async function hideTable(tableId: string) {
+    await api.applyUserActions(doc.id, [
+      ['AddRecord', '_grist_ACLResources', -1, {tableId, colIds: '*'}],
+      ['AddRecord', '_grist_ACLRules', null, {
+        resource: -1, aclFormula: '', permissionsText: '-R',
+      }],
+    ]);
+  }
 });
 
-async function movePage(page: RegExp, target: {before: RegExp}|{after: RegExp}) {
+async function movePage(page: RegExp, target: {before: RegExp}|{after: RegExp}|{into: RegExp}) {
   const targetReg = values(target)[0];
   await driver.withActions(actions => actions
     .move({origin: driver.findContent('.test-treeview-itemHeader', page)})
@@ -496,3 +622,19 @@ async function movePage(page: RegExp, target: {before: RegExp}|{after: RegExp})
     })
     .release());
 }
+
+
+async function insertPage(page: RegExp, into: RegExp) {
+  await driver.withActions(actions => actions
+    .move({origin: driver.findContent('.test-treeview-itemHeader', page)})
+    .move({origin: driver.findContent('.test-treeview-itemHeaderWrapper', page)
+      .find('.test-treeview-handle')})
+    .press()
+    .move({origin: driver.findContent('.test-treeview-itemHeader', into),
+      y: 5
+    })
+    .pause(1500) // wait for a target to be highlighted
+    .release()
+  );
+  await gu.waitForServer();
+}

test/nbrowser/gristUtils.ts

@@ -16,7 +16,8 @@ import { FullUser, UserProfile } from 'app/common/LoginSessionAPI';
 import { resetOrg } from 'app/common/resetOrg';
 import { UserAction } from 'app/common/DocActions';
 import { TestState } from 'app/common/TestState';
-import { Organization as APIOrganization, DocStateComparison, UserAPIImpl, Workspace } from 'app/common/UserAPI';
+import { Organization as APIOrganization, DocStateComparison,
+         UserAPI, UserAPIImpl, Workspace } from 'app/common/UserAPI';
 import { Organization } from 'app/gen-server/entity/Organization';
 import { Product } from 'app/gen-server/entity/Product';
 import { create } from 'app/server/lib/create';
@@ -688,6 +689,11 @@ export async function waitForDocToLoad(timeoutMs: number = 10000): Promise<void>
   await waitForServer();
 }
 
+export async function reloadDoc() {
+  await driver.navigate().refresh();
+  await waitForDocToLoad();
+}
+
 /**
  * Wait for the doc list to show, to know that workspaces are fetched, and imports enabled.
  */
@@ -930,6 +936,51 @@ export function getPageNames(): Promise<string[]> {
   return driver.findAll('.test-docpage-label', (e) => e.getText());
 }
 
+export interface PageTree {
+  label: string;
+  children?: PageTree[];
+}
+/**
+ * Returns a current page tree as a JSON object.
+ */
+export async function getPageTree(): Promise<PageTree[]> {
+  const allPages = await driver.findAll('.test-docpage-label');
+  const root: PageTree = {label: 'root', children: []};
+  const stack: PageTree[] = [root];
+  let current = 0;
+  for(const page of allPages) {
+    const label = await page.getText();
+    const offset = await page.findClosest('.test-treeview-itemHeader').find('.test-treeview-offset');
+    const level = parseInt((await offset.getCssValue('width')).replace("px", "")) / 10;
+    if (level === current) {
+      const parent = stack.pop()!;
+      parent.children ??= [];
+      parent.children.push({label});
+      stack.push(parent);
+    } else if (level > current) {
+      current = level;
+      const child = {label};
+      const grandFather = stack.pop()!;
+      grandFather.children ??= [];
+      const father = grandFather.children[grandFather.children.length - 1];
+      father.children ??= [];
+      father.children.push(child);
+      stack.push(grandFather);
+      stack.push(father);
+    } else {
+      while (level < current) {
+        stack.pop();
+        current--;
+      }
+      const parent = stack.pop()!;
+      parent.children ??= [];
+      parent.children.push({label});
+      stack.push(parent);
+    }
+  }
+  return root.children!;
+}
+
 /**
  * Adds a new empty table using the 'Add New' menu.
  */
@@ -2685,6 +2736,32 @@ export function withComments() {
   });
 }
 
+/**
+ * Helper to revert ACL changes. It first saves the current ACL data, and
+ * then removes everything and adds it back.
+ */
+export async function beginAclTran(api: UserAPI, docId: string) {
+  const oldRes = await api.getTable(docId, '_grist_ACLResources');
+  const oldRules = await api.getTable(docId, '_grist_ACLRules');
+
+  return async () => {
+    const newRes = await api.getTable(docId, '_grist_ACLResources');
+    const newRules = await api.getTable(docId, '_grist_ACLRules');
+    const restoreRes = {tableId: oldRes.tableId, colIds: oldRes.colIds};
+    const restoreRules = {
+      resource: oldRules.resource,
+      aclFormula: oldRules.aclFormula,
+      permissionsText: oldRules.permissionsText
+    };
+    await api.applyUserActions(docId, [
+      ['BulkRemoveRecord', '_grist_ACLRules', newRules.id],
+      ['BulkRemoveRecord', '_grist_ACLResources', newRes.id],
+      ['BulkAddRecord', '_grist_ACLResources', oldRes.id, restoreRes],
+      ['BulkAddRecord', '_grist_ACLRules', oldRules.id, restoreRules],
+    ]);
+  };
+}
+
 } // end of namespace gristUtils
 
 stackWrapOwnMethods(gristUtils);