Skip to content

Commit

Permalink
(core) Allow linking summary tables based on ref/reflist columns (exc…
Browse files Browse the repository at this point in the history
…ept `group`)

Summary:
Relax the restriction in `selectBy.isValidLink` so that summary tables can be linked by a column like other tables, except the `group` column. See the discussion on https://grist.slack.com/archives/C0234CPPXPA/p1651773623256959 (the replies are on the following message) for more info on this decision.

Tweaked `LinkingState.ts` since linking with summary tables can now involve a column.

Test Plan: Added a new nbrowser test and fixture checking the options to select by given a summary table with a few ref/reflist columns. Manually tested the behaviour of each option.

Reviewers: jarek

Reviewed By: jarek

Differential Revision: https://phab.getgrist.com/D3416
  • Loading branch information
alexmojaki committed May 12, 2022
1 parent 6c90de4 commit b878395
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 3 deletions.
2 changes: 1 addition & 1 deletion app/client/components/LinkingState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ export class LinkingState extends Disposable {
}
} else if (srcColId && isRefListType(srcCol.type())) {
this.filterColValues = this._srcCellFilter('id', 'in');
} else if (isSummaryOf(srcSection.table(), tgtSection.table())) {
} else if (!srcColId && isSummaryOf(srcSection.table(), tgtSection.table())) {
// We filter summary tables when a summary section is linked to a more detailed one without
// specifying src or target column. The filtering is on the shared group-by column (i.e. all
// those in the srcSection).
Expand Down
9 changes: 7 additions & 2 deletions app/client/ui/selectBy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,13 @@ function isValidLink(source: LinkNode, target: LinkNode) {
return false;
}

// summary table can only link to and from the main node (node with no column)
if ((source.isSummary || target.isSummary) && (source.column || target.column)) {
// cannot link to the somewhat special 'group' reflist column of summary tables
// because in most cases it's equivalent to the usual summary table linking but potentially slower,
// and in other cases it may cause confusion with overwhelming options.
if (
(source.isSummary && source.column?.colId.peek() === "group") ||
(target.isSummary && target.column?.colId.peek() === "group")
) {
return false;
}

Expand Down

0 comments on commit b878395

Please sign in to comment.