From 61942f6f4bc53dbacb943a094edcbfc905bbd49f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaros=C5=82aw=20Sadzi=C5=84ski?= Date: Wed, 24 Jul 2024 13:33:53 +0200 Subject: [PATCH] (core) Adding confirmation before remove last widget for a table Summary: When last widget for a table is removed, user is informed about that and can decide between removing the widget and removing both table and widget Test Plan: Updated Reviewers: georgegevoian Reviewed By: georgegevoian Differential Revision: https://phab.getgrist.com/D4295 --- app/client/components/LayoutTray.ts | 33 ++--- app/client/components/ViewLayout.ts | 120 +++++++++++++++++-- app/client/models/entities/TableRec.ts | 3 + app/client/models/entities/ViewSectionRec.ts | 5 +- test/nbrowser/SelectBySummary.ts | 4 +- test/nbrowser/ViewLayoutCollapse.ts | 98 +++++++++++++++ test/nbrowser/Views.ntest.js | 3 +- test/nbrowser/gristUtils.ts | 13 ++ 8 files changed, 249 insertions(+), 30 deletions(-) diff --git a/app/client/components/LayoutTray.ts b/app/client/components/LayoutTray.ts index 99c92941..d79453ef 100644 --- a/app/client/components/LayoutTray.ts +++ b/app/client/components/LayoutTray.ts @@ -195,7 +195,7 @@ export class LayoutTray extends DisposableWithEvents { box.dispose(); // And ask the viewLayout to save the specs. - viewLayout.saveLayoutSpec(); + viewLayout.saveLayoutSpec().catch(reportError); }, restoreSection: () => { // Get the section that is collapsed and clicked (we are setting this value). @@ -206,23 +206,28 @@ export class LayoutTray extends DisposableWithEvents { viewLayout.viewModel.activeCollapsedSections.peek().filter(x => x !== leafId) ); viewLayout.viewModel.activeSectionId(leafId); - viewLayout.saveLayoutSpec(); + viewLayout.saveLayoutSpec().catch(reportError); }, // Delete collapsed section. - deleteCollapsedSection: () => { + deleteCollapsedSection: async () => { // This section is still in the view (but not in the layout). So we can just remove it. const leafId = viewLayout.viewModel.activeCollapsedSectionId(); if (!leafId) { return; } - this.viewLayout.removeViewSection(leafId); - // We need to manually update the layout. Main layout editor doesn't care about missing sections. - // but we can't afford that. Without removing it, user can add another section that will be collapsed - // from the start, as the id will be the same as the one we just removed. - const currentSpec = viewLayout.viewModel.layoutSpecObj(); - const validSections = new Set(viewLayout.viewModel.viewSections.peek().peek().map(vs => vs.id.peek())); - validSections.delete(leafId); - currentSpec.collapsed = currentSpec.collapsed - ?.filter(x => typeof x.leaf === 'number' && validSections.has(x.leaf)); - viewLayout.saveLayoutSpec(currentSpec); + + viewLayout.docModel.docData.bundleActions('removing section', async () => { + if (!await this.viewLayout.removeViewSection(leafId)) { + return; + } + // We need to manually update the layout. Main layout editor doesn't care about missing sections. + // but we can't afford that. Without removing it, user can add another section that will be collapsed + // from the start, as the id will be the same as the one we just removed. + const currentSpec = viewLayout.viewModel.layoutSpecObj(); + const validSections = new Set(viewLayout.viewModel.viewSections.peek().peek().map(vs => vs.id.peek())); + validSections.delete(leafId); + currentSpec.collapsed = currentSpec.collapsed + ?.filter(x => typeof x.leaf === 'number' && validSections.has(x.leaf)); + await viewLayout.saveLayoutSpec(currentSpec); + }).catch(reportError); } }; this.autoDispose(commands.createGroup(commandGroup, this, true)); @@ -843,7 +848,7 @@ class ExternalLeaf extends Disposable implements Dropped { // and the section won't be created on time. this.model.viewLayout.layoutEditor.triggerUserEditStop(); // Manually save the layout. - this.model.viewLayout.saveLayoutSpec(); + this.model.viewLayout.saveLayoutSpec().catch(reportError); } })); diff --git a/app/client/components/ViewLayout.ts b/app/client/components/ViewLayout.ts index f017f992..5ba495bd 100644 --- a/app/client/components/ViewLayout.ts +++ b/app/client/components/ViewLayout.ts @@ -20,7 +20,12 @@ import {reportError} from 'app/client/models/errors'; import {getTelemetryWidgetTypeFromVS} from 'app/client/ui/widgetTypesMap'; import {isNarrowScreen, mediaSmall, testId, theme} from 'app/client/ui2018/cssVars'; import {icon} from 'app/client/ui2018/icons'; +import {ISaveModalOptions, saveModal} from 'app/client/ui2018/modals'; import {DisposableWithEvents} from 'app/common/DisposableWithEvents'; +import {makeT} from 'app/client/lib/localization'; +import {urlState} from 'app/client/models/gristUrlState'; +import {cssRadioCheckboxOptions, radioCheckboxOption} from 'app/client/ui2018/checkbox'; +import {cssLink} from 'app/client/ui2018/links'; import {mod} from 'app/common/gutil'; import { Computed, @@ -39,6 +44,8 @@ import * as ko from 'knockout'; import debounce from 'lodash/debounce'; import * as _ from 'underscore'; +const t = makeT('ViewLayout'); + // tslint:disable:no-console const viewSectionTypes: {[key: string]: any} = { @@ -125,7 +132,7 @@ export class ViewLayout extends DisposableWithEvents implements IDomComponent { this.listenTo(this.layout, 'layoutUserEditStop', () => { this.isResizing.set(false); this.layoutSaveDelay.schedule(1000, () => { - this.saveLayoutSpec(); + this.saveLayoutSpec().catch(reportError); }); }); @@ -187,7 +194,7 @@ export class ViewLayout extends DisposableWithEvents implements IDomComponent { })); const commandGroup = { - deleteSection: () => { this.removeViewSection(this.viewModel.activeSectionId()); }, + deleteSection: () => { this.removeViewSection(this.viewModel.activeSectionId()).catch(reportError); }, nextSection: () => { this._otherSection(+1); }, prevSection: () => { this._otherSection(-1); }, printSection: () => { printViewSection(this.layout, this.viewModel.activeSection()).catch(reportError); }, @@ -265,31 +272,83 @@ export class ViewLayout extends DisposableWithEvents implements IDomComponent { this._savePending.set(false); // Cancel the automatic delay. this.layoutSaveDelay.cancel(); - if (!this.layout) { return; } + if (!this.layout) { return Promise.resolve(); } // Only save layout changes when the document isn't read-only. if (!this.gristDoc.isReadonly.get()) { if (!specs) { specs = this.layout.getLayoutSpec(); specs.collapsed = this.viewModel.activeCollapsedSections.peek().map((leaf)=> ({leaf})); } - this.viewModel.layoutSpecObj.setAndSave(specs).catch(reportError); + return this.viewModel.layoutSpecObj.setAndSave(specs).catch(reportError); } this._onResize(); + return Promise.resolve(); } - // Removes a view section from the current view. Should only be called if there is - // more than one viewsection in the view. - public removeViewSection(viewSectionRowId: number) { + /** + * Removes a view section from the current view. Should only be called if there is more than + * one viewsection in the view. + * @returns A promise that resolves with true when the view section is removed. If user was + * prompted and decided to cancel, the promise resolves with false. + */ + public async removeViewSection(viewSectionRowId: number) { this.maximized.set(null); const viewSection = this.viewModel.viewSections().all().find(s => s.getRowId() === viewSectionRowId); if (!viewSection) { throw new Error(`Section not found: ${viewSectionRowId}`); } + const tableId = viewSection.table.peek().tableId.peek(); - const widgetType = getTelemetryWidgetTypeFromVS(viewSection); - logTelemetryEvent('deletedWidget', {full: {docIdDigest: this.gristDoc.docId(), widgetType}}); + // Check if this is a UserTable (not summary) and if so, if it is available on any other page + // we have access to (or even on this page but in different widget). If yes, then we are safe + // to remove it, otherwise we need to warn the user. - this.gristDoc.docData.sendAction(['RemoveViewSection', viewSectionRowId]).catch(reportError); + const logTelemetry = () => { + const widgetType = getTelemetryWidgetTypeFromVS(viewSection); + logTelemetryEvent('deletedWidget', {full: {docIdDigest: this.gristDoc.docId(), widgetType}}); + }; + + const isUserTable = () => viewSection.table.peek().isSummary.peek() === false; + + const notInAnyOtherSection = () => { + // Get all viewSection we have access to, and check if the table is used in any of them. + const others = this.gristDoc.docModel.viewSections.rowModels + .filter(vs => !vs.isDisposed()) + .filter(vs => vs.id.peek() !== viewSectionRowId) + .filter(vs => vs.isRaw.peek() === false) + .filter(vs => vs.isRecordCard.peek() === false) + .filter(vs => vs.tableId.peek() === viewSection.tableId.peek()); + return others.length === 0; + }; + + const REMOVED = true, IGNORED = false; + + const possibleActions = { + [DELETE_WIDGET]: async () => { + logTelemetry(); + await this.gristDoc.docData.sendAction(['RemoveViewSection', viewSectionRowId]); + return REMOVED; + }, + [DELETE_DATA]: async () => { + logTelemetry(); + await this.gristDoc.docData.sendActions([ + ['RemoveViewSection', viewSectionRowId], + ['RemoveTable', tableId], + ]); + return REMOVED; + }, + [CANCEL]: async () => IGNORED, + }; + + const tableName = () => viewSection.table.peek().tableNameDef.peek(); + + const needPrompt = isUserTable() && notInAnyOtherSection(); + + const decision = needPrompt + ? widgetRemovalPrompt(tableName()) + : Promise.resolve(DELETE_WIDGET as PromptAction); + + return possibleActions[await decision](); } public rebuildLayout(layoutSpec: BoxSpec) { @@ -417,6 +476,47 @@ export class ViewLayout extends DisposableWithEvents implements IDomComponent { } } +const DELETE_WIDGET = 'deleteOnlyWidget'; +const DELETE_DATA = 'deleteDataAndWidget'; +const CANCEL = 'cancel'; +type PromptAction = typeof DELETE_WIDGET | typeof DELETE_DATA | typeof CANCEL; + +function widgetRemovalPrompt(tableName: string): Promise { + return new Promise((resolve) => { + saveModal((ctl, owner): ISaveModalOptions => { + const selected = Observable.create(owner, ''); + const saveDisabled = Computed.create(owner, use => use(selected) === ''); + const saveFunc = async () => selected.get() && resolve(selected.get() as PromptAction); + owner.onDispose(() => resolve(CANCEL)); + return { + title: t('Table {{tableName}} will no longer be visible', { tableName }), + body: dom('div', + testId('removePopup'), + cssRadioCheckboxOptions( + radioCheckboxOption(selected, DELETE_DATA, t("Delete data and this widget.")), + radioCheckboxOption(selected, DELETE_WIDGET, + t( + `Keep data and delete widget. Table will remain available in {{rawDataLink}}`, + { + rawDataLink: cssLink( + t('raw data page'), + urlState().setHref({docPage: 'data'}), + {target: '_blank'}, + ) + } + ) + ), + ), + ), + saveDisabled, + saveLabel: t("Delete"), + saveFunc, + width: 'fixed-wide', + }; + }); + }); +} + const cssLayoutBox = styled('div', ` @media screen and ${mediaSmall} { &-active, &-inactive { diff --git a/app/client/models/entities/TableRec.ts b/app/client/models/entities/TableRec.ts index 4cb2297b..1aa650d3 100644 --- a/app/client/models/entities/TableRec.ts +++ b/app/client/models/entities/TableRec.ts @@ -39,6 +39,7 @@ export interface TableRec extends IRowModel<"_grist_Tables"> { // If user can select this table in various places. // Note: Some hidden tables can still be visible on RawData view. isHidden: ko.Computed; + isSummary: ko.Computed; tableColor: string; disableAddRemoveRows: ko.Computed; @@ -68,6 +69,8 @@ export function createTableRec(this: TableRec, docModel: DocModel): void { this.primaryTableId = ko.pureComputed(() => this.summarySourceTable() ? this.summarySource().tableId() : this.tableId()); + this.isSummary = this.autoDispose(ko.pureComputed(() => Boolean(this.summarySourceTable()))); + this.groupByColumns = ko.pureComputed(() => this.columns().all().filter(c => c.summarySourceCol())); this.groupDesc = ko.pureComputed(() => { diff --git a/app/client/models/entities/ViewSectionRec.ts b/app/client/models/entities/ViewSectionRec.ts index efcf45a6..181fcd86 100644 --- a/app/client/models/entities/ViewSectionRec.ts +++ b/app/client/models/entities/ViewSectionRec.ts @@ -93,9 +93,12 @@ export interface ViewSectionRec extends IRowModel<"_grist_Views_section">, RuleO // in which case the UI prevents various things like hiding columns or changing the widget type. isRaw: ko.Computed; - tableRecordCard: ko.Computed + /** Is this table card viewsection (the one available after pressing spacebar) */ isRecordCard: ko.Computed; + /** Card record viewSection for associated table (might be the same section) */ + tableRecordCard: ko.Computed; + /** True if this section is disabled. Currently only used by Record Card sections. */ disabled: modelUtil.KoSaveableObservable; diff --git a/test/nbrowser/SelectBySummary.ts b/test/nbrowser/SelectBySummary.ts index a2bd492e..77bd4794 100644 --- a/test/nbrowser/SelectBySummary.ts +++ b/test/nbrowser/SelectBySummary.ts @@ -178,9 +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. - const menu = await gu.openSectionMenu('viewLayout', 'TABLE1'); - await menu.findContent('.test-cmd-name', 'Delete widget').click(); - await gu.waitForServer(); + await gu.deleteWidget('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 index 9086e921..3c522373 100644 --- a/test/nbrowser/ViewLayoutCollapse.ts +++ b/test/nbrowser/ViewLayoutCollapse.ts @@ -761,6 +761,96 @@ describe("ViewLayoutCollapse", function() { await gu.checkForErrors(); }); + it('should prompt when last section is removed from tray', async () => { + const revert = await gu.begin(); + + // Add brand new table and collapse it. + await gu.addNewSection('Table', 'New Table', {tableName: 'ToCollapse'}); + await collapseByMenu('ToCollapse'); + + // Now try to remove it, we should see prompt. + await openCollapsedSectionMenu('ToCollapse'); + await driver.find('.test-section-delete').click(); + assert.match( + await driver.find('.test-modal-title').getText(), + /Table ToCollapse will no longer be visible/ + ); + + // Select first option, to delete both table and widget. + await driver.find('.test-option-deleteDataAndWidget').click(); + await driver.find('.test-modal-confirm').click(); + await gu.waitForServer(); + + // Make sure it is removed. + assert.deepEqual(await collapsedSectionTitles(), []); + assert.deepEqual(await visibleTables(), ['Companies', 'Investments']); + await gu.sendKeys(Key.ESCAPE); + + // Single undo should add it back. + await gu.undo(); + assert.deepEqual(await collapsedSectionTitles(), ['TOCOLLAPSE']); + assert.deepEqual(await visibleTables(), ['Companies', 'Investments', 'ToCollapse']); + + // Now do the same but, keep data. + await openCollapsedSectionMenu('ToCollapse'); + await driver.find('.test-section-delete').click(); + await driver.findWait('.test-modal-dialog', 100); + await driver.find('.test-option-deleteOnlyWidget').click(); + await driver.find('.test-modal-confirm').click(); + await gu.waitForServer(); + + // Make sure it is removed. + assert.deepEqual(await collapsedSectionTitles(), []); + assert.deepEqual(await visibleTables(), ['Companies', 'Investments', 'ToCollapse']); + + // Test single undo. + await gu.undo(); + assert.deepEqual(await collapsedSectionTitles(), ['TOCOLLAPSE']); + assert.deepEqual(await visibleTables(), ['Companies', 'Investments', 'ToCollapse']); + + // Uncollapse it, and do the same with normal section. + await addToMainByMenu('ToCollapse'); + + // Now try to remove it, we should see prompt. + assert.include( + await driver.findAll('.test-viewsection-title', e => e.getText()), 'TOCOLLAPSE'); + + await gu.openSectionMenu('viewLayout', 'ToCollapse'); + await driver.find('.test-section-delete').click(); + await driver.findWait('.test-modal-dialog', 100); + await driver.find('.test-option-deleteOnlyWidget').click(); + await driver.find('.test-modal-confirm').click(); + await gu.waitForServer(); + assert.notInclude( + await driver.findAll('.test-viewsection-title', e => e.getText()), 'TOCOLLAPSE'); + assert.deepEqual(await visibleTables(), ['Companies', 'Investments', 'ToCollapse']); + // Test undo. + await gu.undo(); + assert.include( + await driver.findAll('.test-viewsection-title', e => e.getText()), 'TOCOLLAPSE'); + + // Do the same but delete data and widget. + await gu.openSectionMenu('viewLayout', 'ToCollapse'); + await driver.find('.test-section-delete').click(); + await driver.findWait('.test-modal-dialog', 100); + await driver.find('.test-option-deleteDataAndWidget').click(); + await driver.find('.test-modal-confirm').click(); + await gu.waitForServer(); + + // Make sure it is removed. + assert.notInclude( + await driver.findAll('.test-viewsection-title', e => e.getText()), 'TOCOLLAPSE'); + assert.deepEqual(await visibleTables(), ['Companies', 'Investments']); + + // Test undo. + await gu.undo(); + assert.include( + await driver.findAll('.test-viewsection-title', e => e.getText()), 'TOCOLLAPSE'); + assert.deepEqual(await visibleTables(), ['Companies', 'Investments', 'ToCollapse']); + + await revert(); + }); + it("should switch active section when collapsed", async () => { const revert = await gu.begin(); await gu.selectSectionByTitle(gu.exactMatch(COMPANIES)); @@ -989,3 +1079,11 @@ async function waitForSave() { await gu.waitForServer(); }, 3000); } + +async function visibleTables() { + await driver.findWait('.test-dp-add-new', 2000).doClick(); + await driver.find('.test-dp-add-new-page').doClick(); + const titles = await driver.findAll('.test-wselect-table', e => e.getText()); + await gu.sendKeys(Key.ESCAPE); + return titles.map(x => x.trim()).filter(Boolean).filter(x => x !== 'New Table'); +} diff --git a/test/nbrowser/Views.ntest.js b/test/nbrowser/Views.ntest.js index 9b8e2e8a..5da3560d 100644 --- a/test/nbrowser/Views.ntest.js +++ b/test/nbrowser/Views.ntest.js @@ -113,8 +113,7 @@ describe('Views.ntest', function() { await gu.waitForServer(); await gu.actions.viewSection('TABLE4').selectSection(); // Delete the section - await gu.actions.viewSection('TABLE4').selectMenuOption('viewLayout', 'Delete widget'); - await gu.waitForServer(); + await gu.deleteWidget('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 index fe36fbe0..6fc26dd6 100644 --- a/test/nbrowser/gristUtils.ts +++ b/test/nbrowser/gristUtils.ts @@ -3791,6 +3791,19 @@ export async function waitForAccessDenied() { }); } +/** + * Deletes a widget by title. Optionally confirms deletion only for the widget without the data. + */ +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(); +} + } // end of namespace gristUtils stackWrapOwnMethods(gristUtils);