fix(history): save status on version create (#19737)

e2e/tests/content-manager/history.spec.ts

@@ -83,7 +83,6 @@ describeOnCondition(hasFutureFlag)('History', () => {
       await page.waitForURL(HISTORY_URL);
       await expect(versionCards).toHaveCount(2);
       // Assert the most recent version is the current version
-      versionCards.first().click();
       await expect(titleInput).toHaveValue('Being from Kansas City');
       // Assert the previous version in the list is the expected version
       const previousVersion = versionCards.nth(1);
@@ -92,29 +91,44 @@ describeOnCondition(hasFutureFlag)('History', () => {
       await expect(previousVersion.getByText('(current)')).not.toBeVisible();
 
       // Go back to the entry
-      // await page.getByRole('link', { name: 'Back' }).click();
+      await page.getByRole('link', { name: 'Back' }).click();
 
       /**
        * Publish
-       *
-       * TODO: Fix publish
-       * The publish action in the middleware used to create history versions has a different shape than the other actions.
-       * This leaves us with null for status and relatedDocumentId in the history version.
-       *
        */
-      // await page.getByRole('button', { name: 'Publish' }).click();
+      await page.getByRole('button', { name: 'Publish' }).click();
-      // await page.getByRole('link', { name: 'History' }).click();
+      // Go to the history page
-      // await page.waitForURL(
+      await page.getByRole('button', { name: /more actions/i }).click();
-      //   '**/content-manager/collection-types/api::article.article/*/history?plugins\\[i18n\\]\\[locale\\]=en'
+      await page.getByRole('menuitem', { name: /content history/i }).click();
-      // );
+      await page.waitForURL(HISTORY_URL);
-      // await expect(versionCards).toHaveCount(3);
+      // Publish also creates a new draft so we expect the count to increase by 2
-      // // Assert the current version is the most recent published version
+      await expect(versionCards).toHaveCount(4);
-      // await expect(titleInput).toHaveValue('Being from Kansas City');
+      // Assert the current version is the most recent published version
-      // await expect(currentVersion.getByText('Published')).toBeVisible();
+      await expect(titleInput).toHaveValue('Being from Kansas City');
-      // // Assert the previous version in the list is the expected version
+      // The current version is the most recent draft
-      // await expect(versionCards.nth(1).getByText('Draft')).toBeVisible();
+      await expect(currentVersion.getByText('Draft')).toBeVisible();
-      // versionCards.nth(1).click();
+      await expect(titleInput).toHaveValue('Being from Kansas City');
-      // await expect(titleInput).toHaveValue('Being from Kansas City');
+      // The second in the list is the published version
+      await expect(previousVersion.getByText('Published')).toBeVisible();
+      previousVersion.click();
+      await expect(titleInput).toHaveValue('Being from Kansas City');
+
+      // Go back to the entry
+      await page.getByRole('link', { name: 'Back' }).click();
+
+      /**
+       * Modified
+       */
+      await titleInput.fill('Being from Kansas City, Missouri');
+      await page.getByRole('button', { name: 'Save' }).click();
+      // Go to the history page
+      await page.getByRole('button', { name: /more actions/i }).click();
+      await page.getByRole('menuitem', { name: /content history/i }).click();
+      await page.waitForURL(HISTORY_URL);
+      await expect(versionCards).toHaveCount(5);
+      // Assert the current version is the modified version
+      await expect(currentVersion.getByText('Modified')).toBeVisible();
+      await expect(titleInput).toHaveValue('Being from Kansas City, Missouri');
     });
   });
 
@@ -171,6 +185,7 @@ describeOnCondition(hasFutureFlag)('History', () => {
       await expect(idRegex.test(page.url())).toBe(true);
       // Assert the most recent version is the current version
       const currentVersion = versionCards.nth(0);
+      const previousVersion = versionCards.nth(1);
       await expect(currentVersion.getByText('(current)')).toBeVisible();
       await expect(currentVersion.getByText('Draft')).toBeVisible();
       await expect(titleInput).toBeDisabled();
@@ -191,35 +206,47 @@ describeOnCondition(hasFutureFlag)('History', () => {
       await page.waitForURL(HISTORY_URL);
       await expect(versionCards).toHaveCount(2);
       // Assert the most recent version is the current version
-      versionCards.first().click();
       await expect(titleInput).toHaveValue('Welcome to AFC Richmond');
       // Assert the previous version in the list is the expected version
-      versionCards.nth(1).click();
+      previousVersion.click();
       await expect(titleInput).toHaveValue('AFC Richmond');
 
       // Go back to the entry
-      // await page.getByRole('link', { name: 'Back' }).click();
+      await page.getByRole('link', { name: 'Back' }).click();
 
       /**
        * Publish
-       *
-       * TODO: Fix publish
-       * The publish action in the middleware used to create history versions has a different shape than the other actions.
-       * This leaves us with null for status and relatedDocumentId in the history version.
-       *
        */
-      // await page.getByRole('button', { name: 'Publish' }).click();
+      await page.getByRole('button', { name: 'Publish' }).click();
-      // await page.getByRole('link', { name: 'History' }).click();
+      await page.getByRole('button', { name: /more actions/i }).click();
-      // await page.waitForURL('**/content-manager/single-types/api::homepage.homepage/history**');
+      await page.getByRole('menuitem', { name: /content history/i }).click();
-      // await expect(versionCards).toHaveCount(3);
+      await page.waitForURL('**/content-manager/single-types/api::homepage.homepage/history**');
-      // // Assert the current version is the most recent published version
+      // Publish also creates a new draft so we expect the count to increase by 2
-      // await expect(titleInput).toHaveValue('Welcome to AFC Richmond!');
+      await expect(versionCards).toHaveCount(4);
-      // // TODO: Assert the version is marked as published when publishing works
+      // The current version is the most recent draft
-      // await expect(currentVersion.getByText('Published')).toBeVisible();
+      await expect(currentVersion.getByText('Draft')).toBeVisible();
-      // // Assert the previous version in the list is the expected version
+      await expect(titleInput).toHaveValue('Welcome to AFC Richmond');
-      // await expect(versionCards.nth(1).getByText('Draft')).toBeVisible();
+      // The second in the list is the published version
-      // versionCards.nth(1).click();
+      previousVersion.click();
-      // await expect(titleInput).toHaveValue('Welcome to AFC Richmond');
+      await expect(previousVersion.getByText('Published')).toBeVisible();
+      await expect(titleInput).toHaveValue('Welcome to AFC Richmond');
+
+      // Go back to the entry
+      await page.getByRole('link', { name: 'Back' }).click();
+
+      /**
+       * Modified
+       */
+      await titleInput.fill('Welcome to AFC Richmond!');
+      await page.getByRole('button', { name: 'Save' }).click();
+      // Go to the history page
+      await page.getByRole('button', { name: /more actions/i }).click();
+      await page.getByRole('menuitem', { name: /content history/i }).click();
+      await page.waitForURL(HISTORY_URL);
+      await expect(versionCards).toHaveCount(5);
+      // Assert the current version is the most recent published version
+      await expect(titleInput).toHaveValue('Welcome to AFC Richmond!');
+      await expect(currentVersion.getByText('Modified')).toBeVisible();
     });
   });
 });

