(core) Two way reference polish

Summary:
- Fixing BulkRemoveRecord bug
- Rewriting copy on the `delete reverse column` dialog
- Rewriting text on the `reassign reference dialog`
- Adding tooltip that explains why 2-way references are not enabled for formula columns

Test Plan: Added tests

Reviewers: georgegevoian

Reviewed By: georgegevoian

Subscribers: paulfitz

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

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 */
+`);

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,
   ),

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),
+          targetTable: cssCode(Pets),
-          sourceTable: dom('i', Owners),
+          sourceTable: cssCode(Owners),
-          targetName: dom('b', Azor),
+          targetName: cssName(Azor),
-          oldSourceName: dom('b', Bob),
+          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),
+            targetTable: cssCode(Pets),
-            sourceTable: dom('i', Owners),
+            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()),
+          sourceTable: cssCode(colRec.table().tableNameDef()),
-          sourceName: dom('b', Ann),
+          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<DocAction> {
 
 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 */
+  }
+`);

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<Tooltip>,
   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<Tooltip>,
   options: WithInfoTooltipOptions = {},
 ) {
   const {variant = 'click', domArgs, iconDomArgs, popupOptions} = options;

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<string>;
   private _reverseType: Computed<string>;
   private _disabled: Computed<boolean>;
+  private _tooltip: Computed<Tooltip>;
 
   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 refCol = column.reverseColModel.peek().label.peek() || column.reverseColModel.peek().colId.peek();
-    const revTableName = column.reverseColModel.peek().table.peek().tableNameDef.peek();
+    const refTable = column.reverseColModel.peek().table.peek().tableNameDef.peek();
 
-    const promptTitle = t('Delete column {{column}} in table {{table}}?', {
+    const promptTitle = t('Delete two-way reference?');
-      column: dom('b', revColumnLabel),
-      table: dom('b', revTableName),
-    });
 
     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}}.', {
+    const explanation = t(
-      column: dom('b', myName),
+      'This will delete the reference column {{refCol}} in table {{refTable}}. The reference column ' +
-      table: dom('b', myTable)
+      '{{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;
+`);

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):
+    # Load pets sample.
-  return rec.reverseCol and rec.reverseCol.type.startswith('Ref:')
+    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__":

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:
+        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(ref_col.table_id,
+          self._BulkUpdateRecord_decoded(table_id, rows, columns)
-          [row_id for (row_id, value) in updates],
+        else:
-          { ref_col.col_id: [value for (row_id, value) in updates] }
+          # 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):

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 = {