From 22436168166c023ef54a2264920add76c568fbca Mon Sep 17 00:00:00 2001
From: Alexander Lesnenko <alxnddr@users.noreply.github.com>
Date: Wed, 12 May 2021 23:41:27 +0300
Subject: [PATCH] format links on the table visualization (#15964)
* fix question level link display settings
* add cypress spec for the question-level link settings on the table visualization
* Address review comments: add a spec for clicn behavior with view_as = link, fix admin column settings
* revert a straightforward fix for hiding irrelevant settings
* fixup! Address review comments: add a spec for clicn behavior with view_as = link, fix admin column settings
* prettier
* handle link_text null value
---
frontend/src/metabase/lib/formatting.js | 94 ++++++++++++++-----
.../components/ChartSettingsWidget.jsx | 13 ++-
.../components/settings/ChartSettingInput.jsx | 1 +
.../visualizations/visualizations/Table.jsx | 89 ++++++++++--------
.../test/metabase/lib/formatting.unit.spec.js | 84 +++++++++++++++--
.../scenarios/visualizations/table.cy.spec.js | 53 +++++++++++
6 files changed, 260 insertions(+), 74 deletions(-)
create mode 100644 frontend/test/metabase/scenarios/visualizations/table.cy.spec.js
diff --git a/frontend/src/metabase/lib/formatting.js b/frontend/src/metabase/lib/formatting.js
index 41a3a722c70..8223609437b 100644
--- a/frontend/src/metabase/lib/formatting.js
+++ b/frontend/src/metabase/lib/formatting.js
@@ -38,7 +38,10 @@ import {
getTimeFormatFromStyle,
hasHour,
} from "metabase/lib/formatting/date";
-import { renderLinkTextForClick } from "metabase/lib/formatting/link";
+import {
+ renderLinkTextForClick,
+ renderLinkURLForClick,
+} from "metabase/lib/formatting/link";
import { NULL_NUMERIC_VALUE, NULL_DISPLAY_VALUE } from "metabase/lib/constants";
import type Field from "metabase-lib/lib/metadata/Field";
@@ -547,9 +550,14 @@ const EMAIL_ALLOW_LIST_REGEX = /^(?=.{1,254}$)(?=.{1,64}@)[-!#$%&'*+/0-9=?A-Z^_`
export function formatEmail(
value: Value,
- { jsx, rich, view_as = "auto", link_text }: FormattingOptions = {},
+ { jsx, rich, view_as = "auto", link_text, clicked }: FormattingOptions = {},
) {
const email = String(value);
+ const label =
+ clicked && link_text
+ ? renderLinkTextForClick(link_text, getDataFromClicked(clicked))
+ : null;
+
if (
jsx &&
rich &&
@@ -557,7 +565,7 @@ export function formatEmail(
EMAIL_ALLOW_LIST_REGEX.test(email)
) {
return (
- <ExternalLink href={"mailto:" + email}>{link_text || email}</ExternalLink>
+ <ExternalLink href={"mailto:" + email}>{label || email}</ExternalLink>
);
} else {
return email;
@@ -585,35 +593,69 @@ function isDefaultLinkProtocol(protocol) {
);
}
-export function formatUrl(value: Value, options: FormattingOptions = {}) {
- const { jsx, rich, view_as, column, link_text } = options;
- const url = value;
+function getLinkUrl(value, { view_as, link_url, clicked, column }) {
+ const isExplicitLink = view_as === "link";
+ const hasCustomizedUrl = link_url && clicked;
- const protocol = getUrlProtocol(url);
- if (
- jsx &&
- rich &&
- protocol &&
- isSafeProtocol(protocol) &&
- (view_as === undefined
- ? isURL(column) || isDefaultLinkProtocol(protocol)
- : view_as === "link"
- ? true
- : view_as === "auto"
- ? isDefaultLinkProtocol(protocol)
- : false)
- ) {
- const urlText =
- link_text ||
- getRemappedValue(value, options) ||
- formatValue(value, { ...options, view_as: null });
+ if (isExplicitLink && hasCustomizedUrl) {
+ return renderLinkURLForClick(link_url, getDataFromClicked(clicked));
+ }
+
+ const protocol = getUrlProtocol(value);
+ const isValueSafeLink = protocol && isSafeProtocol(protocol);
+
+ if (!isValueSafeLink) {
+ return null;
+ }
+
+ if (isExplicitLink) {
+ return value;
+ }
+
+ const isDefaultProtocol = protocol && isDefaultLinkProtocol(protocol);
+ const isMaybeLink = view_as === "auto";
+
+ if (isMaybeLink && isDefaultProtocol) {
+ return value;
+ }
+
+ if (view_as === undefined && (isURL(column) || isDefaultProtocol)) {
+ return value;
+ }
+
+ return null;
+}
+
+function getLinkText(value, options) {
+ const { view_as, link_text, clicked } = options;
+
+ const isExplicitLink = view_as === "link";
+ const hasCustomizedText = link_text && clicked;
+
+ if (isExplicitLink && hasCustomizedText) {
+ return renderLinkTextForClick(link_text, getDataFromClicked(clicked));
+ }
+
+ return (
+ getRemappedValue(value, options) ||
+ formatValue(value, { ...options, view_as: null })
+ );
+}
+
+export function formatUrl(value, options = {}) {
+ const { jsx, rich } = options;
+
+ const url = getLinkUrl(value, options);
+
+ if (jsx && rich && url) {
+ const text = getLinkText(value, options);
return (
<ExternalLink className="link link--wrappable" href={url}>
- {urlText}
+ {text}
</ExternalLink>
);
} else {
- return url;
+ return value;
}
}
diff --git a/frontend/src/metabase/visualizations/components/ChartSettingsWidget.jsx b/frontend/src/metabase/visualizations/components/ChartSettingsWidget.jsx
index f6925458618..b2d962ce650 100644
--- a/frontend/src/metabase/visualizations/components/ChartSettingsWidget.jsx
+++ b/frontend/src/metabase/visualizations/components/ChartSettingsWidget.jsx
@@ -2,10 +2,12 @@
import React from "react";
import cx from "classnames";
+import Icon from "metabase/components/Icon";
const ChartSettingsWidget = ({
title,
description,
+ hint,
hidden,
disabled,
widget: Widget,
@@ -28,7 +30,16 @@ const ChartSettingsWidget = ({
disable: disabled,
})}
>
- {title && <h4 className="mb1 flex align-center">{title}</h4>}
+ {title && (
+ <h4 className="mb1 flex align-center">
+ {title}
+ {hint && (
+ <span className="flex ml1">
+ <Icon name="info" size={14} tooltip={hint} />
+ </span>
+ )}
+ </h4>
+ )}
{description && <div className="mb1">{description}</div>}
{Widget && (
<Widget
diff --git a/frontend/src/metabase/visualizations/components/settings/ChartSettingInput.jsx b/frontend/src/metabase/visualizations/components/settings/ChartSettingInput.jsx
index ea3e2f68337..e017d65b119 100644
--- a/frontend/src/metabase/visualizations/components/settings/ChartSettingInput.jsx
+++ b/frontend/src/metabase/visualizations/components/settings/ChartSettingInput.jsx
@@ -6,6 +6,7 @@ import InputBlurChange from "metabase/components/InputBlurChange";
const ChartSettingInput = ({ value, onChange, ...props }) => (
<InputBlurChange
{...props}
+ data-testid={props.id}
className="input block full"
value={value}
onBlurChange={e => onChange(e.target.value)}
diff --git a/frontend/src/metabase/visualizations/visualizations/Table.jsx b/frontend/src/metabase/visualizations/visualizations/Table.jsx
index a3ee062d289..654d1864e23 100644
--- a/frontend/src/metabase/visualizations/visualizations/Table.jsx
+++ b/frontend/src/metabase/visualizations/visualizations/Table.jsx
@@ -14,7 +14,6 @@ import {
isMetric,
isDimension,
isNumber,
- isString,
isURL,
isEmail,
isImageURL,
@@ -240,49 +239,59 @@ export default class Table extends Component {
widget: "toggle",
};
}
- if (isString(column)) {
- let defaultValue = null;
- const options: { name: string, value: null | string }[] = [
- { name: t`Off`, value: null },
- ];
- if (!column.semantic_type || isURL(column)) {
- defaultValue = "link";
- options.push({ name: t`Link`, value: "link" });
- }
- if (!column.semantic_type || isEmail(column)) {
- defaultValue = "email_link";
- options.push({ name: t`Email link`, value: "email_link" });
- }
- if (!column.semantic_type || isImageURL(column) || isAvatarURL(column)) {
- defaultValue = isAvatarURL(column) ? "image" : "link";
- options.push({ name: t`Image`, value: "image" });
- }
- if (!column.semantic_type) {
- defaultValue = "auto";
- options.push({ name: t`Automatic`, value: "auto" });
- }
-
- if (options.length > 1) {
- settings["view_as"] = {
- title: t`View as link or image`,
- widget: "select",
- default: defaultValue,
- props: {
- options,
- },
- };
- }
- settings["link_text"] = {
- title: t`Link text`,
- widget: "input",
- default: null,
- getHidden: (column, settings) =>
- settings["view_as"] !== "link" &&
- settings["view_as"] !== "email_link",
+ let defaultValue = !column.semantic_type || isURL(column) ? "link" : null;
+
+ const options = [
+ { name: t`Off`, value: null },
+ { name: t`Link`, value: "link" },
+ ];
+
+ if (!column.semantic_type || isEmail(column)) {
+ defaultValue = "email_link";
+ options.push({ name: t`Email link`, value: "email_link" });
+ }
+ if (!column.semantic_type || isImageURL(column) || isAvatarURL(column)) {
+ defaultValue = isAvatarURL(column) ? "image" : "link";
+ options.push({ name: t`Image`, value: "image" });
+ }
+ if (!column.semantic_type) {
+ defaultValue = "auto";
+ options.push({ name: t`Automatic`, value: "auto" });
+ }
+
+ if (options.length > 1) {
+ settings["view_as"] = {
+ title: t`View as link or image`,
+ widget: "select",
+ default: defaultValue,
+ props: {
+ options,
+ },
};
}
+ const linkFieldsHint = t`You can use the value of any column here like this: {{COLUMN}}`;
+
+ settings["link_text"] = {
+ title: t`Link text`,
+ widget: "input",
+ hint: linkFieldsHint,
+ default: null,
+ getHidden: (_, settings) =>
+ settings["view_as"] !== "link" && settings["view_as"] !== "email_link",
+ readDependencies: ["view_as"],
+ };
+
+ settings["link_url"] = {
+ title: t`Link URL`,
+ widget: "input",
+ hint: linkFieldsHint,
+ default: null,
+ getHidden: (_, settings) => settings["view_as"] !== "link",
+ readDependencies: ["view_as"],
+ };
+
return settings;
};
diff --git a/frontend/test/metabase/lib/formatting.unit.spec.js b/frontend/test/metabase/lib/formatting.unit.spec.js
index 27a25bc737a..bd12bd2757b 100644
--- a/frontend/test/metabase/lib/formatting.unit.spec.js
+++ b/frontend/test/metabase/lib/formatting.unit.spec.js
@@ -337,14 +337,84 @@ describe("formatting", () => {
}),
).toEqual("data:text/plain;charset=utf-8,hello%20world");
});
- it("should return link component for type/URL and view_as = link", () => {
- const formatted = formatUrl("http://whatever", {
- jsx: true,
- rich: true,
- column: { semantic_type: TYPE.URL },
- view_as: "link",
+
+ describe("when view_as = link", () => {
+ it("should return link component for type/URL and view_as = link", () => {
+ const formatted = formatUrl("http://whatever", {
+ jsx: true,
+ rich: true,
+ column: { semantic_type: TYPE.URL },
+ view_as: "link",
+ });
+ expect(isElementOfType(formatted, ExternalLink)).toEqual(true);
+ });
+
+ it("should return link component using link_url and link_text when specified", () => {
+ const formatted = formatUrl("http://not.metabase.com", {
+ jsx: true,
+ rich: true,
+ link_text: "metabase link",
+ link_url: "http://metabase.com",
+ view_as: "link",
+ clicked: {},
+ });
+
+ expect(isElementOfType(formatted, ExternalLink)).toEqual(true);
+ expect(formatted.props.children).toEqual("metabase link");
+ expect(formatted.props.href).toEqual("http://metabase.com");
+ });
+
+ it("should return link component using link_text and the value as url when link_url is empty", () => {
+ const formatted = formatUrl("http://metabase.com", {
+ jsx: true,
+ rich: true,
+ link_text: "metabase link",
+ link_url: "",
+ view_as: "link",
+ clicked: {},
+ });
+
+ expect(isElementOfType(formatted, ExternalLink)).toEqual(true);
+ expect(formatted.props.children).toEqual("metabase link");
+ expect(formatted.props.href).toEqual("http://metabase.com");
+ });
+
+ it("should return link component using link_url and the value as text when link_text is empty", () => {
+ const formatted = formatUrl("metabase link", {
+ jsx: true,
+ rich: true,
+ link_text: "",
+ link_url: "http://metabase.com",
+ view_as: "link",
+ clicked: {},
+ });
+
+ expect(isElementOfType(formatted, ExternalLink)).toEqual(true);
+ expect(formatted.props.children).toEqual("metabase link");
+ expect(formatted.props.href).toEqual("http://metabase.com");
+ });
+
+ it("should not return an ExternalLink in jsx + rich mode if there's click behavior", () => {
+ const formatted = formatValue("http://metabase.com/", {
+ jsx: true,
+ rich: true,
+ click_behavior: {
+ linkTemplate: "foo",
+ linkTextTemplate: "bar",
+ linkType: "url",
+ type: "link",
+ },
+ link_text: "metabase link",
+ link_url: "http://metabase.com",
+ view_as: "link",
+ clicked: {},
+ });
+
+ // it is not a link set on the question level
+ expect(isElementOfType(formatted, ExternalLink)).toEqual(false);
+ // it is formatted as a link cell for the dashboard level click behavior
+ expect(formatted.props.className).toEqual("link link--wrappable");
});
- expect(isElementOfType(formatted, ExternalLink)).toEqual(true);
});
it("should not crash if column is null", () => {
diff --git a/frontend/test/metabase/scenarios/visualizations/table.cy.spec.js b/frontend/test/metabase/scenarios/visualizations/table.cy.spec.js
new file mode 100644
index 00000000000..c84687e3731
--- /dev/null
+++ b/frontend/test/metabase/scenarios/visualizations/table.cy.spec.js
@@ -0,0 +1,53 @@
+import { restore, visitQuestionAdhoc, popover } from "__support__/e2e/cypress";
+import { SAMPLE_DATASET } from "__support__/e2e/cypress_sample_dataset";
+
+const { PEOPLE_ID } = SAMPLE_DATASET;
+
+const testQuery = {
+ database: 1,
+ query: {
+ "source-table": PEOPLE_ID,
+ },
+ type: "query",
+};
+
+describe("scenarios > visualizations > table", () => {
+ beforeEach(() => {
+ restore();
+ cy.signInAsNormalUser();
+ });
+
+ it("should allow to display any column as link with extrapolated url and text", () => {
+ visitQuestionAdhoc({
+ dataset_query: testQuery,
+ display: "table",
+ });
+
+ cy.findByText("City").click();
+
+ popover().within(() => {
+ cy.icon("gear").click();
+ });
+
+ cy.findByText("Off").click();
+
+ popover().within(() => {
+ cy.findByText("Link").click();
+ });
+
+ cy.findByTestId("link_text").type("{{CITY}} {{ID}} fixed text", {
+ parseSpecialCharSequences: false,
+ });
+ cy.findByTestId("link_url").type("http://metabase.com/people/{{ID}}", {
+ parseSpecialCharSequences: false,
+ });
+
+ cy.findByText("Done").click();
+
+ cy.findByText("Wood River 1 fixed text").should(
+ "have.attr",
+ "href",
+ "http://metabase.com/people/1",
+ );
+ });
+});
--
GitLab