Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Consider an empty page when renaming and duplicating #7979

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions apps/app/src/server/routes/apiv3/pages.js
Original file line number Diff line number Diff line change
Expand Up @@ -529,7 +529,7 @@ module.exports = (crowi) => {
// check whether path starts slash
newPagePath = addHeadingSlash(newPagePath);

const isExist = await Page.count({ path: newPagePath }) > 0;
const isExist = await Page.exists({ path: newPagePath, isEmpty: false });
if (isExist) {
// if page found, cannot rename to that path
return res.apiv3Err(new ErrorV3(`${newPagePath} already exists`, 'already_exists'), 409);
Expand Down Expand Up @@ -757,7 +757,7 @@ module.exports = (crowi) => {
}

// check page existence
const isExist = (await Page.count({ path: newPagePath })) > 0;
const isExist = (await Page.exists({ path: newPagePath, isEmpty: false }));
if (isExist) {
return res.apiv3Err(new ErrorV3(`Page exists '${newPagePath})'`, 'already_exists'), 409);
}
Expand Down
2 changes: 1 addition & 1 deletion apps/app/src/server/service/page.ts
Original file line number Diff line number Diff line change
Expand Up @@ -375,7 +375,7 @@ class PageService {

const activity = await this.crowi.activityService.createActivity(parameters);

const isExist = await Page.exists({ path: newPagePath });
const isExist = await Page.exists({ path: newPagePath, isEmpty: false });
if (isExist) {
throw Error(`Page already exists at ${newPagePath}`);
}
Expand Down
85 changes: 75 additions & 10 deletions apps/app/test/integration/service/v5.public-page.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -135,9 +135,13 @@ describe('PageService page operations with only public pages', () => {
const pageIdForRename9 = new mongoose.Types.ObjectId();
const pageIdForRename10 = new mongoose.Types.ObjectId();
const pageIdForRename11 = new mongoose.Types.ObjectId();
const pageIdForRename12 = new mongoose.Types.ObjectId();
const pageIdForRename13 = new mongoose.Types.ObjectId();
const pageIdForRename14 = new mongoose.Types.ObjectId();

const childPageIdForRename1 = new mongoose.Types.ObjectId();
const childPageIdForRename2 = new mongoose.Types.ObjectId();
const childPageIdForRename3 = new mongoose.Types.ObjectId();
const childPageIdForRename4 = new mongoose.Types.ObjectId();
const childPageIdForRename5 = new mongoose.Types.ObjectId();
const childPageIdForRename7 = new mongoose.Types.ObjectId();

const pageIdForRename16 = new mongoose.Types.ObjectId();

Expand Down Expand Up @@ -235,22 +239,39 @@ describe('PageService page operations with only public pages', () => {
},
{
_id: pageIdForRename10,
path: '/v5_ChildForRename1',
path: '/v5_ParentForRename10',
grant: Page.GRANT_PUBLIC,
creator: dummyUser1,
lastUpdateUser: dummyUser1._id,
parent: rootPage._id,
},
{
_id: pageIdForRename11,
path: '/v5_ParentForRename11',
grant: Page.GRANT_PUBLIC,
creator: dummyUser1,
lastUpdateUser: dummyUser1._id,
parent: rootPage._id,
isEmpty: true,
},
{
_id: childPageIdForRename1,
path: '/v5_ChildForRename1',
grant: Page.GRANT_PUBLIC,
creator: dummyUser1,
lastUpdateUser: dummyUser1._id,
parent: rootPage._id,
},
{
_id: childPageIdForRename2,
path: '/v5_ChildForRename2',
grant: Page.GRANT_PUBLIC,
creator: dummyUser1,
lastUpdateUser: dummyUser1._id,
parent: rootPage._id,
},
{
_id: pageIdForRename12,
_id: childPageIdForRename3,
path: '/v5_ChildForRename3',
grant: Page.GRANT_PUBLIC,
creator: dummyUser1,
Expand All @@ -259,23 +280,23 @@ describe('PageService page operations with only public pages', () => {
updatedAt: new Date('2021'),
},
{
_id: pageIdForRename13,
_id: childPageIdForRename4,
path: '/v5_ChildForRename4',
grant: Page.GRANT_PUBLIC,
creator: dummyUser1,
lastUpdateUser: dummyUser1._id,
parent: rootPage._id,
},
{
_id: pageIdForRename14,
_id: childPageIdForRename5,
path: '/v5_ChildForRename5',
grant: Page.GRANT_PUBLIC,
creator: dummyUser1,
lastUpdateUser: dummyUser1._id,
parent: rootPage._id,
},
{
_id: pageIdForRename16,
_id: childPageIdForRename7,
path: '/v5_ChildForRename7',
grant: Page.GRANT_PUBLIC,
parent: rootPage._id,
Expand All @@ -286,15 +307,15 @@ describe('PageService page operations with only public pages', () => {
grant: Page.GRANT_PUBLIC,
creator: dummyUser1,
lastUpdateUser: dummyUser1._id,
parent: pageIdForRename14,
parent: childPageIdForRename5,
updatedAt: new Date('2021'),
},
{
path: '/v5_ChildForRename7/v5_GrandchildForRename7',
grant: Page.GRANT_PUBLIC,
creator: dummyUser1,
lastUpdateUser: dummyUser1._id,
parent: pageIdForRename16,
parent: childPageIdForRename7,
},
{
_id: pageIdForRename17,
Expand Down Expand Up @@ -470,6 +491,7 @@ describe('PageService page operations with only public pages', () => {
const pageIdForDuplicate13 = new mongoose.Types.ObjectId();
const pageIdForDuplicate14 = new mongoose.Types.ObjectId();
const pageIdForDuplicate15 = new mongoose.Types.ObjectId();
const pageIdForDuplicate16 = new mongoose.Types.ObjectId();

// revision ids
const revisionIdForDuplicate1 = new mongoose.Types.ObjectId();
Expand Down Expand Up @@ -606,6 +628,13 @@ describe('PageService page operations with only public pages', () => {
parent: pageIdForDuplicate14,
revision: revisionIdForDuplicate12,
},
{
_id: pageIdForDuplicate16,
path: '/v5_PageForDuplicate16',
grant: Page.GRANT_PUBLIC,
parent: rootPage._id,
isEmpty: true,
},
]);

await Revision.insertMany([
Expand Down Expand Up @@ -1370,6 +1399,24 @@ describe('PageService page operations with only public pages', () => {

expect(isThrown).toBe(true);
});
test('Should rename/move to the path that exists as an empty page', async() => {
const page = await Page.findOne({ path: '/v5_ParentForRename10' });
const pageDistination = await Page.findOne({ path: '/v5_ParentForRename11', isEmpty: true });
expect(page).toBeTruthy();
expect(pageDistination).toBeTruthy();
expect(pageDistination.isEmpty).toBe(true);

const newPath = '/v5_ParentForRename11';
const renamedPage = await renamePage(page, newPath, dummyUser1, {}, {
ip: '::ffff:127.0.0.1',
endpoint: '/_api/v3/pages/rename',
});

expect(xssSpy).toHaveBeenCalled();
expect(renamedPage.path).toBe(newPath);
expect(renamedPage.isEmpty).toBe(false);
expect(renamedPage._id).toStrictEqual(page._id);
});
test('Rename non-empty page path to its descendant non-empty page path', async() => {
const initialPathForPage1 = '/v5_pageForRename17';
const initialPathForPage2 = '/v5_pageForRename17/v5_pageForRename18';
Expand Down Expand Up @@ -1696,6 +1743,24 @@ describe('PageService page operations with only public pages', () => {
expect(isThrown).toBe(true);
});

test('Should duplicate to the path that exists as an empty page', async() => {
const page = await Page.findOne({ path: '/v5_PageForDuplicate1' });
expect(page).toBeTruthy();

const newPagePath = '/v5_PageForDuplicate16';
const duplicatedPage = await duplicate(page, newPagePath, dummyUser1, false);

const duplicatedRevision = await Revision.findOne({ pageId: duplicatedPage._id });
const baseRevision = await Revision.findOne({ pageId: page._id });

// new path
expect(xssSpy).toHaveBeenCalled();
expect(duplicatedPage.path).toBe(newPagePath);
expect(duplicatedPage._id).not.toStrictEqual(page._id);
expect(duplicatedPage.revision).toStrictEqual(duplicatedRevision._id);
expect(duplicatedRevision.body).toEqual(baseRevision.body);
});

test('Should duplicate multiple pages', async() => {
const basePage = await Page.findOne({ path: '/v5_PageForDuplicate3' });
const revision = await Revision.findOne({ pageId: basePage._id });
Expand Down