From 75f2dc1a01d9518e5c04a6532e815c1f87d7958e Mon Sep 17 00:00:00 2001
From: Mark Bastian <markbastian@gmail.com>
Date: Mon, 22 Jan 2024 21:10:43 -0700
Subject: [PATCH] Formatting for time types (#37942)
* Formatting for time types
Currently, the frontend does not do any special formatting for time of day types. These are always formatted as 12 hour AM/PM times. This PR adds a new function to `frontend/src/metabase/lib/formatting/time.ts`, `formatTimeWithOptions`, that applies user-defined formatting to the time. Previously, `formatTime` was called, which takes no formatting options, so none can be applied.
Fixes #11398
* e2e tests for time of day formatting.
* Update frontend/src/metabase/lib/formatting/time.ts
Co-authored-by: Emmad Usmani <emmadusmani@berkeley.edu>
* Replacing formatTime with formatTimeWithOptions
This replaces uses of `formatTime` with `formatTimeWithOptions`, updates unit tests, removes references to `formatTime`, and incorporates some code suggestions.
* Unit tests
* Adding test id for field info popover.
* Refactoring `formatTimeWithOptions` to `formatTime`
* Moved `TimeOnlyOptions` interface to formatting types and made it an ancestor of `OptionsType`
---------
Co-authored-by: Emmad Usmani <emmadusmani@berkeley.edu>
---
.../visualizations-tabular/table.cy.spec.js | 52 +++++++++++++++++++
frontend/src/metabase/lib/formatting/time.ts | 32 +++++++++---
frontend/src/metabase/lib/formatting/types.ts | 13 +++--
.../src/metabase/lib/formatting/value.tsx | 2 +-
.../src/metabase/static-viz/lib/format.ts | 2 +-
.../TableInteractive/TableInteractive.jsx | 4 ++
.../test/metabase/lib/formatting.unit.spec.js | 13 ++++-
7 files changed, 104 insertions(+), 14 deletions(-)
diff --git a/e2e/test/scenarios/visualizations-tabular/table.cy.spec.js b/e2e/test/scenarios/visualizations-tabular/table.cy.spec.js
index 1c55395d5e3..e113927e07d 100644
--- a/e2e/test/scenarios/visualizations-tabular/table.cy.spec.js
+++ b/e2e/test/scenarios/visualizations-tabular/table.cy.spec.js
@@ -373,6 +373,58 @@ describe("scenarios > visualizations > table > conditional formatting", () => {
});
});
+describe("scenarios > visualizations > table > time formatting (#11398)", () => {
+ const singleTimeQuery = `
+ WITH t1 AS (SELECT TIMESTAMP '2023-01-01 18:34:00' AS time_value),
+ t2 AS (SELECT CAST(time_value AS TIME) AS creation_time
+ FROM t1)
+ SELECT *
+ FROM t2;
+ `;
+
+ beforeEach(() => {
+ restore();
+ cy.signInAsNormalUser();
+ });
+
+ it("should work with time columns", { tags: ["@external"] }, () => {
+ cy.createNativeQuestion(
+ {
+ name: "11398",
+ native: {
+ query: singleTimeQuery,
+ },
+ },
+ { visitQuestion: true },
+ );
+
+ // Open the formatting menu
+ cy.findByTestId("field-info-popover").click();
+
+ cy.findByTestId("drill-through-section").within(() => {
+ cy.icon("gear").click();
+ });
+
+ cy.findByTestId("column-formatting-settings").within(() => {
+ // Set to hours, minutes, seconds, 24-hour clock
+ cy.findByText("HH:MM:SS").click();
+ cy.findByText("17:24 (24-hour clock)").click();
+ });
+
+ // And you should find the result
+ cy.findByRole("gridcell").findByText("18:34:00");
+
+ cy.findByTestId("column-formatting-settings").within(() => {
+ // Add millisecond display and change back to 12 hours
+ cy.findByText("HH:MM:SS.MS").click();
+ cy.findByText("5:24 PM (12-hour clock)").click();
+ });
+
+ // And you should find the result
+ cy.findByRole("gridcell").findByText("6:34:00.000 PM");
+ });
+});
+
function headerCells() {
return cy.findAllByTestId("header-cell");
}
diff --git a/frontend/src/metabase/lib/formatting/time.ts b/frontend/src/metabase/lib/formatting/time.ts
index 926debbe986..f2db3622d4b 100644
--- a/frontend/src/metabase/lib/formatting/time.ts
+++ b/frontend/src/metabase/lib/formatting/time.ts
@@ -4,6 +4,7 @@ import type { Moment } from "moment-timezone";
import { parseTime, parseTimestamp } from "metabase/lib/time";
import type { DatetimeUnit } from "metabase-types/api/query";
+import type { TimeOnlyOptions } from "metabase/lib/formatting/types";
import {
DEFAULT_TIME_STYLE,
getTimeFormatFromStyle,
@@ -28,12 +29,6 @@ export function duration(milliseconds: number) {
return ngettext(msgid`${seconds} second`, `${seconds} seconds`, seconds);
}
-export function formatTime(time: Moment) {
- const parsedTime = parseTime(time);
-
- return parsedTime.isValid() ? parsedTime.format("LT") : String(time);
-}
-
interface TimeWithUnitType {
local?: boolean;
time_enabled?: "minutes" | "milliseconds" | "seconds" | boolean;
@@ -67,3 +62,28 @@ export function formatTimeWithUnit(
return m.format(timeFormat);
}
+
+export function formatTime(
+ time: Moment,
+ unit: DatetimeUnit = "default",
+ options: TimeOnlyOptions = {},
+) {
+ const parsedTime = parseTime(time);
+
+ const timeStyle = options.time_style ?? DEFAULT_TIME_STYLE;
+
+ let timeEnabled;
+ if (options.time_enabled) {
+ timeEnabled = options.time_enabled;
+ } else if (hasHour(unit)) {
+ timeEnabled = "minute";
+ } else {
+ timeEnabled = null;
+ }
+
+ const timeFormat =
+ options.time_format ??
+ getTimeFormatFromStyle(timeStyle, unit, timeEnabled as any);
+
+ return parsedTime.isValid() ? parsedTime.format(timeFormat) : String(time);
+}
diff --git a/frontend/src/metabase/lib/formatting/types.ts b/frontend/src/metabase/lib/formatting/types.ts
index 3056b615ef2..21414dfbe0b 100644
--- a/frontend/src/metabase/lib/formatting/types.ts
+++ b/frontend/src/metabase/lib/formatting/types.ts
@@ -1,4 +1,11 @@
-export interface OptionsType {
+export interface TimeOnlyOptions {
+ local?: boolean;
+ time_enabled?: "minutes" | "milliseconds" | "seconds" | null;
+ time_format?: string;
+ time_style?: string;
+}
+
+export interface OptionsType extends TimeOnlyOptions {
click_behavior?: any;
clicked?: any;
column?: any;
@@ -12,7 +19,6 @@ export interface OptionsType {
jsx?: boolean;
link_text?: string;
link_url?: string;
- local?: boolean;
majorWidth?: number;
markdown_template?: any;
maximumFractionDigits?: number;
@@ -25,9 +31,6 @@ export interface OptionsType {
removeYear?: boolean;
rich?: boolean;
suffix?: string;
- time_enabled?: "minutes" | "milliseconds" | "seconds" | null;
- time_format?: string;
- time_style?: string;
type?: string;
view_as?: string | null;
weekday_enabled?: boolean;
diff --git a/frontend/src/metabase/lib/formatting/value.tsx b/frontend/src/metabase/lib/formatting/value.tsx
index 0d5cf6c8c8e..2e34ad9a18d 100644
--- a/frontend/src/metabase/lib/formatting/value.tsx
+++ b/frontend/src/metabase/lib/formatting/value.tsx
@@ -177,7 +177,7 @@ export function formatValueRaw(
} else if (isEmail(column)) {
return formatEmail(value as string, options);
} else if (isTime(column)) {
- return formatTime(value as Moment);
+ return formatTime(value as Moment, column.unit, options);
} else if (column && column.unit != null) {
return formatDateTimeWithUnit(
value as string | number,
diff --git a/frontend/src/metabase/static-viz/lib/format.ts b/frontend/src/metabase/static-viz/lib/format.ts
index 0cb042789bf..69bc1ebceed 100644
--- a/frontend/src/metabase/static-viz/lib/format.ts
+++ b/frontend/src/metabase/static-viz/lib/format.ts
@@ -57,7 +57,7 @@ export const formatStaticValue = (value: unknown, options: OptionsType) => {
if (value == null) {
formattedValue = JSON.stringify(null);
} else if (isTime(column)) {
- formattedValue = formatTime(value as Moment);
+ formattedValue = formatTime(value as Moment, column.unit, options);
} else if (column?.unit != null) {
formattedValue = formatDateTimeWithUnit(
value as string | number,
diff --git a/frontend/src/metabase/visualizations/components/TableInteractive/TableInteractive.jsx b/frontend/src/metabase/visualizations/components/TableInteractive/TableInteractive.jsx
index 1ec775a5bdd..1c9eabbcac7 100644
--- a/frontend/src/metabase/visualizations/components/TableInteractive/TableInteractive.jsx
+++ b/frontend/src/metabase/visualizations/components/TableInteractive/TableInteractive.jsx
@@ -698,6 +698,8 @@ class TableInteractive extends Component {
const isSorted = sortDirection != null;
const isAscending = sortDirection === "asc";
+ const fieldInfoPopoverTestId = "field-info-popover";
+
return (
<TableDraggable
/* needs to be index+name+counter so Draggable resets after each drag */
@@ -793,6 +795,7 @@ class TableInteractive extends Component {
className="Icon mr1"
name={isAscending ? "chevronup" : "chevrondown"}
size={10}
+ data-testid={fieldInfoPopoverTestId}
/>
)}
{columnTitle}
@@ -801,6 +804,7 @@ class TableInteractive extends Component {
className="Icon ml1"
name={isAscending ? "chevronup" : "chevrondown"}
size={10}
+ data-testid={fieldInfoPopoverTestId}
/>
)}
</Ellipsified>,
diff --git a/frontend/test/metabase/lib/formatting.unit.spec.js b/frontend/test/metabase/lib/formatting.unit.spec.js
index bf9a482dcca..9a29fc09091 100644
--- a/frontend/test/metabase/lib/formatting.unit.spec.js
+++ b/frontend/test/metabase/lib/formatting.unit.spec.js
@@ -612,12 +612,23 @@ describe("formatting", () => {
];
test.each(FORMAT_TIME_TESTS)(
- `parseTime(%p) to be %p`,
+ `formatTime(%p) to be %p`,
(value, resultStr) => {
const result = formatTime(value);
expect(result).toBe(resultStr);
},
);
+
+ it("should use options when formatting times", () => {
+ const value = "20:34:56";
+ const t12 = formatTime(value, "default", {});
+ expect(t12).toBe("8:34 PM");
+ const t24 = formatTime(value, "default", {
+ time_enabled: "minutes",
+ time_style: "HH:mm",
+ });
+ expect(t24).toBe("20:34");
+ });
});
describe("formatTimeWithUnit", () => {
--
GitLab