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>

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");
 }

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);
+}

frontend/src/metabase/lib/formatting/types.ts

-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;

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,

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,

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>,

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", () => {