(core) Distinct style rules for summary columns

Summary:
Summary columns now have their own conditional rules,
which are not shared with sister columns.

Test Plan: New test

Reviewers: alexmojaki

Reviewed By: alexmojaki

Subscribers: dsagal

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

sandbox/grist/migrations.py

@@ -924,3 +924,39 @@ def migration28(tdset):
         doc_actions.append(actions.ModifyColumn(table.tableId, col.colId, {"type": "Attachments"}))
 
   return tdset.apply_doc_actions(doc_actions)
+
+
+@migration(schema_version=29)
+def migration29(tdset):
+  # This migration is fixing an error on summary tables with conditional rules.
+  # On summary tables all formula columns with the same name were updated together,
+  # which caused a situation where some summary columns have rules from diffrent tables.
+  # This migration is removing those rules in such columns.
+
+  tables = {table.id: table
+          for table in actions.transpose_bulk_action(tdset.all_tables["_grist_Tables"])}
+  columns = {col.id: col
+          for col in actions.transpose_bulk_action(tdset.all_tables["_grist_Tables_column"])}
+  doc_actions = []
+
+  def is_valid_rule(parentId, rule_id):
+    # Valid rule should be an existing column,
+    rule_col = columns.get(rule_id)
+    # in the same table.
+    return rule_col and rule_col.parentId == parentId
+
+  for col in columns.values():
+    if col.rules:
+      # Parse rules (they are a json encoded array like '[15]')
+      rules = safe_parse(col.rules)
+      # Remove all conditional styles if anything about rules is invalid.
+      if not (
+        isinstance(rules, list) and
+        all(is_valid_rule(col.parentId, ruleId) for ruleId in rules)
+      ):
+        doc_actions.append(actions.UpdateRecord('_grist_Tables_column', col.id, {
+          "rules": None,
+          "widgetOptions": summary._copy_widget_options(col.widgetOptions)
+        }))
+
+  return tdset.apply_doc_actions(doc_actions)

sandbox/grist/schema.py

@@ -15,7 +15,7 @@ import six
 
 import actions
 
