fix: pins should not be inherited (#849)

* fix: add a regression test that should fail

* fix: do not show inherited pins

* fix: apply review comments

cypress/e2e/pinned.cy.ts

@@ -9,6 +9,7 @@ import {
   FOLDER_WITH_PINNED_ITEMS,
   FOLDER_WITH_SUBFOLDER_ITEM,
   PINNED_AND_HIDDEN_ITEM,
+  PINNED_ITEMS_SHOULD_NOT_INHERIT,
   PUBLIC_FOLDER_WITH_PINNED_ITEMS,
 } from '../fixtures/items';
 import { MEMBERS } from '../fixtures/members';
@@ -109,3 +110,48 @@ describe('Pinned and hidden children', () => {
     cy.get(`#${ITEM_PINNED_ID}`).should('not.exist');
   });
 });
+
+describe('Pinned should not be inherited', () => {
+  beforeEach(() => {
+    cy.setUpApi({
+      items: PINNED_ITEMS_SHOULD_NOT_INHERIT,
+    });
+  });
+  it('Shows only current level pins', () => {
+    const parent = PINNED_ITEMS_SHOULD_NOT_INHERIT[0];
+    const rootDocument = PINNED_ITEMS_SHOULD_NOT_INHERIT[1];
+    const child1 = PINNED_ITEMS_SHOULD_NOT_INHERIT[2];
+    const child2 = PINNED_ITEMS_SHOULD_NOT_INHERIT[3];
+    cy.visit(
+      buildContentPagePath({
+        rootId: parent.id,
+        itemId: parent.id,
+      }),
+    );
+
+    cy.get(`#${buildDocumentId(rootDocument.id)}`).should('be.visible');
+
+    // child folder 1
+    cy.visit(
+      buildContentPagePath({
+        rootId: parent.id,
+        itemId: child1.id,
+      }),
+    );
+    cy.get(`#${ITEM_PINNED_ID}`).should('not.exist');
+
+    // child folder 2
+    cy.visit(
+      buildContentPagePath({
+        rootId: parent.id,
+        itemId: child2.id,
+      }),
+    );
+    // pinned items should be open and contain the pinned item from child 2
+    cy.get(`#${ITEM_PINNED_ID}`)
+      .should('be.visible')
+      .and('contain.text', 'I am pinned from child 2');
+    // don't show the parent pinned item
+    cy.get(`#${buildDocumentId(rootDocument.id)}`).should('not.exist');
+  });
+});

cypress/fixtures/items.ts

