From 8df9d954ec8a007e284db44c2622feb866783ebb Mon Sep 17 00:00:00 2001
From: Ariya Hidayat <ariya@metabase.com>
Date: Fri, 30 Apr 2021 10:27:54 -0700
Subject: [PATCH] Custom expression editor: correctly accept the suggestion
(#15872)
This is carried out via lexical replacement, i.e. finding the token to
be replaced, instead of the rather-brittle and often-incorrect regex for
prefix/postfix.
* Always place the caret after the replacement text
* Only accept suggested "(" when there is no forthcoming "(" already
---
.../src/metabase/lib/expressions/suggest.js | 19 ---------
.../expressions/ExpressionEditorTextfield.jsx | 39 ++++++++++-------
.../scenarios/question/notebook.cy.spec.js | 42 +++++++++++++++++++
3 files changed, 66 insertions(+), 34 deletions(-)
diff --git a/frontend/src/metabase/lib/expressions/suggest.js b/frontend/src/metabase/lib/expressions/suggest.js
index d34397440bc..1558e6961f2 100644
--- a/frontend/src/metabase/lib/expressions/suggest.js
+++ b/frontend/src/metabase/lib/expressions/suggest.js
@@ -90,16 +90,6 @@ export function suggest({
partialSuggestionMode = true;
}
- const identifierTrimOptions = lastInputTokenIsUnclosedIdentifierString
- ? {
- // use the last token's pattern anchored to the end of the text
- prefixTrim: new RegExp(lastInputToken.tokenType.PATTERN.source + "$"),
- }
- : {
- prefixTrim: new RegExp(Identifier.PATTERN.source + "$"),
- postfixTrim: new RegExp("^" + Identifier.PATTERN.source + "\\s*"),
- };
-
const context = getContext({
cst,
tokenVector,
@@ -174,7 +164,6 @@ export function suggest({
alternates: EDITOR_FK_SYMBOLS.symbols.map(symbol =>
getDimensionName(dimension, symbol),
),
- ...identifierTrimOptions,
})),
);
}
@@ -184,7 +173,6 @@ export function suggest({
type: "segments",
name: segment.name,
text: formatSegmentName(segment),
- ...identifierTrimOptions,
})),
);
}
@@ -194,7 +182,6 @@ export function suggest({
type: "metrics",
name: metric.name,
text: formatMetricName(metric),
- ...identifierTrimOptions,
})),
);
}
@@ -248,9 +235,6 @@ export function suggest({
type: "functions",
name: "case",
text: "case(",
- postfixText: ")",
- prefixTrim: /\w+$/,
- postfixTrim: /^\w+(\(\)?|$)/,
};
finalSuggestions.push(caseSuggestion);
}
@@ -317,9 +301,6 @@ function functionSuggestion(type, clause, parens = true) {
type: type,
name: name,
text: name + (parens ? "(" : " "),
- postfixText: parens ? ")" : " ",
- prefixTrim: /\w+$/,
- postfixTrim: parens ? /^\w+(\(\)?|$)/ : /^\w+\s*/,
};
}
diff --git a/frontend/src/metabase/query_builder/components/expressions/ExpressionEditorTextfield.jsx b/frontend/src/metabase/query_builder/components/expressions/ExpressionEditorTextfield.jsx
index 654064ce3ed..4a7b1973bc3 100644
--- a/frontend/src/metabase/query_builder/components/expressions/ExpressionEditorTextfield.jsx
+++ b/frontend/src/metabase/query_builder/components/expressions/ExpressionEditorTextfield.jsx
@@ -172,22 +172,31 @@ export default class ExpressionEditorTextfield extends React.Component {
const suggestion = suggestions && suggestions[index];
if (suggestion) {
- let prefix = source.slice(0, suggestion.index);
- if (suggestion.prefixTrim) {
- prefix = prefix.replace(suggestion.prefixTrim, "");
- }
- let postfix = source.slice(suggestion.index);
- if (suggestion.postfixTrim) {
- postfix = postfix.replace(suggestion.postfixTrim, "");
- }
- if (!postfix && suggestion.postfixText) {
- postfix = suggestion.postfixText;
+ const { tokens } = tokenize(source);
+ const token = tokens.find(t => t.end >= suggestion.index);
+ if (token) {
+ const prefix = source.slice(0, token.start);
+ const postfix = source.slice(token.end);
+ const suggested = suggestion.text;
+
+ // e.g. source is "isnull(A" and suggested is "isempty("
+ // the result should be "isempty(A" and NOT "isempty((A"
+ const openParen = _.last(suggested) === "(";
+ const alreadyOpenParen = _.first(postfix.trimLeft()) === "(";
+ const extraTrim = openParen && alreadyOpenParen ? 1 : 0;
+ const replacement = suggested.slice(0, suggested.length - extraTrim);
+
+ const updatedExpression = prefix + replacement.trim() + postfix;
+ this.onExpressionChange(updatedExpression);
+ const caretPos = updatedExpression.length - postfix.length;
+ setTimeout(() => {
+ this._setCaretPosition(caretPos, true);
+ });
+ } else {
+ const newExpression = source + suggestion.text;
+ this.onExpressionChange(newExpression);
+ setTimeout(() => this._setCaretPosition(newExpression.length, true));
}
-
- this.onExpressionChange(prefix + suggestion.text + postfix);
- setTimeout(() =>
- this._setCaretPosition((prefix + suggestion.text).length, true),
- );
}
};
diff --git a/frontend/test/metabase/scenarios/question/notebook.cy.spec.js b/frontend/test/metabase/scenarios/question/notebook.cy.spec.js
index 8f70940e991..4f9d731a097 100644
--- a/frontend/test/metabase/scenarios/question/notebook.cy.spec.js
+++ b/frontend/test/metabase/scenarios/question/notebook.cy.spec.js
@@ -907,6 +907,48 @@ describe("scenarios > question > notebook", () => {
cy.get("[contenteditable='true']").type("[Price] ");
cy.contains("/").should("not.exist");
});
+
+ it("should correctly accept the chosen field suggestion", () => {
+ openProductsTable({ mode: "notebook" });
+ cy.findByText("Custom column").click();
+ cy.get("[contenteditable='true']").type(
+ "[Rating]{leftarrow}{leftarrow}{leftarrow}",
+ );
+
+ // accept the only suggested item, i.e. "[Rating]"
+ cy.get("[contenteditable='true']").type("{enter}");
+
+ // if the replacement is correct -> "[Rating]"
+ // if the replacement is wrong -> "[Rating] ng"
+ cy.get("[contenteditable='true']")
+ .contains("[Rating] ng")
+ .should("not.exist");
+ });
+
+ it("should correctly accept the chosen function suggestion", () => {
+ openProductsTable({ mode: "notebook" });
+ cy.findByText("Custom column").click();
+ cy.get("[contenteditable='true']").type("LTRIM([Title])");
+
+ // Place the cursor between "is" and "empty"
+ cy.get("[contenteditable='true']").type(
+ Array(13)
+ .fill("{leftarrow}")
+ .join(""),
+ );
+
+ // accept the first suggested function, i.e. "length"
+ cy.get("[contenteditable='true']").type("{enter}");
+
+ cy.get("[contenteditable='true']").contains("length([Title])");
+ });
+ });
+
+ it("should correctly insert function suggestion with the opening parenthesis", () => {
+ openProductsTable({ mode: "notebook" });
+ cy.findByText("Custom column").click();
+ cy.get("[contenteditable='true']").type("LOW{enter}");
+ cy.get("[contenteditable='true']").contains("lower(");
});
});
--
GitLab