-SCHEMA_VERSION = 28
+SCHEMA_VERSION = 29
 
 def make_column(col_id, col_type, formula='', isFormula=False):
   return {

sandbox/grist/summary.py

@@ -35,6 +35,39 @@ def _get_colinfo_dict(col_info, with_id=False):
   return col_values
 
 
+def skip_rules_update(col, col_values):
+  """
+  Rules for summary tables can't be derived from source columns. This function
+  removes (and kips original) rules settings when updating summary tables.
+  """
+
+  # Remove rules from updates.
+  col_values = {k: v for k, v in six.iteritems(col_values) if k != 'rules'}
+
+  try:
+    # New widgetOptions to use.
+    new_widgetOptions = json.loads(col_values.get('widgetOptions', ''))
+  except ValueError:
+    # If we are not updating widgetOptions (or they are
+    # not a valid json string, i.e. in tests), just return the original updates.
+    return col_values
+
+  try:
+    # Original widgetOptions (maybe with styling rules "ruleOptions").
+    widgetOptions = json.loads(col.widgetOptions or '')
+  except ValueError:
+    widgetOptions = {}
+
+  # Keep the original rulesOptions if any, and ignore any new one.
+  new_widgetOptions.pop("rulesOptions", "")
+  rulesOptions = widgetOptions.get('rulesOptions')
+  if rulesOptions:
+    new_widgetOptions['rulesOptions'] = rulesOptions
+
+  col_values['widgetOptions'] = json.dumps(new_widgetOptions)
+  return col_values
+
+
 def _copy_widget_options(options):
   """Copies widgetOptions for a summary group-by column (omitting conditional formatting rules)"""
   if not options:

sandbox/grist/test_rules.py

@@ -1,4 +1,8 @@
 # -*- coding: utf-8 -*-
+import json
+
+from collections import namedtuple
+from summary import skip_rules_update
 import testutil
 import test_engine
 
@@ -52,6 +56,41 @@ class TestRules(test_engine.EngineTestCase):
     rule = list(rules)[rule_index]
     return self.apply_user_action(['RemoveColumn', 'Inventory', rule.colId])
 
+  def test_summary_updates(self):
+    Col = namedtuple('Col', 'widgetOptions')
+    col = Col(None)
+    # Should remove rules from update
+    self.assertEqual({}, skip_rules_update(col, {'rules': [15]}))
+    # Should leave col_updates untouched when there are no rules.
+    col_updates = {'type': 'Int'}
+    self.assertEqual(col_updates, skip_rules_update(col, col_updates))
+
+    # Should return same dict when not updating ruleOptions
+    col_updates = {'widgetOptions': '{"color": "red"}'}
+    self.assertEqual(col_updates, skip_rules_update(col, col_updates))
+    col = Col('{"color": "red"}')
+    self.assertEqual(col_updates, skip_rules_update(col, col_updates))
+
+    # Should remove ruleOptions from update
+    col_updates = {'widgetOptions': '{"rulesOptions": [{"color": "black"}], "color": "blue"}'}
+    self.assertEqual({'widgetOptions': '{"color": "blue"}'},
+                         skip_rules_update(col, col_updates))
+    col_updates = {'widgetOptions': '{"rulesOptions": [], "color": "blue"}'}
+    self.assertEqual({'widgetOptions': '{"color": "blue"}'},
+                         skip_rules_update(col, col_updates))
+
+    # Should preserve original ruleOptions
+    col = Col('{"rulesOptions": [{"color":"red"}], "color": "blue"}')
+    col_updates = {'widgetOptions': '{"rulesOptions": [{"color": "black"}], "color": "red"}'}
+    updated = skip_rules_update(col, col_updates)
+    self.assertEqual({"rulesOptions": [{"color": "red"}], "color": "red"},
+                         json.loads(updated.get('widgetOptions')))
+    col_updates = {'widgetOptions': '{"color": "red"}'}
+    updated = skip_rules_update(col, col_updates)
+    self.assertEqual({"rulesOptions": [{"color": "red"}], "color": "red"},
+                         json.loads(updated.get('widgetOptions')))
+
+
   def test_simple_rules(self):
     self.load_sample(self.sample)
     # Mark all records with Stock = 0

sandbox/grist/useractions.py

@@ -748,10 +748,6 @@ class UserActions(object):
     # Returns a list of (col, values) pairs (containing the input column but possibly more).
     # Note that it may modify col_values in-place, and may reuse it for multiple results.
 
-    results = []
-    def add(cols, value_dict):
-      results.extend((c, value_dict) for c in cols)
-
     # If changing label, sync it to colId unless untieColIdFromLabel flag is set.
     if 'label' in col_values and not col_values.get('untieColIdFromLabel',col.untieColIdFromLabel):
       col_values.setdefault('colId', col_values['label'])
@@ -780,13 +776,19 @@ class UserActions(object):
       col_values.setdefault('rules', None)
       col_values.setdefault('displayCol', 0)
 
+    # Collect all updates for dependent summary columns.
+    results = []
+    def add(cols, value_dict):
+      results.extend((c, summary.skip_rules_update(c, value_dict)) for c in cols)
+
     source_table = col.parentId.summarySourceTable
     if source_table:  # This is a summary-table column.
       # Disallow isFormula changes.
       if has_diff_value(col_values, 'isFormula', col.isFormula):
         raise ValueError("Cannot change summary column '%s' between formula and data" % col.colId)
 
-      if col.isFormula:
+      # Don't update any sister helper columns.
+      if col.isFormula and not col.colId.startswith("gristHelper"):
         # Get all same-named formula columns from other summary tables for the same source table,
         # and apply the same changes to them.
         add(self._get_sister_columns(source_table, col), col_values)