Merge pull request #5411 from weseek/fix/bookmark-operation

fix: Bookmark operation

packages/app/src/components/Common/Dropdown/PageItemControl.tsx

@@ -88,7 +88,7 @@ const PageItemControlDropdownMenu = React.memo((props: DropdownMenuProps): JSX.E
       return;
     }
     await onClickRevertMenuItem(pageId);
-  }, [onClickRevertMenuItem]);
+  }, [onClickRevertMenuItem, pageId]);
 
 
   // eslint-disable-next-line react-hooks/rules-of-hooks
@@ -212,7 +212,8 @@ export const PageItemControlSubstance = (props: PageItemControlSubstanceProps):
   const shouldFetch = fetchOnInit === true || (!isIPageInfoForOperation(presetPageInfo) && isOpen);
   const shouldMutate = fetchOnInit === true || !isIPageInfoForOperation(presetPageInfo);
 
-  const { data: fetchedPageInfo, mutate: mutatePageInfo } = useSWRxPageInfo(shouldFetch ? pageId : null);
+  const { data: fetchedPageInfo } = useSWRxPageInfo(shouldFetch ? pageId : null);
+  const { mutate: mutatePageInfo } = useSWRxPageInfo(shouldMutate ? pageId : null);
 
   // mutate after handle event
   const bookmarkMenuItemClickHandler = useCallback(async(_pageId: string, _newValue: boolean) => {

packages/app/src/components/PageList/PageListItemL.tsx

@@ -11,6 +11,10 @@ import urljoin from 'url-join';
 
 import { UserPicture, PageListMeta } from '@growi/ui';
 import { DevidedPagePath } from '@growi/core';
+
+
+import { ISelectable } from '~/client/interfaces/selectable-all';
+import { bookmark, unbookmark } from '~/client/services/page-operation';
 import { useIsDeviceSmallerThanLg } from '~/stores/ui';
 import {
   usePageRenameModal, usePageDuplicateModal, usePageDeleteModal, usePutBackPageModal,
@@ -20,11 +24,10 @@ import {
 } from '~/interfaces/page';
 import { IPageSearchMeta, isIPageSearchMeta } from '~/interfaces/search';
 import { OnDeletedFunction } from '~/interfaces/ui';
+import LinkedPagePath from '~/models/linked-page-path';
 
 import { ForceHideMenuItems, PageItemControl } from '../Common/Dropdown/PageItemControl';
-import LinkedPagePath from '~/models/linked-page-path';
 import PagePathHierarchicalLink from '../PagePathHierarchicalLink';
-import { ISelectable } from '~/client/interfaces/selectable-all';
 
 type Props = {
   page: IPageWithMeta | IPageWithMeta<IPageInfoAll & IPageSearchMeta>,
@@ -92,6 +95,11 @@ const PageListItemLSubstance: ForwardRefRenderFunction<ISelectable, Props> = (pr
     }
   }, [isDeviceSmallerThanLg, onClickItem, pageData._id]);
 
+  const bookmarkMenuItemClickHandler = async(_pageId: string, _newValue: boolean): Promise<void> => {
+    const bookmarkOperation = _newValue ? bookmark : unbookmark;
+    await bookmarkOperation(_pageId);
+  };
+
   const duplicateMenuItemClickHandler = useCallback(() => {
     const page = {
       pageId: pageData._id,
@@ -206,6 +214,7 @@ const PageListItemLSubstance: ForwardRefRenderFunction<ISelectable, Props> = (pr
                   pageInfo={pageMeta}
                   isEnableActions={isEnableActions}
                   forceHideMenuItems={forceHideMenuItems}
+                  onClickBookmarkMenuItem={bookmarkMenuItemClickHandler}
                   onClickRenameMenuItem={renameMenuItemClickHandler}
                   onClickDuplicateMenuItem={duplicateMenuItemClickHandler}
                   onClickDeleteMenuItem={deleteMenuItemClickHandler}

packages/app/src/server/routes/apiv3/bookmarks.js

@@ -258,6 +258,11 @@ module.exports = (crowi) => {
    */
   router.put('/', accessTokenParser, loginRequiredStrictly, csrf, validator.bookmarks, apiV3FormValidator, async(req, res) => {
     const { pageId, bool } = req.body;
+    const userId = req.user?._id;
+
+    if (userId == null) {
+      return res.apiv3Err('A logged in user is required.');
+    }
 
     let bookmark;
     try {
@@ -265,15 +270,29 @@ module.exports = (crowi) => {
       if (page == null) {
         return res.apiv3Err(`Page '${pageId}' is not found or forbidden`);
       }
-      if (bool) {
-        bookmark = await Bookmark.add(page, req.user);
 
-        const pageEvent = crowi.event('page');
-        // in-app notification
-        pageEvent.emit('bookmark', page, req.user);
+      bookmark = await Bookmark.findByPageIdAndUserId(page._id, req.user._id);
+
+      if (bookmark == null) {
+        if (bool) {
+          bookmark = await Bookmark.add(page, req.user);
+
+          const pageEvent = crowi.event('page');
+          // in-app notification
+          pageEvent.emit('bookmark', page, req.user);
+        }
+        else {
+          logger.warn(`Removing the bookmark for ${page._id} by ${req.user._id} failed because the bookmark does not exist.`);
+        }
       }
       else {
-        bookmark = await Bookmark.removeBookmark(page, req.user);
+        // eslint-disable-next-line no-lonely-if
+        if (bool) {
+          logger.warn(`Adding the bookmark for ${page._id} by ${req.user._id} failed because the bookmark has already exist.`);
+        }
+        else {
+          bookmark = await Bookmark.removeBookmark(page, req.user);
+        }
       }
     }
     catch (err) {
@@ -281,8 +300,10 @@ module.exports = (crowi) => {
       return res.apiv3Err(err, 500);
     }
 
-    bookmark.depopulate('page');
-    bookmark.depopulate('user');
+    if (bookmark != null) {
+      bookmark.depopulate('page');
+      bookmark.depopulate('user');
+    }
 
     return res.apiv3({ bookmark });
   });

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

@@ -266,9 +266,9 @@ class PageService {
       };
     }
 
-    const isBookmarked = await Bookmark.findByPageIdAndUserId(pageId, user._id);
-    const isLiked = page.isLiked(user);
-    const isAbleToDeleteCompletely = this.canDeleteCompletely((page.creator as IUserHasId)?._id, user);
+    const isBookmarked: boolean = (await Bookmark.findByPageIdAndUserId(pageId, user._id)) != null;
+    const isLiked: boolean = page.isLiked(user);
+    const isAbleToDeleteCompletely: boolean = this.canDeleteCompletely((page.creator as IUserHasId)?._id, user);
 
     const subscription = await Subscription.findByUserIdAndTargetId(user._id, pageId);
 

packages/app/src/stores/page.tsx

@@ -82,8 +82,11 @@ export const useSWRxPageInfo = (
     shareLinkId?: string | null,
 ): SWRResponse<IPageInfo | IPageInfoForOperation, Error> => {
 
+  // assign null if shareLinkId is undefined in order to identify SWR key only by pageId
+  const fixedShareLinkId = shareLinkId ?? null;
+
   return useSWRImmutable(
-    pageId != null ? ['/page/info', pageId, shareLinkId] : null,
+    pageId != null ? ['/page/info', pageId, fixedShareLinkId] : null,
     (endpoint, pageId, shareLinkId) => apiv3Get(endpoint, { pageId, shareLinkId }).then(response => response.data),
   );
 };