Merge pull request #4101 from weseek/fix/GW-6897-behavior-when-moving-recursively

Fix/gw 6897 behavior when moving recursively

packages/app/src/server/service/page.js

@@ -3,6 +3,7 @@ import loggerFactory from '~/utils/logger';
 
 const mongoose = require('mongoose');
 const escapeStringRegexp = require('escape-string-regexp');
+const streamToPromise = require('stream-to-promise');
 
 const logger = loggerFactory('growi:models:page');
 const debug = require('debug')('growi:models:page');
@@ -49,7 +50,6 @@ class PageService {
     return this.prepareShoudDeletePagesByRedirectTo(pagePath, redirectToPagePathMapping, pagePaths);
   }
 
-
   async renamePage(page, newPagePath, user, options, isRecursively = false) {
 
     const Page = this.crowi.model('Page');
@@ -62,6 +62,11 @@ class PageService {
     // sanitize path
     newPagePath = this.crowi.xss.process(newPagePath); // eslint-disable-line no-param-reassign
 
+    // create descendants first
+    if (isRecursively) {
+      await this.renameDescendantsWithStream(page, newPagePath, user, options);
+    }
+
     const update = {};
     // update Page
     update.path = newPagePath;
@@ -80,7 +85,7 @@ class PageService {
     }
 
     if (isRecursively) {
-      this.renameDescendantsWithStream(page, newPagePath, user, options);
+      await this.renameDescendantsWithStream(page, newPagePath, user, options);
     }
 
     this.pageEvent.emit('delete', page, user, socketClientId);
@@ -189,6 +194,8 @@ class PageService {
     readStream
       .pipe(createBatchStream(BULK_REINDEX_SIZE))
       .pipe(writeStream);
+
+    await streamToPromise(readStream);
   }
 
 

packages/app/src/test/service/page.test.js

@@ -14,6 +14,14 @@ let parentForRename1;
 let parentForRename2;
 let parentForRename3;
 let parentForRename4;
+let parentForRename5;
+let parentForRename6;
+let parentForRename7;
+let parentForRename8;
+let parentForRename9;
+
+let irrelevantPage1;
+let irrelevantPage2;
 
 let childForRename1;
 let childForRename2;
@@ -94,6 +102,48 @@ describe('PageService', () => {
         creator: testUser1,
         lastUpdateUser: testUser1,
       },
+      {
+        path: '/parentForRename5',
+        grant: Page.GRANT_PUBLIC,
+        creator: testUser1,
+        lastUpdateUser: testUser1,
+      },
+      {
+        path: '/parentForRename6',
+        grant: Page.GRANT_PUBLIC,
+        creator: testUser1,
+        lastUpdateUser: testUser1,
+      },
+      {
+        path: '/level1/level2',
+        grant: Page.GRANT_PUBLIC,
+        creator: testUser1,
+        lastUpdateUser: testUser1,
+      },
+      {
+        path: '/level1/level2/child',
+        grant: Page.GRANT_PUBLIC,
+        creator: testUser1,
+        lastUpdateUser: testUser1,
+      },
+      {
+        path: '/level1/level2/level2',
+        grant: Page.GRANT_PUBLIC,
+        creator: testUser1,
+        lastUpdateUser: testUser1,
+      },
+      {
+        path: '/parentForRename6-2021H1',
+        grant: Page.GRANT_PUBLIC,
+        creator: testUser1,
+        lastUpdateUser: testUser1,
+      },
+      {
+        path: '/level1-2021H1',
+        grant: Page.GRANT_PUBLIC,
+        creator: testUser1,
+        lastUpdateUser: testUser1,
+      },
       {
         path: '/parentForRename1/child',
         grant: Page.GRANT_PUBLIC,
@@ -183,6 +233,14 @@ describe('PageService', () => {
     parentForRename2 = await Page.findOne({ path: '/parentForRename2' });
     parentForRename3 = await Page.findOne({ path: '/parentForRename3' });
     parentForRename4 = await Page.findOne({ path: '/parentForRename4' });
+    parentForRename5 = await Page.findOne({ path: '/parentForRename5' });
+    parentForRename6 = await Page.findOne({ path: '/parentForRename6' });
+    parentForRename7 = await Page.findOne({ path: '/level1/level2' });
+    parentForRename8 = await Page.findOne({ path: '/level1/level2/child' });
+    parentForRename9 = await Page.findOne({ path: '/level1/level2/level2' });
+
+    irrelevantPage1 = await Page.findOne({ path: '/parentForRename6-2021H1' });
+    irrelevantPage2 = await Page.findOne({ path: '/level1-2021H1' });
 
     parentForDuplicate = await Page.findOne({ path: '/parentForDuplicate' });
 
@@ -232,6 +290,36 @@ describe('PageService', () => {
     xssSpy = jest.spyOn(crowi.xss, 'process').mockImplementation(path => path);
   });
 
+  describe('rename page without using renameDescendantsWithStreamSpy', () => {
+    test('rename page with different tree with isRecursively [deeper]', async() => {
+      const resultPage = await crowi.pageService.renamePage(parentForRename6, '/parentForRename6/renamedChild', testUser1, {}, true);
+      const wrongPage = await Page.findOne({ path: '/parentForRename6/renamedChild/renamedChild' });
+      const expectPage1 = await Page.findOne({ path: '/parentForRename6/renamedChild' });
+      const expectPage2 = await Page.findOne({ path: '/parentForRename6-2021H1' });
+
+      expect(resultPage.path).toEqual(expectPage1.path);
+      expect(expectPage2.path).not.toBeNull();
+
+      // Check that pages that are not to be renamed have not been renamed
+      expect(wrongPage).toBeNull();
+    });
+
+    test('rename page with different tree with isRecursively [shallower]', async() => {
+      await crowi.pageService.renamePage(parentForRename7, '/level1', testUser1, {}, true);
+      const expectPage1 = await Page.findOne({ path: '/level1' });
+      const expectPage2 = await Page.findOne({ path: '/level1/child' });
+      const expectPage3 = await Page.findOne({ path: '/level1/level2/level2' });
+      const expectPage4 = await Page.findOne({ path: '/level1-2021H1' });
+
+      expect(expectPage1).not.toBeNull();
+      expect(expectPage2).not.toBeNull();
+      expect(expectPage3).not.toBeNull();
+
+      // Check that pages that are not to be renamed have not been renamed
+      expect(expectPage4).not.toBeNull();
+    });
+  });
+
   describe('rename page', () => {
     let pageEventSpy;
     let renameDescendantsWithStreamSpy;
@@ -328,6 +416,16 @@ describe('PageService', () => {
         expect(redirectedFromPageRevision).toBeNull();
       });
 
+      test('rename page with different tree with isRecursively', async() => {
+
+        const resultPage = await crowi.pageService.renamePage(parentForRename5, '/parentForRename5/renamedChild', testUser1, {}, true);
+        const wrongPage = await Page.findOne({ path: '/parentForRename5/renamedChild/renamedChild' });
+        const expectPage = await Page.findOne({ path: '/parentForRename5/renamedChild' });
+
+        expect(resultPage.path).toEqual(expectPage.path);
+        expect(wrongPage).toBeNull();
+      });
+
     });
 
     test('renameDescendants without options', async() => {
@@ -396,7 +494,6 @@ describe('PageService', () => {
     });
   });
 
-
   describe('duplicate page', () => {
     let duplicateDescendantsWithStreamSpy;