From edc969ce4247f810fe5fce806a19d77115ca80f1 Mon Sep 17 00:00:00 2001
From: Ngoc Khuat <qn.khuat@gmail.com>
Date: Fri, 26 May 2023 18:34:33 +0700
Subject: [PATCH] Move logic to infer revision.description to BE (#30502)
* Dashboard Tab entity (#29802)
* add dashboard tab entity
* Dashboard tabs CRUD (#29861)
* Dashboard tabs frontend (#30189)
* rename `DashboardHeader.tsx` to `DashboardHeaderView` to avoid collision with `DashboardHeader.jsx`
commit-id:7cdfd86a
* add tabs to dash
commit-id:d3c2fa74
* improve code structure and other fixes
* rename `dashboardtab_id` to `dashboard_tab_id`
* remove explicit `position` logic
* improve code quality
* add renaming functionality
* add two new tabs when creating first tab
* hide tabs if there is only one
* add unit test for `DashboardTabs` component
* refactor
* fix type errors
* add `tabId` to text cards
* add `tabId` to link cards
* add `tabId` to action button
* use disabled state when only one tab remains
* refetch cards with filter value when changing tabs
* add e2e test
* fix broken e2e tests
* add horizontal scrolling
* fix unit test
* fix create tab button being on right
* fix performance issue when switching tabs
* add `ttag` to `tabs.ts`
* use `aria-label` in e2e test helper
* fix type error after rebasing
* making ordered_tabs optional for tests purposes
* fix failing unit tests
* fix failing e2e tests
* fix flaky revision history e2e test
* fix type error after merging feature branch with master and rebasing
* fix revisions unit test
* fix `ActionParmatersInputsForm.tsx` to stop `actions-on-dashboards.cy.spec.js` from flaking
---------
Co-authored-by: Ngoc Khuat <qn.khuat@gmail.com>
* migrating revision logic to BE
* handle description for multiple cards add/remove
* test fixes
* adds tests
* make sures events are started so tests work
* remove additional `:maybe` that was added while rebasing
* add title and has_multile_changes to revision
* fixes potential test flake
* more tests
* remove debug code
* handles collection change description in card and dashboard
* fixes fail to build
* use backend for revision history (#30772)
* use BE response for revision history
* update `service.unit.spec.js`
* sort revision and moderation events for question timeline
* test fixes
* create revision directly instead of via API to avoid flake
* reverted to an earlier revision => reverted to an earlier version
* translate the model name too
* Rename functions:
- diff-strs -> diff-strings
- diff-strings -> diff-strings*
- diff-strings* -> diff-string
* move the diff to diff-strings*
* use case instead of map for model-str->i18n-str
* remove title from API response
* do not translate test model
* rename var `title_text` to `titleText`
* address comments
* stringify the display name
* fix revert button not working
* add loading wrapper to fix unit test
* fix tests
* fix type error
* fix QuestionActivityTimeline test
* add a dot for edited this
---------
Co-authored-by: Emmad Usmani <emmadusmani@berkeley.edu>
---
.../models/models-revision-history.cy.spec.js | 2 +-
.../reproductions/12581.cy.spec.js | 4 +-
e2e/test/scenarios/question/saved.cy.spec.js | 2 +-
.../metabase-enterprise/moderation/service.js | 4 +-
.../moderation/service.unit.spec.js | 4 +-
.../src/metabase-types/api/mocks/revision.ts | 21 +
frontend/src/metabase-types/api/mocks/user.ts | 13 +-
frontend/src/metabase-types/api/revision.ts | 23 +-
frontend/src/metabase-types/api/user.ts | 8 +
.../components/Timeline/Timeline.styled.tsx} | 29 +-
.../common/components/Timeline/Timeline.tsx | 84 +++
.../common/components/Timeline/index.ts | 1 +
.../common/components/Timeline/utils.ts | 31 +
.../use-database-list-query.ts | 1 +
.../use-entity-list-query.ts | 7 +-
.../use-metric-list-query.ts | 1 +
.../use-question-list-query.ts | 1 +
.../hooks/use-revision-list-query/index.ts | 1 +
.../use-revision-list-query.ts | 19 +
.../use-revision-list-query.unit.spec.tsx | 48 ++
.../use-schema-list-query.ts | 1 +
.../use-segment-list-query.ts | 1 +
.../use-table-list-query.ts | 1 +
.../common/hooks/use-user-list-query/index.ts | 1 +
.../use-user-list-query.ts | 19 +
.../use-user-list-query.unit.spec.tsx | 48 ++
.../metabase/components/Timeline/Timeline.jsx | 122 ---
.../metabase/components/Timeline/index.jsx | 1 -
.../metabase/dashboard/actions/revisions.js | 3 +-
.../components/DashboardInfoSidebar.tsx | 72 +-
.../components/DashboardSidebars.jsx | 2 +-
.../src/metabase/lib/revisions/components.jsx | 80 --
.../lib/revisions/components.styled.tsx | 23 -
.../lib/revisions/components.unit.spec.js | 75 --
frontend/src/metabase/lib/revisions/index.js | 1 -
.../src/metabase/lib/revisions/revisions.js | 293 --------
.../lib/revisions/revisions.unit.spec.js | 699 ------------------
frontend/src/metabase/plugins/index.ts | 19 +-
.../query_builder/actions/core/core.js | 3 +-
.../components/QuestionActivityTimeline.jsx | 94 ---
.../QuestionActivityTimeline.styled.jsx | 10 -
.../QuestionActivityTimeline.styled.tsx | 10 +
.../components/QuestionActivityTimeline.tsx | 71 ++
.../QuestionActivityTimeline.unit.spec.js | 42 +-
.../view/sidebars/QuestionInfoSidebar.tsx | 2 +-
.../test/__support__/server-mocks/revision.ts | 6 +
.../test/__support__/server-mocks/user.ts | 6 +
src/metabase/api/dashboard.clj | 7 +-
src/metabase/models/dashboard.clj | 70 +-
src/metabase/models/dashboard_card.clj | 14 +-
src/metabase/models/revision.clj | 33 +-
src/metabase/models/revision/diff.clj | 88 ++-
src/metabase/util/i18n.clj | 16 +-
test/metabase/api/dashboard_test.clj | 106 +--
test/metabase/api/metric_test.clj | 86 +--
test/metabase/api/revision_test.clj | 348 +++++++--
test/metabase/api/segment_test.clj | 92 +--
test/metabase/api/user_test.clj | 3 +-
test/metabase/events/revision_test.clj | 3 +-
test/metabase/models/dashboard_test.clj | 206 ++++--
test/metabase/models/revision/diff_test.clj | 37 -
test/metabase/models/revision_test.clj | 175 +++--
62 files changed, 1318 insertions(+), 1975 deletions(-)
create mode 100644 frontend/src/metabase-types/api/mocks/revision.ts
rename frontend/src/metabase/{components/Timeline/Timeline.styled.jsx => common/components/Timeline/Timeline.styled.tsx} (56%)
create mode 100644 frontend/src/metabase/common/components/Timeline/Timeline.tsx
create mode 100644 frontend/src/metabase/common/components/Timeline/index.ts
create mode 100644 frontend/src/metabase/common/components/Timeline/utils.ts
create mode 100644 frontend/src/metabase/common/hooks/use-revision-list-query/index.ts
create mode 100644 frontend/src/metabase/common/hooks/use-revision-list-query/use-revision-list-query.ts
create mode 100644 frontend/src/metabase/common/hooks/use-revision-list-query/use-revision-list-query.unit.spec.tsx
create mode 100644 frontend/src/metabase/common/hooks/use-user-list-query/index.ts
create mode 100644 frontend/src/metabase/common/hooks/use-user-list-query/use-user-list-query.ts
create mode 100644 frontend/src/metabase/common/hooks/use-user-list-query/use-user-list-query.unit.spec.tsx
delete mode 100644 frontend/src/metabase/components/Timeline/Timeline.jsx
delete mode 100644 frontend/src/metabase/components/Timeline/index.jsx
delete mode 100644 frontend/src/metabase/lib/revisions/components.jsx
delete mode 100644 frontend/src/metabase/lib/revisions/components.styled.tsx
delete mode 100644 frontend/src/metabase/lib/revisions/components.unit.spec.js
delete mode 100644 frontend/src/metabase/lib/revisions/index.js
delete mode 100644 frontend/src/metabase/lib/revisions/revisions.js
delete mode 100644 frontend/src/metabase/lib/revisions/revisions.unit.spec.js
delete mode 100644 frontend/src/metabase/query_builder/components/QuestionActivityTimeline.jsx
delete mode 100644 frontend/src/metabase/query_builder/components/QuestionActivityTimeline.styled.jsx
create mode 100644 frontend/src/metabase/query_builder/components/QuestionActivityTimeline.styled.tsx
create mode 100644 frontend/src/metabase/query_builder/components/QuestionActivityTimeline.tsx
create mode 100644 frontend/test/__support__/server-mocks/revision.ts
create mode 100644 frontend/test/__support__/server-mocks/user.ts
delete mode 100644 test/metabase/models/revision/diff_test.clj
diff --git a/e2e/test/scenarios/models/models-revision-history.cy.spec.js b/e2e/test/scenarios/models/models-revision-history.cy.spec.js
index 758a701bb18..69cccef19e0 100644
--- a/e2e/test/scenarios/models/models-revision-history.cy.spec.js
+++ b/e2e/test/scenarios/models/models-revision-history.cy.spec.js
@@ -23,7 +23,7 @@ describe("scenarios > models > revision history", () => {
cy.location("pathname").should("match", /^\/question\/3/);
cy.get(".LineAreaBarChart");
- revertTo("^Turned this into a model");
+ revertTo("You edited this");
cy.wait("@modelQuery3");
cy.location("pathname").should("match", /^\/model\/3/);
diff --git a/e2e/test/scenarios/native-filters/reproductions/12581.cy.spec.js b/e2e/test/scenarios/native-filters/reproductions/12581.cy.spec.js
index 023a5c5eca4..5bd4e4ea9c5 100644
--- a/e2e/test/scenarios/native-filters/reproductions/12581.cy.spec.js
+++ b/e2e/test/scenarios/native-filters/reproductions/12581.cy.spec.js
@@ -62,13 +62,13 @@ describe("issue 12581", () => {
cy.findByTestId("revision-history-button").click();
// Make sure sidebar opened and the history loaded
// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
- cy.findByText("You created this");
+ cy.findByText(/You created this/i);
cy.findByTestId("question-revert-button").click(); // Revert to the first revision
cy.wait("@dataset");
// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
- cy.findByText("You reverted to an earlier revision");
+ cy.findByText(/You reverted to an earlier version/i);
// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
cy.findByText(/Open Editor/i).click();
diff --git a/e2e/test/scenarios/question/saved.cy.spec.js b/e2e/test/scenarios/question/saved.cy.spec.js
index 10ebea22bc9..909b4fdaa5e 100644
--- a/e2e/test/scenarios/question/saved.cy.spec.js
+++ b/e2e/test/scenarios/question/saved.cy.spec.js
@@ -166,7 +166,7 @@ describe("scenarios > question > saved", () => {
});
// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
- cy.findByText(/reverted to an earlier revision/i);
+ cy.findByText(/reverted to an earlier version/i);
// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
cy.findByText(/This is a question/i).should("not.exist");
});
diff --git a/enterprise/frontend/src/metabase-enterprise/moderation/service.js b/enterprise/frontend/src/metabase-enterprise/moderation/service.js
index bf537ce6886..1bf8002cc23 100644
--- a/enterprise/frontend/src/metabase-enterprise/moderation/service.js
+++ b/enterprise/frontend/src/metabase-enterprise/moderation/service.js
@@ -112,7 +112,7 @@ function getModerationReviewEventText(review, moderatorDisplayName) {
}
export function getModerationTimelineEvents(reviews, usersById, currentUser) {
- return reviews.map((review, index) => {
+ return reviews.map(review => {
const moderator = usersById[review.moderator_id];
const moderatorDisplayName = getModeratorDisplayName(
moderator,
@@ -124,7 +124,7 @@ export function getModerationTimelineEvents(reviews, usersById, currentUser) {
: getIconForReview(review);
return {
- timestamp: new Date(review.created_at).valueOf(),
+ timestamp: new Date(review.created_at).toISOString(),
icon,
title: text,
};
diff --git a/enterprise/frontend/src/metabase-enterprise/moderation/service.unit.spec.js b/enterprise/frontend/src/metabase-enterprise/moderation/service.unit.spec.js
index 8212e6476b7..b6d8e7cfbaa 100644
--- a/enterprise/frontend/src/metabase-enterprise/moderation/service.unit.spec.js
+++ b/enterprise/frontend/src/metabase-enterprise/moderation/service.unit.spec.js
@@ -259,12 +259,12 @@ describe("moderation/service", () => {
expect(getModerationTimelineEvents(reviews, usersById)).toEqual([
{
- timestamp: expect.any(Number),
+ timestamp: reviews[0].created_at,
icon: getStatusIcon("verified"),
title: "Foo verified this",
},
{
- timestamp: expect.any(Number),
+ timestamp: reviews[1].created_at,
icon: getRemovedReviewStatusIcon(),
title: "A moderator removed verification",
},
diff --git a/frontend/src/metabase-types/api/mocks/revision.ts b/frontend/src/metabase-types/api/mocks/revision.ts
new file mode 100644
index 00000000000..cccf167aade
--- /dev/null
+++ b/frontend/src/metabase-types/api/mocks/revision.ts
@@ -0,0 +1,21 @@
+import { Revision } from "metabase-types/api";
+
+export const createMockRevision = (opts?: Partial<Revision>): Revision => {
+ return {
+ id: 1,
+ description: "created this",
+ message: null,
+ timestamp: "2023-05-16T13:33:30.198622-07:00",
+ is_creation: true,
+ is_reversion: false,
+ has_multiple_changes: false,
+ user: {
+ id: 1,
+ first_name: "Admin",
+ last_name: "Test",
+ common_name: "Admin Test",
+ },
+ diff: null,
+ ...opts,
+ };
+};
diff --git a/frontend/src/metabase-types/api/mocks/user.ts b/frontend/src/metabase-types/api/mocks/user.ts
index a89a48688ba..d79dc6a2fab 100644
--- a/frontend/src/metabase-types/api/mocks/user.ts
+++ b/frontend/src/metabase-types/api/mocks/user.ts
@@ -1,4 +1,4 @@
-import { User, UserInfo } from "metabase-types/api";
+import { User, UserInfo, UserListResult } from "metabase-types/api";
export const createMockUser = (opts?: Partial<User>): User => ({
id: 1,
@@ -23,6 +23,17 @@ export const createMockUser = (opts?: Partial<User>): User => ({
...opts,
});
+export const createMockerUserListResult = (
+ opts?: Partial<UserListResult>,
+): UserListResult => ({
+ id: 1,
+ first_name: "Testy",
+ last_name: "Tableton",
+ common_name: "Testy Tableton",
+ email: "user@metabase.test",
+ ...opts,
+});
+
export const createMockUserInfo = (opts?: Partial<UserInfo>): UserInfo => ({
id: 1,
first_name: "Testy",
diff --git a/frontend/src/metabase-types/api/revision.ts b/frontend/src/metabase-types/api/revision.ts
index 27b6ab9fe7e..83758c04234 100644
--- a/frontend/src/metabase-types/api/revision.ts
+++ b/frontend/src/metabase-types/api/revision.ts
@@ -1,12 +1,21 @@
-import { BaseUser } from "./user";
-
export interface Revision {
- description: string;
id: number;
+ description: string;
+ message: string | null;
+ timestamp: string;
is_creation: boolean;
is_reversion: boolean;
- message?: string | null;
- user: BaseUser;
- diff: { before: Record<string, any>; after: Record<string, any> };
- timestamp: string;
+ has_multiple_changes: boolean;
+ diff: { before: Record<string, any>; after: Record<string, any> } | null;
+ user: {
+ id: number;
+ first_name: string;
+ last_name: string;
+ common_name: string;
+ };
+}
+
+export interface RevisionListQuery {
+ model_type: string;
+ model_id: number | string;
}
diff --git a/frontend/src/metabase-types/api/user.ts b/frontend/src/metabase-types/api/user.ts
index 914cfc1e8d1..70cefe2ae84 100644
--- a/frontend/src/metabase-types/api/user.ts
+++ b/frontend/src/metabase-types/api/user.ts
@@ -31,6 +31,14 @@ export interface User extends BaseUser {
} | null;
}
+export interface UserListResult {
+ id: UserId;
+ first_name: string | null;
+ last_name: string | null;
+ common_name: string;
+ email: string;
+}
+
// Used when hydrating `creator` property
export type UserInfo = Pick<
BaseUser,
diff --git a/frontend/src/metabase/components/Timeline/Timeline.styled.jsx b/frontend/src/metabase/common/components/Timeline/Timeline.styled.tsx
similarity index 56%
rename from frontend/src/metabase/components/Timeline/Timeline.styled.jsx
rename to frontend/src/metabase/common/components/Timeline/Timeline.styled.tsx
index 49643370bd4..58cc3cbec50 100644
--- a/frontend/src/metabase/components/Timeline/Timeline.styled.jsx
+++ b/frontend/src/metabase/common/components/Timeline/Timeline.styled.tsx
@@ -1,35 +1,30 @@
import styled from "@emotion/styled";
+
import { color } from "metabase/lib/colors";
-import Icon from "metabase/components/Icon";
import Button from "metabase/core/components/Button";
export const TimelineContainer = styled.ul`
position: relative;
- margin-left: ${props => props.leftShift}px;
- margin-bottom: ${props => props.bottomShift}px;
+ margin-left: 0.5rem;
+ margin-bottom: 0.5rem;
`;
-export const TimelineItem = styled.li`
+export const TimelineEvent = styled.li`
display: flex;
align-items: start;
justify-content: start;
- transform: translateX(-${props => props.leftShift}px);
+ transform: translateX(-0.5rem);
white-space: pre-line;
width: 100%;
margin-bottom: 1.5rem;
`;
-export const ItemIcon = styled(Icon)`
- position: relative;
- color: ${props => (props.color ? color(props.color) : color("text-light"))};
-`;
-
-export const ItemBody = styled.div`
+export const EventBody = styled.div`
margin-left: 0.5rem;
flex: 1;
`;
-export const ItemHeader = styled.div`
+export const EventHeader = styled.div`
font-weight: 700;
display: flex;
justify-content: space-between;
@@ -46,15 +41,11 @@ export const Timestamp = styled.time`
padding-bottom: 0.5rem;
`;
-export const ItemFooter = styled.div`
- margin-top: 0.5rem;
-`;
-
// shift the border down slightly so that it doesn't appear above the top-most icon
// also using a negative `bottom` to connect the border with the event icon beneath it
export const Border = styled.div`
position: absolute;
- top: ${props => props.borderShift}px;
- left: ${props => props.borderShift}px;
- bottom: calc(-1rem - ${props => props.borderShift}px);
+ top: 0.5rem;
+ left: 0.5rem;
+ bottom: -1.5rem;
`;
diff --git a/frontend/src/metabase/common/components/Timeline/Timeline.tsx b/frontend/src/metabase/common/components/Timeline/Timeline.tsx
new file mode 100644
index 00000000000..6030003514d
--- /dev/null
+++ b/frontend/src/metabase/common/components/Timeline/Timeline.tsx
@@ -0,0 +1,84 @@
+import React from "react";
+import _ from "underscore";
+import { t } from "ttag";
+import { getRelativeTime } from "metabase/lib/time";
+
+import type { RevisionOrModerationEvent } from "metabase/plugins";
+import type { Revision } from "metabase-types/api";
+import Button from "metabase/core/components/Button";
+import Icon from "metabase/components/Icon";
+import Tooltip from "metabase/core/components/Tooltip";
+
+import { color } from "metabase/lib/colors";
+import {
+ TimelineContainer,
+ TimelineEvent,
+ Border,
+ EventBody,
+ EventHeader,
+ Timestamp,
+} from "./Timeline.styled";
+
+interface TimelineProps {
+ events: RevisionOrModerationEvent[];
+ "data-testid": string;
+ canWrite: boolean;
+ revert: (revision: Revision) => void;
+ className?: string;
+}
+
+export function Timeline({
+ events,
+ "data-testid": dataTestId,
+ canWrite,
+ revert,
+ className,
+}: TimelineProps) {
+ return (
+ <TimelineContainer className={className} data-testid={dataTestId}>
+ {events.map((event, index) => {
+ const { icon, title, description, timestamp, revision } = event;
+ const isNotLastEvent = index !== events.length - 1;
+ const isNotFirstEvent = index !== 0;
+
+ return (
+ <TimelineEvent key={revision?.id ?? `${title}-${timestamp}`}>
+ {isNotLastEvent && <Border />}
+ <EventIcon icon={icon} />
+ <EventBody>
+ <EventHeader>
+ <span>{title}</span>
+ {revision && canWrite && isNotFirstEvent && (
+ <Tooltip tooltip={t`Revert to this version`}>
+ <Button
+ icon="revert"
+ onlyIcon
+ borderless
+ onClick={() => revert(revision)}
+ data-testid="question-revert-button"
+ aria-label={t`revert to ${title}`}
+ />
+ </Tooltip>
+ )}
+ </EventHeader>
+ <Timestamp dateTime={timestamp}>
+ {getRelativeTime(timestamp)}
+ </Timestamp>
+ {revision?.has_multiple_changes && <div>{description}</div>}
+ </EventBody>
+ </TimelineEvent>
+ );
+ })}
+ </TimelineContainer>
+ );
+}
+
+function EventIcon({ icon }: { icon: RevisionOrModerationEvent["icon"] }) {
+ if (_.isObject(icon) && (!icon.name || !icon.color)) {
+ return null;
+ }
+ if (_.isObject(icon)) {
+ return <Icon name={icon.name} color={color(icon.color)} size={16} />;
+ }
+ return <Icon name={icon} color={color("text-light")} size={16} />;
+}
diff --git a/frontend/src/metabase/common/components/Timeline/index.ts b/frontend/src/metabase/common/components/Timeline/index.ts
new file mode 100644
index 00000000000..23bd871b648
--- /dev/null
+++ b/frontend/src/metabase/common/components/Timeline/index.ts
@@ -0,0 +1 @@
+export * from "./Timeline";
diff --git a/frontend/src/metabase/common/components/Timeline/utils.ts b/frontend/src/metabase/common/components/Timeline/utils.ts
new file mode 100644
index 00000000000..74f5883b732
--- /dev/null
+++ b/frontend/src/metabase/common/components/Timeline/utils.ts
@@ -0,0 +1,31 @@
+import { t } from "ttag";
+
+import type { Revision, User } from "metabase-types/api";
+
+export function getTimelineEvents({
+ revisions = [],
+ currentUser,
+}: {
+ revisions: Revision[] | undefined;
+ currentUser: User | null;
+}) {
+ return revisions.map(r => {
+ // If > 1 item's fields are changed in a single revision,
+ // the changes are batched into a single string like:
+ // "added a description, moved cards around and archived this"
+ // Batched messages can be long, so if the revision's diff contains > 1 field,
+ // we want to show the changelog in a description and set a title to just "User edited this"
+ // If only one field is changed, we just show everything in the title
+ // like "John added a description"
+ const titleText = r.has_multiple_changes ? t`edited this.` : r.description;
+ return {
+ title: `${
+ r.user.id === currentUser?.id ? t`You` : r.user.common_name
+ } ${titleText}`,
+ description: r.description,
+ timestamp: r.timestamp,
+ icon: "pencil",
+ revision: r,
+ };
+ });
+}
diff --git a/frontend/src/metabase/common/hooks/use-database-list-query/use-database-list-query.ts b/frontend/src/metabase/common/hooks/use-database-list-query/use-database-list-query.ts
index 4e0660a2f62..d257c26586f 100644
--- a/frontend/src/metabase/common/hooks/use-database-list-query/use-database-list-query.ts
+++ b/frontend/src/metabase/common/hooks/use-database-list-query/use-database-list-query.ts
@@ -14,6 +14,7 @@ export const useDatabaseListQuery = (
fetchList: Databases.actions.fetchList,
getList: Databases.selectors.getList,
getLoading: Databases.selectors.getLoading,
+ getLoaded: Databases.selectors.getLoaded,
getError: Databases.selectors.getError,
});
};
diff --git a/frontend/src/metabase/common/hooks/use-entity-list-query/use-entity-list-query.ts b/frontend/src/metabase/common/hooks/use-entity-list-query/use-entity-list-query.ts
index 0db9c12bcc6..6bd034014cf 100644
--- a/frontend/src/metabase/common/hooks/use-entity-list-query/use-entity-list-query.ts
+++ b/frontend/src/metabase/common/hooks/use-entity-list-query/use-entity-list-query.ts
@@ -18,6 +18,7 @@ export interface UseEntityListOwnProps<TItem, TQuery = never> {
options: EntityQueryOptions<TQuery>,
) => TItem[] | undefined;
getLoading: (state: State, options: EntityQueryOptions<TQuery>) => boolean;
+ getLoaded: (state: State, options: EntityQueryOptions<TQuery>) => boolean;
getError: (state: State, options: EntityQueryOptions<TQuery>) => unknown;
}
@@ -43,21 +44,23 @@ export const useEntityListQuery = <TItem, TQuery = never>(
fetchList,
getList,
getLoading,
+ getLoaded,
getError,
}: UseEntityListOwnProps<TItem, TQuery>,
): UseEntityListQueryResult<TItem> => {
const options = { entityQuery };
const data = useSelector(state => getList(state, options));
const isLoading = useSelector(state => getLoading(state, options));
+ const isLoaded = useSelector(state => getLoaded(state, options));
const error = useSelector(state => getError(state, options));
const dispatch = useDispatch();
useDeepCompareEffect(() => {
- if (enabled) {
+ if (enabled && !isLoaded) {
const action = dispatch(fetchList(entityQuery, { reload }));
Promise.resolve(action).catch(() => undefined);
}
- }, [dispatch, fetchList, entityQuery, reload, enabled]);
+ }, [dispatch, fetchList, entityQuery, reload, enabled, isLoaded]);
return { data, isLoading, error };
};
diff --git a/frontend/src/metabase/common/hooks/use-metric-list-query/use-metric-list-query.ts b/frontend/src/metabase/common/hooks/use-metric-list-query/use-metric-list-query.ts
index 13db1b8ce3a..972a8b60fee 100644
--- a/frontend/src/metabase/common/hooks/use-metric-list-query/use-metric-list-query.ts
+++ b/frontend/src/metabase/common/hooks/use-metric-list-query/use-metric-list-query.ts
@@ -13,6 +13,7 @@ export const useMetricListQuery = (
fetchList: Metrics.actions.fetchList,
getList: Metrics.selectors.getList,
getLoading: Metrics.selectors.getLoading,
+ getLoaded: Metrics.selectors.getLoaded,
getError: Metrics.selectors.getError,
});
};
diff --git a/frontend/src/metabase/common/hooks/use-question-list-query/use-question-list-query.ts b/frontend/src/metabase/common/hooks/use-question-list-query/use-question-list-query.ts
index ed7f8da7d20..03e2ae23c6e 100644
--- a/frontend/src/metabase/common/hooks/use-question-list-query/use-question-list-query.ts
+++ b/frontend/src/metabase/common/hooks/use-question-list-query/use-question-list-query.ts
@@ -14,6 +14,7 @@ export const useQuestionListQuery = (
fetchList: Questions.actions.fetchList,
getList: Questions.selectors.getList,
getLoading: Questions.selectors.getLoading,
+ getLoaded: Questions.selectors.getLoaded,
getError: Questions.selectors.getError,
});
};
diff --git a/frontend/src/metabase/common/hooks/use-revision-list-query/index.ts b/frontend/src/metabase/common/hooks/use-revision-list-query/index.ts
new file mode 100644
index 00000000000..175d43a6823
--- /dev/null
+++ b/frontend/src/metabase/common/hooks/use-revision-list-query/index.ts
@@ -0,0 +1 @@
+export * from "./use-revision-list-query";
diff --git a/frontend/src/metabase/common/hooks/use-revision-list-query/use-revision-list-query.ts b/frontend/src/metabase/common/hooks/use-revision-list-query/use-revision-list-query.ts
new file mode 100644
index 00000000000..b99babba4c1
--- /dev/null
+++ b/frontend/src/metabase/common/hooks/use-revision-list-query/use-revision-list-query.ts
@@ -0,0 +1,19 @@
+import RevisionEntity from "metabase/entities/revisions";
+import {
+ useEntityListQuery,
+ UseEntityListQueryProps,
+ UseEntityListQueryResult,
+} from "metabase/common/hooks/use-entity-list-query";
+import type { Revision, RevisionListQuery } from "metabase-types/api";
+
+export const useRevisionListQuery = (
+ props: UseEntityListQueryProps<RevisionListQuery> = {},
+): UseEntityListQueryResult<Revision> => {
+ return useEntityListQuery(props, {
+ fetchList: RevisionEntity.actions.fetchList,
+ getList: RevisionEntity.selectors.getList,
+ getLoading: RevisionEntity.selectors.getLoading,
+ getLoaded: RevisionEntity.selectors.getLoaded,
+ getError: RevisionEntity.selectors.getError,
+ });
+};
diff --git a/frontend/src/metabase/common/hooks/use-revision-list-query/use-revision-list-query.unit.spec.tsx b/frontend/src/metabase/common/hooks/use-revision-list-query/use-revision-list-query.unit.spec.tsx
new file mode 100644
index 00000000000..4942914c850
--- /dev/null
+++ b/frontend/src/metabase/common/hooks/use-revision-list-query/use-revision-list-query.unit.spec.tsx
@@ -0,0 +1,48 @@
+import React from "react";
+
+import { setupRevisionsEndpoints } from "__support__/server-mocks/revision";
+import {
+ renderWithProviders,
+ screen,
+ waitForElementToBeRemoved,
+} from "__support__/ui";
+import { createMockRevision } from "metabase-types/api/mocks/revision";
+import LoadingAndErrorWrapper from "metabase/components/LoadingAndErrorWrapper/LoadingAndErrorWrapper";
+
+import { useRevisionListQuery } from "./use-revision-list-query";
+
+const TEST_REVISION = createMockRevision();
+
+function TestComponent() {
+ const { data = [], isLoading, error } = useRevisionListQuery();
+
+ if (isLoading || error) {
+ return <LoadingAndErrorWrapper loading={isLoading} error={error} />;
+ }
+
+ return (
+ <div>
+ {data.map(revision => (
+ <div key={revision.id}>{revision.description}</div>
+ ))}
+ </div>
+ );
+}
+
+function setup() {
+ setupRevisionsEndpoints([TEST_REVISION]);
+ renderWithProviders(<TestComponent />);
+}
+
+describe("useRevisionListQuery", () => {
+ it("should be initially loading", () => {
+ setup();
+ expect(screen.getByText("Loading...")).toBeInTheDocument();
+ });
+
+ it("should show data from the response", async () => {
+ setup();
+ await waitForElementToBeRemoved(() => screen.queryByText("Loading..."));
+ expect(screen.getByText(TEST_REVISION.description)).toBeInTheDocument();
+ });
+});
diff --git a/frontend/src/metabase/common/hooks/use-schema-list-query/use-schema-list-query.ts b/frontend/src/metabase/common/hooks/use-schema-list-query/use-schema-list-query.ts
index ee0568a8cf1..591415a4e4c 100644
--- a/frontend/src/metabase/common/hooks/use-schema-list-query/use-schema-list-query.ts
+++ b/frontend/src/metabase/common/hooks/use-schema-list-query/use-schema-list-query.ts
@@ -14,6 +14,7 @@ export const useSchemaListQuery = (
fetchList: Schemas.actions.fetchList,
getList: Schemas.selectors.getList,
getLoading: Schemas.selectors.getLoading,
+ getLoaded: Schemas.selectors.getLoaded,
getError: Schemas.selectors.getError,
});
};
diff --git a/frontend/src/metabase/common/hooks/use-segment-list-query/use-segment-list-query.ts b/frontend/src/metabase/common/hooks/use-segment-list-query/use-segment-list-query.ts
index 75cec17f77d..ba23a0281db 100644
--- a/frontend/src/metabase/common/hooks/use-segment-list-query/use-segment-list-query.ts
+++ b/frontend/src/metabase/common/hooks/use-segment-list-query/use-segment-list-query.ts
@@ -13,6 +13,7 @@ export const useSegmentListQuery = (
fetchList: Segments.actions.fetchList,
getList: Segments.selectors.getList,
getLoading: Segments.selectors.getLoading,
+ getLoaded: Segments.selectors.getLoaded,
getError: Segments.selectors.getError,
});
};
diff --git a/frontend/src/metabase/common/hooks/use-table-list-query/use-table-list-query.ts b/frontend/src/metabase/common/hooks/use-table-list-query/use-table-list-query.ts
index c5a149d7bea..26b0388412e 100644
--- a/frontend/src/metabase/common/hooks/use-table-list-query/use-table-list-query.ts
+++ b/frontend/src/metabase/common/hooks/use-table-list-query/use-table-list-query.ts
@@ -14,6 +14,7 @@ export const useTableListQuery = (
fetchList: Tables.actions.fetchList,
getList: Tables.selectors.getList,
getLoading: Tables.selectors.getLoading,
+ getLoaded: Tables.selectors.getLoaded,
getError: Tables.selectors.getError,
});
};
diff --git a/frontend/src/metabase/common/hooks/use-user-list-query/index.ts b/frontend/src/metabase/common/hooks/use-user-list-query/index.ts
new file mode 100644
index 00000000000..327907614b5
--- /dev/null
+++ b/frontend/src/metabase/common/hooks/use-user-list-query/index.ts
@@ -0,0 +1 @@
+export * from "./use-user-list-query";
diff --git a/frontend/src/metabase/common/hooks/use-user-list-query/use-user-list-query.ts b/frontend/src/metabase/common/hooks/use-user-list-query/use-user-list-query.ts
new file mode 100644
index 00000000000..7717cc5a73d
--- /dev/null
+++ b/frontend/src/metabase/common/hooks/use-user-list-query/use-user-list-query.ts
@@ -0,0 +1,19 @@
+import Users from "metabase/entities/users";
+import {
+ useEntityListQuery,
+ UseEntityListQueryProps,
+ UseEntityListQueryResult,
+} from "metabase/common/hooks/use-entity-list-query";
+import type { UserListResult } from "metabase-types/api";
+
+export const useUserListQuery = (
+ props: UseEntityListQueryProps<Record<string, never>> = {},
+): UseEntityListQueryResult<UserListResult> => {
+ return useEntityListQuery(props, {
+ fetchList: Users.actions.fetchList,
+ getList: Users.selectors.getList,
+ getLoading: Users.selectors.getLoading,
+ getLoaded: Users.selectors.getLoaded,
+ getError: Users.selectors.getError,
+ });
+};
diff --git a/frontend/src/metabase/common/hooks/use-user-list-query/use-user-list-query.unit.spec.tsx b/frontend/src/metabase/common/hooks/use-user-list-query/use-user-list-query.unit.spec.tsx
new file mode 100644
index 00000000000..ccf26dd38a5
--- /dev/null
+++ b/frontend/src/metabase/common/hooks/use-user-list-query/use-user-list-query.unit.spec.tsx
@@ -0,0 +1,48 @@
+import React from "react";
+
+import { setupUsersEndpoints } from "__support__/server-mocks/user";
+import {
+ renderWithProviders,
+ screen,
+ waitForElementToBeRemoved,
+} from "__support__/ui";
+import { createMockUserInfo } from "metabase-types/api/mocks";
+import LoadingAndErrorWrapper from "metabase/components/LoadingAndErrorWrapper/LoadingAndErrorWrapper";
+
+import { useUserListQuery } from "./use-user-list-query";
+
+const TEST_USER = createMockUserInfo();
+
+function TestComponent() {
+ const { data = [], isLoading, error } = useUserListQuery();
+
+ if (isLoading || error) {
+ return <LoadingAndErrorWrapper loading={isLoading} error={error} />;
+ }
+
+ return (
+ <div>
+ {data.map(user => (
+ <div key={user.id}>{user.common_name}</div>
+ ))}
+ </div>
+ );
+}
+
+function setup() {
+ setupUsersEndpoints([TEST_USER]);
+ renderWithProviders(<TestComponent />);
+}
+
+describe("useUserListQuery", () => {
+ it("should be initially loading", () => {
+ setup();
+ expect(screen.getByText("Loading...")).toBeInTheDocument();
+ });
+
+ it("should show data from the response", async () => {
+ setup();
+ await waitForElementToBeRemoved(() => screen.queryByText("Loading..."));
+ expect(screen.getByText("Testy Tableton")).toBeInTheDocument();
+ });
+});
diff --git a/frontend/src/metabase/components/Timeline/Timeline.jsx b/frontend/src/metabase/components/Timeline/Timeline.jsx
deleted file mode 100644
index 72405cf8ddc..00000000000
--- a/frontend/src/metabase/components/Timeline/Timeline.jsx
+++ /dev/null
@@ -1,122 +0,0 @@
-import React, { useMemo } from "react";
-import PropTypes from "prop-types";
-import _ from "underscore";
-import { t } from "ttag";
-import { getRelativeTime } from "metabase/lib/time";
-
-import Button from "metabase/core/components/Button";
-import Tooltip from "metabase/core/components/Tooltip";
-
-import {
- TimelineContainer,
- TimelineItem,
- Border,
- ItemIcon,
- ItemBody,
- ItemHeader,
- Timestamp,
- ItemFooter,
-} from "./Timeline.styled";
-
-Timeline.propTypes = {
- className: PropTypes.string,
- items: PropTypes.arrayOf(
- PropTypes.shape({
- timestamp: PropTypes.number.isRequired,
- icon: PropTypes.oneOfType([PropTypes.string, PropTypes.object])
- .isRequired,
- title: PropTypes.node.isRequired,
- description: PropTypes.string,
- renderFooter: PropTypes.bool,
- }),
- ),
- renderFooter: PropTypes.func,
- revertFn: PropTypes.func,
- "data-testid": PropTypes.string,
-};
-
-function Timeline({
- className,
- items = [],
- renderFooter,
- revertFn,
- "data-testid": dataTestId,
-}) {
- const iconSize = 16;
- const halfIconSize = iconSize / 2;
-
- const sortedFormattedItems = useMemo(() => {
- return items
- .sort((a, b) => b.timestamp - a.timestamp)
- .map(item => {
- return {
- ...item,
- formattedTimestamp: getRelativeTime(item.timestamp),
- };
- });
- }, [items]);
-
- return (
- <TimelineContainer
- leftShift={halfIconSize}
- bottomShift={halfIconSize}
- className={className}
- data-testid={dataTestId}
- >
- {sortedFormattedItems.map((item, index) => {
- const {
- icon,
- title,
- titleText,
- description,
- timestamp,
- formattedTimestamp,
- isRevertable,
- revision,
- } = item;
- const key = item.key == null ? index : item.key;
- const isNotLastEvent = index !== sortedFormattedItems.length - 1;
- const iconProps = _.isObject(icon)
- ? icon
- : {
- name: icon,
- };
-
- return (
- <TimelineItem key={key} leftShift={halfIconSize}>
- {isNotLastEvent && <Border borderShift={halfIconSize} />}
- <ItemIcon {...iconProps} size={iconSize} />
- <ItemBody>
- <ItemHeader>
- {title}
- {isRevertable && revertFn && (
- <Tooltip tooltip={t`Revert to this version`}>
- <Button
- icon="revert"
- onlyIcon
- borderless
- onClick={() => revertFn(revision)}
- data-testid="question-revert-button"
- aria-label={t`revert to ${titleText}`}
- />
- </Tooltip>
- )}
- </ItemHeader>
- <Timestamp datetime={timestamp}>{formattedTimestamp}</Timestamp>
- <div>{description}</div>
- {_.isFunction(renderFooter) && (
- <ItemFooter>{renderFooter(item)}</ItemFooter>
- )}
- </ItemBody>
- </TimelineItem>
- );
- })}
- </TimelineContainer>
- );
-}
-
-export default Object.assign(Timeline, {
- ItemBody,
- ItemHeader,
- ItemIcon,
-});
diff --git a/frontend/src/metabase/components/Timeline/index.jsx b/frontend/src/metabase/components/Timeline/index.jsx
deleted file mode 100644
index ed6f31d60de..00000000000
--- a/frontend/src/metabase/components/Timeline/index.jsx
+++ /dev/null
@@ -1 +0,0 @@
-export { default } from "./Timeline";
diff --git a/frontend/src/metabase/dashboard/actions/revisions.js b/frontend/src/metabase/dashboard/actions/revisions.js
index 8a83fc13fdd..f11ed58192d 100644
--- a/frontend/src/metabase/dashboard/actions/revisions.js
+++ b/frontend/src/metabase/dashboard/actions/revisions.js
@@ -1,4 +1,5 @@
import { createThunkAction } from "metabase/lib/redux";
+import Revision from "metabase/entities/revisions";
import { fetchDashboard, fetchDashboardCardData } from "./data-fetching";
@@ -7,7 +8,7 @@ export const revertToRevision = createThunkAction(
REVERT_TO_REVISION,
revision => {
return async dispatch => {
- await revision.revert();
+ await dispatch(Revision.objectActions.revert(revision));
await dispatch(fetchDashboard(revision.model_id, null));
await dispatch(
fetchDashboardCardData({ reload: false, clearCache: true }),
diff --git a/frontend/src/metabase/dashboard/components/DashboardInfoSidebar.tsx b/frontend/src/metabase/dashboard/components/DashboardInfoSidebar.tsx
index fa9953800d6..c59c60a7e9b 100644
--- a/frontend/src/metabase/dashboard/components/DashboardInfoSidebar.tsx
+++ b/frontend/src/metabase/dashboard/components/DashboardInfoSidebar.tsx
@@ -1,18 +1,14 @@
-import React, { useCallback, useMemo } from "react";
-import _ from "underscore";
+import React, { useCallback } from "react";
import { t } from "ttag";
-import { connect } from "react-redux";
import { PLUGIN_CACHING } from "metabase/plugins";
+import { useDispatch, useSelector } from "metabase/lib/redux";
import MetabaseSettings from "metabase/lib/settings";
-import DefaultTimeline from "metabase/components/Timeline";
+import { Timeline } from "metabase/common/components/Timeline";
import EditableText from "metabase/core/components/EditableText";
-import { Dashboard, Revision as RevisionType, User } from "metabase-types/api";
-import { State } from "metabase-types/store";
-import Revision from "metabase/entities/revisions";
-import { getRevisionEventsForTimeline } from "metabase/lib/revisions";
+import { Dashboard } from "metabase-types/api";
import { getUser } from "metabase/selectors/user";
import {
@@ -23,7 +19,8 @@ import {
import Toggle from "metabase/core/components/Toggle";
import FormField from "metabase/core/components/FormField";
import { useUniqueId } from "metabase/hooks/use-unique-id";
-import { useDispatch } from "metabase/lib/redux";
+import { getTimelineEvents } from "metabase/common/components/Timeline/utils";
+import { useRevisionListQuery } from "metabase/common/hooks/use-revision-list-query";
import {
DashboardInfoSidebarRoot,
HistoryHeader,
@@ -37,20 +34,19 @@ interface DashboardInfoSidebarProps {
dashboard: Dashboard;
setDashboardAttribute: (name: string, value: DashboardAttributeType) => void;
saveDashboardAndCards: (preserveParameters?: boolean) => void;
- revisions: RevisionType[];
- currentUser: User;
- revertToRevision: (revision: RevisionType) => void;
}
-const DashboardInfoSidebar = ({
+export function DashboardInfoSidebar({
dashboard,
setDashboardAttribute,
saveDashboardAndCards,
- revisions,
- currentUser,
- revertToRevision,
-}: DashboardInfoSidebarProps) => {
- const canWrite = dashboard.can_write;
+}: DashboardInfoSidebarProps) {
+ const { data: revisions } = useRevisionListQuery({
+ query: { model_type: "dashboard", model_id: dashboard.id },
+ });
+
+ const currentUser = useSelector(getUser);
+ const dispatch = useDispatch();
const showCaching =
PLUGIN_CACHING.isEnabled() && MetabaseSettings.get("enable-query-caching");
@@ -71,7 +67,6 @@ const DashboardInfoSidebar = ({
[saveDashboardAndCards, setDashboardAttribute],
);
- const dispatch = useDispatch();
const handleToggleAutoApplyFilters = useCallback(
(isAutoApplyingFilters: boolean) => {
dispatch(toggleAutoApplyFilters(isAutoApplyingFilters));
@@ -79,15 +74,6 @@ const DashboardInfoSidebar = ({
[dispatch],
);
- const events = useMemo(
- () =>
- getRevisionEventsForTimeline(revisions, {
- currentUser,
- canWrite,
- }),
- [revisions, currentUser, canWrite],
- );
-
const autoApplyFilterToggleId = useUniqueId();
return (
@@ -129,33 +115,13 @@ const DashboardInfoSidebar = ({
<ContentSection>
<HistoryHeader>{t`History`}</HistoryHeader>
- <DefaultTimeline
- items={events}
+ <Timeline
+ events={getTimelineEvents({ revisions, currentUser })}
data-testid="dashboard-history-list"
- revertFn={revertToRevision}
+ revert={revision => dispatch(revertToRevision(revision))}
+ canWrite={dashboard.can_write}
/>
</ContentSection>
</DashboardInfoSidebarRoot>
);
-};
-
-const mapStateToProps = (state: State) => ({
- currentUser: getUser(state),
-});
-
-const mapDispatchToProps = {
- revertToRevision,
-};
-
-// eslint-disable-next-line import/no-default-export -- deprecated usage
-export default _.compose(
- Revision.loadList({
- query: (state: State, props: DashboardInfoSidebarProps) => ({
- model_type: "dashboard",
- model_id: props.dashboard.id,
- }),
- wrapped: true,
- loadingAndErrorWrapper: false,
- }),
- connect(mapStateToProps, mapDispatchToProps),
-)(DashboardInfoSidebar);
+}
diff --git a/frontend/src/metabase/dashboard/components/DashboardSidebars.jsx b/frontend/src/metabase/dashboard/components/DashboardSidebars.jsx
index 452d7bc8b02..882e1ac1e46 100644
--- a/frontend/src/metabase/dashboard/components/DashboardSidebars.jsx
+++ b/frontend/src/metabase/dashboard/components/DashboardSidebars.jsx
@@ -8,7 +8,7 @@ import ParameterSidebar from "metabase/parameters/components/ParameterSidebar";
import SharingSidebar from "metabase/sharing/components/SharingSidebar";
import * as MetabaseAnalytics from "metabase/lib/analytics";
import ClickBehaviorSidebar from "./ClickBehaviorSidebar";
-import DashboardInfoSidebar from "./DashboardInfoSidebar";
+import { DashboardInfoSidebar } from "./DashboardInfoSidebar";
import { AddCardSidebar } from "./add-card-sidebar/AddCardSidebar";
import { ActionSidebar } from "./ActionSidebar";
diff --git a/frontend/src/metabase/lib/revisions/components.jsx b/frontend/src/metabase/lib/revisions/components.jsx
deleted file mode 100644
index 42c5e18eb96..00000000000
--- a/frontend/src/metabase/lib/revisions/components.jsx
+++ /dev/null
@@ -1,80 +0,0 @@
-import React, { useMemo } from "react";
-import PropTypes from "prop-types";
-import styled from "@emotion/styled";
-import { t } from "ttag";
-import { color } from "metabase/lib/colors";
-import { capitalize } from "metabase/lib/formatting";
-import RawEntityLink from "metabase/entities/containers/EntityLink";
-import { getRevisionTitleText } from "./revisions";
-
-export const EntityLink = styled(RawEntityLink)`
- color: ${color("brand")};
- cursor: pointer;
- text-decoration: none;
-
- :hover {
- text-decoration: underline;
- }
-`;
-
-EntityLink.defaultProps = {
- dispatchApiErrorEvent: false,
-};
-
-const revisionTitlePropTypes = {
- username: PropTypes.string.isRequired,
- message: PropTypes.node.isRequired,
- event: PropTypes.node,
- revertFn: PropTypes.func,
-};
-
-export function RevisionTitle({ username, message }) {
- return <span>{getRevisionTitleText(username, message)}</span>;
-}
-
-RevisionTitle.propTypes = revisionTitlePropTypes;
-
-const revisionBatchedDescriptionPropTypes = {
- changes: PropTypes.arrayOf(PropTypes.node).isRequired,
- fallback: PropTypes.string,
-};
-
-export function RevisionBatchedDescription({ changes, fallback }) {
- const formattedChanges = useMemo(() => {
- let result = [];
-
- changes.forEach((change, i) => {
- 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, fallback]);
-
- return <span>{formattedChanges}</span>;
-}
-
-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 });
-}
-
-RevisionBatchedDescription.propTypes = revisionBatchedDescriptionPropTypes;
diff --git a/frontend/src/metabase/lib/revisions/components.styled.tsx b/frontend/src/metabase/lib/revisions/components.styled.tsx
deleted file mode 100644
index 7cf7336ada1..00000000000
--- a/frontend/src/metabase/lib/revisions/components.styled.tsx
+++ /dev/null
@@ -1,23 +0,0 @@
-import styled from "@emotion/styled";
-import Button from "metabase/core/components/Button";
-import { color } from "metabase/lib/colors";
-
-export const RevertButton = styled(Button)`
- padding: 0;
- border: none;
- color: ${color("text-dark")};
- position: relative;
- top: 2px;
-
- &:hover {
- background-color: transparent;
- color: ${color("accent3")};
- }
-`;
-
-export const RevisionTitleContainer = styled.span`
- display: flex;
- width: 100%;
- justify-content: space-between;
- align-items: start;
-`;
diff --git a/frontend/src/metabase/lib/revisions/components.unit.spec.js b/frontend/src/metabase/lib/revisions/components.unit.spec.js
deleted file mode 100644
index 5f2e1db4b9c..00000000000
--- a/frontend/src/metabase/lib/revisions/components.unit.spec.js
+++ /dev/null
@@ -1,75 +0,0 @@
-import React from "react";
-import { render, renderWithProviders, screen } from "__support__/ui";
-import { getCollectionChangeDescription } from "./revisions";
-import { RevisionTitle, RevisionBatchedDescription } from "./components";
-
-describe("RevisionTitle", () => {
- it("renders correctly", () => {
- render(<RevisionTitle username="Alex" message="added a description" />);
- expect(screen.getByText("Alex added a description")).toBeInTheDocument();
- });
-});
-
-describe("RevisionBatchedDescription", () => {
- const originalWarn = console.warn;
-
- beforeAll(() => {
- console.warn = jest.fn();
- });
-
- afterAll(() => {
- console.warn = originalWarn;
- });
-
- it("correctly renders two change records", () => {
- render(
- <RevisionBatchedDescription
- changes={["added a description", "archived this"]}
- />,
- );
- expect(
- screen.getByText("Added a description and archived this"),
- ).toBeInTheDocument();
- });
-
- it("correctly renders more than two change records", () => {
- render(
- <RevisionBatchedDescription
- changes={["renamed this", "added a description", "archived this"]}
- />,
- );
- expect(
- screen.getByText("Renamed this, added a description and archived this"),
- ).toBeInTheDocument();
- });
-
- it("correctly renders changes with JSX", () => {
- render(
- <RevisionBatchedDescription
- changes={["renamed this", ["moved to", <p key="1">Our analytics</p>]]}
- />,
- );
- expect(screen.getByText("Renamed this and moved to")).toBeInTheDocument();
- expect(screen.getByText("Our analytics")).toBeInTheDocument();
- });
-
- 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/index.js b/frontend/src/metabase/lib/revisions/index.js
deleted file mode 100644
index 4442f1f0bed..00000000000
--- a/frontend/src/metabase/lib/revisions/index.js
+++ /dev/null
@@ -1 +0,0 @@
-export * from "./revisions";
diff --git a/frontend/src/metabase/lib/revisions/revisions.js b/frontend/src/metabase/lib/revisions/revisions.js
deleted file mode 100644
index ac86182f2eb..00000000000
--- a/frontend/src/metabase/lib/revisions/revisions.js
+++ /dev/null
@@ -1,293 +0,0 @@
-import React from "react";
-import { t, jt, ngettext, msgid } from "ttag";
-import {
- EntityLink,
- RevisionTitle,
- RevisionBatchedDescription,
-} from "./components";
-
-const CHANGE_TYPE = {
- ADD: "new",
- UPDATE: "update",
- REMOVE: "remove",
-};
-
-function getCardsArraySafe(cards) {
- // Cards diff will contain null values for cards that were not changed
- // Also for e.g. new card revision, the 'before' state can be just null
- // like { before: null, after: [ null, null, { ...cardInfo } ] }
- // So we need to filter out null values to get a correct revision description
- return Array.isArray(cards) ? cards.filter(Boolean) : [];
-}
-
-function hasSeries(card) {
- // card can be null or an object
- return typeof card.series !== "undefined";
-}
-
-function hasSeriesChange(cards) {
- return cards.some(hasSeries);
-}
-
-function getAddedRemovedCardIds(_prevCards, _cards) {
- const prevCardIds = getCardsArraySafe(_prevCards).map(c => c.id);
- const cardIds = getCardsArraySafe(_cards).map(c => c.id);
-
- const addedCardIds = cardIds.filter(id => !prevCardIds.includes(id));
- const removedCardIds = prevCardIds.filter(id => !cardIds.includes(id));
-
- return { addedCardIds, removedCardIds };
-}
-
-function getDashboardCardsChangeType(_prevCards, _cards) {
- const { addedCardIds, removedCardIds } = getAddedRemovedCardIds(
- _prevCards,
- _cards,
- );
-
- const types = [];
- if (addedCardIds.length > 0) {
- types.push(CHANGE_TYPE.ADD);
- }
- if (removedCardIds.length > 0) {
- types.push(CHANGE_TYPE.REMOVE);
- }
- if (addedCardIds.length === 0 && removedCardIds.length === 0) {
- types.push(CHANGE_TYPE.UPDATE);
- }
- return types;
-}
-
-function getChangeTypes(field, before, after) {
- if (field === "cards") {
- return getDashboardCardsChangeType(before, after);
- }
- if (before == null && after != null) {
- return [CHANGE_TYPE.ADD];
- }
- if (before != null && after == null) {
- return [CHANGE_TYPE.REMOVE];
- }
- return [CHANGE_TYPE.UPDATE];
-}
-
-function getSeriesChangeDescription(prevCards, cards) {
- const changedCardIndex = prevCards.findIndex(hasSeries);
- const seriesBefore = prevCards[changedCardIndex].series || [];
- const seriesAfter = cards[changedCardIndex].series || [];
- if (seriesBefore.length === seriesAfter.length) {
- return t`modified question's series`;
- }
- return seriesAfter.length > seriesBefore.length
- ? t`added series to a question`
- : t`removed series from a question`;
-}
-
-export function getCollectionChangeDescription(prevCollectionId, collectionId) {
- const key = `collection-from-${prevCollectionId}-to-${collectionId}`;
- return [
- jt`moved this to ${(
- <EntityLink
- key={key}
- entityId={collectionId || "root"}
- entityType="collections"
- fallback={t`Unknown`}
- />
- )}`,
- ];
-}
-
-const CHANGE_DESCRIPTIONS = {
- // Common
- name: {
- [CHANGE_TYPE.UPDATE]: (oldName, newName) => t`renamed this to "${newName}"`,
- },
- description: {
- [CHANGE_TYPE.ADD]: t`added a description`,
- [CHANGE_TYPE.UPDATE]: t`changed the description`,
- },
- archived: {
- [CHANGE_TYPE.UPDATE]: (wasArchived, isArchived) =>
- isArchived ? t`archived this` : t`unarchived this`,
- },
- collection_id: {
- [CHANGE_TYPE.ADD]: getCollectionChangeDescription,
- [CHANGE_TYPE.UPDATE]: getCollectionChangeDescription,
- [CHANGE_TYPE.REMOVE]: getCollectionChangeDescription,
- },
-
- // Questions & Models
- dataset: {
- [CHANGE_TYPE.UPDATE]: (wasDataset, isDataset) =>
- isDataset
- ? t`turned this into a model`
- : t`changed this from a model to a saved question`,
- },
- dataset_query: {
- [CHANGE_TYPE.ADD]: t`edited the question`,
- [CHANGE_TYPE.UPDATE]: t`edited the question`,
- },
- display: {
- [CHANGE_TYPE.UPDATE]: (prevDisplay, display) =>
- t`changed the display from ${prevDisplay} to ${display}`,
- },
- visualization_settings: {
- [CHANGE_TYPE.ADD]: t`changed the visualization settings`,
- [CHANGE_TYPE.UPDATE]: t`changed the visualization settings`,
- [CHANGE_TYPE.REMOVE]: t`changed the visualization settings`,
- },
- result_metadata: {
- [CHANGE_TYPE.ADD]: t`edited the metadata`,
- [CHANGE_TYPE.UPDATE]: t`edited the metadata`,
- [CHANGE_TYPE.REMOVE]: t`edited the metadata`,
- },
-
- // Dashboards
- cards: {
- [CHANGE_TYPE.ADD]: (_prevCards, _cards) => {
- const { addedCardIds } = getAddedRemovedCardIds(_prevCards, _cards);
- const count = addedCardIds.length;
- return ngettext(msgid`added a card`, `added ${count} cards`, count);
- },
- [CHANGE_TYPE.UPDATE]: (_prevCards, _cards) => {
- const prevCards = getCardsArraySafe(_prevCards);
- const cards = getCardsArraySafe(_cards);
- if (hasSeriesChange(prevCards) || hasSeriesChange(cards)) {
- return getSeriesChangeDescription(prevCards, cards);
- }
- return t`rearranged the cards`;
- },
- [CHANGE_TYPE.REMOVE]: (_prevCards, _cards) => {
- const { removedCardIds } = getAddedRemovedCardIds(_prevCards, _cards);
- const count = removedCardIds.length;
- return ngettext(msgid`removed a card`, `removed ${count} cards`, count);
- },
- },
-};
-
-export function hasDiff(revision) {
- return Boolean(
- revision.diff && (revision.diff.before || revision.diff.after),
- );
-}
-
-export function getChangedFields(revision) {
- if (!hasDiff(revision)) {
- return [];
- }
- const registeredFields = Object.keys(CHANGE_DESCRIPTIONS);
-
- // There are cases when either 'before' or 'after' states are null
- // So we need to pick another one
- const fields = Object.keys(revision.diff.after || revision.diff.before);
-
- return fields.filter(field => registeredFields.includes(field));
-}
-
-export function getRevisionTitleText(username, message) {
- return `${username} ${message}`;
-}
-
-export function getRevisionDescription(revision) {
- const { diff, is_creation, is_reversion } = revision;
- if (is_creation) {
- return t`created this`;
- }
- if (is_reversion) {
- return t`reverted to an earlier revision`;
- }
-
- const { before, after } = diff;
- let changes = [];
- getChangedFields(revision).forEach(fieldName => {
- const valueBefore = before?.[fieldName];
- const valueAfter = after?.[fieldName];
- const changeTypes = getChangeTypes(fieldName, valueBefore, valueAfter);
- changeTypes.forEach(changeType => {
- const description = CHANGE_DESCRIPTIONS[fieldName]?.[changeType];
- changes.push(
- typeof description === "function"
- ? description(valueBefore, valueAfter)
- : description,
- );
- });
- });
- changes = changes.filter(Boolean);
-
- return changes.length === 1 ? changes[0] : changes;
-}
-
-export function isValidRevision(revision) {
- if (revision.is_creation || revision.is_reversion) {
- return true;
- }
- return getChangedFields(revision).length > 0;
-}
-
-function getRevisionUsername(revision, currentUser) {
- const revisionUser = revision.user;
- return revisionUser.id === currentUser?.id
- ? t`You`
- : revisionUser.common_name;
-}
-
-function getRevisionEpochTimestamp(revision) {
- return new Date(revision.timestamp).valueOf();
-}
-
-export const REVISION_EVENT_ICON = "pencil";
-
-export function getRevisionEventsForTimeline(
- revisions = [],
- { currentUser, canWrite = false },
- revertFn,
-) {
- return revisions
- .filter(isValidRevision)
- .map((revision, index) => {
- const isRevertable = canWrite && index !== 0;
- const username = getRevisionUsername(revision, currentUser);
- const changes = getRevisionDescription(revision);
-
- const event = {
- timestamp: getRevisionEpochTimestamp(revision),
- icon: REVISION_EVENT_ICON,
- isRevertable,
- revision,
- };
-
- const isChangeEvent = !revision.is_creation && !revision.is_reversion;
-
- // For some events, like moving an item to another collection,
- // the `changes` object are an array, however they represent a single change
- // This happens when we need to have JSX inside a message (e.g. a link to a new collection)
- const isMultipleFieldsChange =
- Array.isArray(changes) && changes.length > 1;
-
- // If > 1 item's fields are changed in a single revision,
- // the changes are batched into a single string like:
- // "added a description, moved cards around and archived this"
- // Batched messages can be long, so if the revision's diff contains > 1 field,
- // we want to show the changelog in a description and set a title to just "User edited this"
- // If only one field is changed, we just show everything in the title
- // like "John added a description"
- let message;
- if (isChangeEvent && isMultipleFieldsChange) {
- message = t`edited this`;
- event.title = <RevisionTitle username={username} message={message} />;
- event.description = (
- <RevisionBatchedDescription
- changes={changes}
- fallback={revision.description}
- />
- );
- } else {
- message = changes;
- event.title = <RevisionTitle username={username} message={message} />;
- }
- event.titleText = getRevisionTitleText(username, message);
-
- return event;
- })
- .filter(Boolean);
-}
diff --git a/frontend/src/metabase/lib/revisions/revisions.unit.spec.js b/frontend/src/metabase/lib/revisions/revisions.unit.spec.js
deleted file mode 100644
index 55693505fea..00000000000
--- a/frontend/src/metabase/lib/revisions/revisions.unit.spec.js
+++ /dev/null
@@ -1,699 +0,0 @@
-import React from "react";
-import {
- REVISION_EVENT_ICON,
- getRevisionEventsForTimeline,
- getRevisionDescription,
- getChangedFields,
- isValidRevision,
-} from "./revisions";
-import { RevisionTitle } from "./components";
-
-const DEFAULT_TIMESTAMP = "2016-05-08T02:02:07.441Z";
-const DEFAULT_EPOCH_TIMESTAMP = new Date(DEFAULT_TIMESTAMP).valueOf();
-
-function getRevision({
- isCreation = false,
- isReversion = false,
- userId = 1,
- username = "Foo",
- before,
- after,
- timestamp = DEFAULT_TIMESTAMP,
-} = {}) {
- return {
- diff: {
- before,
- after,
- },
- user: {
- id: userId,
- common_name: username,
- },
- is_creation: isCreation,
- is_reversion: isReversion,
- timestamp,
- };
-}
-
-function getSimpleRevision({ field, before, after, ...rest }) {
- return getRevision({
- ...rest,
- before: {
- [field]: before,
- },
- after: {
- [field]: after,
- },
- });
-}
-
-describe("getRevisionDescription | common", () => {
- it("handles initial revision (entity created)", () => {
- const revision = getRevision({ isCreation: true });
- expect(getRevisionDescription(revision)).toBe("created this");
- });
-
- it("handles reversions", () => {
- const revision = getRevision({ isReversion: true });
- expect(getRevisionDescription(revision)).toBe(
- "reverted to an earlier revision",
- );
- });
-
- it("handles renames", () => {
- const revision = getSimpleRevision({
- field: "name",
- before: "Orders",
- after: "Orders by Month",
- });
- expect(getRevisionDescription(revision)).toBe(
- 'renamed this to "Orders by Month"',
- );
- });
-
- it("handles description added", () => {
- const revision = getSimpleRevision({
- field: "description",
- before: null,
- after: "Hello there",
- });
- expect(getRevisionDescription(revision)).toBe("added a description");
- });
-
- it("handles description change", () => {
- const revision = getSimpleRevision({
- field: "description",
- before: "Hello",
- after: "Hello there",
- });
- expect(getRevisionDescription(revision)).toBe("changed the description");
- });
-
- it("handles archive revision", () => {
- const revision = getSimpleRevision({
- field: "archived",
- before: false,
- after: true,
- });
- expect(getRevisionDescription(revision)).toBe("archived this");
- });
-
- it("handles unarchive revision", () => {
- const revision = getSimpleRevision({
- field: "archived",
- before: true,
- after: false,
- });
- expect(getRevisionDescription(revision)).toBe("unarchived this");
- });
-
- it("returns an array of two changes", () => {
- const revision = getRevision({
- before: {
- name: "Orders",
- archived: true,
- },
- after: {
- name: "Orders by Month",
- archived: false,
- },
- });
- expect(getRevisionDescription(revision)).toEqual([
- 'renamed this to "Orders by Month"',
- "unarchived this",
- ]);
- });
-
- it("returns an array of multiple changes", () => {
- const revision = getRevision({
- before: {
- name: "Orders",
- description: null,
- archived: true,
- },
- after: {
- name: "Orders by Month",
- description: "Test",
- archived: false,
- },
- });
- expect(getRevisionDescription(revision)).toEqual([
- 'renamed this to "Orders by Month"',
- "added a description",
- "unarchived this",
- ]);
- });
-
- it("returns an empty array if can't find a friendly message", () => {
- const revision = getSimpleRevision({
- field: "some_field",
- before: 1,
- after: 2,
- });
- expect(getRevisionDescription(revision)).toEqual([]);
- });
-
- it("filters out unknown change types", () => {
- const revision = getRevision({
- before: {
- description: null,
- archived: null,
- },
- after: {
- description: "Test",
- archived: false,
- },
- });
- expect(getRevisionDescription(revision)).toBe("added a description");
- });
-
- it("filters out messages for unknown fields from a complex diff", () => {
- const revision = getRevision({
- before: {
- some_field: 1,
- name: "orders",
- },
- after: {
- some_field: 2,
- name: "Orders",
- },
- });
- expect(getRevisionDescription(revision)).toBe('renamed this to "Orders"');
- });
-
- it("prefers 'after' state to find changed fields", () => {
- const revision = getRevision({
- before: {
- display: "table",
- },
- after: {
- display: "bar",
- visualization_settings: { "some-flag": true },
- dataset_query: {},
- },
- });
- expect(getRevisionDescription(revision)).toEqual([
- "changed the display from table to bar",
- "changed the visualization settings",
- "edited the question",
- ]);
- });
-});
-
-describe("getRevisionDescription | questions", () => {
- it("handles query change revision", () => {
- const revision = getSimpleRevision({
- field: "dataset_query",
- before: { "source-table": 1 },
- after: { "source-table": 2 },
- });
-
- expect(getRevisionDescription(revision)).toBe("edited the question");
- });
-
- it("handles query change revision when before state is null", () => {
- const revision = getSimpleRevision({
- field: "dataset_query",
- before: null,
- after: { "source-table": 2 },
- });
-
- expect(getRevisionDescription(revision)).toBe("edited the question");
- });
-
- it("handles added visualization settings revision", () => {
- const revision = getSimpleRevision({
- field: "visualization_settings",
- before: null,
- after: { "table.pivot": true },
- });
-
- expect(getRevisionDescription(revision)).toBe(
- "changed the visualization settings",
- );
- });
-
- it("handles visualization settings changes revision", () => {
- const revision = getSimpleRevision({
- field: "visualization_settings",
- before: {},
- after: { "table.pivot": true },
- });
-
- expect(getRevisionDescription(revision)).toBe(
- "changed the visualization settings",
- );
- });
-
- it("handles removed visualization settings revision", () => {
- const revision = getSimpleRevision({
- field: "visualization_settings",
- before: { "table.pivot": true },
- after: null,
- });
-
- expect(getRevisionDescription(revision)).toBe(
- "changed the visualization settings",
- );
- });
-
- it("handles turning a question into a model", () => {
- const revision = getSimpleRevision({
- field: "dataset",
- before: false,
- after: true,
- });
-
- expect(getRevisionDescription(revision)).toBe("turned this into a model");
- });
-
- it("handles turning a model back into a saved question", () => {
- const revision = getSimpleRevision({
- field: "dataset",
- before: true,
- after: false,
- });
-
- expect(getRevisionDescription(revision)).toBe(
- "changed this from a model to a saved question",
- );
- });
-
- it("handles metadata changes for models", () => {
- const revision = getSimpleRevision({
- field: "result_metadata",
- before: [{ foo: "" }],
- after: [{ foo: "bar" }],
- });
-
- expect(getRevisionDescription(revision)).toBe("edited the metadata");
- });
-});
-
-describe("getRevisionDescription | dashboards", () => {
- it("handles added card revision", () => {
- const revision = getSimpleRevision({
- field: "cards",
- before: [{ id: 1 }, { id: 2 }],
- after: [{ id: 1 }, { id: 2 }, { id: 3 }],
- });
- expect(getRevisionDescription(revision)).toBe("added a card");
- });
-
- it("handles added multiple cards revision", () => {
- const revision = getSimpleRevision({
- field: "cards",
- before: [{ id: 1 }, { id: 2 }],
- after: [{ id: 1 }, { id: 2 }, { id: 3 }, { id: 4 }, { id: 5 }],
- });
- expect(getRevisionDescription(revision)).toBe("added 3 cards");
- });
-
- it("filters null card values for new card revision", () => {
- const revision = getRevision({
- before: null,
- after: {
- cards: [null, null, { id: 1 }],
- },
- });
- expect(getRevisionDescription(revision)).toBe("added a card");
- });
-
- it("handles first card added revision", () => {
- const revision = getRevision({
- before: null,
- after: {
- cards: [{ id: 1 }],
- },
- });
- expect(getRevisionDescription(revision)).toBe("added a card");
- });
-
- it("handles removed card revision", () => {
- const revision = getSimpleRevision({
- field: "cards",
- before: [{ id: 1 }, { id: 2 }],
- after: [{ id: 1 }],
- });
- expect(getRevisionDescription(revision)).toBe("removed a card");
- });
-
- it("filters null card values for removed card revision", () => {
- const revision = getSimpleRevision({
- field: "cards",
- before: [null, { id: 1 }, { id: 2 }],
- after: [null, { id: 1 }],
- });
- expect(getRevisionDescription(revision)).toBe("removed a card");
- });
-
- it("handles removed cards revision", () => {
- const revision = getSimpleRevision({
- field: "cards",
- before: [{ id: 1 }, { id: 2 }, { id: 3 }],
- after: [{ id: 1 }],
- });
- expect(getRevisionDescription(revision)).toBe("removed 2 cards");
- });
-
- it("handles all cards removed revision", () => {
- const revision = getRevision({
- before: {
- cards: [{ id: 1 }, { id: 2 }, { id: 3 }],
- },
- after: null,
- });
- expect(getRevisionDescription(revision)).toBe("removed 3 cards");
- });
-
- it("handles rearranged cards revision", () => {
- const revision = getSimpleRevision({
- field: "cards",
- before: [{ id: 1 }, { id: 2 }, { id: 3 }],
- after: [{ id: 2 }, { id: 1 }, { id: 3 }],
- });
- expect(getRevisionDescription(revision)).toBe("rearranged the cards");
- });
-
- it("handles added series revision", () => {
- const revision = getSimpleRevision({
- field: "cards",
- before: [{ series: null }],
- after: [{ series: [4] }],
- });
- expect(getRevisionDescription(revision)).toBe("added series to a question");
- });
-
- it("handles removed series revision", () => {
- const revision = getSimpleRevision({
- field: "cards",
- before: [{ series: [4] }],
- after: [{ series: null }],
- });
- expect(getRevisionDescription(revision)).toBe(
- "removed series from a question",
- );
- });
-
- it("handles modified series revision", () => {
- const revision = getSimpleRevision({
- field: "cards",
- before: [{ series: [4, 5] }],
- after: [{ series: [5, 4] }],
- });
- expect(getRevisionDescription(revision)).toBe("modified question's series");
- });
-});
-
-describe("getRevisionEvents", () => {
- const latestRevisionEvent = getRevision({
- isReversion: true,
- description: "bar",
- username: "Bar",
- });
-
- const changeEvent = getRevision({
- before: {
- description: null,
- },
- after: {
- description: "some description is now here",
- },
- });
-
- const creationEvent = getRevision({
- isCreation: true,
- description: "foo",
- });
-
- function getExpectedEvent(opts) {
- return {
- timestamp: DEFAULT_EPOCH_TIMESTAMP,
- icon: REVISION_EVENT_ICON,
- ...opts,
- };
- }
-
- const revisionEvents = [latestRevisionEvent, changeEvent, creationEvent];
-
- it("should convert a revision object into an object for use in a <Timeline /> component", () => {
- const timelineEvents = getRevisionEventsForTimeline(revisionEvents, {
- canWrite: false,
- });
-
- expect(timelineEvents).toEqual([
- getExpectedEvent({
- title: (
- <RevisionTitle
- username="Bar"
- message="reverted to an earlier revision"
- />
- ),
- titleText: "Bar reverted to an earlier revision",
- isRevertable: false,
- revision: latestRevisionEvent,
- }),
- getExpectedEvent({
- title: <RevisionTitle username="Foo" message="added a description" />,
- titleText: "Foo added a description",
- isRevertable: false,
- revision: changeEvent,
- }),
- getExpectedEvent({
- title: <RevisionTitle username="Foo" message="created this" />,
- titleText: "Foo created this",
- isRevertable: false,
- revision: creationEvent,
- }),
- ]);
- });
-
- it("should set the `isRevertable` to false when the user doesn't have write access", () => {
- const timelineEvents = getRevisionEventsForTimeline(revisionEvents, {
- canWrite: false,
- });
-
- expect(timelineEvents.every(event => event.isRevertable)).toBe(false);
- });
-
- it("should set the `isRevertable` to true on all revisions that are not the most recent revision when the user has write access", () => {
- const timelineEvents = getRevisionEventsForTimeline(revisionEvents, {
- canWrite: true,
- });
-
- expect(timelineEvents[0].isRevertable).toBe(false);
- expect(timelineEvents[1].isRevertable).toBe(true);
- });
-
- it("should drop invalid revisions", () => {
- const timelineEvents = getRevisionEventsForTimeline(
- [
- changeEvent,
- getRevision({
- before: null,
- after: null,
- }),
- ],
- { canWrite: true },
- );
- expect(timelineEvents).toEqual([
- getExpectedEvent({
- title: <RevisionTitle username="Foo" message="added a description" />,
- titleText: "Foo added a description",
- isRevertable: false,
- revision: changeEvent,
- }),
- ]);
- });
-
- it("should drop revisions with not registered fields", () => {
- const timelineEvents = getRevisionEventsForTimeline(
- [
- changeEvent,
- getRevision({
- before: {
- "dont-know-this-field": 1,
- },
- after: { "dont-know-this-field": 2 },
- }),
- ],
- { canWrite: true },
- );
- expect(timelineEvents).toEqual([
- getExpectedEvent({
- title: <RevisionTitle username="Foo" message="added a description" />,
- titleText: "Foo added a description",
- isRevertable: false,
- revision: changeEvent,
- }),
- ]);
- });
-
- it("should use 'You' instead of a username if it's current user", () => {
- const currentUser = { id: 5 };
- const event = getRevision({
- isCreation: true,
- userId: currentUser.id,
- });
- const timelineEvents = getRevisionEventsForTimeline([event], {
- currentUser,
- });
-
- expect(timelineEvents).toEqual([
- getExpectedEvent({
- title: <RevisionTitle username="You" message="created this" />,
- titleText: "You created this",
- isRevertable: false,
- revision: event,
- }),
- ]);
- });
-});
-
-describe("isValidRevision", () => {
- it("returns false if there is no diff and it's not creation or reversion action", () => {
- const revision = getRevision({
- isCreation: false,
- isReversion: false,
- before: null,
- after: null,
- });
- expect(isValidRevision(revision)).toBe(false);
- });
-
- it("returns false if diff contains only unknown fields", () => {
- const revision = getRevision({
- before: {
- not_registered_field: 1,
- },
- after: {
- not_registered_field: 2,
- },
- });
- expect(isValidRevision(revision)).toBe(false);
- });
-
- it("returns true for creation revision", () => {
- const revision = getRevision({
- isCreation: true,
- isReversion: false,
- before: null,
- after: null,
- });
- expect(isValidRevision(revision)).toBe(true);
- });
-
- it("returns true for reversion revision", () => {
- const revision = getRevision({
- isCreation: false,
- isReversion: true,
- before: null,
- after: null,
- });
- expect(isValidRevision(revision)).toBe(true);
- });
-
- it("returns true for change revision", () => {
- const revision = getSimpleRevision({
- field: "name",
- before: "orders",
- after: "Orders",
- });
- expect(isValidRevision(revision)).toBe(true);
- });
-
- it("returns true if 'before' state is null, but 'after' state is present", () => {
- const revision = getRevision({
- before: null,
- after: {
- cards: [1],
- },
- });
- expect(isValidRevision(revision)).toBe(true);
- });
-
- it("returns true if 'after' state is null, but 'before' state is present", () => {
- const revision = getRevision({
- before: {
- cards: [1],
- },
- after: null,
- });
- expect(isValidRevision(revision)).toBe(true);
- });
-});
-
-describe("getChangedFields", () => {
- it("returns a list of changed fields", () => {
- const revision = getRevision({
- before: {
- name: "Orders",
- description: null,
- },
- after: {
- name: "Orders by Month",
- description: "Hello",
- },
- });
- expect(getChangedFields(revision)).toEqual(["name", "description"]);
- });
-
- it("returns a list of changed fields if 'before' state is null", () => {
- const revision = getRevision({
- before: null,
- after: {
- cards: [1],
- },
- });
- expect(getChangedFields(revision)).toEqual(["cards"]);
- });
-
- it("returns a list of changed fields if 'after' state is null", () => {
- const revision = getRevision({
- before: {
- cards: [1],
- },
- after: null,
- });
- expect(getChangedFields(revision)).toEqual(["cards"]);
- });
-
- it("returns a list with a single changed field", () => {
- const revision = getRevision({
- before: {
- description: null,
- },
- after: {
- description: "Hello",
- },
- });
- expect(getChangedFields(revision)).toEqual(["description"]);
- });
-
- it("filters out unknown fields", () => {
- const revision = getRevision({
- before: {
- dont_know_this_field: null,
- },
- after: {
- dont_know_this_field: "Hello",
- },
- });
- expect(getChangedFields(revision)).toEqual([]);
- });
-
- it("returns empty array if diff is missing", () => {
- const revision = {
- diff: null,
- };
- expect(getChangedFields(revision)).toEqual([]);
- });
-
- it("returns empty array if 'before' and 'after' states missing", () => {
- const revision = getRevision({
- before: null,
- after: null,
- });
- expect(getChangedFields(revision)).toEqual([]);
- });
-});
diff --git a/frontend/src/metabase/plugins/index.ts b/frontend/src/metabase/plugins/index.ts
index d76318f1946..bab05eb3a81 100644
--- a/frontend/src/metabase/plugins/index.ts
+++ b/frontend/src/metabase/plugins/index.ts
@@ -16,16 +16,15 @@ import type {
Dataset,
Group,
GroupsPermissions,
+ Revision,
User,
+ UserListResult,
} from "metabase-types/api";
import type { AdminPathKey, State } from "metabase-types/store";
import type Question from "metabase-lib/Question";
import { PluginGroupManagersType } from "./types";
-// Plugin integration points. All exports must be objects or arrays so they can be mutated by plugins.
-const array = () => [];
-
// functions called when the application is started
export const PLUGIN_APP_INIT_FUCTIONS = [];
@@ -144,6 +143,14 @@ export const PLUGIN_COLLECTION_COMPONENTS = {
PluginPlaceholder as FormCollectionAuthorityLevelPicker,
};
+export type RevisionOrModerationEvent = {
+ title: string;
+ timestamp: string;
+ icon: string | { name: string; color: string } | Record<string, never>;
+ description?: string;
+ revision?: Revision;
+};
+
export const PLUGIN_MODERATION = {
isEnabled: () => false,
QuestionModerationIcon: PluginPlaceholder,
@@ -153,7 +160,11 @@ export const PLUGIN_MODERATION = {
ModerationStatusIcon: PluginPlaceholder,
getStatusIcon: (moderated_status?: string): string | IconProps | undefined =>
undefined,
- getModerationTimelineEvents: array,
+ getModerationTimelineEvents: (
+ reviews: any,
+ usersById: Record<string, UserListResult>,
+ currentUser: User | null,
+ ) => [] as RevisionOrModerationEvent[],
getMenuItems: (
question?: Question,
isModerator?: boolean,
diff --git a/frontend/src/metabase/query_builder/actions/core/core.js b/frontend/src/metabase/query_builder/actions/core/core.js
index e5c4be891a6..47878e2082e 100644
--- a/frontend/src/metabase/query_builder/actions/core/core.js
+++ b/frontend/src/metabase/query_builder/actions/core/core.js
@@ -18,6 +18,7 @@ import Databases from "metabase/entities/databases";
import { ModelIndexes } from "metabase/entities/model-indexes";
import { fetchAlertsForQuestion } from "metabase/alert/alert";
+import Revision from "metabase/entities/revisions";
import {
cardIsEquivalent,
cardQueryIsEquivalent,
@@ -293,7 +294,7 @@ export const revertToRevision = createThunkAction(
REVERT_TO_REVISION,
revision => {
return async dispatch => {
- await revision.revert();
+ await dispatch(Revision.objectActions.revert(revision));
await dispatch(reloadCard());
};
},
diff --git a/frontend/src/metabase/query_builder/components/QuestionActivityTimeline.jsx b/frontend/src/metabase/query_builder/components/QuestionActivityTimeline.jsx
deleted file mode 100644
index 58fcceb2f4b..00000000000
--- a/frontend/src/metabase/query_builder/components/QuestionActivityTimeline.jsx
+++ /dev/null
@@ -1,94 +0,0 @@
-import React, { useMemo } from "react";
-import PropTypes from "prop-types";
-import { t } from "ttag";
-import { connect } from "react-redux";
-import _ from "underscore";
-
-import { PLUGIN_MODERATION } from "metabase/plugins";
-import { getRevisionEventsForTimeline } from "metabase/lib/revisions";
-import { revertToRevision } from "metabase/query_builder/actions";
-import { getUser } from "metabase/selectors/user";
-
-import Revision from "metabase/entities/revisions";
-import User from "metabase/entities/users";
-import { Timeline, Header } from "./QuestionActivityTimeline.styled";
-
-const { getModerationTimelineEvents } = PLUGIN_MODERATION;
-
-const mapStateToProps = (state, props) => ({
- currentUser: getUser(state),
-});
-
-const mapDispatchToProps = {
- revertToRevision,
-};
-
-export default _.compose(
- User.loadList({
- loadingAndErrorWrapper: false,
- }),
- Revision.loadList({
- query: (state, props) => ({
- model_type: "card",
- model_id: props.question.id(),
- }),
- wrapped: true,
- }),
- connect(mapStateToProps, mapDispatchToProps),
-)(QuestionActivityTimeline);
-
-QuestionActivityTimeline.propTypes = {
- question: PropTypes.object.isRequired,
- revisions: PropTypes.array,
- users: PropTypes.array,
- currentUser: PropTypes.object.isRequired,
- revertToRevision: PropTypes.func.isRequired,
-};
-
-export function QuestionActivityTimeline({
- question,
- revisions,
- users,
- currentUser,
- revertToRevision,
-}) {
- const usersById = useMemo(() => _.indexBy(users, "id"), [users]);
- const canWrite = question.canWrite();
- const moderationReviews = question.getModerationReviews();
-
- const events = useMemo(() => {
- const moderationEvents = getModerationTimelineEvents(
- moderationReviews,
- usersById,
- currentUser,
- );
- const revisionEvents = getRevisionEventsForTimeline(
- revisions,
- {
- currentUser,
- canWrite,
- },
- revertToRevision,
- );
-
- return [...revisionEvents, ...moderationEvents];
- }, [
- canWrite,
- moderationReviews,
- revisions,
- usersById,
- currentUser,
- revertToRevision,
- ]);
-
- return (
- <div>
- <Header>{t`History`}</Header>
- <Timeline
- items={events}
- data-testid="saved-question-history-list"
- revertFn={revertToRevision}
- />
- </div>
- );
-}
diff --git a/frontend/src/metabase/query_builder/components/QuestionActivityTimeline.styled.jsx b/frontend/src/metabase/query_builder/components/QuestionActivityTimeline.styled.jsx
deleted file mode 100644
index bb5aeab2c1e..00000000000
--- a/frontend/src/metabase/query_builder/components/QuestionActivityTimeline.styled.jsx
+++ /dev/null
@@ -1,10 +0,0 @@
-import styled from "@emotion/styled";
-import DefaultTimeline from "metabase/components/Timeline";
-
-export const Header = styled.h3`
- margin-bottom: 1rem;
-`;
-
-export const Timeline = styled(DefaultTimeline)`
- padding-bottom: 1em;
-`;
diff --git a/frontend/src/metabase/query_builder/components/QuestionActivityTimeline.styled.tsx b/frontend/src/metabase/query_builder/components/QuestionActivityTimeline.styled.tsx
new file mode 100644
index 00000000000..9dbc535a30b
--- /dev/null
+++ b/frontend/src/metabase/query_builder/components/QuestionActivityTimeline.styled.tsx
@@ -0,0 +1,10 @@
+import styled from "@emotion/styled";
+import { Timeline as BaseTimeline } from "metabase/common/components/Timeline";
+
+export const Header = styled.h3`
+ margin-bottom: 1rem;
+`;
+
+export const Timeline = styled(BaseTimeline)`
+ padding-bottom: 1em;
+`;
diff --git a/frontend/src/metabase/query_builder/components/QuestionActivityTimeline.tsx b/frontend/src/metabase/query_builder/components/QuestionActivityTimeline.tsx
new file mode 100644
index 00000000000..9e81be9b492
--- /dev/null
+++ b/frontend/src/metabase/query_builder/components/QuestionActivityTimeline.tsx
@@ -0,0 +1,71 @@
+import React, { useMemo } from "react";
+import { t } from "ttag";
+import _ from "underscore";
+
+import { PLUGIN_MODERATION } from "metabase/plugins";
+import { revertToRevision } from "metabase/query_builder/actions";
+import { getUser } from "metabase/selectors/user";
+
+import { useDispatch, useSelector } from "metabase/lib/redux";
+import { getTimelineEvents } from "metabase/common/components/Timeline/utils";
+import { useRevisionListQuery } from "metabase/common/hooks/use-revision-list-query";
+import { useUserListQuery } from "metabase/common/hooks/use-user-list-query";
+import LoadingAndErrorWrapper from "metabase/components/LoadingAndErrorWrapper/LoadingAndErrorWrapper";
+import type Question from "metabase-lib/Question";
+
+import { Timeline, Header } from "./QuestionActivityTimeline.styled";
+
+const { getModerationTimelineEvents } = PLUGIN_MODERATION;
+
+interface QuestionActivityTimelineProps {
+ question: Question;
+}
+
+export function QuestionActivityTimeline({
+ question,
+}: QuestionActivityTimelineProps) {
+ const {
+ data: revisions,
+ isLoading,
+ error,
+ } = useRevisionListQuery({
+ query: { model_type: "card", model_id: question.id() },
+ });
+ const { data: users } = useUserListQuery();
+
+ const currentUser = useSelector(getUser);
+ const dispatch = useDispatch();
+
+ const usersById = useMemo(() => _.indexBy(users ?? [], "id"), [users]);
+ const moderationReviews = question.getModerationReviews();
+
+ const events = useMemo(() => {
+ const moderationEvents = getModerationTimelineEvents(
+ moderationReviews,
+ usersById,
+ currentUser,
+ );
+ const revisionEvents = getTimelineEvents({ revisions, currentUser });
+
+ return [...revisionEvents, ...moderationEvents].sort(
+ (a, b) =>
+ new Date(b.timestamp).getTime() - new Date(a.timestamp).getTime(),
+ );
+ }, [moderationReviews, revisions, usersById, currentUser]);
+
+ if (isLoading || error) {
+ return <LoadingAndErrorWrapper loading={isLoading} error={error} />;
+ }
+
+ return (
+ <div>
+ <Header>{t`History`}</Header>
+ <Timeline
+ events={events}
+ data-testid="saved-question-history-list"
+ revert={revision => dispatch(revertToRevision(revision))}
+ canWrite={question.canWrite()}
+ />
+ </div>
+ );
+}
diff --git a/frontend/src/metabase/query_builder/components/QuestionActivityTimeline.unit.spec.js b/frontend/src/metabase/query_builder/components/QuestionActivityTimeline.unit.spec.js
index b18975afa31..789998389c9 100644
--- a/frontend/src/metabase/query_builder/components/QuestionActivityTimeline.unit.spec.js
+++ b/frontend/src/metabase/query_builder/components/QuestionActivityTimeline.unit.spec.js
@@ -1,8 +1,15 @@
import React from "react";
-import { render, screen } from "@testing-library/react";
-import userEvent from "@testing-library/user-event";
-import { createMockUser } from "metabase-types/api/mocks";
+
+import {
+ renderWithProviders,
+ screen,
+ waitForElementToBeRemoved,
+} from "__support__/ui";
+import { createMockUser, createMockUserInfo } from "metabase-types/api/mocks";
import { QuestionActivityTimeline } from "metabase/query_builder/components/QuestionActivityTimeline";
+import { createMockRevision } from "metabase-types/api/mocks/revision";
+import { setupRevisionsEndpoints } from "__support__/server-mocks/revision";
+import { setupUsersEndpoints } from "__support__/server-mocks/user";
const REVISIONS = [
{
@@ -23,17 +30,20 @@ const REVISIONS = [
},
];
-function setup({ question }) {
- const revertToRevision = jest.fn().mockReturnValue(Promise.resolve());
- render(
+async function setup({ question }) {
+ setupRevisionsEndpoints([
+ createMockRevision(),
+ createMockRevision({ id: 2 }),
+ ]);
+ setupUsersEndpoints([createMockUserInfo()]);
+ renderWithProviders(
<QuestionActivityTimeline
question={question}
revisions={REVISIONS}
currentUser={createMockUser()}
- revertToRevision={revertToRevision}
/>,
);
- return { revertToRevision };
+ await waitForElementToBeRemoved(() => screen.queryByText("Loading..."));
}
describe("QuestionActivityTimeline", () => {
@@ -41,10 +51,11 @@ describe("QuestionActivityTimeline", () => {
const question = {
canWrite: () => false,
getModerationReviews: () => [],
+ id: () => 1,
};
- it("should not render revert action buttons", () => {
- setup({ question });
+ it("should not render revert action buttons", async () => {
+ await setup({ question });
expect(() => screen.getByTestId("question-revert-button")).toThrow();
});
});
@@ -53,17 +64,12 @@ describe("QuestionActivityTimeline", () => {
const question = {
canWrite: () => true,
getModerationReviews: () => [],
+ id: () => 1,
};
- it("should render revert action buttons", () => {
- setup({ question });
+ it("should render revert action buttons", async () => {
+ await setup({ question });
expect(screen.getByTestId("question-revert-button")).toBeInTheDocument();
});
-
- it("should call revertToRevision when revert button is clicked", () => {
- const { revertToRevision } = setup({ question });
- userEvent.click(screen.getByTestId("question-revert-button"));
- expect(revertToRevision).toHaveBeenCalled();
- });
});
});
diff --git a/frontend/src/metabase/query_builder/components/view/sidebars/QuestionInfoSidebar.tsx b/frontend/src/metabase/query_builder/components/view/sidebars/QuestionInfoSidebar.tsx
index 644ce6fd6d5..6939ff073be 100644
--- a/frontend/src/metabase/query_builder/components/view/sidebars/QuestionInfoSidebar.tsx
+++ b/frontend/src/metabase/query_builder/components/view/sidebars/QuestionInfoSidebar.tsx
@@ -9,7 +9,7 @@ import MetabaseSettings from "metabase/lib/settings";
import * as Urls from "metabase/lib/urls";
import Link from "metabase/core/components/Link";
-import QuestionActivityTimeline from "metabase/query_builder/components/QuestionActivityTimeline";
+import { QuestionActivityTimeline } from "metabase/query_builder/components/QuestionActivityTimeline";
import Question from "metabase-lib/Question";
diff --git a/frontend/test/__support__/server-mocks/revision.ts b/frontend/test/__support__/server-mocks/revision.ts
new file mode 100644
index 00000000000..490464f1724
--- /dev/null
+++ b/frontend/test/__support__/server-mocks/revision.ts
@@ -0,0 +1,6 @@
+import fetchMock from "fetch-mock";
+import { Revision } from "metabase-types/api";
+
+export function setupRevisionsEndpoints(revisions: Revision[]) {
+ fetchMock.get("path:/api/revision", revisions);
+}
diff --git a/frontend/test/__support__/server-mocks/user.ts b/frontend/test/__support__/server-mocks/user.ts
new file mode 100644
index 00000000000..0eda1fcc687
--- /dev/null
+++ b/frontend/test/__support__/server-mocks/user.ts
@@ -0,0 +1,6 @@
+import fetchMock from "fetch-mock";
+import { UserListResult } from "metabase-types/api";
+
+export function setupUsersEndpoints(users: UserListResult[]) {
+ fetchMock.get("path:/api/user", users);
+}
diff --git a/src/metabase/api/dashboard.clj b/src/metabase/api/dashboard.clj
index f55d217ea84..bc5dca62cda 100644
--- a/src/metabase/api/dashboard.clj
+++ b/src/metabase/api/dashboard.clj
@@ -621,10 +621,9 @@
(defn- delete-dashcards! [{dashboard-id :id :as _dashboard} dashcard-ids]
(when (seq dashcard-ids)
- (dashboard-card/delete-dashboard-cards!
- (t2/select DashboardCard :id [:in dashcard-ids])
- dashboard-id
- api/*current-user-id*)))
+ (let [dashboard-cards (t2/select DashboardCard :id [:in dashcard-ids])]
+ (dashboard-card/delete-dashboard-cards! dashcard-ids)
+ (events/publish-event! :dashboard-remove-cards {:id dashboard-id :actor_id api/*current-user-id* :dashcards dashboard-cards}))))
(defn- do-update-dashcards!
[dashboard current-cards new-cards]
diff --git a/src/metabase/models/dashboard.clj b/src/metabase/models/dashboard.clj
index d5fed29a9a1..23b1394190d 100644
--- a/src/metabase/models/dashboard.clj
+++ b/src/metabase/models/dashboard.clj
@@ -27,7 +27,7 @@
[metabase.public-settings :as public-settings]
[metabase.query-processor.async :as qp.async]
[metabase.util :as u]
- [metabase.util.i18n :as i18n :refer [tru]]
+ [metabase.util.i18n :as i18n :refer [deferred-tru deferred-trun tru]]
[metabase.util.log :as log]
[metabase.util.malli :as mu]
[metabase.util.malli.schema :as ms]
@@ -181,7 +181,7 @@
(defmethod revision/serialize-instance :model/Dashboard
[_model _id dashboard]
(-> dashboard
- (select-keys [:description :name :cache_ttl :auto_apply_filters])
+ (select-keys [:collection_id :description :name :cache_ttl :auto_apply_filters])
(assoc :cards (vec (for [dashboard-card (ordered-cards dashboard)]
(-> (select-keys dashboard-card [:size_x :size_y :row :col :id :card_id])
(assoc :series (mapv :id (dashboard-card/series dashboard-card)))))))))
@@ -203,7 +203,7 @@
current-card (id->current-card dashcard-id)]
(cond
;; If card is in current-cards but not serialized-cards then we need to delete it
- (not serialized-card) (dashboard-card/delete-dashboard-cards! [current-card] dashboard-id user-id)
+ (not serialized-card) (dashboard-card/delete-dashboard-cards! [(:id current-card)])
;; If card is in serialized-cards but not current-cards we need to add it
(not current-card) (dashboard-card/create-dashboard-cards! [(assoc serialized-card
@@ -215,50 +215,54 @@
serialized-dashboard)
-(defmethod revision/diff-str :model/Dashboard
- [_model dashboard1 dashboard2]
- (let [[removals changes] (diff dashboard1 dashboard2)
+(defmethod revision/diff-strings :model/Dashboard
+ [_model prev-dashboard dashboard]
+ (let [[removals changes] (diff prev-dashboard dashboard)
check-series-change (fn [idx card-changes]
(when (and (:series card-changes)
- (get-in dashboard1 [:cards idx :card_id]))
- (let [num-seriesâ‚ (count (get-in dashboard1 [:cards idx :series]))
- num-seriesâ‚‚ (count (get-in dashboard2 [:cards idx :series]))]
+ (get-in prev-dashboard [:cards idx :card_id]))
+ (let [num-seriesâ‚ (count (get-in prev-dashboard [:cards idx :series]))
+ num-seriesâ‚‚ (count (get-in dashboard [:cards idx :series]))]
(cond
(< num-seriesâ‚ num-seriesâ‚‚)
- (format "added some series to card %d" (get-in dashboard1 [:cards idx :card_id]))
+ (deferred-tru "added some series to card {0}" (get-in prev-dashboard [:cards idx :card_id]))
(> num-seriesâ‚ num-seriesâ‚‚)
- (format "removed some series from card %d" (get-in dashboard1 [:cards idx :card_id]))
+ (deferred-tru "removed some series from card {0}" (get-in prev-dashboard [:cards idx :card_id]))
:else
- (format "modified the series on card %d" (get-in dashboard1 [:cards idx :card_id]))))))]
- (-> [(when (and dashboard1 (:name changes))
- (format "renamed it from \"%s\" to \"%s\"" (:name dashboard1) (:name dashboard2)))
- (when (:description changes)
- (cond
- (nil? (:description dashboard1)) "added a description"
- (nil? (:description dashboard2)) "removed the description"
- :else (format "changed the description from \"%s\" to \"%s\""
- (:description dashboard1) (:description dashboard2))))
+ (deferred-tru "modified the series on card {0}" (get-in prev-dashboard [:cards idx :card_id]))))))]
+ (-> [(when-let [default-description (build-sentence ((get-method revision/diff-strings :default) Dashboard prev-dashboard dashboard))]
+ (cond-> default-description
+ (str/ends-with? default-description ".") (subs 0 (dec (count default-description)))))
(when (:cache_ttl changes)
(cond
- (nil? (:cache_ttl dashboard1)) "added a cache ttl"
- (nil? (:cache_ttl dashboard2)) "removed the cache ttl"
- :else (format "changed the cache ttl from \"%s\" to \"%s\""
- (:cache_ttl dashboard1) (:cache_ttl dashboard2))))
+ (nil? (:cache_ttl prev-dashboard)) (deferred-tru "added a cache ttl")
+ (nil? (:cache_ttl dashboard)) (deferred-tru "removed the cache ttl")
+ :else (deferred-tru "changed the cache ttl from \"{0}\" to \"{1}\""
+ (:cache_ttl prev-dashboard) (:cache_ttl dashboard))))
(when (or (:cards changes) (:cards removals))
- (let [num-cards1 (count (:cards dashboard1))
- num-cards2 (count (:cards dashboard2))]
+ (let [prev-card-ids (set (map :id (:cards prev-dashboard)))
+ num-prev-cards (count prev-card-ids)
+ new-card-ids (set (map :id (:cards dashboard)))
+ num-new-cards (count new-card-ids)
+ num-cards-diff (abs (- num-prev-cards num-new-cards))]
(cond
- (< num-cards1 num-cards2) "added a card"
- (> num-cards1 num-cards2) "removed a card"
- :else "rearranged the cards")))
+ (and
+ (set/subset? prev-card-ids new-card-ids)
+ (< num-prev-cards num-new-cards)) (deferred-trun "added a card" "added {0} cards" num-cards-diff)
+ (and
+ (set/subset? new-card-ids prev-card-ids)
+ (> num-prev-cards num-new-cards)) (deferred-trun "removed a card" "removed {0} cards" num-cards-diff)
+ (and (= num-prev-cards num-new-cards)
+ (= prev-card-ids new-card-ids)) (deferred-tru "rearranged the cards")
+ :else (deferred-tru "modified the cards"))))
+
(let [f (comp boolean :auto_apply_filters)]
- (when (not= (f dashboard1) (f dashboard2))
- (format "set auto apply filters to %s" (str (f dashboard2)))))]
+ (when (not= (f prev-dashboard) (f dashboard))
+ (deferred-tru "set auto apply filters to {0}" (str (f dashboard)))))]
(concat (map-indexed check-series-change (:cards changes)))
- (->> (filter identity)
- build-sentence))))
+ (->> (filter identity)))))
(defn has-tabs?
"Check if a dashboard has tabs."
diff --git a/src/metabase/models/dashboard_card.clj b/src/metabase/models/dashboard_card.clj
index b3627c5abe3..18158758110 100644
--- a/src/metabase/models/dashboard_card.clj
+++ b/src/metabase/models/dashboard_card.clj
@@ -5,7 +5,6 @@
[metabase.db :as mdb]
[metabase.db.query :as mdb.query]
[metabase.db.util :as mdb.u]
- [metabase.events :as events]
[metabase.models.card :refer [Card]]
[metabase.models.dashboard-card-series :refer [DashboardCardSeries]]
[metabase.models.interface :as mi]
@@ -231,14 +230,11 @@
(defn delete-dashboard-cards!
"Delete DashboardCards of a Dasbhoard."
- [dashboard-cards dashboard-id actor-id]
- {:pre [(coll? dashboard-cards)
- (integer? actor-id)]}
- (let [dashcard-ids (map :id dashboard-cards)]
- (t2/with-transaction [_conn]
- (t2/delete! PulseCard :dashboard_card_id [:in dashcard-ids])
- (t2/delete! DashboardCard :id [:in dashcard-ids]))
- (events/publish-event! :dashboard-remove-cards {:id dashboard-id :actor_id actor-id :dashcards dashboard-cards})))
+ [dashboard-card-ids]
+ {:pre [(coll? dashboard-card-ids)]}
+ (t2/with-transaction [_conn]
+ (t2/delete! PulseCard :dashboard_card_id [:in dashboard-card-ids])
+ (t2/delete! DashboardCard :id [:in dashboard-card-ids])))
;;; ----------------------------------------------- Link cards ----------------------------------------------------
diff --git a/src/metabase/models/revision.clj b/src/metabase/models/revision.clj
index bdf860ae714..c6318059080 100644
--- a/src/metabase/models/revision.clj
+++ b/src/metabase/models/revision.clj
@@ -3,10 +3,10 @@
[clojure.data :as data]
[metabase.db.util :as mdb.u]
[metabase.models.interface :as mi]
- [metabase.models.revision.diff :refer [diff-string]]
+ [metabase.models.revision.diff :refer [build-sentence diff-strings*]]
[metabase.models.user :refer [User]]
[metabase.util :as u]
- [metabase.util.i18n :refer [tru]]
+ [metabase.util.i18n :refer [deferred-tru tru]]
[methodical.core :as methodical]
[toucan.hydrate :refer [hydrate]]
[toucan.models :as models]
@@ -46,15 +46,16 @@
{:before before
:after after})))
-(defmulti diff-str
- "Return a string describing the difference between `object-1` and `object-2`."
+(defmulti diff-strings
+ "Return a seq of string describing the difference between `object-1` and `object-2`.
+
+ Each string in the seq should be i18n-ed."
{:arglists '([model object-1 object-2])}
mi/dispatch-on-model)
-(defmethod diff-str :default
+(defmethod diff-strings :default
[model o1 o2]
- (when-let [[before after] (data/diff o1 o2)]
- (diff-string (name model) before after)))
+ (diff-strings* (name model) o1 o2))
;;; ----------------------------------------------- Entity & Lifecycle -----------------------------------------------
@@ -92,12 +93,26 @@
;;; # Functions
+(defn- revision-changes
+ [model prev-revision revision]
+ (cond
+ (:is_creation revision) [(deferred-tru "created this")]
+ (:is_reversion revision) [(deferred-tru "reverted to an earlier version")]
+ :else (diff-strings model (:object prev-revision) (:object revision))))
+
+(defn- revision-description-info
+ [model prev-revision revision]
+ (let [changes (revision-changes model prev-revision revision)]
+ {:description (build-sentence changes)
+ ;; this is used on FE
+ :has_multiple_changes (> (count changes) 1)}))
+
(defn add-revision-details
"Add enriched revision data such as `:diff` and `:description` as well as filter out some unnecessary props."
[model revision prev-revision]
(-> revision
- (assoc :diff (diff-map model (:object prev-revision) (:object revision))
- :description (diff-str model (:object prev-revision) (:object revision)))
+ (assoc :diff (diff-map model (:object prev-revision) (:object revision)))
+ (merge (revision-description-info model prev-revision revision))
;; add revision user details
(hydrate :user)
(update :user select-keys [:id :first_name :last_name :common_name])
diff --git a/src/metabase/models/revision/diff.clj b/src/metabase/models/revision/diff.clj
index 8f2976fa43d..d688d11a534 100644
--- a/src/metabase/models/revision/diff.clj
+++ b/src/metabase/models/revision/diff.clj
@@ -1,33 +1,61 @@
(ns metabase.models.revision.diff
(:require
[clojure.core.match :refer [match]]
- [clojure.string :as str]))
+ [clojure.data :as data]
+ [metabase.util.i18n :refer [deferred-tru]]
+ [toucan2.core :as t2]))
-(defn- diff-string* [k v1 v2]
+(defn- diff-string [k v1 v2 identifier]
(match [k v1 v2]
[:name _ _]
- (format "renamed it from \"%s\" to \"%s\"" v1 v2)
+ (deferred-tru "renamed {0} from \"{1}\" to \"{2}\"" identifier v1 v2)
+
+ [:description nil _]
+ (deferred-tru "added a description")
+
+ [:description (_ :guard some?) _]
+ (deferred-tru "changed the description")
[:private true false]
- "made it public"
+ (deferred-tru "made {0} public" identifier)
[:private false true]
- "made it private"
+ (deferred-tru "made {0} private" identifier)
+
+ [:archived false true]
+ (deferred-tru "unarchived this")
+
+ [:archived true false]
+ (deferred-tru "archived this")
+
+ [:dataset false true]
+ (deferred-tru "turned this into a model")
- [:updated_at _ _]
- nil
+ [:dataset false false]
+ (deferred-tru "changed this from a model to a saved question")
+
+ [:display _ _]
+ (deferred-tru "changed the display from {0} to {1}" (name v1) (name v2))
[:result_metadata _ _]
- nil
+ (deferred-tru "edited the metadata")
[:dataset_query _ _]
- "modified the query"
+ (deferred-tru "modified the query")
+
+ [:collection_id nil (coll-id :guard int?)]
+ (deferred-tru "moved {0} to {1}" identifier (t2/select-one-fn :name 'Collection coll-id))
+
+ [:collection_id (prev-coll-id :guard int?) (coll-id :guard int?)]
+ (deferred-tru "moved {0} from {1} to {2}"
+ identifier
+ (t2/select-one-fn :name 'Collection prev-coll-id)
+ (t2/select-one-fn :name 'Collection coll-id))
[:visualization_settings _ _]
- "changed the visualization settings"
+ (deferred-tru "changed the visualization settings")
- [_ _ _]
- (format "changed %s from \"%s\" to \"%s\"" (name k) v1 v2)))
+ :else nil))
(defn build-sentence
"Join parts of a sentence together to build a compound one."
@@ -35,16 +63,26 @@
(when (seq parts)
(cond
(= (count parts) 1) (str (first parts) \.)
- (= (count parts) 2) (format "%s and %s." (first parts) (second parts))
- :else (format "%s, %s" (first parts) (build-sentence (rest parts))))))
-
-(defn diff-string
- "Create a string describing how `o1` is different from `o2`.
- The directionality of the statement should indicate that `o1` changed into `o2`."
- [t before after]
- (when before
- (let [ks (keys before)]
- (some-> (filter identity (for [k ks]
- (diff-string* k (k before) (k after))))
- build-sentence
- (str/replace-first #" it " (format " this %s " t))))))
+ (= (count parts) 2) (str (first parts) " " (deferred-tru "and") " " (second parts) \.)
+ :else (str (first parts) ", " (build-sentence (rest parts))))))
+
+(defn ^:private model-str->i18n-str
+ [model-str]
+ (case model-str
+ "Dashboard" (deferred-tru "Dashboard")
+ "Card" (deferred-tru "Card")
+ "Segment" (deferred-tru "Segment")
+ "Metric" (deferred-tru "Metric")))
+
+(defn diff-strings*
+ "Create a seq of string describing how `o1` is different from `o2`.
+ The directionality of the statement should indicate that `o1` changed into `o2`."
+ [model o1 o2]
+ (when-let [[before after] (data/diff o1 o2)]
+ (let [ks (keys (or after before))
+ model-name (model-str->i18n-str model)]
+ (filter identity
+ (map-indexed (fn [i k]
+ (diff-string k (k before) (k after)
+ (if (zero? i) (deferred-tru "this {0}" model-name) (deferred-tru "it"))))
+ ks)))))
diff --git a/src/metabase/util/i18n.clj b/src/metabase/util/i18n.clj
index 3e58869b9fe..dc0cd846aa1 100644
--- a/src/metabase/util/i18n.clj
+++ b/src/metabase/util/i18n.clj
@@ -217,7 +217,9 @@
The first argument should be the singular form; the second argument should be the plural form, and the third argument
should be `n`. `n` can be interpolated into the translated string using the `{0}` placeholder syntax, but no
- additional placeholders are supported."
+ additional placeholders are supported.
+
+ (deferred-trun \"{0} table\" \"{0} tables\" n)"
[format-string format-string-pl n]
(validate-n format-string format-string-pl)
`(UserLocalizedString. ~format-string ~[n] ~{:n n :format-string-pl format-string-pl}))
@@ -227,7 +229,9 @@
The first argument should be the singular form; the second argument should be the plural form, and the third argument
should be `n`. `n` can be interpolated into the translated string using the `{0}` placeholder syntax, but no
- additional placeholders are supported."
+ additional placeholders are supported.
+
+ (trun \"{0} table\" \"{0} tables\" n)"
[format-string format-string-pl n]
`(str* (deferred-trun ~format-string ~format-string-pl ~n)))
@@ -236,7 +240,9 @@
The first argument should be the singular form; the second argument should be the plural form, and the third argument
should be `n`. `n` can be interpolated into the translated string using the `{0}` placeholder syntax, but no
- additional placeholders are supported."
+ additional placeholders are supported.
+
+ (deferred-trsn \"{0} table\" \"{0} tables\" n)"
[format-string format-string-pl n]
(validate-n format-string format-string-pl)
`(SiteLocalizedString. ~format-string ~[n] ~{:n n :format-string-pl format-string-pl}))
@@ -246,7 +252,9 @@
The first argument should be the singular form; the second argument should be the plural form, and the third argument
should be `n`. `n` can be interpolated into the translated string using the `{0}` placeholder syntax, but no
- additional placeholders are supported."
+ additional placeholders are supported.
+
+ (trsn \"{0} table\" \"{0} tables\" n)"
[format-string format-string-pl n]
`(str* (deferred-trsn ~format-string ~format-string-pl ~n)))
diff --git a/test/metabase/api/dashboard_test.clj b/test/metabase/api/dashboard_test.clj
index e97ec452a0d..f753a132d2d 100644
--- a/test/metabase/api/dashboard_test.clj
+++ b/test/metabase/api/dashboard_test.clj
@@ -2044,25 +2044,27 @@
:card_id 123
:series [8 9]}]}
:message "updated"}]]
- (is (= [{:is_reversion false
- :is_creation false
- :message "updated"
- :user (-> (user-details (mt/fetch-user :crowberto))
- (dissoc :email :date_joined :last_login :is_superuser :is_qbnewb))
- :diff {:before {:name "b"
- :description nil
- :cards [{:series nil, :size_y 4, :size_x 4}]}
- :after {:name "c"
- :description "something"
- :cards [{:series [8 9], :size_y 3, :size_x 5}]}}
- :description "renamed it from \"b\" to \"c\", added a description, rearranged the cards and added some series to card 123."}
- {:is_reversion false
- :is_creation true
- :message nil
- :user (-> (user-details (mt/fetch-user :rasta))
- (dissoc :email :date_joined :last_login :is_superuser :is_qbnewb))
- :diff nil
- :description "added a card."}]
+ (is (= [{:is_reversion false
+ :is_creation false
+ :message "updated"
+ :user (-> (user-details (mt/fetch-user :crowberto))
+ (dissoc :email :date_joined :last_login :is_superuser :is_qbnewb))
+ :diff {:before {:name "b"
+ :description nil
+ :cards [{:series nil, :size_y 4, :size_x 4}]}
+ :after {:name "c"
+ :description "something"
+ :cards [{:series [8 9], :size_y 3, :size_x 5}]}}
+ :has_multiple_changes true
+ :description "added a description and renamed it from \"b\" to \"c\", rearranged the cards and added some series to card 123."}
+ {:is_reversion false
+ :is_creation true
+ :message nil
+ :user (-> (user-details (mt/fetch-user :rasta))
+ (dissoc :email :date_joined :last_login :is_superuser :is_qbnewb))
+ :diff nil
+ :has_multiple_changes false
+ :description "created this."}]
(doall (for [revision (mt/user-http-request :crowberto :get 200 (format "dashboard/%d/revisions" dashboard-id))]
(dissoc revision :timestamp :id))))))))
@@ -2092,41 +2094,45 @@
:description nil
:cards []}
:message "updated"}]]
- (is (= {:is_reversion true
- :is_creation false
- :message nil
- :user (-> (user-details (mt/fetch-user :crowberto))
- (dissoc :email :date_joined :last_login :is_superuser :is_qbnewb))
- :diff {:before {:name "b"}
- :after {:name "a"}}
- :description "renamed it from \"b\" to \"a\"."}
+ (is (= {:is_reversion true
+ :is_creation false
+ :message nil
+ :user (-> (user-details (mt/fetch-user :crowberto))
+ (dissoc :email :date_joined :last_login :is_superuser :is_qbnewb))
+ :diff {:before {:name "b"}
+ :after {:name "a"}}
+ :has_multiple_changes false
+ :description "reverted to an earlier version."}
(dissoc (mt/user-http-request :crowberto :post 200 (format "dashboard/%d/revert" dashboard-id)
{:revision_id revision-id})
:id :timestamp)))
- (is (= [{:is_reversion true
- :is_creation false
- :message nil
- :user (-> (user-details (mt/fetch-user :crowberto))
- (dissoc :email :date_joined :last_login :is_superuser :is_qbnewb))
- :diff {:before {:name "b"}
- :after {:name "a"}}
- :description "renamed it from \"b\" to \"a\"."}
- {:is_reversion false
- :is_creation false
- :message "updated"
- :user (-> (user-details (mt/fetch-user :crowberto))
- (dissoc :email :date_joined :last_login :is_superuser :is_qbnewb))
- :diff {:before {:name "a"}
- :after {:name "b"}}
- :description "renamed it from \"a\" to \"b\"."}
- {:is_reversion false
- :is_creation true
- :message nil
- :user (-> (user-details (mt/fetch-user :rasta))
- (dissoc :email :date_joined :last_login :is_superuser :is_qbnewb))
- :diff nil
- :description "rearranged the cards."}]
+ (is (= [{:is_reversion true
+ :is_creation false
+ :message nil
+ :user (-> (user-details (mt/fetch-user :crowberto))
+ (dissoc :email :date_joined :last_login :is_superuser :is_qbnewb))
+ :diff {:before {:name "b"}
+ :after {:name "a"}}
+ :has_multiple_changes false
+ :description "reverted to an earlier version."}
+ {:is_reversion false
+ :is_creation false
+ :message "updated"
+ :user (-> (user-details (mt/fetch-user :crowberto))
+ (dissoc :email :date_joined :last_login :is_superuser :is_qbnewb))
+ :diff {:before {:name "a"}
+ :after {:name "b"}}
+ :has_multiple_changes false
+ :description "renamed this Dashboard from \"a\" to \"b\"."}
+ {:is_reversion false
+ :is_creation true
+ :message nil
+ :user (-> (user-details (mt/fetch-user :rasta))
+ (dissoc :email :date_joined :last_login :is_superuser :is_qbnewb))
+ :diff nil
+ :has_multiple_changes false
+ :description "created this."}]
(doall (for [revision (mt/user-http-request :crowberto :get 200 (format "dashboard/%d/revisions" dashboard-id))]
(dissoc revision :timestamp :id))))))))
diff --git a/test/metabase/api/metric_test.clj b/test/metabase/api/metric_test.clj
index f924924432c..153663e34d2 100644
--- a/test/metabase/api/metric_test.clj
+++ b/test/metabase/api/metric_test.clj
@@ -269,21 +269,21 @@
:object {:name "c"
:definition {:filter [:and [:> 1 25]]}}
:message "updated"}]]
- (is (= [{:is_reversion false
- :is_creation false
- :message "updated"
- :user (-> (user-details (mt/fetch-user :crowberto))
- (dissoc :email :date_joined :last_login :is_superuser :is_qbnewb))
- :diff {:name {:before "b" :after "c"}}
- :description "renamed this Metric from \"b\" to \"c\"."}
- {:is_reversion false
- :is_creation true
- :message nil
- :user (-> (user-details (mt/fetch-user :rasta))
- (dissoc :email :date_joined :last_login :is_superuser :is_qbnewb))
- :diff {:name {:after "b"}
- :definition {:after {:filter [">" ["field" 1 nil] 25]}}}
- :description nil}]
+ (is (=? [{:is_reversion false
+ :is_creation false
+ :message "updated"
+ :user (-> (user-details (mt/fetch-user :crowberto))
+ (dissoc :email :date_joined :last_login :is_superuser :is_qbnewb))
+ :diff {:name {:before "b" :after "c"}}
+ :description "renamed this Metric from \"b\" to \"c\"."}
+ {:is_reversion false
+ :is_creation true
+ :message nil
+ :user (-> (user-details (mt/fetch-user :rasta))
+ (dissoc :email :date_joined :last_login :is_superuser :is_qbnewb))
+ :diff {:name {:after "b"}
+ :definition {:after {:filter [">" ["field" 1 nil] 25]}}}
+ :description "created this."}]
(for [revision (mt/user-http-request :rasta :get 200 (format "metric/%d/revisions" id))]
(dissoc revision :timestamp :id)))))))
@@ -351,39 +351,39 @@
:query {:filter [:= [:field 10 nil] 20]}}}
:message "updated"}]]
(testing "API response"
- (is (= {:is_reversion true
- :is_creation false
- :message nil
- :user (dissoc (user-details (mt/fetch-user :crowberto)) :email :date_joined :last_login :is_superuser :is_qbnewb)
- :diff {:name {:before "Changed Metric Name"
- :after "One Metric to rule them all, one metric to define them"}}
- :description "renamed this Metric from \"Changed Metric Name\" to \"One Metric to rule them all, one metric to define them\"."}
- (dissoc (mt/user-http-request
- :crowberto :post 200 (format "metric/%d/revert" id) {:revision_id revision-id}) :id :timestamp))))
- (testing "full list of final revisions, first one should be same as the revision returned by the endpoint"
- (is (= [{:is_reversion true
+ (is (=? {:is_reversion true
:is_creation false
:message nil
:user (dissoc (user-details (mt/fetch-user :crowberto)) :email :date_joined :last_login :is_superuser :is_qbnewb)
:diff {:name {:before "Changed Metric Name"
:after "One Metric to rule them all, one metric to define them"}}
- :description "renamed this Metric from \"Changed Metric Name\" to \"One Metric to rule them all, one metric to define them\"."}
- {:is_reversion false
- :is_creation false
- :message "updated"
- :user (dissoc (user-details (mt/fetch-user :crowberto)) :email :date_joined :last_login :is_superuser :is_qbnewb)
- :diff {:name {:after "Changed Metric Name"
- :before "One Metric to rule them all, one metric to define them"}}
- :description "renamed this Metric from \"One Metric to rule them all, one metric to define them\" to \"Changed Metric Name\"."}
- {:is_reversion false
- :is_creation true
- :message nil
- :user (dissoc (user-details (mt/fetch-user :rasta)) :email :date_joined :last_login :is_superuser :is_qbnewb)
- :diff {:name {:after "One Metric to rule them all, one metric to define them"}
- :description {:after "One metric to bring them all, and in the DataModel bind them"}
- :definition {:after {:database 123
- :query {:filter ["=" ["field" 10 nil] 20]}}}}
- :description nil}]
+ :description "reverted to an earlier version."}
+ (dissoc (mt/user-http-request
+ :crowberto :post 200 (format "metric/%d/revert" id) {:revision_id revision-id}) :id :timestamp))))
+ (testing "full list of final revisions, first one should be same as the revision returned by the endpoint"
+ (is (=? [{:is_reversion true
+ :is_creation false
+ :message nil
+ :user (dissoc (user-details (mt/fetch-user :crowberto)) :email :date_joined :last_login :is_superuser :is_qbnewb)
+ :diff {:name {:before "Changed Metric Name"
+ :after "One Metric to rule them all, one metric to define them"}}
+ :description "reverted to an earlier version."}
+ {:is_reversion false
+ :is_creation false
+ :message "updated"
+ :user (dissoc (user-details (mt/fetch-user :crowberto)) :email :date_joined :last_login :is_superuser :is_qbnewb)
+ :diff {:name {:after "Changed Metric Name"
+ :before "One Metric to rule them all, one metric to define them"}}
+ :description "renamed this Metric from \"One Metric to rule them all, one metric to define them\" to \"Changed Metric Name\"."}
+ {:is_reversion false
+ :is_creation true
+ :message nil
+ :user (dissoc (user-details (mt/fetch-user :rasta)) :email :date_joined :last_login :is_superuser :is_qbnewb)
+ :diff {:name {:after "One Metric to rule them all, one metric to define them"}
+ :description {:after "One metric to bring them all, and in the DataModel bind them"}
+ :definition {:after {:database 123
+ :query {:filter ["=" ["field" 10 nil] 20]}}}}
+ :description "created this."}]
(for [revision (mt/user-http-request
:crowberto :get 200 (format "metric/%d/revisions" id))]
(dissoc revision :timestamp :id)))))))
diff --git a/test/metabase/api/revision_test.clj b/test/metabase/api/revision_test.clj
index ecf9313f548..f789f820325 100644
--- a/test/metabase/api/revision_test.clj
+++ b/test/metabase/api/revision_test.clj
@@ -11,9 +11,10 @@
[metabase.test.fixtures :as fixtures]
[metabase.util :as u]
[toucan.util.test :as tt]
- [toucan2.core :as t2]))
+ [toucan2.core :as t2]
+ [toucan2.tools.with-temp :as t2.with-temp]))
-(use-fixtures :once (fixtures/initialize :db :test-users :web-server))
+(use-fixtures :once (fixtures/initialize :db :test-users :web-server :events))
(def ^:private rasta-revision-info
(delay
@@ -23,22 +24,21 @@
(for [revision (mt/user-http-request :rasta :get "revision" :entity entity, :id object-id)]
(dissoc revision :timestamp :id)))
-(defn- create-card-revision [card is-creation? user]
+(defn- create-card-revision [card-id is-creation? user]
(revision/push-revision!
- :object card
+ :object (t2/select-one Card :id card-id)
:entity Card
- :id (:id card)
+ :id card-id
:user-id (test.users/user->id user)
:is-creation? is-creation?))
-;;; TODO -- seems weird that this fetches the Dashboard while the Card version above does not ?
(defn- create-dashboard-revision!
"Fetch the latest version of a Dashboard and save a revision entry for it. Returns the fetched Dashboard."
- [dash is-creation? user]
+ [dash-id is-creation? user]
(revision/push-revision!
- :object (t2/select-one Dashboard :id (:id dash))
+ :object (t2/select-one Dashboard :id dash-id)
:entity Dashboard
- :id (:id dash)
+ :id dash-id
:user-id (test.users/user->id user)
:is-creation? is-creation?))
@@ -52,50 +52,55 @@
;; case with no revisions (maintains backwards compatibility with old installs before revisions)
(deftest no-revisions-test
(testing "Loading revisions, where there are no revisions, should work"
- (is (= [{:user {}, :diff nil, :description nil}]
+ (is (= [{:user {}, :diff nil, :description nil, :has_multiple_changes false}]
(tt/with-temp Card [{:keys [id]}]
(get-revisions :card id))))))
;; case with single creation revision
(deftest single-revision-test
(testing "Loading a single revision works"
- (is (= [{:is_reversion false
- :is_creation true
- :message nil
- :user @rasta-revision-info
- :diff nil
- :description nil}]
+ (is (= [{:is_reversion false
+ :is_creation true
+ :message nil
+ :user @rasta-revision-info
+ :diff nil
+ :has_multiple_changes false
+ :description "created this."}]
(tt/with-temp Card [{:keys [id] :as card}]
- (create-card-revision card true :rasta)
+ (create-card-revision (:id card) true :rasta)
(get-revisions :card id))))))
;; case with multiple revisions, including reversion
(deftest multiple-revisions-with-reversion-test
(testing "Creating multiple revisions, with a reversion, works"
(tt/with-temp Card [{:keys [id name], :as card}]
- (is (= [{:is_reversion true
- :is_creation false
- :message "because i wanted to"
- :user @rasta-revision-info
- :diff {:before {:name "something else"}
- :after {:name name}}
- :description (format "renamed this Card from \"something else\" to \"%s\"." name)}
- {:is_reversion false
- :is_creation false
- :message nil
- :user @rasta-revision-info
- :diff {:before {:name name}
- :after {:name "something else"}}
- :description (format "renamed this Card from \"%s\" to \"something else\"." name)}
- {:is_reversion false
- :is_creation true
- :message nil
- :user @rasta-revision-info
- :diff nil
- :description nil}]
+ (is (= [{:is_reversion true
+ :is_creation false
+ :message "because i wanted to"
+ :user @rasta-revision-info
+ :diff {:before {:name "something else"}
+ :after {:name name}}
+ :description "reverted to an earlier version."
+ :has_multiple_changes false}
+ {:is_reversion false
+ :is_creation false
+ :message nil
+ :user @rasta-revision-info
+ :diff {:before {:name name}
+ :after {:name "something else"}}
+ :description (format "renamed this Card from \"%s\" to \"something else\"." name)
+ :has_multiple_changes false}
+ {:is_reversion false
+ :is_creation true
+ :message nil
+ :user @rasta-revision-info
+ :diff nil
+ :description "created this."
+ :has_multiple_changes false}]
(do
- (create-card-revision card true :rasta)
- (create-card-revision (assoc card :name "something else") false :rasta)
+ (create-card-revision (:id card) true :rasta)
+ (t2/update! Card {:name "something else"})
+ (create-card-revision (:id card) false :rasta)
(t2/insert! Revision
:model "Card"
:model_id id
@@ -117,7 +122,7 @@
(tt/with-temp* [Dashboard [{:keys [id] :as dash}]
Card [{card-id :id, :as card}]]
(is (=? {:id id}
- (create-dashboard-revision! dash true :rasta)))
+ (create-dashboard-revision! (:id dash) true :rasta)))
(let [dashcard (first (t2/insert-returning-instances! DashboardCard
:dashboard_id id
:card_id (:id card)
@@ -126,44 +131,48 @@
:row 0
:col 0))]
(is (=? {:id id}
- (create-dashboard-revision! dash false :rasta)))
+ (create-dashboard-revision! (:id dash) false :rasta)))
(is (pos? (t2/delete! (t2/table-name DashboardCard) :id (:id dashcard)))))
(is (=? {:id id}
- (create-dashboard-revision! dash false :rasta)))
+ (create-dashboard-revision! (:id dash) false :rasta)))
(testing "Revert to the previous revision, allowed because rasta has permissions on parent collection"
(let [[_ {previous-revision-id :id}] (revision/revisions Dashboard id)]
(is (=? {:id int?
- :description "added a card."}
+ :description "reverted to an earlier version."}
(mt/user-http-request :rasta :post 200 "revision/revert" {:entity :dashboard
:id id
:revision_id previous-revision-id})))))
- (is (= [{:is_reversion true
- :is_creation false
- :message nil
- :user @rasta-revision-info
- :diff {:before {:cards nil}
- :after {:cards [{:size_x 4, :size_y 4, :row 0, :col 0, :card_id card-id, :series []}]}}
- :description "added a card."}
- {:is_reversion false
- :is_creation false
- :message nil
- :user @rasta-revision-info
- :diff {:before {:cards [{:size_x 4, :size_y 4, :row 0, :col 0, :card_id card-id, :series []}]}
- :after {:cards nil}}
- :description "removed a card."}
- {:is_reversion false
- :is_creation false
- :message nil
- :user @rasta-revision-info
- :diff {:before {:cards nil}
- :after {:cards [{:size_x 4, :size_y 4, :row 0, :col 0, :card_id card-id, :series []}]}}
- :description "added a card."}
- {:is_reversion false
- :is_creation true
- :message nil
- :user @rasta-revision-info
- :diff nil
- :description "rearranged the cards and set auto apply filters to true."}]
+ (is (= [{:is_reversion true
+ :is_creation false
+ :message nil
+ :user @rasta-revision-info
+ :diff {:before {:cards nil}
+ :after {:cards [{:size_x 4, :size_y 4, :row 0, :col 0, :card_id card-id, :series []}]}}
+ :has_multiple_changes false
+ :description "reverted to an earlier version."}
+ {:is_reversion false
+ :is_creation false
+ :message nil
+ :user @rasta-revision-info
+ :diff {:before {:cards [{:size_x 4, :size_y 4, :row 0, :col 0, :card_id card-id, :series []}]}
+ :after {:cards nil}}
+ :has_multiple_changes false
+ :description "removed a card."}
+ {:is_reversion false
+ :is_creation false
+ :message nil
+ :user @rasta-revision-info
+ :diff {:before {:cards nil}
+ :after {:cards [{:size_x 4, :size_y 4, :row 0, :col 0, :card_id card-id, :series []}]}}
+ :has_multiple_changes false
+ :description "added a card."}
+ {:is_reversion false
+ :is_creation true
+ :message nil
+ :user @rasta-revision-info
+ :diff nil
+ :has_multiple_changes false
+ :description "created this."}]
(->> (get-revisions :dashboard id)
(mapv (fn [rev]
(if-not (:diff rev)
@@ -177,10 +186,10 @@
(mt/with-non-admin-groups-no-root-collection-perms
(mt/with-temp* [Collection [collection {:name "Personal collection"}]
Dashboard [dashboard {:collection_id (u/the-id collection) :name "Personal dashboard"}]]
- (create-dashboard-revision! dashboard true :crowberto)
+ (create-dashboard-revision! (:id dashboard) true :crowberto)
;; update so that the revision is accepted
(t2/update! Dashboard :id (:id dashboard) {:name "Personal dashboard edited"})
- (create-dashboard-revision! dashboard false :crowberto)
+ (create-dashboard-revision! (:id dashboard) false :crowberto)
(let [dashboard-id (u/the-id dashboard)
[_ {prev-rev-id :id}] (revision/revisions Dashboard dashboard-id)
update-req {:entity :dashboard, :id dashboard-id, :revision_id prev-rev-id}]
@@ -189,3 +198,198 @@
;; with-non-admin-groups-no-root-collection-perms wrapper)
(is (= "You don't have permissions to do that."
(mt/user-http-request :rasta :post "revision/revert" update-req))))))))
+
+(deftest dashboard-revision-description-test
+ (testing "revision description for dashboard are generated correctly"
+ (t2.with-temp/with-temp
+ [Collection {coll-id :id} {:name "New Collection"}
+ Card {card-id-1 :id} {:name "Card 1"}
+ Card {card-id-2 :id} {:name "Card 2"}
+ Dashboard {dashboard-id :id} {:name "A dashboard"}]
+ ;; 0. create the dashboard
+ (create-dashboard-revision! dashboard-id true :crowberto)
+
+ ;; 1. rename
+ (t2/update! Dashboard :id dashboard-id {:name "New name"})
+ (create-dashboard-revision! dashboard-id false :crowberto)
+
+ ;; 2. add description
+ (t2/update! Dashboard :id dashboard-id {:description "A beautiful dashboard"})
+ (create-dashboard-revision! dashboard-id false :crowberto)
+
+ ;; 3. add 2 cards
+ (let [dashcard-ids (t2/insert-returning-pks! DashboardCard [{:dashboard_id dashboard-id
+ :card_id card-id-1
+ :size_x 4
+ :size_y 4
+ :col 1
+ :row 1}
+ {:dashboard_id dashboard-id
+ :card_id card-id-2
+ :size_x 4
+ :size_y 4
+ :col 1
+ :row 1}])]
+ (create-dashboard-revision! dashboard-id false :crowberto)
+
+ ;; 4. remove 1 card
+ (t2/delete! DashboardCard :id (first dashcard-ids))
+ (create-dashboard-revision! dashboard-id false :crowberto)
+
+ ;; 5. arrange cards
+ (t2/update! DashboardCard :id (second dashcard-ids) {:col 2
+ :row 2})
+ (create-dashboard-revision! dashboard-id false :crowberto))
+
+ ;; 6. Move to a new collection
+ (t2/update! Dashboard :id dashboard-id {:collection_id coll-id})
+ (create-dashboard-revision! dashboard-id false :crowberto)
+
+ ;; 7. revert to an earlier revision
+ (let [earlier-revision-id (t2/select-one-pk Revision :model "Dashboard" :model_id dashboard-id {:order-by [[:timestamp :desc]]})]
+ (revision/revert! :entity Dashboard :id dashboard-id :user-id (mt/user->id :crowberto) :revision-id earlier-revision-id))
+
+ (is (= [{:description "reverted to an earlier version."
+ :has_multiple_changes false}
+ {:description "moved this Dashboard to New Collection.",
+ :has_multiple_changes false}
+ {:description "rearranged the cards."
+ :has_multiple_changes false}
+ {:description "removed a card."
+ :has_multiple_changes false}
+ {:description "added 2 cards."
+ :has_multiple_changes false}
+ {:description "added a description."
+ :has_multiple_changes false}
+ {:description "renamed this Dashboard from \"A dashboard\" to \"New name\"."
+ :has_multiple_changes false}
+ {:description "created this."
+ :has_multiple_changes false}]
+ (map #(select-keys % [:description :has_multiple_changes])
+ (mt/user-http-request :crowberto :get 200 "revision" :entity "dashboard" :id dashboard-id)))))))
+
+
+(deftest card-revision-description-test
+ (testing "revision description for card are generated correctly"
+ (t2.with-temp/with-temp
+ [Collection {coll-id :id} {:name "New Collection"}
+ Card {card-id :id} {:name "A card"
+ :display "table"
+ :dataset_query (mt/mbql-query venues)
+ :visualization_settings {}}]
+ ;; 0. create the card
+ (create-card-revision card-id true :crowberto)
+
+ ;; 1. rename
+ (t2/update! Card :id card-id {:name "New name"})
+ (create-card-revision card-id false :crowberto)
+
+ ;; 2. turn to a model
+ (t2/update! Card :id card-id {:dataset true})
+ (create-card-revision card-id false :crowberto)
+
+ ;; 3. edit query and metadata
+ (t2/update! Card :id card-id {:dataset_query (mt/mbql-query venues {:aggregation [[:count]]})
+ :display "scalar"})
+ (create-card-revision card-id false :crowberto)
+
+ ;; 4. add description
+ (t2/update! Card :id card-id {:description "meaningful number"})
+ (create-card-revision card-id false :crowberto)
+
+
+ ;; 5. change collection
+ (t2/update! Card :id card-id {:collection_id coll-id})
+ (create-card-revision card-id false :crowberto)
+
+ ;; 6. revert to an earlier revision
+ (let [earlier-revision-id (t2/select-one-pk Revision :model "Card" :model_id card-id {:order-by [[:timestamp :desc]]})]
+ (revision/revert! :entity Card :id card-id :user-id (mt/user->id :crowberto) :revision-id earlier-revision-id))
+
+ (is (= [{:description "reverted to an earlier version.",
+ :has_multiple_changes false}
+ {:description "moved this Card to New Collection.",
+ :has_multiple_changes false}
+ {:description "added a description."
+ :has_multiple_changes false}
+ {:description "changed the display from table to scalar, modified the query and edited the metadata."
+ :has_multiple_changes true}
+ {:description "turned this into a model and edited the metadata."
+ :has_multiple_changes true}
+ {:description "renamed this Card from \"A card\" to \"New name\"."
+ :has_multiple_changes false}
+ {:description "created this."
+ :has_multiple_changes false}]
+ (map #(select-keys % [:description :has_multiple_changes])
+ (mt/user-http-request :crowberto :get 200 "revision" :entity "card" :id card-id)))))))
+
+(deftest revision-descriptions-are-i18ned-test
+ (mt/with-mock-i18n-bundles {"fr" {:messages {"created this" "créé ceci"
+ "added a description" "ajouté une description"
+ "renamed {0} from \"{1}\" to \"{2}\"" "renommé {0} de {1} à {2}"
+ "this {0}" "ce {0}"
+ "edited this." "édité ceci."
+ "and" "et"
+ "Card" "Carte"
+ "reverted to an earlier version" "est revenu à une version antérieure"}}}
+ (mt/with-temporary-setting-values [site-locale "fr"]
+ (testing "revisions description are translated"
+ (t2.with-temp/with-temp
+ [Card {card-id :id} {:name "A card"
+ :display "table"
+ :dataset_query (mt/mbql-query venues)
+ :visualization_settings {}}]
+ ;; 0. create the card
+ (create-card-revision card-id true :crowberto)
+
+ ;; 1. rename
+ (t2/update! Card :id card-id {:description "meaningful number"
+ :name "New name"})
+ (create-card-revision card-id false :crowberto)
+
+
+ ;; 2. revert to an earlier revision
+ (let [earlier-revision-id (t2/select-one-pk Revision :model "Card" :model_id card-id {:order-by [[:timestamp :desc]]})]
+ (revision/revert! :entity Card :id card-id :user-id (mt/user->id :crowberto) :revision-id earlier-revision-id))
+
+ (is (= [{:description "est revenu à une version antérieure."
+ :has_multiple_changes false}
+ {:description "renommé ce Carte de A card à New name et ajouté une description."
+ :has_multiple_changes true}
+ {:description "créé ceci."
+ :has_multiple_changes false}]
+ (map #(select-keys % [:description :has_multiple_changes])
+ (mt/user-http-request :crowberto :get 200 "revision" :entity "card" :id card-id))))
+ (t2/delete! :model/Card :id card-id))))))
+
+(deftest revert-does-not-create-new-revision
+ (testing "revert a dashboard that previously added cards should not recreate duplicate revisions(#30869)"
+ (t2.with-temp/with-temp
+ [Dashboard {dashboard-id :id} {:name "A dashboard"}]
+ ;; 0. create the dashboard
+ (create-dashboard-revision! dashboard-id true :crowberto)
+
+ ;; 1. add 2 cards
+ (t2/insert-returning-pks! DashboardCard [{:dashboard_id dashboard-id
+ :size_x 4
+ :size_y 4
+ :col 1
+ :row 1}
+ {:dashboard_id dashboard-id
+ :size_x 4
+ :size_y 4
+ :col 1
+ :row 1}])
+ (create-dashboard-revision! dashboard-id false :crowberto)
+
+ (let [earlier-revision-id (t2/select-one-pk Revision :model "Dashboard" :model_id dashboard-id {:order-by [[:timestamp :desc]]})]
+ (revision/revert! :entity Dashboard :id dashboard-id :user-id (mt/user->id :crowberto) :revision-id earlier-revision-id))
+
+ (is (= [{:description "reverted to an earlier version."
+ :has_multiple_changes false}
+ {:description "added 2 cards."
+ :has_multiple_changes false}
+ {:description "created this."
+ :has_multiple_changes false}]
+ (map #(select-keys % [:description :has_multiple_changes])
+ (mt/user-http-request :crowberto :get 200 "revision" :entity "dashboard" :id dashboard-id)))))))
diff --git a/test/metabase/api/segment_test.clj b/test/metabase/api/segment_test.clj
index 1a064fe98cd..3b4debc0ba3 100644
--- a/test/metabase/api/segment_test.clj
+++ b/test/metabase/api/segment_test.clj
@@ -293,21 +293,21 @@
:object {:name "c"
:definition {:filter [:and [:> 1 25]]}}
:message "updated"}]]
- (is (= [{:is_reversion false
- :is_creation false
- :message "updated"
- :user (-> (user-details (mt/fetch-user :crowberto))
- (dissoc :email :date_joined :last_login :is_superuser :is_qbnewb))
- :diff {:name {:before "b" :after "c"}}
- :description "renamed this Segment from \"b\" to \"c\"."}
- {:is_reversion false
- :is_creation true
- :message nil
- :user (-> (user-details (mt/fetch-user :rasta))
- (dissoc :email :date_joined :last_login :is_superuser :is_qbnewb))
- :diff {:name {:after "b"}
- :definition {:after {:filter [">" ["field" 1 nil] 25]}}}
- :description nil}]
+ (is (=? [{:is_reversion false
+ :is_creation false
+ :message "updated"
+ :user (-> (user-details (mt/fetch-user :crowberto))
+ (dissoc :email :date_joined :last_login :is_superuser :is_qbnewb))
+ :diff {:name {:before "b" :after "c"}}
+ :description "renamed this Segment from \"b\" to \"c\"."}
+ {:is_reversion false
+ :is_creation true
+ :message nil
+ :user (-> (user-details (mt/fetch-user :rasta))
+ (dissoc :email :date_joined :last_login :is_superuser :is_qbnewb))
+ :diff {:name {:after "b"}
+ :definition {:after {:filter [">" ["field" 1 nil] 25]}}}
+ :description "created this."}]
(for [revision (mt/user-http-request :rasta :get 200 (format "segment/%d/revisions" id))]
(dissoc revision :timestamp :id)))))))
@@ -365,43 +365,43 @@
:definition {:filter [:= [:field 2 nil] "cans"]}}
:message "updated"}]]
(testing "the api response"
- (is (= {:is_reversion true
- :is_creation false
- :message nil
- :user (-> (user-details (mt/fetch-user :crowberto))
- (dissoc :email :date_joined :last_login :is_superuser :is_qbnewb))
- :diff {:name {:before "Changed Segment Name"
- :after "One Segment to rule them all, one segment to define them"}}
- :description "renamed this Segment from \"Changed Segment Name\" to \"One Segment to rule them all, one segment to define them\"."}
- (-> (mt/user-http-request :crowberto :post 200 (format "segment/%d/revert" id) {:revision_id revision-id})
- (dissoc :id :timestamp)))))
-
- (testing "full list of final revisions, first one should be same as the revision returned by the endpoint"
- (is (= [{:is_reversion true
+ (is (=? {:is_reversion true
:is_creation false
:message nil
:user (-> (user-details (mt/fetch-user :crowberto))
(dissoc :email :date_joined :last_login :is_superuser :is_qbnewb))
:diff {:name {:before "Changed Segment Name"
:after "One Segment to rule them all, one segment to define them"}}
- :description "renamed this Segment from \"Changed Segment Name\" to \"One Segment to rule them all, one segment to define them\"."}
- {:is_reversion false
- :is_creation false
- :message "updated"
- :user (-> (user-details (mt/fetch-user :crowberto))
- (dissoc :email :date_joined :last_login :is_superuser :is_qbnewb))
- :diff {:name {:after "Changed Segment Name"
- :before "One Segment to rule them all, one segment to define them"}}
- :description "renamed this Segment from \"One Segment to rule them all, one segment to define them\" to \"Changed Segment Name\"."}
- {:is_reversion false
- :is_creation true
- :message nil
- :user (-> (user-details (mt/fetch-user :rasta))
- (dissoc :email :date_joined :last_login :is_superuser :is_qbnewb))
- :diff {:name {:after "One Segment to rule them all, one segment to define them"}
- :description {:after "One segment to bring them all, and in the DataModel bind them"}
- :definition {:after {:filter ["=" ["field" 2 nil] "cans"]}}}
- :description nil}]
+ :description "reverted to an earlier version."}
+ (-> (mt/user-http-request :crowberto :post 200 (format "segment/%d/revert" id) {:revision_id revision-id})
+ (dissoc :id :timestamp)))))
+
+ (testing "full list of final revisions, first one should be same as the revision returned by the endpoint"
+ (is (=? [{:is_reversion true
+ :is_creation false
+ :message nil
+ :user (-> (user-details (mt/fetch-user :crowberto))
+ (dissoc :email :date_joined :last_login :is_superuser :is_qbnewb))
+ :diff {:name {:before "Changed Segment Name"
+ :after "One Segment to rule them all, one segment to define them"}}
+ :description "reverted to an earlier version."}
+ {:is_reversion false
+ :is_creation false
+ :message "updated"
+ :user (-> (user-details (mt/fetch-user :crowberto))
+ (dissoc :email :date_joined :last_login :is_superuser :is_qbnewb))
+ :diff {:name {:after "Changed Segment Name"
+ :before "One Segment to rule them all, one segment to define them"}}
+ :description "renamed this Segment from \"One Segment to rule them all, one segment to define them\" to \"Changed Segment Name\"."}
+ {:is_reversion false
+ :is_creation true
+ :message nil
+ :user (-> (user-details (mt/fetch-user :rasta))
+ (dissoc :email :date_joined :last_login :is_superuser :is_qbnewb))
+ :diff {:name {:after "One Segment to rule them all, one segment to define them"}
+ :description {:after "One segment to bring them all, and in the DataModel bind them"}
+ :definition {:after {:filter ["=" ["field" 2 nil] "cans"]}}}
+ :description "created this."}]
(for [revision (mt/user-http-request :crowberto :get 200 (format "segment/%d/revisions" id))]
(dissoc revision :timestamp :id))))))))
diff --git a/test/metabase/api/user_test.clj b/test/metabase/api/user_test.clj
index 7339f0919a1..965e6ccd900 100644
--- a/test/metabase/api/user_test.clj
+++ b/test/metabase/api/user_test.clj
@@ -270,13 +270,12 @@
:group_ids [(u/the-id (perms-group/all-users))]
:personal_collection_id true
:custom_homepage nil
- :has_question_and_dashboard false
:is_installer (= 1 (mt/user->id :rasta))
:has_invited_second_user (= 1 (mt/user->id :rasta))})
(dissoc :is_qbnewb :last_login))
(-> (mt/user-http-request :rasta :get 200 "user/current")
mt/boolean-ids-and-timestamps
- (dissoc :is_qbnewb :last_login))))))
+ (dissoc :is_qbnewb :last_login :has_question_and_dashboard))))))
(testing "check that `has_question_and_dashboard` is `true`."
(mt/with-temp* [Dashboard [_ {:name "dash1" :creator_id (mt/user->id :rasta)}]
Card [_ {:name "card1" :display "table" :creator_id (mt/user->id :rasta)}]]
diff --git a/test/metabase/events/revision_test.clj b/test/metabase/events/revision_test.clj
index e4453c0f519..d7e13e30dcf 100644
--- a/test/metabase/events/revision_test.clj
+++ b/test/metabase/events/revision_test.clj
@@ -44,7 +44,8 @@
:visualization_settings {}})
(defn- dashboard->revision-object [dashboard]
- {:description nil
+ {:collection_id (:collection_id dashboard)
+ :description nil
:cache_ttl nil
:auto_apply_filters true
:name (:name dashboard)})
diff --git a/test/metabase/models/dashboard_test.clj b/test/metabase/models/dashboard_test.clj
index 5e835a4aef7..6ffe1f91abd 100644
--- a/test/metabase/models/dashboard_test.clj
+++ b/test/metabase/models/dashboard_test.clj
@@ -11,6 +11,7 @@
[metabase.models.interface :as mi]
[metabase.models.permissions :as perms]
[metabase.models.revision :as revision]
+ [metabase.models.revision.diff :refer [build-sentence]]
[metabase.models.serialization :as serdes]
[metabase.models.user :as user]
[metabase.test :as mt]
@@ -18,7 +19,8 @@
[metabase.test.util :as tu]
[metabase.util :as u]
[toucan.util.test :as tt]
- [toucan2.core :as t2])
+ [toucan2.core :as t2]
+ [toucan2.tools.with-temp :as t2.with-temp])
(:import
(java.time LocalDateTime)))
@@ -36,6 +38,7 @@
DashboardCardSeries [_ {:dashboardcard_id dashcard-id, :card_id series-id-2, :position 1}]]
(is (= {:name "Test Dashboard"
:auto_apply_filters true
+ :collection_id nil
:description nil
:cache_ttl nil
:cards [{:size_x 4
@@ -55,77 +58,139 @@
(deftest diff-dashboards-str-test
- (is (= "renamed it from \"Diff Test\" to \"Diff Test Changed\" and added a description."
- (revision/diff-str
- Dashboard
- {:name "Diff Test"
- :description nil
- :cards []}
- {:name "Diff Test Changed"
- :description "foobar"
- :cards []})))
+ (is (= "added a description and renamed it from \"Diff Test\" to \"Diff Test Changed\"."
+ (build-sentence
+ (revision/diff-strings
+ Dashboard
+ {:name "Diff Test"
+ :description nil
+ :cards []}
+ {:name "Diff Test Changed"
+ :description "foobar"
+ :cards []}))))
(is (= "added a card."
- (revision/diff-str
- Dashboard
- {:name "Diff Test"
- :description nil
- :cards []}
- {:name "Diff Test"
- :description nil
- :cards [{:size_x 4
- :size_y 4
- :row 0
- :col 0
- :id 1
- :card_id 1
- :series []}]})))
+ (build-sentence
+ (revision/diff-strings
+ Dashboard
+ {:name "Diff Test"
+ :description nil
+ :cards []}
+ {:name "Diff Test"
+ :description nil
+ :cards [{:size_x 4
+ :size_y 4
+ :row 0
+ :col 0
+ :id 1
+ :card_id 1
+ :series []}]}))))
(is (= "set auto apply filters to false."
- (revision/diff-str
- Dashboard
- {:name "Diff Test"
- :auto_apply_filters true}
- {:name "Diff Test"
- :auto_apply_filters false})))
-
- (is (= "changed the cache ttl from \"333\" to \"1227\", rearranged the cards, modified the series on card 1 and added some series to card 2."
- (revision/diff-str
- Dashboard
- {:name "Diff Test"
- :description nil
- :cache_ttl 333
- :cards [{:size_x 4
- :size_y 4
- :row 0
- :col 0
- :id 1
- :card_id 1
- :series [5 6]}
- {:size_x 4
- :size_y 4
- :row 0
- :col 0
- :id 2
- :card_id 2
- :series []}]}
- {:name "Diff Test"
- :description nil
- :cache_ttl 1227
- :cards [{:size_x 4
- :size_y 4
- :row 0
- :col 0
- :id 1
- :card_id 1
- :series [4 5]}
- {:size_x 4
- :size_y 4
- :row 2
- :col 0
- :id 2
- :card_id 2
- :series [3 4 5]}]}))))
+ (build-sentence
+ (revision/diff-strings
+ Dashboard
+ {:name "Diff Test"
+ :auto_apply_filters true}
+ {:name "Diff Test"
+ :auto_apply_filters false}))))
+
+ (is (= "changed the cache ttl from \"333\" to \"1,227\", rearranged the cards, modified the series on card 1 and added some series to card 2."
+ (build-sentence
+ (revision/diff-strings
+ Dashboard
+ {:name "Diff Test"
+ :description nil
+ :cache_ttl 333
+ :cards [{:size_x 4
+ :size_y 4
+ :row 0
+ :col 0
+ :id 1
+ :card_id 1
+ :series [5 6]}
+ {:size_x 4
+ :size_y 4
+ :row 0
+ :col 0
+ :id 2
+ :card_id 2
+ :series []}]}
+ {:name "Diff Test"
+ :description nil
+ :cache_ttl 1227
+ :cards [{:size_x 4
+ :size_y 4
+ :row 0
+ :col 0
+ :id 1
+ :card_id 1
+ :series [4 5]}
+ {:size_x 4
+ :size_y 4
+ :row 2
+ :col 0
+ :id 2
+ :card_id 2
+ :series [3 4 5]}]}))))
+
+ (is (= "added a card."
+ (build-sentence
+ (revision/diff-strings
+ Dashboard
+ {:cards [{:id 1} {:id 2}]}
+ {:cards [{:id 1} {:id 2} {:id 3}]}))))
+
+ (is (= "removed a card."
+ (build-sentence
+ (revision/diff-strings
+ Dashboard
+ {:cards [{:id 1} {:id 2}]}
+ {:cards [{:id 1}]}))))
+
+ (is (= "rearranged the cards."
+ (build-sentence
+ (revision/diff-strings
+ Dashboard
+ {:cards [{:id 1 :row 0} {:id 2 :row 1}]}
+ {:cards [{:id 1 :row 1} {:id 2 :row 2}]}))))
+
+ (is (= "modified the cards."
+ (build-sentence
+ (revision/diff-strings
+ Dashboard
+ {:cards [{:id 1} {:id 2}]}
+ {:cards [{:id 1} {:id 3}]}))))
+
+ (is (= "renamed it from \"Apple\" to \"Next\" and modified the cards."
+ (build-sentence
+ (revision/diff-strings
+ Dashboard
+ {:name "Apple"
+ :cards [{:id 1} {:id 2}]}
+ {:name "Next"
+ :cards [{:id 1} {:id 3}]}))))
+ (t2.with-temp/with-temp
+ [Collection {coll-id :id} {:name "New collection"}]
+ (is (= "moved this Dashboard to New collection."
+ (build-sentence
+ (revision/diff-strings
+ Dashboard
+ {:name "Apple"}
+ {:name "Apple"
+ :collection_id coll-id})))))
+ (t2.with-temp/with-temp
+ [Collection {coll-id-1 :id} {:name "Old collection"}
+ Collection {coll-id-2 :id} {:name "New collection"}]
+ (is (= "moved this Dashboard from Old collection to New collection."
+ (build-sentence
+ (revision/diff-strings
+ Dashboard
+ {:name "Apple"
+ :collection_id coll-id-1}
+ {:name "Apple"
+ :collection_id coll-id-2}))))))
+
(deftest revert-dashboard!-test
(tt/with-temp* [Dashboard [{dashboard-id :id, :as dashboard} {:name "Test Dashboard"}]
@@ -143,6 +208,7 @@
empty-dashboard {:name "Revert Test"
:description "something"
:auto_apply_filters true
+ :collection_id nil
:cache_ttl nil
:cards []}
serialized-dashboard (revision/serialize-instance Dashboard (:id dashboard) dashboard)]
@@ -151,6 +217,7 @@
:description nil
:cache_ttl nil
:auto_apply_filters true
+ :collection_id nil
:cards [{:size_x 4
:size_y 4
:row 0
@@ -160,7 +227,7 @@
:series true}]}
(update serialized-dashboard :cards check-ids))))
(testing "delete the dashcard and modify the dash attributes"
- (dashboard-card/delete-dashboard-cards! [dashboard-card] dashboard-id (test.users/user->id :rasta))
+ (dashboard-card/delete-dashboard-cards! [(:id dashboard-card)])
(t2/update! Dashboard dashboard-id
{:name "Revert Test"
:auto_apply_filters false
@@ -175,6 +242,7 @@
:description nil
:cache_ttl nil
:auto_apply_filters true
+ :collection_id nil
:cards [{:size_x 4
:size_y 4
:row 0
diff --git a/test/metabase/models/revision/diff_test.clj b/test/metabase/models/revision/diff_test.clj
deleted file mode 100644
index 88430a02cab..00000000000
--- a/test/metabase/models/revision/diff_test.clj
+++ /dev/null
@@ -1,37 +0,0 @@
-(ns metabase.models.revision.diff-test
- (:require
- [clojure.data :as data]
- [clojure.test :refer :all]
- [metabase.models.revision.diff :as diff]))
-
-(deftest rename-test
- (testing (str "Check that pattern matching allows specialization and that string only reflects the keys that have "
- "changed")
- (let [[before after] (data/diff {:name "Tips by State", :private false}
- {:name "Spots by State", :private false})]
- (is (= "renamed this card from \"Tips by State\" to \"Spots by State\"."
- (diff/diff-string "card" before after))))))
-
-(deftest make-private-test
- (let [[before after] (data/diff {:name "Spots by State", :private false}
- {:name "Spots by State", :private true})]
- (is (= "made this card private."
- (diff/diff-string "card" before after)))))
-
-(deftest change-priority-test
- (let [[before after] (data/diff {:priority "Important"}
- {:priority "Regular"})]
- (is (= "changed priority from \"Important\" to \"Regular\"."
- (diff/diff-string "card" before after)))))
-
-(deftest multiple-changes-test
- (let [[before after] (data/diff {:name "Tips by State", :private false}
- {:name "Spots by State", :private true})]
- (is (= "made this card private and renamed it from \"Tips by State\" to \"Spots by State\"."
- (diff/diff-string "card" before after))))
-
- (let [[before after] (data/diff {:name "Tips by State", :private false, :priority "Important"}
- {:name "Spots by State", :private true, :priority "Regular"})]
- (is (= (str "changed priority from \"Important\" to \"Regular\", made this card private and renamed it from "
- "\"Tips by State\" to \"Spots by State\".")
- (diff/diff-string "card" before after)))))
diff --git a/test/metabase/models/revision_test.clj b/test/metabase/models/revision_test.clj
index 11031821698..b650aed3002 100644
--- a/test/metabase/models/revision_test.clj
+++ b/test/metabase/models/revision_test.clj
@@ -4,7 +4,9 @@
[metabase.models.card :refer [Card]]
[metabase.models.interface :as mi]
[metabase.models.revision :as revision :refer [Revision]]
+ [metabase.models.revision.diff :refer [build-sentence]]
[metabase.test :as mt]
+ [metabase.util.i18n :refer [deferred-tru]]
[toucan.models :as models]
[toucan2.core :as t2]))
@@ -13,6 +15,17 @@
(models/defmodel ^:private FakedCard :report_card)
+(use-fixtures :each (fn [thunk]
+ (with-redefs [metabase.models.revision.diff/model-str->i18n-str (fn [model-str]
+ (case model-str
+ "Dashboard" (deferred-tru "Dashboard")
+ "Card" (deferred-tru "Card")
+ "Segment" (deferred-tru "Segment")
+ "Metric" (deferred-tru "Metric")
+ "NonExistModel" "NonExistModel"
+ "FakeCard" "FakeCard"))]
+ (thunk))))
+
(defmethod revision/serialize-instance FakedCard
[_model _id obj]
(into {} (assoc obj :serialized true)))
@@ -25,10 +38,10 @@
[_model o1 o2]
{:o1 (when o1 (into {} o1)), :o2 (when o2 (into {} o2))})
-(defmethod revision/diff-str FakedCard
+(defmethod revision/diff-strings FakedCard
[_model o1 o2]
(when o1
- (str "BEFORE=" (into {} o1) ",AFTER=" (into {} o2))))
+ [(str "BEFORE=" (into {} o1) ",AFTER=" (into {} o2))]))
(defn- push-fake-revision! [card-id & {:keys [message] :as object}]
(revision/push-revision!
@@ -52,40 +65,35 @@
(testing (str "Check that pattern matching allows specialization and that string only reflects the keys that have "
"changed")
(is (= "renamed this Card from \"Tips by State\" to \"Spots by State\"."
- ((get-method revision/diff-str :default)
- Card
- {:name "Tips by State", :private false}
- {:name "Spots by State", :private false})))
+ (build-sentence
+ ((get-method revision/diff-strings :default)
+ Card
+ {:name "Tips by State", :private false}
+ {:name "Spots by State", :private false}))))
(is (= "made this Card private."
- ((get-method revision/diff-str :default)
- Card
- {:name "Spots by State", :private false}
- {:name "Spots by State", :private true})))))
-
-(deftest fallback-description-test
- (testing "Check the fallback sentence fragment for key without specialized sentence fragment"
- (is (= "changed priority from \"Important\" to \"Regular\"."
- ((get-method revision/diff-str :default)
- Card
- {:priority "Important"}
- {:priority "Regular"})))))
+ (build-sentence
+ ((get-method revision/diff-strings :default)
+ Card
+ {:name "Spots by State", :private false}
+ {:name "Spots by State", :private true}))))))
(deftest multiple-changes-test
(testing "Check that 2 changes are handled nicely"
(is (= "made this Card private and renamed it from \"Tips by State\" to \"Spots by State\"."
- ((get-method revision/diff-str :default)
- Card
- {:name "Tips by State", :private false}
- {:name "Spots by State", :private true}))))
+ (build-sentence
+ ((get-method revision/diff-strings :default)
+ Card
+ {:name "Tips by State", :private false}
+ {:name "Spots by State", :private true})))))
(testing "Check that several changes are handled nicely"
- (is (= (str "changed priority from \"Important\" to \"Regular\", made this Card private and renamed it from "
- "\"Tips by State\" to \"Spots by State\".")
- ((get-method revision/diff-str :default)
- Card
- {:name "Tips by State", :private false, :priority "Important"}
- {:name "Spots by State", :private true, :priority "Regular"})))))
+ (is (= (str "turned this into a model, made it private and renamed it from \"Tips by State\" to \"Spots by State\".")
+ (build-sentence
+ ((get-method revision/diff-strings :default)
+ Card
+ {:name "Tips by State", :private false, :dataset false}
+ {:name "Spots by State", :private true, :dataset true}))))))
;;; # REVISIONS + PUSH-REVISION!
@@ -167,13 +175,14 @@
(mt/with-temp Card [{card-id :id}]
(push-fake-revision! card-id, :name "Initial Name")
(push-fake-revision! card-id, :name "Modified Name")
- (is (= {:is_creation false
- :is_reversion false
- :message nil
- :user {:id (mt/user->id :rasta), :common_name "Rasta Toucan", :first_name "Rasta", :last_name "Toucan"}
- :diff {:o1 {:name "Initial Name", :serialized true}
- :o2 {:name "Modified Name", :serialized true}}
- :description "BEFORE={:name \"Initial Name\", :serialized true},AFTER={:name \"Modified Name\", :serialized true}"}
+ (is (= {:is_creation false
+ :is_reversion false
+ :message nil
+ :user {:id (mt/user->id :rasta), :common_name "Rasta Toucan", :first_name "Rasta", :last_name "Toucan"}
+ :diff {:o1 {:name "Initial Name", :serialized true}
+ :o2 {:name "Modified Name", :serialized true}}
+ :has_multiple_changes false
+ :description "BEFORE={:name \"Initial Name\", :serialized true},AFTER={:name \"Modified Name\", :serialized true}."}
(let [revisions (revision/revisions FakedCard card-id)]
(assert (= 2 (count revisions)))
(-> (revision/add-revision-details FakedCard (first revisions) (last revisions))
@@ -186,13 +195,14 @@
(push-fake-revision! card-id, :name "Tips Created by Day")
(is (= [(mi/instance
Revision
- {:is_reversion false,
- :is_creation false,
- :message nil,
- :user {:id (mt/user->id :rasta), :common_name "Rasta Toucan", :first_name "Rasta", :last_name "Toucan"},
- :diff {:o1 nil
- :o2 {:name "Tips Created by Day", :serialized true}}
- :description nil})]
+ {:is_reversion false,
+ :is_creation false,
+ :message nil,
+ :user {:id (mt/user->id :rasta), :common_name "Rasta Toucan", :first_name "Rasta", :last_name "Toucan"},
+ :diff {:o1 nil
+ :o2 {:name "Tips Created by Day", :serialized true}}
+ :has_multiple_changes false
+ :description nil})]
(->> (revision/revisions+details FakedCard card-id)
(map #(dissoc % :timestamp :id :model_id))))))))
@@ -203,23 +213,25 @@
(push-fake-revision! card-id, :name "Spots Created by Day")
(is (= [(mi/instance
Revision
- {:is_reversion false,
- :is_creation false,
- :message nil
- :user {:id (mt/user->id :rasta), :common_name "Rasta Toucan", :first_name "Rasta", :last_name "Toucan"},
- :diff {:o1 {:name "Tips Created by Day", :serialized true}
- :o2 {:name "Spots Created by Day", :serialized true}}
- :description (str "BEFORE={:name \"Tips Created by Day\", :serialized true},AFTER="
- "{:name \"Spots Created by Day\", :serialized true}")})
+ {:is_reversion false,
+ :is_creation false,
+ :message nil
+ :user {:id (mt/user->id :rasta), :common_name "Rasta Toucan", :first_name "Rasta", :last_name "Toucan"},
+ :diff {:o1 {:name "Tips Created by Day", :serialized true}
+ :o2 {:name "Spots Created by Day", :serialized true}}
+ :has_multiple_changes false
+ :description (str "BEFORE={:name \"Tips Created by Day\", :serialized true},AFTER="
+ "{:name \"Spots Created by Day\", :serialized true}.")})
(mi/instance
Revision
- {:is_reversion false,
- :is_creation false,
- :message nil
- :user {:id (mt/user->id :rasta), :common_name "Rasta Toucan", :first_name "Rasta", :last_name "Toucan"},
- :diff {:o1 nil
- :o2 {:name "Tips Created by Day", :serialized true}}
- :description nil})]
+ {:is_reversion false,
+ :is_creation false,
+ :message nil
+ :user {:id (mt/user->id :rasta), :common_name "Rasta Toucan", :first_name "Rasta", :last_name "Toucan"},
+ :diff {:o1 nil
+ :o2 {:name "Tips Created by Day", :serialized true}}
+ :has_multiple_changes false
+ :description nil})]
(->> (revision/revisions+details FakedCard card-id)
(map #(dissoc % :timestamp :id :model_id))))))))
@@ -280,3 +292,52 @@
:message nil})]
(->> (revision/revisions FakedCard card-id)
(map #(dissoc % :timestamp :id :model_id)))))))))
+
+(deftest generic-models-revision-title+description-test
+ (doseq [model ["NonExistModel" "Card" "Dashboard"]]
+ (testing (format "revision for %s models" (if (nil? model) "generic" model))
+ (testing "creation"
+ (is (= {:has_multiple_changes false
+ :description "created this."}
+ (#'revision/revision-description-info model
+ nil
+ {:object {:name "New Object"}
+ :is_reversion false
+ :is_creation true}))))
+
+ (testing "reversion"
+ (is (= {:has_multiple_changes false
+ :description "reverted to an earlier version."}
+ (#'revision/revision-description-info model
+ {:object {:name "New Object"}
+ :is_reversion false
+ :is_creation false}
+ {:object {:name "New Object"}
+ :is_reversion true
+ :is_creation false}))))
+
+ (testing "multiple changes"
+ {:description "changed the display from table to bar and turned this into a model."
+ :has_multiple_changes true}
+ (#'revision/revision-description-info model
+ {:object {:dataset false
+ :display :table}
+ :is_reversion false
+ :is_creation false}
+ {:object {:dataset true
+ :display :bar}
+ :is_reversion false
+ :is_creation false}))
+
+ (testing "changes contains unspecified keys will not be mentioned"
+ (is (= {:description "turned this into a model."
+ :has_multiple_changes false}
+ (#'revision/revision-description-info model
+ {:object {:dataset false
+ :unknown_key false}
+ :is_reversion false
+ :is_creation false}
+ {:object {:dataset true
+ :unknown_key false}
+ :is_reversion false
+ :is_creation false})))))))
--
GitLab