diff --git a/app/client/ui/DocTutorial.ts b/app/client/ui/DocTutorial.ts index 2fd1c037..efd53720 100644 --- a/app/client/ui/DocTutorial.ts +++ b/app/client/ui/DocTutorial.ts @@ -503,3 +503,14 @@ const cssSkipIcon = styled(icon, ` height: 20px; margin: 0px -3px; `); + +// This is aligned with css in ./DocTutorial.css +export const cssCode = styled('code', ` + padding: 2px 5px; + color: ${theme.tutorialsPopupCodeFg}; + background: ${theme.tutorialsPopupCodeBg}; + border: 1px solid ${theme.tutorialsPopupCodeBorder}; + white-space: pre-wrap; + word-wrap: break-word; + font-size: max(12.6px, 90%); /* 90% of 13px */ +`); diff --git a/app/client/ui/GristTooltips.ts b/app/client/ui/GristTooltips.ts index bf0c1eb7..651ef4f1 100644 --- a/app/client/ui/GristTooltips.ts +++ b/app/client/ui/GristTooltips.ts @@ -45,6 +45,7 @@ export type Tooltip = | 'setRefDropdownCondition' | 'communityWidgets' | 'twoWayReferences' + | 'twoWayReferencesDisabled' | 'reasignTwoWayReference'; export type TooltipContentFunc = (...domArgs: DomElementArg[]) => DomContents; @@ -166,16 +167,23 @@ see or edit which parts of your document.') ), twoWayReferences: (...args: DomElementArg[]) => cssTooltipContent( dom('div', - t('Creates a reverse column in target table that can be edited from either end.') + t('Creates a new Reference List column in the target table, with both this ' + + 'and the target columns editable and synchronized.') + ), + ...args, + ), + twoWayReferencesDisabled: (...args: DomElementArg[]) => cssTooltipContent( + dom('div', + t('Two-way references are not currently supported for Formula or Trigger Formula columns') ), ...args, ), reasignTwoWayReference: (...args: DomElementArg[]) => cssTooltipContent( dom('div', - t('This limitation occurs when one end of a two-way reference is configured as a single Reference.') + t('This limitation occurs when one column in a two-way reference has the Reference type.') ), dom('div', - t('To allow multiple assignments, change the type of the Reference column to Reference List.') + t(`To allow multiple assignments, change the referenced column's type to Reference List.`) ), ...args, ), diff --git a/app/client/ui/buildReassignModal.ts b/app/client/ui/buildReassignModal.ts index 8cd1fa6c..beffdbbe 100644 --- a/app/client/ui/buildReassignModal.ts +++ b/app/client/ui/buildReassignModal.ts @@ -1,6 +1,7 @@ import * as commands from 'app/client/components/commands'; import {makeT} from 'app/client/lib/localization'; import {ColumnRec, DocModel} from 'app/client/models/DocModel'; +import {cssCode} from 'app/client/ui/DocTutorial'; import {withInfoTooltip} from 'app/client/ui/tooltips'; import {bigBasicButton, bigPrimaryButton, textButton} from 'app/client/ui2018/buttons'; import {labeledSquareCheckbox} from 'app/client/ui2018/checkbox'; @@ -101,10 +102,10 @@ export async function buildReassignModal(options: { `{{targetTable}} record {{targetName}} is already assigned to {{sourceTable}} record \ {{oldSourceName}}.`, { - targetTable: dom('i', Pets), - sourceTable: dom('i', Owners), - targetName: dom('b', Azor), - oldSourceName: dom('b', Bob), + targetTable: cssCode(Pets), + sourceTable: cssCode(Owners), + targetName: cssName(Azor), + oldSourceName: cssName(Bob), }); return cssBulletLine(text); @@ -117,11 +118,11 @@ export async function buildReassignModal(options: { // Task is the name of the revRec table const Pets = revRec.table().tableNameDef(); const Owners = colRec.table().tableNameDef(); - return dom('div', [ + return cssHigherLine([ t(`Each {{targetTable}} record may only be assigned to a single {{sourceTable}} record.`, { - targetTable: dom('i', Pets), - sourceTable: dom('i', Owners), + targetTable: cssCode(Pets), + sourceTable: cssCode(Owners), }) ]); } @@ -151,14 +152,14 @@ export async function buildReassignModal(options: { const Ann = rowDisplay(colRec.table().tableId(), newRowId, colRec.colId()) as string; const singleText = () => t(`Reassign to {{sourceTable}} record {{sourceName}}.`, { - sourceTable: dom('i', colRec.table().tableNameDef()), - sourceName: dom('b', Ann), + sourceTable: cssCode(colRec.table().tableNameDef()), + sourceName: cssName(Ann), }); const multiText = () => t(`Reassign to new {{sourceTable}} records.`, { - sourceTable: dom('i', colRec.table().tableNameDef()), + sourceTable: cssCode(colRec.table().tableNameDef()), }); - return labeledSquareCheckbox(checked, multiple ? multiText() : singleText()); + return cssCheckbox(checked, multiple ? multiText() : singleText()); } } @@ -368,9 +369,23 @@ function* bulkToSingle(actions: DocAction[]): Iterable { const cssBulletLine = styled('div', ` margin-bottom: 8px; + line-height: 22px; &::before { content: '•'; margin-right: 4px; color: ${theme.lightText}; } `); + +const cssHigherLine = styled('div', ` + line-height: 22px; +`); + +const cssName = (text: string) => dom('span', `"${text}"`); + +const cssCheckbox = styled(labeledSquareCheckbox, ` + line-height: 22px; + & > span { + overflow: unset; /* make some room for cssCode */ + } +`); diff --git a/app/client/ui/tooltips.ts b/app/client/ui/tooltips.ts index f1529bbc..48b7e5c3 100644 --- a/app/client/ui/tooltips.ts +++ b/app/client/ui/tooltips.ts @@ -6,13 +6,13 @@ */ import {logTelemetryEvent} from 'app/client/lib/telemetry'; -import {GristTooltips, Tooltip, TooltipContentFunc} from 'app/client/ui/GristTooltips'; +import {GristTooltips, Tooltip} from 'app/client/ui/GristTooltips'; import {prepareForTransition} from 'app/client/ui/transitions'; import {testId, theme, vars} from 'app/client/ui2018/cssVars'; import {icon} from 'app/client/ui2018/icons'; import {makeLinks} from 'app/client/ui2018/links'; import {menuCssClass} from 'app/client/ui2018/menus'; -import {dom, DomContents, DomElementArg, DomElementMethod, styled} from 'grainjs'; +import {BindableValue, dom, DomContents, DomElementArg, DomElementMethod, styled} from 'grainjs'; import Popper from 'popper.js'; import {cssMenu, cssMenuItem, defaultMenuOptions, IPopupOptions, setPopupToCreateDom} from 'popweasel'; import merge = require('lodash/merge'); @@ -324,12 +324,12 @@ export type InfoTooltipVariant = 'click' | 'hover'; * Renders an info icon that shows a tooltip with the specified `content`. */ export function infoTooltip( - tooltip: Tooltip|TooltipContentFunc, + tooltip: BindableValue, options: InfoTooltipOptions = {}, ...domArgs: DomElementArg[] ) { const {variant = 'click'} = options; - const content = typeof tooltip === 'function' ? tooltip() : GristTooltips[tooltip](); + const content = dom.domComputed(tooltip, (t) => GristTooltips[t]()); const onOpen = () => logTelemetryEvent('viewedTip', {full: {tipName: tooltip}}); switch (variant) { case 'click': { @@ -433,7 +433,7 @@ export interface WithInfoTooltipOptions { */ export function withInfoTooltip( domContents: DomContents, - tooltip: Tooltip, + tooltip: BindableValue, options: WithInfoTooltipOptions = {}, ) { const {variant = 'click', domArgs, iconDomArgs, popupOptions} = options; diff --git a/app/client/widgets/ReverseReferenceConfig.ts b/app/client/widgets/ReverseReferenceConfig.ts index c37827cb..cc2e5724 100644 --- a/app/client/widgets/ReverseReferenceConfig.ts +++ b/app/client/widgets/ReverseReferenceConfig.ts @@ -2,6 +2,8 @@ import {allCommands} from 'app/client/components/commands'; import {makeT} from 'app/client/lib/localization'; import {TableRec} from 'app/client/models/DocModel'; import {ViewFieldRec} from 'app/client/models/entities/ViewFieldRec'; +import {cssCode} from 'app/client/ui/DocTutorial'; +import {Tooltip} from 'app/client/ui/GristTooltips'; import { cssLabelText, cssRow, @@ -27,6 +29,7 @@ export class ReverseReferenceConfig extends Disposable { private _reverseColumn: Computed; private _reverseType: Computed; private _disabled: Computed; + private _tooltip: Computed; constructor(private _field: ViewFieldRec) { super(); @@ -54,6 +57,11 @@ export class ReverseReferenceConfig extends Disposable { const column = use(this._field.column); return Boolean(use(column.formula)); }); + this._tooltip = Computed.create(this, (use) => { + return use(this._disabled) + ? 'twoWayReferencesDisabled' + : 'twoWayReferences'; + }); } public buildDom() { @@ -69,8 +77,8 @@ export class ReverseReferenceConfig extends Disposable { testId('add-reverse-columm'), dom.prop('disabled', this._disabled), ), - 'twoWayReferences' - ), + this._tooltip + ) ), ]), dom.maybe(this._isConfigured, () => cssTwoWayConfig( @@ -94,7 +102,7 @@ export class ReverseReferenceConfig extends Disposable { cssContent( cssClipLine( cssClipItem( - cssCapitalize(t('Table'), dom.style('margin-right', '8px')), + cssCapitalize(t('Target table'), dom.style('margin-right', '8px')), dom('span', dom.text(this._reverseTable)), ), ), @@ -135,20 +143,21 @@ export class ReverseReferenceConfig extends Disposable { await column.removeReverseColumn(); }; - const revColumnLabel = column.reverseColModel.peek().label.peek() || column.reverseColModel.peek().colId.peek(); - const revTableName = column.reverseColModel.peek().table.peek().tableNameDef.peek(); + const refCol = column.reverseColModel.peek().label.peek() || column.reverseColModel.peek().colId.peek(); + const refTable = column.reverseColModel.peek().table.peek().tableNameDef.peek(); - const promptTitle = t('Delete column {{column}} in table {{table}}?', { - column: dom('b', revColumnLabel), - table: dom('b', revTableName), - }); + const promptTitle = t('Delete two-way reference?'); const myTable = column.table.peek().tableNameDef.peek(); const myName = column.label.peek() || column.colId.peek(); - const explanation = t('It is the reverse of the reference column {{column}} in table {{table}}.', { - column: dom('b', myName), - table: dom('b', myTable) + const explanation = t( + 'This will delete the reference column {{refCol}} in table {{refTable}}. The reference column ' + + '{{myName}} will remain in the current table {{myTable}}.', { + refCol: dom('b', refCol), + refTable: cssCode(refTable), + myName: dom('b', myName), + myTable: cssCode(myTable), }); confirmModal( @@ -156,7 +165,7 @@ export class ReverseReferenceConfig extends Disposable { t('Delete'), onConfirm, { - explanation, + explanation: cssHigherLine(explanation), width: 'fixed-wide' } ); @@ -234,3 +243,7 @@ const cssGrayText = styled('span', ` const cssIconAccent = styled(icon, ` --icon-color: ${theme.accentIcon}; `); + +const cssHigherLine = styled('div', ` + line-height: 1.5; +`); diff --git a/sandbox/grist/test_twoway_refs.py b/sandbox/grist/test_twoway_refs.py index 5ea91b3d..263f0888 100644 --- a/sandbox/grist/test_twoway_refs.py +++ b/sandbox/grist/test_twoway_refs.py @@ -20,6 +20,9 @@ Bob = 2 Penny = 3 EmptyList = None +def uniqueReferences(rec): + return rec.reverseCol and rec.reverseCol.type.startswith('Ref:') + class TestTwoWayReferences(test_engine.EngineTestCase): def get_col_rec(self, tableId, colId): @@ -319,6 +322,16 @@ class TestTwoWayReferences(test_engine.EngineTestCase): # Add a pet named Rex with Bob as owner self.apply_user_action(["AddRecord", "Pets", 1, {"Name": "Rex", "Owner": Bob}]) + self.assertTableData("Owners", cols="subset", data=[ + ["id", "Name"], + [Alice, "Alice"], + [Bob, "Bob"], + ]) + self.assertTableData("Pets", cols="subset", data=[ + ["id", "Name", "Owner"], + [Rex, "Rex", Bob], + ]) + def test_uniques(self): self.load_pets() @@ -1192,9 +1205,78 @@ class TestTwoWayReferences(test_engine.EngineTestCase): [Rex, "Rex", Alice], ]) + def test_back_loop(self): + """ + Test that updating reverse column doesn't cause infinite loop. + """ -def uniqueReferences(rec): - return rec.reverseCol and rec.reverseCol.type.startswith('Ref:') + # Load pets sample. + self.load_pets() + + # Add reverse column for Owner. + self.apply_user_action(["AddReverseColumn", 'Pets', 'Owner']) + + # Convert Pets to Ref:Owners. + self.apply_user_action(["ModifyColumn", "Owners", "Pets", {"type": "Ref:Pets"}]) + + # Check the data. + self.assertTableData("Pets", cols="subset", data=[ + ["id", "Name", "Owner"], + [1, "Rex", Bob], + ]) + + self.assertTableData("Owners", cols="subset", data=[ + ["id", "Name", "Pets"], + [1, "Alice", Empty], + [2, "Bob", Rex], + ]) + + # Now move Rex to Alice using Pets table. + self.apply_user_action(["UpdateRecord", "Pets", Rex, {"Owner": Alice}]) + + + def test_remove_in_bulk(self): + """ + Test that we can remove many rows at the same time. PReviously it ended up in an error, + as the reverse column was trying to update the removed row. + """ + + # Load pets sample. + self.load_pets() + + # Add another dog. + self.apply_user_action(["AddRecord", "Pets", Pluto, {"Name": "Pluto"}]) + + # Add reverse column for Owner. + self.apply_user_action(["AddReverseColumn", 'Pets', 'Owner']) + + # Add Pluto to Bob. + self.apply_user_action(["UpdateRecord", "Pets", Pluto, {"Owner": Alice}]) + + # Test the data. + self.assertTableData("Pets", cols="subset", data=[ + ["id", "Name", "Owner"], + [Rex, "Rex", Bob], + [Pluto, "Pluto", Alice], + ]) + self.assertTableData("Owners", cols="subset", data=[ + ["id", "Name", "Pets"], + [1, "Alice", [Pluto]], + [2, "Bob", [Rex]], + ]) + + # Now remove both dogs. + self.apply_user_action(["BulkRemoveRecord", "Pets", [Rex, Pluto]]) + + # Make sure we see the data. + self.assertTableData("Pets", cols="subset", data=[ + ["id", "Name", "Owner"], + ]) + self.assertTableData("Owners", cols="subset", data=[ + ["id", "Name", "Pets"], + [1, "Alice", EmptyList], + [2, "Bob", EmptyList], + ]) if __name__ == "__main__": diff --git a/sandbox/grist/useractions.py b/sandbox/grist/useractions.py index f8c8c678..3ffb77fe 100644 --- a/sandbox/grist/useractions.py +++ b/sandbox/grist/useractions.py @@ -1164,13 +1164,19 @@ class UserActions(object): continue updates = ref_col.get_updates_for_removed_target_rows(row_id_set) if updates: - # Previously we sent this as a docaction. Now we do a proper useraction with all the - # processing that involves, e.g. triggering two-way-reference updates, and also all the - # metadata checks and updates. - self._BulkUpdateRecord_decoded(ref_col.table_id, - [row_id for (row_id, value) in updates], - { ref_col.col_id: [value for (row_id, value) in updates] } - ) + table_id = ref_col.table_id + rows = [row_id for (row_id, value) in updates] + columns = {ref_col.col_id: [value for (row_id, value) in updates]} + if ref_col.table_id.startswith('_grist_'): + # Previously we sent this as a docaction. Now we do a proper useraction with all the + # processing that involves, e.g. triggering two-way-reference updates, and also all the + # metadata checks and updates. + self._BulkUpdateRecord_decoded(table_id, rows, columns) + else: + # But for normal user tables (with two-way references), we must still use the docaction, + # otherwise we'd invoke two-way update logic, and the reverse column would try to update + # rows that we just deleted. + self._do_doc_action(actions.BulkUpdateRecord(table_id, rows, columns)) @useraction def RemoveRecord(self, table_id, row_id): diff --git a/test/nbrowser/TwoWayReference.ts b/test/nbrowser/TwoWayReference.ts index 58df770e..5b924895 100644 --- a/test/nbrowser/TwoWayReference.ts +++ b/test/nbrowser/TwoWayReference.ts @@ -134,7 +134,7 @@ describe('TwoWayReference', function() { // We are now in a modal dialog. assert.equal( await driver.findWait('.test-modal-dialog label', 100).getText(), - 'Reassign to Owners record Bob.' + 'Reassign to Owners record "Bob".' ); // Reassign it. @@ -599,7 +599,7 @@ describe('TwoWayReference', function() { // We should have an option there. assert.equal( await driver.findWait('.test-modal-dialog label', 100).getText(), - 'Reassign to People record Alice.' + 'Reassign to People record "Alice".' ); // Reassign it. @@ -729,7 +729,7 @@ const removeTwoWay = () => driver.findWait('.test-remove-reverse-column', 100).c const configText = async () => { const text = await driver.findWait('.test-reverse-column-label', 100).getText(); - return text.trim().split('\n').join('').replace('COLUMN', '.').replace("TABLE", ""); + return text.trim().split('\n').join('').replace('COLUMN', '.').replace("TARGET TABLE", ""); }; const removeModal = {