WIP: refactor models/page.js

* refactor Page.findBy*

src/server/models/page.js

@@ -445,15 +445,11 @@ module.exports = function(crowi) {
     });
   };
 
-  pageSchema.statics.findOneById = async function(id) {
-    return this.findOne({_id: id});
-  };
-
   /**
    * @param {string} id ObjectId
    * @param {User} user User instance
    */
-  pageSchema.statics.findOneByIdAndViewer = async function(id, user) {
+  pageSchema.statics.findByIdAndViewer = async function(id, user) {
     validateCrowi();
 
     // const Page = this;
@@ -471,6 +467,40 @@ module.exports = function(crowi) {
     return await queryBuilder.query.exec();
   };
 
+  // find page by path
+  pageSchema.statics.findByPath = function(path) {
+    if (path == null) {
+      return null;
+    }
+    return this.findOne({path});
+  };
+
+  pageSchema.statics.findByPathAndViewer = async function(path, user) {
+    validateCrowi();
+
+    if (path == null) {
+      throw new Error('path is required.');
+    }
+
+    // const Page = this;
+    const baseQuery = this.findOne({path});
+
+    const UserGroupRelation = crowi.model('UserGroupRelation');
+    let userGroups = [];
+    if (user != null) {
+      userGroups = await UserGroupRelation.findAllUserGroupIdsRelatedToUser(user);
+    }
+
+    const queryBuilder = new PageQueryBuilder(baseQuery);
+    queryBuilder.addConditionToFilteringByViewer(user, userGroups);
+
+    return await queryBuilder.query.exec();
+  };
+
+  pageSchema.statics.findByRedirectTo = function(path) {
+    return this.findOne({redirectTo: path});
+  };
+
   /**
    * find all templates applicable to the new page
    */
@@ -551,36 +581,6 @@ module.exports = function(crowi) {
     return templateBody;
   };
 
-  // find page by path
-  pageSchema.statics.findPageByPath = function(path) {
-    if (path == null) {
-      return null;
-    }
-    return this.findOne({path});
-  };
-
-  pageSchema.statics.findPageByPathAndViewer = async function(path, user) {
-    validateCrowi();
-
-    if (path == null) {
-      throw new Error('path is required.');
-    }
-
-    // const Page = this;
-    const baseQuery = this.findOne({path});
-
-    const UserGroupRelation = crowi.model('UserGroupRelation');
-    let userGroups = [];
-    if (user != null) {
-      userGroups = await UserGroupRelation.findAllUserGroupIdsRelatedToUser(user);
-    }
-
-    const queryBuilder = new PageQueryBuilder(baseQuery);
-    queryBuilder.addConditionToFilteringByViewer(user, userGroups);
-
-    return await queryBuilder.query.exec();
-  };
-
   pageSchema.statics.findListByPageIds = function(ids, options) {
     validateCrowi();
 
@@ -617,20 +617,6 @@ module.exports = function(crowi) {
     });
   };
 
-  pageSchema.statics.findPageByRedirectTo = function(path) {
-    var Page = this;
-
-    return new Promise(function(resolve, reject) {
-      Page.findOne({redirectTo: path}, function(err, pageData) {
-        if (err || pageData === null) {
-          return reject(err);
-        }
-
-        return resolve(pageData);
-      });
-    });
-  };
-
   pageSchema.statics.findListByCreator = async function(user, option, currentUser) {
     validateCrowi();
 
@@ -975,7 +961,7 @@ module.exports = function(crowi) {
     let savedPage = await pageData.save();
     const newRevision = await Revision.prepareRevision(pageData, body, user);
     const revision = await pushRevision(savedPage, newRevision, user, grant, grantUserGroupId);
-    savedPage = await Page.findPageByPath(revision.path).populate('revision').populate('creator');
+    savedPage = await Page.findByPath(revision.path).populate('revision').populate('creator');
 
     if (isSyncRevisionToHackmd) {
       savedPage = await Page.syncRevisionToHackmd(savedPage);
@@ -1042,15 +1028,18 @@ module.exports = function(crowi) {
   pageSchema.statics.revertDeletedPage = async function(page, user, options) {
     const newPath = this.getRevertDeletedPageName(page.path);
 
-    // 削除時、元ページの path には必ず redirectTo 付きで、ページが作成される。
-    // そのため、そいつは削除してOK
-    // が、redirectTo ではないページが存在している場合それは何かがおかしい。(データ補正が必要)
-    const originPage = await this.findPageByPath(newPath);
-    if (originPage.redirectTo !== page.path) {
-      throw new Error('The new page of to revert is exists and the redirect path of the page is not the deleted page.');
+    const originPage = await this.findByPath(newPath);
+    if (originPage != null) {
+      // 削除時、元ページの path には必ず redirectTo 付きで、ページが作成される。
+      // そのため、そいつは削除してOK
+      // が、redirectTo ではないページが存在している場合それは何かがおかしい。(データ補正が必要)
+      if (originPage.redirectTo !== page.path) {
+        throw new Error('The new page of to revert is exists and the redirect path of the page is not the deleted page.');
+      }
+
+      await this.completelyDeletePage(originPage, options);
     }
 
-    await this.completelyDeletePage(originPage, options);
     await this.updatePageProperty(page, {status: STATUS_PUBLISHED, lastUpdateUser: user});
 
     page.status = STATUS_PUBLISHED;
@@ -1078,44 +1067,31 @@ module.exports = function(crowi) {
   /**
    * This is danger.
    */
-  pageSchema.statics.completelyDeletePage = function(pageData, user, options = {}) {
+  pageSchema.statics.completelyDeletePage = async function(pageData, user, options = {}) {
     validateCrowi();
 
     // Delete Bookmarks, Attachments, Revisions, Pages and emit delete
     const Bookmark = crowi.model('Bookmark')
-      , Attachment = crowi.model('Attachment')
-      , Comment = crowi.model('Comment')
-      , Revision = crowi.model('Revision')
-      , PageGroupRelation = crowi.model('PageGroupRelation')
-      , Page = this
-      , pageId = pageData._id
-      , socketClientId = options.socketClientId || null
-      ;
+    const Attachment = crowi.model('Attachment');
+    const Comment = crowi.model('Comment');
+    const Revision = crowi.model('Revision');
+    const PageGroupRelation = crowi.model('PageGroupRelation');
+    const pageId = pageData._id;
+    const socketClientId = options.socketClientId || null;
 
     debug('Completely delete', pageData.path);
 
-    return new Promise(function(resolve, reject) {
-      Bookmark.removeBookmarksByPageId(pageId)
-      .then(function(done) {
-      }).then(function(done) {
-        return Attachment.removeAttachmentsByPageId(pageId);
-      }).then(function(done) {
-        return Comment.removeCommentsByPageId(pageId);
-      }).then(function(done) {
-        return Revision.removeRevisionsByPath(pageData.path);
-      }).then(function(done) {
-        return Page.removePageById(pageId);
-      }).then(function(done) {
-        return Page.removeRedirectOriginPageByPath(pageData.path);
-      }).then(function(done) {
-        return PageGroupRelation.removeAllByPage(pageData);
-      }).then(function(done) {
-        if (socketClientId != null) {
-          pageEvent.emit('delete', pageData, user, socketClientId); // update as renamed page
-        }
-        resolve(pageData);
-      }).catch(reject);
-    });
+    await Bookmark.removeBookmarksByPageId(pageId);
+    await Attachment.removeAttachmentsByPageId(pageId);
+    await Comment.removeCommentsByPageId(pageId);
+    await Revision.removeRevisionsByPath(pageData.path);
+    await this.findByIdAndRemove(pageId);
+    await this.removeRedirectOriginPageByPath(pageData.path);
+    await PageGroupRelation.removeAllByPage(pageData);
+    if (socketClientId != null) {
+      pageEvent.emit('delete', pageData, user, socketClientId); // update as renamed page
+    }
+    return pageData;
   };
 
   pageSchema.statics.completelyDeletePageRecursively = function(pageData, user, options = {}) {
@@ -1139,28 +1115,11 @@ module.exports = function(crowi) {
     });
   };
 
-  pageSchema.statics.removePageById = function(pageId) {
-    var Page = this;
-
-    return new Promise(function(resolve, reject) {
-      Page.remove({_id: pageId}, function(err, done) {
-        debug('Remove phisiaclly, the page', pageId, err, done);
-        if (err) {
-          return reject(err);
-        }
-
-        resolve(done);
-      });
-    });
-  };
-
-  pageSchema.statics.removePageByPath = function(pagePath) {
-    var Page = this;
-
-    return Page.findPageByPath(pagePath)
-      .then(function(pageData) {
-        return Page.removePageById(pageData.id);
-      });
+  pageSchema.statics.removeByPath = function(path) {
+    if (path == null) {
+      throw new Error('path is required');
+    }
+    return this.findOneAndRemove({ path }).exec();
   };
 
   /**
@@ -1173,22 +1132,22 @@ module.exports = function(crowi) {
    *
    * @param {string} pagePath
    */
-  pageSchema.statics.removeRedirectOriginPageByPath = function(pagePath) {
-    var Page = this;
+  pageSchema.statics.removeRedirectOriginPageByPath = async function(pagePath) {
+    const isExist = await this.count({ path: pagePath }) > 0;
+    if (!isExist) {
+      return;
+    }
 
-    return Page.findPageByRedirectTo(pagePath)
-      .then((redirectOriginPageData) => {
-        // remove
-        return Page.removePageById(redirectOriginPageData.id)
-          // remove recursive
-          .then(() => {
-            return Page.removeRedirectOriginPageByPath(redirectOriginPageData.path);
-          });
-      })
-      .catch((err) => {
-        // do nothing if origin page doesn't exist
-        return Promise.resolve();
-      });
+    const redirectPage = await this.findByRedirectTo(pagePath);
+
+    if (redirectPage == null) {
+      return;
+    }
+
+    // remove
+    await this.findByIdAndRemove(redirectPage.id);
+    // remove recursive
+    await this.removeRedirectOriginPageByPath(redirectPage.path);
   };
 
   pageSchema.statics.rename = async function(pageData, newPagePath, user, options) {

src/server/routes/admin.js

@@ -606,7 +606,7 @@ module.exports = function(crowi, app) {
       return ExternalAccount.remove({user: userData}).then(() => userData);
     })
     .then((userData) => {
-      return Page.removePageByPath(`/user/${username}`).then(() => userData);
+      return Page.removeByPath(`/user/${username}`).then(() => userData);
     })
     .then((userData) => {
       req.flash('successMessage', `${username} さんのアカウントを削除しました`);

src/server/routes/attachment.js

@@ -118,7 +118,7 @@ module.exports = function(crowi, app) {
           .catch(reject);
       }
       else {
-        Page.findPageById(id).then(resolve).catch(reject);
+        Page.findById(id).then(resolve).catch(reject);
       }
     }).then(function(pageData) {
       page = pageData;

src/server/routes/bookmark.js

@@ -46,7 +46,7 @@ module.exports = function(crowi, app) {
   actions.api.add = function(req, res) {
     var pageId = req.body.page_id;
 
-    Page.findPageByIdAndGrantedUser(pageId, req.user)
+    Page.findByIdAndGrantedUser(pageId, req.user)
     .then(function(pageData) {
       if (pageData) {
         return Bookmark.add(pageData, req.user);

src/server/routes/page.js

@@ -173,7 +173,7 @@ module.exports = function(crowi, app) {
     let path = getPathFromRequest(req);
     const revisionId = req.query.revision;
 
-    let page = await Page.findPageByPathAndViewer(path, req.user);
+    let page = await Page.findByPathAndViewer(path, req.user);
 
     if (page == null) {
       next();
@@ -194,7 +194,7 @@ module.exports = function(crowi, app) {
     // check whether this page has portal page
     const potalPagePath = `${path}/`;
 
-    let potalPage = await Page.findPageByPathAndViewer(potalPagePath, req.user);
+    let potalPage = await Page.findByPathAndViewer(potalPagePath, req.user);
 
     const limit = 50;
     const offset = parseInt(req.query.offset)  || 0;
@@ -220,7 +220,7 @@ module.exports = function(crowi, app) {
     // check whether this page has portal page
     const potalPagePath = `${path}/`;
 
-    let potalPage = await Page.findPageByPathAndViewer(potalPagePath, req.user);
+    let potalPage = await Page.findByPathAndViewer(potalPagePath, req.user);
 
     if (potalPage != null) {
       logger.debug('The portal page is found when processing showPageForCrowiBehavior', potalPage._id, potalPage.path);
@@ -235,7 +235,7 @@ module.exports = function(crowi, app) {
     const path = getPathFromRequest(req);
     const revisionId = req.query.revision;
 
-    let page = await Page.findPageByPathAndViewer(path, req.user);
+    let page = await Page.findByPathAndViewer(path, req.user);
 
     if (page == null) {
       // check the page is forbidden or just does not exist.
@@ -473,7 +473,7 @@ module.exports = function(crowi, app) {
   actions.redirector = async function(req, res) {
     const id = req.params.id;
 
-    const page = await Page.findOneByIdAndViewer(id, req.user);
+    const page = await Page.findByIdAndViewer(id, req.user);
 
     if (page != null) {
       return res.redirect(pagePathUtils.encodePagePath(page.path));
@@ -618,7 +618,7 @@ module.exports = function(crowi, app) {
     }
 
     let previousRevision = undefined;
-    let updatedPage = await Page.findOneByIdAndViewer(pageId, req.user)
+    let updatedPage = await Page.findByIdAndViewer(pageId, req.user)
       .then(function(pageData) {
         if (pageData && revisionId !== null && !pageData.isUpdatable(revisionId)) {
           throw new Error('Posted param "revisionId" is outdated.');
@@ -672,7 +672,6 @@ module.exports = function(crowi, app) {
   api.get = async function(req, res) {
     const pagePath = req.query.path || null;
     const pageId = req.query.page_id || null; // TODO: handling
-    const revisionId = req.query.revision_id || null;
 
     if (!pageId && !pagePath) {
       return res.json(ApiResponse.error(new Error('Parameter path or page_id is required.')));
@@ -681,10 +680,10 @@ module.exports = function(crowi, app) {
     let page;
     try {
       if (pageId) { // prioritized
-        page = await Page.findOneByIdAndViewer(pageId, req.user);
+        page = await Page.findByIdAndViewer(pageId, req.user);
       }
       else if (pagePath) {
-        page = await Page.findPageByPathAndViewer(pagePath, req.user);
+        page = await Page.findByPathAndViewer(pagePath, req.user);
       }
       // https://weseek.myjetbrains.com/youtrack/issue/GC-1224
       // TODO populate for backward compatibility
@@ -717,7 +716,7 @@ module.exports = function(crowi, app) {
 
     let page;
     try {
-      page = await Page.findOneByIdAndViewer(pageId, req.user);
+      page = await Page.findByIdAndViewer(pageId, req.user);
       if (req.user != null) {
         page = await page.seen(req.user);
       }
@@ -751,7 +750,7 @@ module.exports = function(crowi, app) {
 
     let page;
     try {
-      page = await Page.findOneByIdAndViewer(pageId, req.user);
+      page = await Page.findByIdAndViewer(pageId, req.user);
       page = await page.like(req.user);
     }
     catch (err) {
@@ -790,7 +789,7 @@ module.exports = function(crowi, app) {
 
     let page;
     try {
-      page = await Page.findOneByIdAndViewer(pageId, req.user);
+      page = await Page.findByIdAndViewer(pageId, req.user);
       page = await page.unlike(req.user);
     }
     catch (err) {
@@ -852,7 +851,7 @@ module.exports = function(crowi, app) {
 
     const options = {socketClientId};
 
-    let page = await Page.findOneByIdAndViewer(pageId, req.user);
+    let page = await Page.findByIdAndViewer(pageId, req.user);
 
     if (page == null) {
       return res.json(ApiResponse.error('The page does not exist.'));
@@ -883,7 +882,7 @@ module.exports = function(crowi, app) {
       }
     }
     catch (err) {
-      logger.error('Error occured while get setting', err, err.stack);
+      logger.error('Error occured while get setting', err);
       return res.json(ApiResponse.error('Failed to delete page.'));
     }
 
@@ -913,7 +912,7 @@ module.exports = function(crowi, app) {
 
     let page;
     try {
-      page = await Page.findOneByIdAndViewer(pageId, req.user);
+      page = await Page.findByIdAndViewer(pageId, req.user);
       if (page == null) {
         throw new Error('The page is not found or the user does not have permission');
       }
@@ -926,7 +925,7 @@ module.exports = function(crowi, app) {
       }
     }
     catch (err) {
-      logger.error('Error occured while get setting', err, err.stack);
+      logger.error('Error occured while get setting', err);
       return res.json(ApiResponse.error('Failed to revert deleted page.'));
     }
 
@@ -962,14 +961,14 @@ module.exports = function(crowi, app) {
       return res.json(ApiResponse.error(`このページ名は作成できません (${newPagePath})`));
     }
 
-    Page.findPageByPath(newPagePath)
+    Page.findByPath(newPagePath)
     .then(function(page) {
       if (page != null) {
         // if page found, cannot cannot rename to that path
         return res.json(ApiResponse.error(`このページ名は作成できません (${newPagePath})。ページが存在します。`));
       }
 
-      Page.findPageById(pageId)
+      Page.findById(pageId)
       .then(function(pageData) {
         page = pageData;
         if (!pageData.isUpdatable(previousRevision)) {
@@ -1014,7 +1013,7 @@ module.exports = function(crowi, app) {
     const pageId = req.body.page_id;
     const newPagePath = Page.normalizePath(req.body.new_path);
 
-    Page.findPageById(pageId)
+    Page.findById(pageId)
       .then(function(pageData) {
         req.body.path = newPagePath;
         req.body.body = pageData.revision.body;
@@ -1035,7 +1034,7 @@ module.exports = function(crowi, app) {
   api.unlink = function(req, res) {
     const pageId = req.body.page_id;
 
-    Page.findPageByIdAndGrantedUser(pageId, req.user)
+    Page.findByIdAndGrantedUser(pageId, req.user)
     .then(function(pageData) {
       debug('Unlink page', pageData._id, pageData.path);
 
@@ -1060,7 +1059,7 @@ module.exports = function(crowi, app) {
       return res.json(ApiResponse.error('param \'pageId\' must not be null'));
     }
 
-    const page = await Page.findPageById(pageId);
+    const page = await Page.findById(pageId);
     if (page == null) {
       return res.json(ApiResponse.error(`Page (id='${pageId}') does not exist`));
     }

src/server/routes/revision.js

@@ -44,7 +44,7 @@ module.exports = function(crowi, app) {
     const pageId = req.query.page_id || null;
 
     if (pageId && crowi.isPageId(pageId)) {
-      Page.findOneByIdAndViewer(pageId, req.user)
+      Page.findByIdAndViewer(pageId, req.user)
       .then(function(pageData) {
         debug('Page found', pageData._id, pageData.path);
         return Revision.findRevisionIdList(pageData.path);
@@ -72,7 +72,7 @@ module.exports = function(crowi, app) {
     const pageId = req.query.page_id || null;
 
     if (pageId) {
-      Page.findOneByIdAndViewer(pageId, req.user)
+      Page.findByIdAndViewer(pageId, req.user)
       .then(function(pageData) {
         debug('Page found', pageData._id, pageData.path);
         return Revision.findRevisionList(pageData.path, {});

src/test/models/page.test.js

@@ -317,10 +317,10 @@ describe('Page', () => {
   });
 
   describe('.findPage', () => {
-    context('findPageById', () => {
+    context('findById', () => {
       it('should find page', done => {
         const pageToFind = createdPages[0];
-        Page.findPageById(pageToFind._id)
+        Page.findById(pageToFind._id)
         .then(pageData => {
           expect(pageData.path).to.equal(pageToFind.path);
           done();
@@ -328,11 +328,11 @@ describe('Page', () => {
       });
     });
 
-    context('findPageByIdAndGrantedUser', () => {
+    context('findByIdAndGrantedUser', () => {
       it('should find page', done => {
         const pageToFind = createdPages[0];
         const grantedUser = createdUsers[0];
-        Page.findPageByIdAndGrantedUser(pageToFind._id, grantedUser)
+        Page.findByIdAndGrantedUser(pageToFind._id, grantedUser)
         .then((pageData) => {
           expect(pageData.path).to.equal(pageToFind.path);
           done();
@@ -345,7 +345,7 @@ describe('Page', () => {
       it('should error by grant', done => {
         const pageToFind = createdPages[0];
         const grantedUser = createdUsers[1];
-        Page.findPageByIdAndGrantedUser(pageToFind._id, grantedUser)
+        Page.findByIdAndGrantedUser(pageToFind._id, grantedUser)
         .then(pageData => {
           done(new Error());
         }).catch(err => {
@@ -355,11 +355,11 @@ describe('Page', () => {
       });
     });
 
-    context('findPageByIdAndGrantedUser granted userGroup', () => {
+    context('findByIdAndGrantedUser granted userGroup', () => {
       it('should find page', done => {
         const pageToFind = createdPages[6];
         const grantedUser = createdUsers[0];
-        Page.findPageByIdAndGrantedUser(pageToFind._id, grantedUser)
+        Page.findByIdAndGrantedUser(pageToFind._id, grantedUser)
         .then(pageData => {
           expect(pageData.path).to.equal(pageToFind.path);
           done();
@@ -372,7 +372,7 @@ describe('Page', () => {
       it('should error by grant userGroup', done => {
         const pageToFind = createdPages[6];
         const grantedUser = createdUsers[2];
-        Page.findPageByIdAndGrantedUser(pageToFind._id, grantedUser)
+        Page.findByIdAndGrantedUser(pageToFind._id, grantedUser)
           .then(pageData => {
             done(new Error());
           }).catch(err => {