(core) Improving experience when editing group-by column.

Summary:
Improving experience when editing group-by column:
- Disable column rename
- Allow changing most widget properties:
 - Color/Background
 - Number format
 - Date/DateTime format (but not the timezone)
 - All toggle options (for toggle column)
- Remove Edit button on Choice Edit
- Changing the underlying column should reset all those options back to the original column.

Test Plan: nbrowser

Reviewers: alexmojaki

Reviewed By: alexmojaki

Subscribers: alexmojaki

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

app/client/components/GridView.js

@@ -918,7 +918,12 @@ GridView.prototype.buildDom = function() {
             kd.style('borderLeftWidth', v.borderWidthPx),
             kd.foreach(v.viewFields(), field => {
               var isEditingLabel = ko.pureComputed({
-                read: () => this.gristDoc.isReadonlyKo() || self.isPreview ? false : editIndex() === field._index(),
+                read: () => {
+                  const goodIndex = () => editIndex() === field._index();
+                  const isReadonly = () => this.gristDoc.isReadonlyKo() || self.isPreview;
+                  const isSummary = () => Boolean(field.column().disableEditData());
+                  return goodIndex() && !isReadonly() && !isSummary();
+                },
                 write: val => editIndex(val ? field._index() : -1)
               }).extend({ rateLimit: 0 });
               let filterTriggerCtl;

app/client/lib/ACSelect.ts

@@ -18,6 +18,7 @@ export interface ACSelectItem extends ACItem {
 export function buildACSelect(
   owner: IDisposableOwner,
   options: {
+    disabled?: Observable<boolean>,
     acIndex: ACIndex<ACSelectItem>,
     valueObs: Observable<string>,
     save: (value: string, item: ACSelectItem|undefined) => Promise<void>|void
@@ -55,6 +56,9 @@ export function buildACSelect(
 
   const onMouseDown = (ev: MouseEvent) => {
     ev.preventDefault();    // Don't let it affect focus, since we focus/blur manually.
+    if (options.disabled?.get()) {
+      return;
+    }
     if (!isOpen()) { textInput.focus(); }
     openOrCommit();
   };
@@ -73,6 +77,7 @@ export function buildACSelect(
       dom.prop('value', valueObs),
       dom.on('focus', (ev, elem) => elem.select()),
       dom.on('blur', commitOrRevert),
+      dom.prop("disabled", (use) => options.disabled ? use(options.disabled) : false),
       dom.onKeyDown({
         Escape: revert,
         Enter: openOrCommit,

app/client/ui/FieldConfig.ts

@@ -58,7 +58,8 @@ export function buildNameConfig(owner: MultiHolder, origColumn: ColumnRec, curso
         cssColTieConnectors(),
         cssToggleButton(icon('FieldReference'),
           cssToggleButton.cls('-selected', (use) => !use(untieColId)),
-          dom.on('click', () => untieColId.saveOnly(!untieColId.peek())),
+          dom.on('click', () => !origColumn.disableModify.peek() && untieColId.saveOnly(!untieColId.peek())),
+          cssToggleButton.cls("-disabled", origColumn.disableModify),
           testId('field-derive-id')
         ),
       )
@@ -391,6 +392,10 @@ const cssToggleButton = styled(cssIconButton, `
   &-selected:hover {
     --icon-color: ${colors.darkGrey};
   }
+  &-disabled, &-disabled:hover {
+    --icon-color: ${colors.light};
+    background-color: var(--grist-color-medium-grey-opaque);
+  }
 `);
 
 const cssColLabelBlock = styled('div', `

app/client/widgets/ChoiceListEntry.ts

@@ -91,7 +91,8 @@ export class ChoiceListEntry extends Disposable {
   constructor(
     private _values: Observable<string[]>,
     private _choiceOptionsByName: Observable<ChoiceOptionsByName>,
-    private _onSave: (values: string[], choiceOptions: ChoiceOptionsByName, renames: Record<string, string>) => void
+    private _onSave: (values: string[], choiceOptions: ChoiceOptionsByName, renames: Record<string, string>) => void,
+    private _disabled: Observable<boolean>
   ) {
     super();
 
@@ -177,22 +178,27 @@ export class ChoiceListEntry extends Disposable {
               )
             ),
             dom.on('click', () => this._startEditing()),
+            cssListBoxInactive.cls("-disabled", this._disabled),
             testId('choice-list-entry')
           ),
+          dom.maybe(use => !use(this._disabled), () =>
             cssButtonRow(
               primaryButton('Edit',
                 dom.on('click', () => this._startEditing()),
                 testId('choice-list-entry-edit')
               )
             )
+          )
         );
       }
     });
   }
 
   private _startEditing(): void {
+    if (!this._disabled.get()) {
       this._isEditing.set(true);
     }
+  }
 
   private _save(): void {
     const tokenField = this._tokenFieldHolder.get();
@@ -369,6 +375,9 @@ const cssListBoxInactive = styled(cssListBox, `
   &:hover {
     border: 1px solid ${colors.hover};
   }
+  &-disabled {
+    cursor: default;
+  }
 `);
 
 const cssListRow = styled('div', `

app/client/widgets/ChoiceTextBox.ts

@@ -5,7 +5,7 @@ import {KoSaveableObservable} from 'app/client/models/modelUtil';
 import {cssLabel, cssRow} from 'app/client/ui/RightPanel';
 import {testId} from 'app/client/ui2018/cssVars';
 import {NTextBox} from 'app/client/widgets/NTextBox';
-import {Computed, dom, styled} from 'grainjs';
+import {Computed, dom, fromKo, styled} from 'grainjs';
 import {choiceToken, DEFAULT_FILL_COLOR, DEFAULT_TEXT_COLOR} from 'app/client/widgets/ChoiceToken';
 
 export interface IChoiceOptions {
@@ -73,7 +73,8 @@ export class ChoiceTextBox extends NTextBox {
           ChoiceListEntry,
           this._choiceValues,
           this._choiceOptionsByName,
-          this.save.bind(this)
+          this.save.bind(this),
+          fromKo(this.field.column().disableEditData)
         )
       )
     ];

app/client/widgets/DateTimeTextBox.js

@@ -66,7 +66,10 @@ DateTimeTextBox.prototype.buildConfigDom = function(isTransformConfig) {
   var self = this;
   return dom('div',
     cssLabel("Timezone"),
-    cssRow(gdom.create(buildTZAutocomplete, moment, fromKo(this._timezone), this._setTimezone)),
+    cssRow(
+      gdom.create(buildTZAutocomplete, moment, fromKo(this._timezone), this._setTimezone,
+        { disabled : fromKo(this.field.column().disableEditData)}),
+      ),
     self.buildDateConfigDom(),
     cssLabel("Time Format"),
     cssRow(dom(select(fromKo(self.standardTimeFormat), self.timeFormatOptions), dom.testId("Widget_timeFormat"))),

app/client/widgets/TZAutocomplete.ts

@@ -47,7 +47,8 @@ export function buildTZAutocomplete(
   owner: IDisposableOwner,
   moment: MomentTimezone,
   valueObs: Observable<string>,
-  save: (value: string) => Promise<void>|void
+  save: (value: string) => Promise<void>|void,
+  options?: { disabled?: Observable<boolean> }
 ) {
   // Set a large maxResults, since it's sometimes nice to see all supported timezones (there are
   // fewer than 1000 in practice).
@@ -69,7 +70,7 @@ export function buildTZAutocomplete(
     }
   };
   return buildACSelect(owner,
-    {acIndex, valueObs, save: saveTZ},
+    {...options, acIndex, valueObs, save: saveTZ},
     testId("tz-autocomplete")
   );
 }

sandbox/grist/test_summary.py

@@ -8,6 +8,7 @@ import logger
 import summary
 import testutil
 import test_engine
+from useractions import allowed_summary_change
 
 from test_engine import Table, Column, View, Section, Field
 
@@ -839,3 +840,96 @@ class Address:
       [ 1,    1.0,  [1,2],    2,        9   ],
       [ 2,    2.0,  [3],      1,        6   ],
     ])
+
+  #----------------------------------------------------------------------
+  # pylint: disable=R0915
+  def test_allow_select_by_change(self):
+    def widgetOptions(n, o):
+      return allowed_summary_change('widgetOptions', n, o)
+
+    # Can make no update on widgetOptions.
+    new = None
+    old = None
+    self.assertTrue(widgetOptions(new, old))
+
+    new = ''
+    old = None
+    self.assertTrue(widgetOptions(new, old))
+
+    new = ''
+    old = ''
+    self.assertTrue(widgetOptions(new, old))
+
+    new = None
+    old = ''
+    self.assertTrue(widgetOptions(new, old))
+
+    # Can update when key was not present
+    new = '{"widget":"TextBox","alignment":"center"}'
+    old = ''
+    self.assertTrue(widgetOptions(new, old))
+
+    new = ''
+    old = '{"widget":"TextBox","alignment":"center"}'
+    self.assertTrue(widgetOptions(new, old))
+
+    # Can update when key was present.
+    new = '{"widget":"TextBox","alignment":"center"}'
+    old = '{"widget":"Spinner","alignment":"center"}'
+    self.assertTrue(widgetOptions(new, old))
+
+    # Can update but must leave other options.
+    new = '{"widget":"TextBox","cant":"center"}'
+    old = '{"widget":"Spinner","cant":"center"}'
+    self.assertTrue(widgetOptions(new, old))
+
+    # Can't add protected property when old was empty.
+    new = '{"widget":"TextBox","cant":"new"}'
+    old = None
+    self.assertFalse(widgetOptions(new, old))
+
+    # Can't remove when there was a protected property.
+    new = None
+    old = '{"widget":"TextBox","cant":"old"}'
+    self.assertFalse(widgetOptions(new, old))
+
+    # Can't update by omitting.
+    new = '{"widget":"TextBox"}'
+    old = '{"widget":"TextBox","cant":"old"}'
+    self.assertFalse(widgetOptions(new, old))
+
+    # Can't update by changing.
+    new = '{"widget":"TextBox","cant":"new"}'
+    old = '{"widget":"TextBox","cant":"old"}'
+    self.assertFalse(widgetOptions(new, old))
+
+    # Can't update by adding.
+    new = '{"widget":"TextBox","cant":"new"}'
+    old = '{"widget":"TextBox"}'
+    self.assertFalse(widgetOptions(new, old))
+
+    # Can update objects
+    new = '{"widget":"TextBox","alignment":{"prop":1},"cant":{"prop":1}}'
+    old = '{"widget":"TextBox","alignment":{"prop":2},"cant":{"prop":1}}'
+    self.assertTrue(widgetOptions(new, old))
+
+    # Can't update objects
+    new = '{"widget":"TextBox","cant":{"prop":1}}'
+    old = '{"widget":"TextBox","cant":{"prop":2}}'
+    self.assertFalse(widgetOptions(new, old))
+
+    # Can't update lists
+    new = '{"widget":"TextBox","cant":[1, 2]}'
+    old = '{"widget":"TextBox","cant":[2, 1]}'
+    self.assertFalse(widgetOptions(new, old))
+
+    # Can update lists
+    new = '{"widget":"TextBox","alignment":[1, 2]}'
+    old = '{"widget":"TextBox","alignment":[3, 2]}'
+    self.assertTrue(widgetOptions(new, old))
+
+    # Can update without changing list.
+    new = '{"widget":"TextBox","dontChange":[1, 2]}'
+    old = '{"widget":"Spinner","dontChange":[1, 2]}'
+    self.assertTrue(widgetOptions(new, old))
+  # pylint: enable=R0915

sandbox/grist/useractions.py

@@ -146,6 +146,32 @@ def guess_type(values, convert=False):
   return "Numeric" if total and counter[True] >= total * 0.9 else "Text"
 
 
+def allowed_summary_change(key, updated, original):
+  """
+  Checks if summary group by column can be modified.
+  """
+  if updated == original:
+    return True
+  elif key == 'widgetOptions':
+    try:
+      updated_options = json.loads(updated or '{}')
+      original_options = json.loads(original or '{}')
+    except ValueError:
+      return False
+    # Unfortunately all widgetOptions are allowed to change, except choice items. But it is
+    # better to list those that can be changed.
+    # TODO: move choice items to separate column
+    allowed_to_change = {'widget', 'dateFormat', 'timeFormat', 'isCustomDateFormat', 'alignment',
+                         'fillColor', 'textColor', 'isCustomTimeFormat', 'isCustomDateFormat',
+                         'numMode', 'numSign', 'decimals', 'maxDecimals', 'currency'}
+    # Helper function to remove protected keys from dictionary.
+    def trim(options):
+      return {k: v for k, v in options.items() if k not in allowed_to_change}
+    return trim(updated_options) == trim(original_options)
+  else:
+    return False
+
+
 class UserActions(object):
   def __init__(self, eng):
     self._engine = eng
@@ -574,7 +600,7 @@ class UserActions(object):
             # Type sometimes must differ (e.g. ChoiceList -> Choice).
             expected = summary.summary_groupby_col_type(expected)
 
-          if value != expected:
+          if not allowed_summary_change(key, value, expected):
             raise ValueError("Cannot modify summary group-by column '%s'" % col.colId)
 
     make_acl_updates = acl.prepare_acl_col_renames(self._docmodel, self, renames)

test/nbrowser/gristUtils.ts

@@ -890,6 +890,38 @@ export async function undo(optCount: number = 1, optTimeout?: number) {
 }
 
 
+/**
+ * Returns a function to undo all user actions from a particular point in time.
+ */
+export async function begin() {
+  const undoStackPointer = () => driver.executeScript<number>(`
+    return window.gristDocPageModel.gristDoc.get()._undoStack._pointer;
+  `);
+  const start = await undoStackPointer();
+  return async () => undo(await undoStackPointer() - start);
+}
+
+/**
+ * Simulates a transaction on the GristDoc. Use with cautions, as there is no guarantee it will undo correctly
+ * in a case of failure.
+ *
+ * Example:
+ *
+ * it('should ...', revertChanges(async function() {
+ * ...
+ * }));
+ */
+export function revertChanges(test: () => Promise<void>) {
+  return async function() {
+    const revert = await begin();
+    try {
+      await test();
+    } finally {
+      await revert();
+    }
+  };
+}
+
 /**
  * Click the Redo button and wait for server. If optCount is given, click Redo that many times.
  */
@@ -1749,7 +1781,7 @@ export async function getDateFormat(): Promise<string> {
 /**
  * Changes date format for date and datetime editor
  */
-export async function setDateFormat(format: string) {
+export async function setDateFormat(format: string|RegExp) {
   await driver.find('[data-test-id=Widget_dateFormat]').click();
   await driver.findContentWait('.test-select-menu .test-select-row', format, 200).click();
   await waitForServer();