@@ -29,9 +29,7 @@ export const DEFAULT_FOLDER_ITEM: MockItem = {
   path: '',
   type: ItemType.FOLDER,
   extra: {
-    [ItemType.FOLDER]: {
-      childrenOrder: [],
-    },
+    [ItemType.FOLDER]: {},
   },
   creator: CURRENT_USER,
   createdAt: new Date().toISOString(),
@@ -104,9 +102,7 @@ export const FOLDER_WITH_SUBFOLDER_ITEM: { items: MockItem[] } = {
       path: 'ecafbd2a_5688_11eb_ae93_0242ac130002',
       type: ItemType.FOLDER,
       extra: {
-        [ItemType.FOLDER]: {
-          childrenOrder: [],
-        },
+        [ItemType.FOLDER]: {},
       },
       settings: {
         isPinned: false,
@@ -167,9 +163,7 @@ export const FOLDER_WITH_SUBFOLDER_ITEM_AND_PARTIAL_ORDER: {
       name: 'parent folder',
       path: 'ecafbd2a_5688_11eb_ae93_0242ac130002',
       extra: {
-        [ItemType.FOLDER]: {
-          childrenOrder: ['fdf09f5a-5688-11eb-ae93-0242ac130003'],
-        },
+        [ItemType.FOLDER]: {},
       },
       settings: {
         isPinned: false,
@@ -253,6 +247,55 @@ export const FOLDER_WITH_PINNED_ITEMS: { items: MockItem[] } = {
   ],
 };
 
+const getPinnedElementWithoutInheritance = (): MockItem[] => {
+  const parent = FolderItemFactory({
+    name: 'Parent folder',
+    creator: CURRENT_USER,
+  });
+  const children = [
+    DocumentItemFactory({
+      name: 'pinned from root',
+      extra: { document: { content: 'I am pinned from parent' } },
+      settings: { isPinned: true },
+      parentItem: parent,
+      creator: CURRENT_USER,
+    }),
+    FolderItemFactory({
+      name: 'child folder 1',
+      settings: { isPinned: false },
+      parentItem: parent,
+      creator: CURRENT_USER,
+    }),
+    FolderItemFactory({
+      name: 'child folder 2',
+      settings: { isPinned: false },
+      parentItem: parent,
+      creator: CURRENT_USER,
+    }),
+  ];
+  const childrenOfChildren = [
+    DocumentItemFactory({
+      name: 'text in children 1',
+      extra: { document: { content: 'Not pinned' } },
+      settings: { isPinned: false },
+      parentItem: children[1],
+      creator: CURRENT_USER,
+    }),
+    DocumentItemFactory({
+      name: 'pinned text in children 2',
+      extra: { document: { content: 'I am pinned from child 2' } },
+      parentItem: children[2],
+      settings: { isPinned: true },
+      creator: CURRENT_USER,
+    }),
+  ];
+  const items = [parent, ...children, ...childrenOfChildren];
+  return items;
+};
+
+export const PINNED_ITEMS_SHOULD_NOT_INHERIT =
+  getPinnedElementWithoutInheritance();
+
 export const PINNED_AND_HIDDEN_ITEM: { items: MockItem[] } = {
   items: [
     {
@@ -563,8 +606,6 @@ export const FOLDER_WITHOUT_CHILDREN_ORDER: { items: MockItem[] } = {
       name: 'parent folder',
       path: 'ecafbd2a_5688_11eb_ae93_0242ac130122',
       extra: {
-        // eslint-disable-next-line @typescript-eslint/ban-ts-comment
-        // @ts-expect-error
         [ItemType.FOLDER]: {},
       },
       settings: {

cypress/support/server.ts

@@ -112,7 +112,7 @@ export const mockGetSharedItems = ({
   ).as('getSharedItems');
 };
 
-export const mockGetAccessibleItems = (items: DiscriminatedItem[]): void => {
+export const mockGetAccessibleItems = (items: MockItem[]): void => {
   cy.intercept(
     {
       method: HttpMethod.Get,

src/modules/item/Item.tsx

@@ -423,7 +423,11 @@ const ItemContent = ({ item }: ItemContentProps) => {
   }
 };
 
-const ItemContentWrapper = ({ item }: { item: PackedItem }) => {
+export const ItemContentWrapper = ({
+  item,
+}: {
+  item: PackedItem;
+}): JSX.Element | null => {
   // An item the user has access to can be hidden (write, admin) so we hide it in player
   if (item.hidden) {
     return null;

src/modules/navigation/tree/LoadingTree.tsx

@@ -2,8 +2,8 @@ import { Skeleton, Stack, Typography } from '@mui/material';
 
 const LoadingTree = (): JSX.Element => (
   <Stack p={1}>
-    {Array.from(Array(5)).map(() => (
-      <Typography>
+    {['elem 1', 'elem 2', 'elem 3', 'elem 4', 'elem 5'].map((el) => (
+      <Typography key={el}>
         <Skeleton variant="text" />
       </Typography>
     ))}

src/modules/rightPanel/SideContent.tsx

@@ -6,15 +6,15 @@ import ExitFullscreenIcon from '@mui/icons-material/FullscreenExit';
 import { Grid, Stack, Tooltip, styled } from '@mui/material';
 import IconButton from '@mui/material/IconButton';
 
-import { DiscriminatedItem, ItemType, getIdsFromPath } from '@graasp/sdk';
+import { DiscriminatedItem } from '@graasp/sdk';
 import { useMobileView } from '@graasp/ui';
 
 import { usePlayerTranslation } from '@/config/i18n';
 import { hooks } from '@/config/queryClient';
 import { useLayoutContext } from '@/contexts/LayoutContext';
 import { PLAYER } from '@/langs/constants';
 import Chatbox from '@/modules/chatbox/Chatbox';
-import Item from '@/modules/item/Item';
+import { ItemContentWrapper } from '@/modules/item/Item';
 
 import { DRAWER_WIDTH, FLOATING_BUTTON_Z_INDEX } from '../../config/constants';
 import {
@@ -79,29 +79,15 @@ const SideContent = ({ content, item }: Props): JSX.Element | null => {
 
   const { t } = usePlayerTranslation();
   const settings = item.settings ?? {};
-  const isFolder = item.type === ItemType.FOLDER;
 
   if (!rootId) {
     return null;
   }
 
-  /* This removes the parents that are higher than the perform root element
-  Ex: if we are in item 6 and the root is 3, when splitting the path we get [ 1, 2, 3, 4, 5, 6 ].
-  However the student cannot go higher than element 3, so we remove the element before 3, this
-  gives us [ 3, 4, 5, 6], which is the visible range of the student. */
-  const parents = getIdsFromPath(item.path);
-  const parentsIds = parents.slice(
-    parents.indexOf(rootId),
-    /* When splitting the path, it returns the current element in the array.
-    However because we use the item components, if the item is not a folder it will be rendered
-    pinned or not. Because we just loop over the parents to get their pinned items.
-    If the item is a folder, we can keep it in the path to show the items that are pinned in it */
-    isFolder ? parents.length : -1,
+  const pinnedItems = children?.filter(
+    ({ settings: s, hidden }) => s.isPinned && !hidden,
   );
-
-  const pinnedCount =
-    children?.filter(({ settings: s, hidden }) => s.isPinned && !hidden)
-      ?.length || 0;
+  const pinnedCount = pinnedItems?.length || 0;
 
   const toggleFullscreen = () => {
     setIsFullscreen(!isFullscreen);
@@ -154,22 +140,23 @@ const SideContent = ({ content, item }: Props): JSX.Element | null => {
   };
 
   const displayPinnedItems = () => {
-    if (!pinnedCount) return null;
-
-    return (
-      <SideDrawer
-        title={t(PLAYER.PINNED_ITEMS)}
-        onClose={togglePinned}
-        open={isPinnedOpen}
-      >
-        {/* show parents pinned items */}
-        <Stack id={ITEM_PINNED_ID} gap={2} mt={1} pb={9}>
-          {parentsIds.map((i) => (
-            <Item key={i} id={i} showPinnedOnly />
-          ))}
-        </Stack>
-      </SideDrawer>
-    );
+    if (pinnedItems?.length) {
+      return (
+        <SideDrawer
+          title={t(PLAYER.PINNED_ITEMS)}
+          onClose={togglePinned}
+          open={isPinnedOpen}
+        >
+          {/* show children pinned items */}
+          <Stack id={ITEM_PINNED_ID} gap={2} mt={1} pb={9}>
+            {pinnedItems.map((pinnedItem) => (
+              <ItemContentWrapper item={pinnedItem} />
+            ))}
+          </Stack>
+        </SideDrawer>
+      );
+    }
+    return null;
   };
 
   return (