From 519f1be93aca00d7f4184fab1b01a89aec965ad8 Mon Sep 17 00:00:00 2001
From: Alex Hall <alex.mojaki@gmail.com>
Date: Tue, 7 Jun 2022 13:42:04 +0200
Subject: [PATCH] (core) Disambiguate label for link between summary table and
 source table with self reference

Summary:
Following up on a small bug introduced in https://phab.getgrist.com/D3464. When a table has a column referencing the same table, then there can be two 'select by' options with the same label which is just the name of a summary table on the same page. The first option is simply filtering based on the summary table. The second option is linking the ref column in the source against the group column in the summary, but the name of the group column is hidden which leads to the ambiguity.

The solution in this diff is to always show the target node (source table) column name if the source node (summary table) column was the hidden group column. This also changes the label in the case where the reference to the source table isn't in the source table - see the updated test. This isn't strictly necessary in this case so I'm not 100% about the desired behaviour, but I don't think it hurts.

Test Plan: Tested disambiguation manually. Updated existing test.

Reviewers: dsagal

Reviewed By: dsagal

Differential Revision: https://phab.getgrist.com/D3472
---
 app/client/ui/selectBy.ts | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/app/client/ui/selectBy.ts b/app/client/ui/selectBy.ts
index e525e3a6..c4287bc0 100644
--- a/app/client/ui/selectBy.ts
+++ b/app/client/ui/selectBy.ts
@@ -93,8 +93,8 @@ function isValidLink(source: LinkNode, target: LinkNode) {
   // Instead the ref column must link against the group column of the summary table, which is allowed above.
   // The 'group' column name will be hidden from the options so it feels like linking using summaryness.
   if (
-    (source.groupbyColumns && !source.column && target.column) ||
-    (target.groupbyColumns && !target.column && source.column)
+    (source.isSummary && !source.column && target.column) ||
+    (target.isSummary && !target.column && source.column)
   ) {
     return false;
   }
@@ -105,9 +105,9 @@ function isValidLink(source: LinkNode, target: LinkNode) {
   if (
     !source.column &&
     !target.column &&
-    target.groupbyColumns && !(
-      source.groupbyColumns &&
-      gutil.isSubset(source.groupbyColumns, target.groupbyColumns)
+    target.isSummary && !(
+      source.isSummary &&
+      gutil.isSubset(source.groupbyColumns!, target.groupbyColumns!)
     )
   ) {
     return false;
@@ -175,8 +175,12 @@ export function selectBy(docModel: DocModel, sources: ViewSectionRec[],
         label += ` ${BLACK_CIRCLE} ${srcNode.column.label.peek()}`;
       }
 
-      // add the target column name (except for 'group') only if target has multiple valid nodes
-      if (hasMany && tgtNode.column && !isSummaryGroup(tgtNode)) {
+      // add the target column name (except for 'group') when clarification is needed, i.e. if either:
+      // - target has multiple valid nodes, or
+      // - source col is 'group' and is thus hidden.
+      //     Need at least one column name to distinguish from simply selecting by summary table.
+      //     This is relevant when a table has a column referencing itself.
+      if (tgtNode.column && !isSummaryGroup(tgtNode) && (hasMany || isSummaryGroup(srcNode))) {
         label += ` ${RIGHT_ARROW} ${tgtNode.column.label.peek()}`;
       }