e2e/tests/content-manager/uniqueness.spec.ts

@@ -74,6 +74,7 @@ test.describe('Uniqueness', () => {
       await page.getByRole(fieldRole, { name: field.name }).fill(field.value);
 
       await clickSave(page);
+      const successLocator = page.getByText('Saved document');
       await expect(page.getByText('Saved document')).toBeVisible();
 
       await page.getByRole('link', { name: 'Unique' }).click();
@@ -90,7 +91,8 @@ test.describe('Uniqueness', () => {
 
       await clickSave(page);
       await expect(page.getByText('Warning:This attribute must be unique')).toBeVisible();
-
+      // Wait for previous success notifications to be removed
+      await expect(successLocator).toHaveCount(0);
       /**
        * Modify the value and try again, this should save successfully
        * Either take the new value provided in the field object or generate a random new one

packages/core/admin/admin/src/components/FormInputs/Time.tsx

@@ -26,7 +26,7 @@ const TimeInput = forwardRef<HTMLInputElement, InputProps>((props, ref) => {
         field.onChange(props.name, time);
       }}
       onClear={() => field.onChange(props.name, undefined)}
-      value={field.value}
+      value={field.value ?? ''}
       {...props}
     />
   );

packages/core/admin/admin/src/content-manager/history/components/VersionContent.tsx

@@ -3,7 +3,6 @@ import * as React from 'react';
 import { Box, ContentLayout, Flex, Grid, GridItem, Typography } from '@strapi/design-system';
 
 import { Form } from '../../../components/Form';
-import { Page } from '../../../components/PageHelpers';
 import {
   InputRenderer,
   type InputRendererProps,
@@ -43,6 +42,7 @@ const VersionContent = () => {
             if (panel.some((row) => row.some((field) => field.type === 'dynamiczone'))) {
               const [row] = panel;
               const [field] = row;
+
               return (
                 <Grid key={field.name} gap={4}>
                   <GridItem col={12} s={12} xs={12}>

packages/core/content-manager/server/src/history/services/__tests__/history.test.ts

@@ -18,10 +18,28 @@ const mockGetRequestContext = jest.fn(() => {
         id: userId,
       },
     },
+    request: {
+      url: '/content-manager/test',
+    },
   };
 });
 
 const mockStrapi = {
+  plugins: {
+    'content-manager': {
+      service: jest.fn(() => ({
+        getMetadata: jest.fn().mockResolvedValue([]),
+        getStatus: jest.fn(),
+      })),
+    },
+    i18n: {
+      service: jest.fn(() => ({
+        getDefaultLocale: jest.fn().mockReturnValue('en'),
+      })),
+    },
+  },
+  // @ts-expect-error - Ignore
+  plugin: (plugin: string) => mockStrapi.plugins[plugin],
   db: {
     query(uid: UID.ContentType) {
       if (uid === HISTORY_VERSION_UID) {
@@ -45,15 +63,15 @@ const mockStrapi = {
       get: jest.fn(),
     },
   },
+  documents: jest.fn(() => ({
+    findOne: jest.fn(),
+  })),
   config: {
     get: () => undefined,
   },
   requestContext: {
     get: mockGetRequestContext,
   },
-  documents: {
-    use: jest.fn(),
-  },
   contentType(uid: UID.ContentType) {
     if (uid === 'api::article.article') {
       return {
@@ -66,6 +84,8 @@ const mockStrapi = {
     }
   },
 };
+// @ts-expect-error - ignore
+mockStrapi.documents.use = jest.fn();
 
 // @ts-expect-error - we're not mocking the full Strapi object
 const historyService = createHistoryService({ strapi: mockStrapi });
@@ -78,6 +98,7 @@ describe('history-version service', () => {
   it('inits service only once', () => {
     historyService.bootstrap();
     historyService.bootstrap();
+    // @ts-expect-error - ignore
     expect(mockStrapi.documents.use).toHaveBeenCalledTimes(1);
   });
 
@@ -96,6 +117,7 @@ describe('history-version service', () => {
 
     const next = jest.fn((context) => ({ ...context, documentId: 'document-id' }));
     await historyService.bootstrap();
+    // @ts-expect-error - ignore
     const historyMiddlewareFunction = mockStrapi.documents.use.mock.calls[0][0];
 
     // Check that we don't break the middleware chain
@@ -110,7 +132,6 @@ describe('history-version service', () => {
 
     // Publish and unpublish actions should be saved in history
     createMock.mockClear();
-    context.action = 'publish';
     await historyMiddlewareFunction(context, next);
     context.action = 'unpublish';
     await historyMiddlewareFunction(context, next);

packages/core/content-manager/server/src/history/services/history.ts

@@ -1,9 +1,9 @@
-import type { LoadedStrapi } from '@strapi/types';
+import type { LoadedStrapi, Documents } from '@strapi/types';
 import { omit, pick } from 'lodash/fp';
 
 import { scheduleJob } from 'node-schedule';
-import { HISTORY_VERSION_UID } from '../constants';
 
+import { HISTORY_VERSION_UID } from '../constants';
 import type { HistoryVersions } from '../../../../shared/contracts';
 
 const DEFAULT_RETENTION_DAYS = 90;
@@ -33,6 +33,32 @@ const createHistoryService = ({ strapi }: { strapi: LoadedStrapi }) => {
     return Math.min(licenseRetentionDays, DEFAULT_RETENTION_DAYS);
   };
 
+  const localesService = strapi.plugin('i18n').service('locales');
+  const getLocaleDictionary = async () => {
+    const locales = (await localesService.find()) || [];
+    return locales.reduce(
+      (
+        acc: Record<string, NonNullable<HistoryVersions.HistoryVersionDataResponse['locale']>>,
+        locale: NonNullable<HistoryVersions.HistoryVersionDataResponse['locale']>
+      ) => {
+        acc[locale.code] = { name: locale.name, code: locale.code };
+
+        return acc;
+      },
+      {}
+    );
+  };
+
+  const getVersionStatus = async (
+    contentTypeUid: HistoryVersions.CreateHistoryVersion['contentType'],
+    document: Documents.AnyDocument | null
+  ) => {
+    const documentMetadataService = strapi.plugin('content-manager').service('document-metadata');
+    const meta = await documentMetadataService.getMetadata(contentTypeUid, document);
+
+    return documentMetadataService.getStatus(document, meta.availableStatus);
+  };
+
   return {
     async bootstrap() {
       // Prevent initializing the service twice
@@ -43,48 +69,62 @@ const createHistoryService = ({ strapi }: { strapi: LoadedStrapi }) => {
        * TODO: Fix the types for the middleware
        */
       strapi.documents.use(async (context, next) => {
-        // @ts-expect-error ContentType is not typed correctly on the context
+        // Ignore requests that are not related to the content manager
-        const contentTypeUid = context.contentType.uid;
+        if (!strapi.requestContext.get()?.request.url.startsWith('/content-manager')) {
-        const params = context.args.at(-1) as any;
-        // Ignore actions that don't mutate documents
-        if (!['create', 'update', 'publish', 'unpublish'].includes(context.action)) {
           return next(context);
         }
 
+        // Ignore actions that don't mutate documents
+        if (
+          !['create', 'update', 'publish', 'unpublish', 'discardDraft'].includes(context.action)
+        ) {
+          return next(context);
+        }
+
+        // @ts-expect-error ContentType is not typed correctly on the context
+        const contentTypeUid = context.contentType.uid;
         // Ignore content types not created by the user
         if (!contentTypeUid.startsWith('api::')) {
           return next(context);
         }
 
+        const result = (await next(context)) as any;
+
+        const documentContext =
+          context.action === 'create'
+            ? // @ts-expect-error The context args are not typed correctly
+              { documentId: result.documentId, locale: context.args[0]?.locale }
+            : { documentId: context.args[0], locale: context.args[1]?.locale };
+
+        const locale = documentContext.locale ?? (await localesService.getDefaultLocale());
+        const document = await strapi
+          .documents(contentTypeUid)
+          .findOne(documentContext.documentId, {
+            locale,
+          });
+        const status = await getVersionStatus(contentTypeUid, document);
+
         const fieldsToIgnore = [
           'createdAt',
           'updatedAt',
           'publishedAt',
           'createdBy',
           'updatedBy',
-          'localizations',
           'locale',
           'strapi_stage',
           'strapi_assignee',
         ];
 
-        /**
-         * Await the middleware stack because for create actions,
-         * the document ID only exists after the creation, which is later in the stack.
-         */
-        const result = (await next(context)) as any;
-
         // Prevent creating a history version for an action that wasn't actually executed
         await strapi.db.transaction(async ({ onCommit }) => {
           onCommit(() => {
             this.createVersion({
               contentType: contentTypeUid,
-              relatedDocumentId: 'documentId' in result ? result.documentId : context.args[0],
+              data: omit(fieldsToIgnore, document),
-              locale: params.locale,
-              // TODO: check if drafts should should be "modified" once D&P is ready
-              status: params.status,
-              data: omit(fieldsToIgnore, params.data),
               schema: omit(fieldsToIgnore, strapi.contentType(contentTypeUid).attributes),
+              relatedDocumentId: documentContext.documentId,
+              locale,
+              status,
             });
           });
         });
@@ -126,28 +166,6 @@ const createHistoryService = ({ strapi }: { strapi: LoadedStrapi }) => {
       });
     },
 
-    /**
-     * TODO: Refactor so i18n can interact history without history itself being concerned about i18n
-     */
-    async getLocaleDictionary() {
-      if (!strapi.plugin('i18n')) {
-        return {};
-      }
-
-      const locales = (await strapi.plugin('i18n').service('locales').find()) || [];
-      return locales.reduce(
-        (
-          acc: Record<string, NonNullable<HistoryVersions.HistoryVersionDataResponse['locale']>>,
-          locale: NonNullable<HistoryVersions.HistoryVersionDataResponse['locale']>
-        ) => {
-          acc[locale.code] = { name: locale.name, code: locale.code };
-
-          return acc;
-        },
-        {}
-      );
-    },
-
     async findVersionsPage(params: HistoryVersions.GetHistoryVersions.Request['query']) {
       const [{ results, pagination }, localeDictionary] = await Promise.all([
         query.findPage({
@@ -162,7 +180,7 @@ const createHistoryService = ({ strapi }: { strapi: LoadedStrapi }) => {
           populate: ['createdBy'],
           orderBy: [{ createdAt: 'desc' }],
         }),
-        this.getLocaleDictionary(),
+        getLocaleDictionary(),
       ]);
 
       const sanitizedResults = results.map((result) => ({