Revision history fixes (#19985)

* Make `getRevisionEvents` accept params object

* Use "You" for current user's revision events

* Fix "moved to" revision display

* Add extra bottom padding to events timeline

* Extract conditions into variables

frontend/src/metabase/lib/revisions/revisions.js

@@ -65,13 +65,15 @@ function getSeriesChangeDescription(prevCards, cards) {
 
 function getCollectionChangeDescription(prevCollectionId, collectionId) {
   const key = `collection-from-${prevCollectionId}-to-${collectionId}`;
-  return jt`moved this to ${(
-    <EntityLink
-      key={key}
-      entityId={collectionId || "root"}
-      entityType="collections"
-    />
-  )}`;
+  return [
+    jt`moved this to ${(
+      <EntityLink
+        key={key}
+        entityId={collectionId || "root"}
+        entityType="collections"
+      />
+    )}`,
+  ];
 }
 
 const CHANGE_DESCRIPTIONS = {
@@ -195,8 +197,11 @@ export function isValidRevision(revision) {
   return getChangedFields(revision).length > 0;
 }
 
-function getRevisionUsername(revision) {
-  return revision.user.common_name;
+function getRevisionUsername(revision, currentUser) {
+  const revisionUser = revision.user;
+  return revisionUser.id === currentUser?.id
+    ? t`You`
+    : revisionUser.common_name;
 }
 
 function getRevisionEpochTimestamp(revision) {
@@ -205,12 +210,15 @@ function getRevisionEpochTimestamp(revision) {
 
 export const REVISION_EVENT_ICON = "pencil";
 
-export function getRevisionEventsForTimeline(revisions = [], canWrite) {
+export function getRevisionEventsForTimeline(
+  revisions = [],
+  { currentUser, canWrite = false },
+) {
   return revisions
     .filter(isValidRevision)
     .map((revision, index) => {
       const isRevertable = canWrite && index !== 0;
-      const username = getRevisionUsername(revision);
+      const username = getRevisionUsername(revision, currentUser);
       const changes = getRevisionDescription(revision);
 
       const event = {
@@ -220,6 +228,14 @@ export function getRevisionEventsForTimeline(revisions = [], canWrite) {
         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"
@@ -227,12 +243,7 @@ export function getRevisionEventsForTimeline(revisions = [], canWrite) {
       // 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"
-
-      if (
-        !revision.is_creation &&
-        !revision.is_reversion &&
-        Array.isArray(changes)
-      ) {
+      if (isChangeEvent && isMultipleFieldsChange) {
         event.title = t`${username} edited this`;
         event.description = <RevisionBatchedDescription changes={changes} />;
       } else {

frontend/src/metabase/lib/revisions/revisions.unit.spec.js

@@ -14,6 +14,7 @@ const DEFAULT_EPOCH_TIMESTAMP = new Date(DEFAULT_TIMESTAMP).valueOf();
 function getRevision({
   isCreation = false,
   isReversion = false,
+  userId = 1,
   username = "Foo",
   before,
   after,
@@ -25,6 +26,7 @@ function getRevision({
       after,
     },
     user: {
+      id: userId,
       common_name: username,
     },
     is_creation: isCreation,
@@ -435,11 +437,9 @@ describe("getRevisionEvents", () => {
   const revisionEvents = [latestRevisionEvent, changeEvent, creationEvent];
 
   it("should convert a revision object into an object for use in a <Timeline /> component", () => {
-    const canWrite = false;
-    const timelineEvents = getRevisionEventsForTimeline(
-      revisionEvents,
-      canWrite,
-    );
+    const timelineEvents = getRevisionEventsForTimeline(revisionEvents, {
+      canWrite: false,
+    });
 
     expect(timelineEvents).toEqual([
       getExpectedEvent({
@@ -466,28 +466,23 @@ describe("getRevisionEvents", () => {
   });
 
   it("should set the `isRevertable` to false when the user doesn't have write access", () => {
-    const canWrite = false;
-    const timelineEvents = getRevisionEventsForTimeline(
-      revisionEvents,
-      canWrite,
-    );
+    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 canWrite = true;
-    const timelineEvents = getRevisionEventsForTimeline(
-      revisionEvents,
-      canWrite,
-    );
+    const timelineEvents = getRevisionEventsForTimeline(revisionEvents, {
+      canWrite: true,
+    });
 
     expect(timelineEvents[0].isRevertable).toBe(false);
     expect(timelineEvents[1].isRevertable).toBe(true);
   });
 
   it("should drop invalid revisions", () => {
-    const canWrite = true;
     const timelineEvents = getRevisionEventsForTimeline(
       [
         changeEvent,
@@ -496,7 +491,7 @@ describe("getRevisionEvents", () => {
           after: null,
         }),
       ],
-      canWrite,
+      { canWrite: true },
     );
     expect(timelineEvents).toEqual([
       getExpectedEvent({
@@ -508,7 +503,6 @@ describe("getRevisionEvents", () => {
   });
 
   it("should drop revisions with not registered fields", () => {
-    const canWrite = true;
     const timelineEvents = getRevisionEventsForTimeline(
       [
         changeEvent,
@@ -519,7 +513,7 @@ describe("getRevisionEvents", () => {
           after: { "dont-know-this-field": 2 },
         }),
       ],
-      canWrite,
+      { canWrite: true },
     );
     expect(timelineEvents).toEqual([
       getExpectedEvent({
@@ -529,6 +523,25 @@ describe("getRevisionEvents", () => {
       }),
     ]);
   });
+
+  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" />,
+        isRevertable: false,
+        revision: event,
+      }),
+    ]);
+  });
 });
 
 describe("isValidRevision", () => {

frontend/src/metabase/query_builder/components/QuestionActivityTimeline.jsx

@@ -16,11 +16,10 @@ import { getQuestionDetailsTimelineDrawerState } from "metabase/query_builder/se
 
 import Revision from "metabase/entities/revisions";
 import User from "metabase/entities/users";
-import Timeline from "metabase/components/Timeline";
 import DrawerSection, {
   STATES as DRAWER_STATES,
 } from "metabase/components/DrawerSection/DrawerSection";
-import { RevertButton } from "./QuestionActivityTimeline.styled";
+import { Timeline, RevertButton } from "./QuestionActivityTimeline.styled";
 
 const { getModerationTimelineEvents } = PLUGIN_MODERATION;
 
@@ -99,7 +98,10 @@ export function QuestionActivityTimeline({
       usersById,
       currentUser,
     );
-    const revisionEvents = getRevisionEventsForTimeline(revisions, canWrite);
+    const revisionEvents = getRevisionEventsForTimeline(revisions, {
+      currentUser,
+      canWrite,
+    });
     return [...revisionEvents, ...moderationEvents];
   }, [canWrite, moderationReviews, revisions, usersById, currentUser]);
 

frontend/src/metabase/query_builder/components/QuestionActivityTimeline.styled.jsx

 import styled from "styled-components";
 import { color } from "metabase/lib/colors";
 import ActionButton from "metabase/components/ActionButton";
+import DefaultTimeline from "metabase/components/Timeline";
+
+export const Timeline = styled(DefaultTimeline)`
+  padding-bottom: 1em;
+`;
 
 export const RevertButton = styled(ActionButton).attrs({
   successClassName: "",