diff --git a/frontend/src/metabase/lib/revisions/components.jsx b/frontend/src/metabase/lib/revisions/components.jsx index 94b6a86866a3877874eb35654db08a4eb2d0f997..20c78cf0ec815761ab08937255c7545f44afe17b 100644 --- a/frontend/src/metabase/lib/revisions/components.jsx +++ b/frontend/src/metabase/lib/revisions/components.jsx @@ -29,26 +29,32 @@ RevisionTitle.propTypes = revisionTitlePropTypes; const revisionBatchedDescriptionPropTypes = { changes: PropTypes.arrayOf(PropTypes.node).isRequired, + fallback: PropTypes.string, }; -export function RevisionBatchedDescription({ changes }) { +export function RevisionBatchedDescription({ changes, fallback }) { const formattedChanges = useMemo(() => { - const result = []; + let result = []; changes.forEach((change, i) => { - const isFirst = i === 0; - result.push(isFirst ? capitalizeChangeRecord(change) : change); - const isLast = i === changes.length - 1; - const isBeforeLast = i === changes.length - 2; - if (isBeforeLast) { - result.push(" " + t`and` + " "); - } else if (!isLast) { - result.push(", "); + try { + const isFirst = i === 0; + result.push(isFirst ? capitalizeChangeRecord(change) : change); + const isLast = i === changes.length - 1; + const isBeforeLast = i === changes.length - 2; + if (isBeforeLast) { + result.push(" " + t`and` + " "); + } else if (!isLast) { + result.push(", "); + } + } catch { + console.warn("Error formatting revision changes", changes); + result = fallback; } }); return result; - }, [changes]); + }, [changes, fallback]); return <span>{formattedChanges}</span>; } @@ -56,6 +62,9 @@ export function RevisionBatchedDescription({ changes }) { function capitalizeChangeRecord(change) { if (Array.isArray(change)) { const [first, ...rest] = change; + if (Array.isArray(first)) { + return capitalizeChangeRecord(first); + } return [capitalize(first, { lowercase: false }), ...rest]; } return capitalize(change, { lowercase: false }); diff --git a/frontend/src/metabase/lib/revisions/components.unit.spec.js b/frontend/src/metabase/lib/revisions/components.unit.spec.js index a76dc2f9971a13a6c446dfee039ff2726f8a67e4..b8b0c0b57888c9be8961bc0c7c6a8cf81613a1d4 100644 --- a/frontend/src/metabase/lib/revisions/components.unit.spec.js +++ b/frontend/src/metabase/lib/revisions/components.unit.spec.js @@ -1,5 +1,6 @@ import React from "react"; -import { render, screen } from "@testing-library/react"; +import { render, renderWithProviders, screen } from "__support__/ui"; +import { getCollectionChangeDescription } from "./revisions"; import { RevisionTitle, RevisionBatchedDescription } from "./components"; describe("RevisionTitle", () => { @@ -10,6 +11,16 @@ describe("RevisionTitle", () => { }); describe("RevisionBatchedDescription", () => { + const originalWarn = console.warn; + + beforeAll(() => { + console.warn = jest.fn(); + }); + + afterAll(() => { + console.warn = originalWarn; + }); + it("correctly renders two change records", () => { render( <RevisionBatchedDescription @@ -40,4 +51,24 @@ describe("RevisionBatchedDescription", () => { ); expect(screen.queryByText("Renamed this and moved to Our analytics")); }); + + it("should handle nested messages (metabase#20414)", () => { + renderWithProviders( + <RevisionBatchedDescription + changes={[getCollectionChangeDescription(1, 2), "edited metadata"]} + />, + ); + expect(screen.getByText(/Moved this to/)).toBeInTheDocument(); + expect(screen.getByText(/edited metadata/)).toBeInTheDocument(); + }); + + it("should use fallback when failing to format changes message", () => { + render( + <RevisionBatchedDescription + changes={[{ key: "try to parse this" }, -1, false]} + fallback="Just a fallback" + />, + ); + expect(screen.getByText("Just a fallback")).toBeInTheDocument(); + }); }); diff --git a/frontend/src/metabase/lib/revisions/revisions.js b/frontend/src/metabase/lib/revisions/revisions.js index 085d5cab722e4ed5b5bf4ffebd763816a42a2d05..d42f826a593a122feb53843dd1ae0f9c4a380fdf 100644 --- a/frontend/src/metabase/lib/revisions/revisions.js +++ b/frontend/src/metabase/lib/revisions/revisions.js @@ -63,7 +63,7 @@ function getSeriesChangeDescription(prevCards, cards) { : t`removed series from a question`; } -function getCollectionChangeDescription(prevCollectionId, collectionId) { +export function getCollectionChangeDescription(prevCollectionId, collectionId) { const key = `collection-from-${prevCollectionId}-to-${collectionId}`; return [ jt`moved this to ${( @@ -245,7 +245,12 @@ export function getRevisionEventsForTimeline( // like "John added a description" if (isChangeEvent && isMultipleFieldsChange) { event.title = t`${username} edited this`; - event.description = <RevisionBatchedDescription changes={changes} />; + event.description = ( + <RevisionBatchedDescription + changes={changes} + fallback={revision.description} + /> + ); } else { event.title = <RevisionTitle username={username} message={changes} />; }