Merge pull request #17384 from strapi/fix/ct-rw-assign-concurrency

api-tests/core/admin/ee/review-workflows-content-types.test.api.js

@@ -32,6 +32,21 @@ const productModel = {
   },
 };
 
+const articleUID = 'api::article.article';
+const articleModel = {
+  draftAndPublish: true,
+  pluginOptions: {},
+  singularName: 'article',
+  pluralName: 'articles',
+  displayName: 'Article',
+  kind: 'collectionType',
+  attributes: {
+    title: {
+      type: 'string',
+    },
+  },
+};
+
 describeOnCondition(edition === 'EE')('Review workflows - Content Types', () => {
   const builder = createTestBuilder();
 
@@ -87,15 +102,22 @@ describeOnCondition(edition === 'EE')('Review workflows - Content Types', () =>
   };
 
   beforeAll(async () => {
-    await builder.addContentTypes([productModel]).build();
+    await builder.addContentTypes([productModel, articleModel]).build();
 
     strapi = await createStrapiInstance();
     requests.admin = await createAuthRequest({ strapi });
 
+    // Create products
     await Promise.all([
       createEntry(productUID, { name: 'Product 1' }),
       createEntry(productUID, { name: 'Product 2' }),
     ]);
+
+    // Create articles
+    await Promise.all([
+      createEntry(articleUID, { title: 'Article 1' }),
+      createEntry(articleUID, { title: 'Article 2' }),
+    ]);
   });
 
   afterAll(async () => {
@@ -228,6 +250,32 @@ describeOnCondition(edition === 'EE')('Review workflows - Content Types', () =>
       });
     });
 
+    // Depends on the previous test
+    describe('Assign multiple content types', () => {
+      test('It should assign multiple content types', async () => {
+        const res = await updateWorkflow(workflow1.id, {
+          contentTypes: [productUID, articleUID],
+        });
+
+        expect(res.status).toBe(200);
+        expect(res.body.data).toMatchObject({ contentTypes: [productUID, articleUID] });
+      });
+
+      test('It should steal content types from other workflows', async () => {
+        // There could be concurrency issues, so assert all workflows are transfered
+        const res = await updateWorkflow(workflow2.id, {
+          contentTypes: [productUID, articleUID],
+        });
+
+        const updatedWorkflow1 = await getWorkflow(workflow1.id);
+
+        expect(res.status).toBe(200);
+        expect(res.body.data).toMatchObject({ contentTypes: [productUID, articleUID] });
+
+        expect(updatedWorkflow1).toMatchObject({ contentTypes: [] });
+      });
+    });
+
     describe('Assign and update stages', () => {
       test('It should assign and update stages', async () => {
         workflow1 = await createWorkflow({ contentTypes: [] }).then((res) => res.body.data);
@@ -276,6 +324,7 @@ describeOnCondition(edition === 'EE')('Review workflows - Content Types', () =>
       }
     });
   });
+
   describe('Creating entity assigned to a workflow', () => {
     let workflow;
     test('When content type is assigned to workflow, new entries should be added to the first stage of the default workflow', async () => {

packages/core/admin/ee/server/services/review-workflows/workflows/__tests__/content-types.test.js

@@ -9,6 +9,7 @@ const stagesServiceMock = {
 
 const workflowsServiceMock = {
   getAssignedWorkflow: jest.fn(),
+  _getAssignedWorkflows: jest.fn(),
 };
 
 const reviewWorkflowsValidationMock = {
@@ -67,7 +68,7 @@ describe('Review Workflows', () => {
       test('should update the configuration of newly added content types', async () => {
         const prevOptions = { testOptions: true };
 
-        workflowsServiceMock.getAssignedWorkflow.mockResolvedValueOnce(null);
+        workflowsServiceMock._getAssignedWorkflows.mockResolvedValueOnce([]);
         CTMPContentTypesServiceMock.findConfiguration.mockResolvedValueOnce({
           options: prevOptions,
         });

packages/core/admin/ee/server/services/review-workflows/workflows/content-types.js

@@ -30,26 +30,35 @@ module.exports = ({ strapi }) => {
      * @param {Workflow.Stage} options.stageId - The new stage to assign the entities to
      */
     async migrate({ srcContentTypes = [], destContentTypes, stageId }) {
-      // Workflows service is using this content-types service, to avoid an infinite loop, we need to get the service in the method
       const workflowsService = getService('workflows', { strapi });
       const { created, deleted } = diffContentTypes(srcContentTypes, destContentTypes);
 
-      await mapAsync(created, async (uid) => {
-        // If it was assigned to another workflow, transfer it from the previous workflow
-        const srcWorkflow = await workflowsService.getAssignedWorkflow(uid);
-        if (srcWorkflow) {
-          // Updates all existing entities stages links to the new stage
-          await stagesService.updateEntitiesStage(uid, { toStageId: stageId });
-          return this.transferContentType(srcWorkflow, uid);
-        }
-        await updateContentTypeConfig(uid, true);
+      await mapAsync(
+        created,
+        async (uid) => {
+          // Content Types should only be assigned to one workflow
+          // However, edge cases can happen, and this handles them
+          const srcWorkflows = await workflowsService._getAssignedWorkflows(uid, {});
 
-        // Create new stages links to the new stage
-        return stagesService.updateEntitiesStage(uid, {
-          fromStageId: null,
-          toStageId: stageId,
-        });
-      });
+          if (srcWorkflows.length) {
+            // Updates all existing entities stages links to the new stage
+            await stagesService.updateEntitiesStage(uid, { toStageId: stageId });
+            // Transfer content types from the previous workflow(s)
+            await mapAsync(srcWorkflows, (srcWorkflow) =>
+              this.transferContentTypes(srcWorkflow, uid)
+            );
+          }
+          await updateContentTypeConfig(uid, true);
+
+          // Create new stages links to the new stage
+          return stagesService.updateEntitiesStage(uid, {
+            fromStageId: null,
+            toStageId: stageId,
+          });
+        },
+        // transferContentTypes can cause race conditions if called in parallel when updating the same workflow
+        { concurrency: 1 }
+      );
 
       await mapAsync(deleted, async (uid) => {
         await updateContentTypeConfig(uid, false);
@@ -58,15 +67,15 @@ module.exports = ({ strapi }) => {
     },
 
     /**
-     * Filters the content types assigned to the previous workflow.
+     * Filters the content types assigned to a workflow
      * @param {Workflow} srcWorkflow - The workflow to transfer from
      * @param {string} uid - The content type uid
      */
-    async transferContentType(srcWorkflow, uid) {
+    async transferContentTypes(srcWorkflow, uid) {
       // Update assignedContentTypes of the previous workflow
       await strapi.entityService.update(WORKFLOW_MODEL_UID, srcWorkflow.id, {
         data: {
-          contentTypes: srcWorkflow.contentTypes.filter((ct) => ct !== uid),
+          contentTypes: srcWorkflow.contentTypes.filter((contentType) => contentType !== uid),
         },
       });
     },

packages/core/admin/ee/server/services/review-workflows/workflows/index.js

@@ -163,11 +163,23 @@ module.exports = ({ strapi }) => {
      * @returns {Promise<object|null>} - Assigned workflow object if found, or null.
      */
     async getAssignedWorkflow(uid, opts = {}) {
-      const workflows = await this.find({
+      const workflows = await this._getAssignedWorkflows(uid, opts);
+      return workflows.length > 0 ? workflows[0] : null;
+    },
+
+    /**
+     * Finds all the assigned workflows for a given content type ID.
+     * Normally, there should only be one workflow assigned to a content type.
+     * However, edge cases can occur where a content type is assigned to multiple workflows.
+     * @param {string} uid - Content type ID to find the assigned workflows for.
+     * @param {object} opts - Options for the query.
+     * @returns {Promise<object[]>} - List of assigned workflow objects.
+     */
+    async _getAssignedWorkflows(uid, opts = {}) {
+      return this.find({
         ...opts,
         filters: { contentTypes: getWorkflowContentTypeFilter({ strapi }, uid) },
       });
-      return workflows.length > 0 ? workflows[0] : null;
     },
 
     /**