From 7c9cb9843ecf7ad5f50e068282d74951ca6975f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaros=C5=82aw=20Sadzi=C5=84ski?= Date: Fri, 28 Oct 2022 10:04:59 +0200 Subject: [PATCH] (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 | 16 +++ app/client/models/TreeModel.ts | 9 +- app/client/models/entities/PageRec.ts | 22 ++-- app/client/ui/Pages.ts | 12 +- app/client/ui/TreeViewComponent.ts | 7 +- app/client/ui2018/pages.ts | 19 ++++ app/common/themes/GristLight.ts | 2 +- test/nbrowser/Pages.ts | 154 +++++++++++++++++++++++++- test/nbrowser/gristUtils.ts | 79 ++++++++++++- 9 files changed, 295 insertions(+), 25 deletions(-) diff --git a/app/client/models/DocModel.ts b/app/client/models/DocModel.ts index c5f81530..a4d33fb9 100644 --- a/app/client/models/DocModel.ts +++ b/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 = this.tabBar.createAllRowsModel('tabPos'); + /** Pages that are shown in the menu. These can include censored pages if they have children. */ + public menuPages: ko.Computed; // Excludes pages hidden by ACL rules or other reasons (e.g. doc-tour) public visibleDocPages: ko.Computed; @@ -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())); } diff --git a/app/client/models/TreeModel.ts b/app/client/models/TreeModel.ts index b077257f..da8bbe41 100644 --- a/app/client/models/TreeModel.ts +++ b/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|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; @@ -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]); } diff --git a/app/client/models/entities/PageRec.ts b/app/client/models/entities/PageRec.ts index 5cc61de3..6ca1c855 100644 --- a/app/client/models/entities/PageRec.ts +++ b/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; isHidden: ko.Computed; + isCensored: ko.Computed; + isSpecial: ko.Computed; } 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: - // - 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. - return !name || (name === 'GristDocTour' && !docModel.showDocTourTable) || isTableHidden(); + return (name === 'GristDocTour' && !docModel.showDocTourTable) || isTableHidden(); + }); + this.isHidden = ko.pureComputed(() => { + return this.isCensored() || this.isSpecial(); }); } diff --git a/app/client/ui/Pages.ts b/app/client/ui/Pages.ts index a3356fee..0021a284 100644 --- a/app/client/ui/Pages.ts +++ b/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(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, activeDoc: GristDoc, pageId: number) { +function buildDomFromTable(pagesTable: MetaTableModel, 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(); diff --git a/app/client/ui/TreeViewComponent.ts b/app/client/ui/TreeViewComponent.ts index 6fa80160..aa8870c0 100644 --- a/app/client/ui/TreeViewComponent.ts +++ b/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 ); diff --git a/app/client/ui2018/pages.ts b/app/client/ui2018/pages.ts index a768cb11..9c0a4ed4 100644 --- a/app/client/ui2018/pages.ts +++ b/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, 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. diff --git a/app/common/themes/GristLight.ts b/app/common/themes/GristLight.ts index 4838e37d..39cd38be 100644 --- a/app/common/themes/GristLight.ts +++ b/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', diff --git a/test/nbrowser/Pages.ts b/test/nbrowser/Pages.ts index 0dae24a6..c9144ec3 100644 --- a/test/nbrowser/Pages.ts +++ b/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(); +} diff --git a/test/nbrowser/gristUtils.ts b/test/nbrowser/gristUtils.ts index c945e417..d9f4ac65 100644 --- a/test/nbrowser/gristUtils.ts +++ b/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 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 { 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 { + 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);