Repository: ProjectMirador/mirador (js)

Issue:
Link to collection disappears when clicking "Show Collection"
I'm having trouble understanding the intended behavior of Mirador when it comes to browsing collections. The following steps seem like a bug to me.

Steps to reproduce:

1. Go to https://projectmirador.org/embed/?iiif-content=https://iiif.io/api/cookbook/recipe/0032-collection/collection.json
2. Click on "Show Collection"
3. Open the first item in the collection: "The Gulf Stream"
4. Click on the hamburger menu in the top-left to "Toggle Sidebar"
5. In the sidebar, there is a subsection for the collection: "Simple Collection Example" with a "Show Collection" button.
6. If you click on that "Show Collection" button, this subsection disappears from the sidebar.
7. If you click "Close" on the collection dialog in the center of the screen, you have no way of getting back to the collection. The "Simple Collection Example" that was previously present in the sidebar is now gone, and it's impossible to get it back.

I can't imagine why this would be intended behavior. Why does the "Simple Collection Example" disappear from the sidebar when you click on "Show Collection"? If you select an item from the collection in the center dialog, it will come back, but not if you click "Close", which puts you in a strange state where the collection is now inaccessible.

Pull request:
Fix #4152, use separate state for collection info panel and collection selection dialog
Replacement for PR #4154; addresses problems mentioned in [this comment](https://github.com/ProjectMirador/mirador/pull/4154#issuecomment-2949717883).

Patch:
diff --git a/src/components/CollectionDialog.jsx b/src/components/CollectionDialog.jsx
index 56067e26..0c0593a6 100644
--- a/src/components/CollectionDialog.jsx
+++ b/src/components/CollectionDialog.jsx
@@ -66,7 +66,7 @@ Placeholder.propTypes = {
  * a dialog providing the possibility to select the collection
  */
 export function CollectionDialog({
-  addWindow, collection = null, collectionPath = [], error = null, hideCollectionDialog,
+  addWindow, collection = null, dialogCollectionPath = [], error = null, hideCollectionDialog,
   isMultipart = false, manifest, manifestId, ready = false,
   setWorkspaceAddVisibility, showCollectionDialog, updateWindow, windowId = null,
 }) {
@@ -81,14 +81,14 @@ export function CollectionDialog({
 
   /** */
   const selectCollection = (c) => {
-    showCollectionDialog(c.id, [...collectionPath, manifestId], windowId);
+    showCollectionDialog(c.id, [...dialogCollectionPath, manifestId], windowId);
   };
 
   /** */
   const goToPreviousCollection = () => {
     showCollectionDialog(
-      collectionPath[collectionPath.length - 1],
-      collectionPath.slice(0, -1),
+      dialogCollectionPath[dialogCollectionPath.length - 1],
+      dialogCollectionPath.slice(0, -1),
       windowId,
     );
   };
@@ -97,10 +97,10 @@ export function CollectionDialog({
   const selectManifest = (m) => {
     if (windowId) {
       updateWindow(windowId, {
-        canvasId: null, collectionPath: [...collectionPath, manifestId], manifestId: m.id,
+        canvasId: null, collectionPath: [...dialogCollectionPath, manifestId], manifestId: m.id,
       });
     } else {
-      addWindow({ collectionPath: [...collectionPath, manifestId], manifestId: m.id });
+      addWindow({ collectionPath: [...dialogCollectionPath, manifestId], manifestId: m.id });
     }
 
     hideDialog();
@@ -230,7 +230,7 @@ export function CollectionDialog({
 CollectionDialog.propTypes = {
   addWindow: PropTypes.func.isRequired,
   collection: PropTypes.object, // eslint-disable-line react/forbid-prop-types
-  collectionPath: PropTypes.arrayOf(PropTypes.string),
+  dialogCollectionPath: PropTypes.arrayOf(PropTypes.string),
   error: PropTypes.string,
   hideCollectionDialog: PropTypes.func.isRequired,
   isMultipart: PropTypes.bool,
diff --git a/src/containers/CollectionDialog.js b/src/containers/CollectionDialog.js
index abf7693d..48c9c783 100644
--- a/src/containers/CollectionDialog.js
+++ b/src/containers/CollectionDialog.js
@@ -26,15 +26,15 @@ const mapDispatchToProps = {
  * @private
  */
 const mapStateToProps = (state, { windowId }) => {
-  const { collectionPath, collectionManifestId: manifestId } = getWindow(state, { windowId });
+  const { collectionManifestId: manifestId, dialogCollectionPath } = getWindow(state, { windowId });
   const manifest = getManifest(state, { manifestId });
 
-  const collectionId = collectionPath && collectionPath[collectionPath.length - 1];
+  const collectionId = dialogCollectionPath && dialogCollectionPath[dialogCollectionPath.length - 1];
   const collection = collectionId && getManifest(state, { manifestId: collectionId });
 
   return {
     collection: collection && getManifestoInstance(state, { manifestId: collection.id }),
-    collectionPath,
+    dialogCollectionPath,
     error: manifest && manifest.error,
     isMultipart: getSequenceBehaviors(state, { manifestId }).includes('multi-part'),
     manifest: manifest && getManifestoInstance(state, { manifestId }),
diff --git a/src/containers/CollectionInfo.js b/src/containers/CollectionInfo.js
index 6f737993..a4e5e129 100644
--- a/src/containers/CollectionInfo.js
+++ b/src/containers/CollectionInfo.js
@@ -13,7 +13,7 @@ import { CollectionInfo } from '../components/CollectionInfo';
  * @memberof WindowSideBarInfoPanel
  * @private
  */
-const mapStateToProps = (state, { companionWindowId, windowId }) => {
+const mapStateToProps = (state, { windowId }) => {
   const { collectionPath } = (getWindow(state, { windowId }) || {});
   const manifestId = collectionPath[collectionPath.length - 1];
 
diff --git a/src/state/actions/window.js b/src/state/actions/window.js
index e7ed508e..9b31ea6d 100644
--- a/src/state/actions/window.js
+++ b/src/state/actions/window.js
@@ -193,9 +193,9 @@ export function setWindowViewType(windowId, viewType) {
 }
 
 /** */
-export function showCollectionDialog(manifestId, collectionPath = [], windowId) {
+export function showCollectionDialog(manifestId, dialogCollectionPath = [], windowId) {
   return {
-    collectionPath,
+    dialogCollectionPath,
     manifestId,
     type: ActionTypes.SHOW_COLLECTION_DIALOG,
     windowId,
diff --git a/src/state/reducers/windows.js b/src/state/reducers/windows.js
index 33853fb8..16b705c1 100644
--- a/src/state/reducers/windows.js
+++ b/src/state/reducers/windows.js
@@ -157,7 +157,7 @@ export const windowsReducer = (state = {}, action) => {
           ...state[action.windowId],
           collectionDialogOn: true,
           collectionManifestId: action.manifestId,
-          collectionPath: action.collectionPath,
+          dialogCollectionPath: action.dialogCollectionPath,
         },
       };
     case ActionTypes.HIDE_COLLECTION_DIALOG:
diff --git a/src/state/sagas/app.js b/src/state/sagas/app.js
index 83a2c407..f10700fd 100644
--- a/src/state/sagas/app.js
+++ b/src/state/sagas/app.js
@@ -43,8 +43,8 @@ export function* importConfig({ config: { thumbnailNavigation, windows } }) {
 
 /** */
 export function* fetchCollectionManifests(action) {
-  const { collectionPath, manifestId } = action;
-  yield call(fetchManifests, manifestId, ...collectionPath);
+  const { dialogCollectionPath, manifestId } = action;
+  yield call(fetchManifests, manifestId, ...dialogCollectionPath);
 }
 
 /** */
diff --git a/src/state/sagas/windows.js b/src/state/sagas/windows.js
index f8dd591c..54029679 100644
--- a/src/state/sagas/windows.js
+++ b/src/state/sagas/windows.js
@@ -267,6 +267,7 @@ export default function* windowsSaga() {
     takeEvery(ActionTypes.ADD_WINDOW, fetchWindowManifest),
     takeEvery(ActionTypes.UPDATE_WINDOW, fetchWindowManifest),
     takeEvery(ActionTypes.UPDATE_WINDOW, setCanvasOnNewSequence),
+    takeEvery(ActionTypes.UPDATE_WINDOW, fetchCollectionManifests),
     takeEvery(ActionTypes.SET_CANVAS, setCurrentAnnotationsOnCurrentCanvas),
     takeEvery(ActionTypes.SET_CANVAS, fetchInfoResponses),
     takeEvery(ActionTypes.UPDATE_COMPANION_WINDOW, fetchCollectionManifests),


Test patch:
diff --git a/__tests__/fixtures/version-2/parentCollection.json b/__tests__/fixtures/version-2/parentCollection.json
new file mode 100644
index 00000000..fefa21d9
--- /dev/null
+++ b/__tests__/fixtures/version-2/parentCollection.json
@@ -0,0 +1,13 @@
+{
+  "@context": "http://iiif.io/api/presentation/2/context.json",
+  "@id": "http://iiif.io/api/presentation/2.1/example/fixtures/parentCollection.json",
+  "@type": "sc:Collection",
+  "label": "Parent Collection of Collection",
+  "collections": [
+    {
+      "@id": "http://iiif.io/api/presentation/2.1/example/fixtures/collection.json",
+      "@type": "sc:Collection",
+      "label": "Collection of Test Cases - label for parent"
+    }
+  ]
+}
diff --git a/__tests__/src/components/CollectionDialog.test.js b/__tests__/src/components/CollectionDialog.test.js
index 403c48b2..1c77e522 100644
--- a/__tests__/src/components/CollectionDialog.test.js
+++ b/__tests__/src/components/CollectionDialog.test.js
@@ -4,21 +4,27 @@ import { Utils } from 'manifesto.js';
 
 import { CollectionDialog } from '../../../src/components/CollectionDialog';
 import collection from '../../fixtures/version-2/collection.json';
+import parentCollection from '../../fixtures/version-2/parentCollection.json';
 
 /** */
 function createWrapper(props) {
-  const manifest = Utils.parseManifest(props.manifest ? props.manifest : collection);
+  const { collection: propsCollection, manifest, ...otherProps } = props;
+  const manifestId = (manifest && manifest['@id']) || collection['@id'];
+  const manifestObject = Utils.parseManifest(manifest || collection);
+  const collectionObject = propsCollection && Utils.parseManifest(propsCollection);
 
   render(<div id="window" />);
 
   return render(
     <CollectionDialog
       addWindow={() => {}}
+      collection={collectionObject}
       classes={{}}
       ready
-      manifest={manifest}
+      manifestId={manifestId}
+      manifest={manifestObject}
       windowId="window"
-      {...props}
+      {...otherProps}
     />,
     { preloadedState: { windows: { window: { id: 'window' } } } },
   );
@@ -32,6 +38,7 @@ describe('CollectionDialog', () => {
     expect(screen.getAllByRole('menuitem')).toHaveLength(55);
     expect(screen.getByRole('menuitem', { name: 'Test 1 Manifest: Minimum Required Fields' })).toBeInTheDocument();
   });
+
   it('when not ready returns placeholder skeleton', () => {
     createWrapper({ ready: false });
 
@@ -39,6 +46,7 @@ describe('CollectionDialog', () => {
 
     expect(screen.getByRole('dialog').querySelectorAll('.MuiSkeleton-root')).toHaveLength(3); // eslint-disable-line testing-library/no-node-access
   });
+
   it('clicking the hide button fires hideCollectionDialog', async () => {
     const user = userEvent.setup();
     const hideCollectionDialog = vi.fn();
@@ -47,4 +55,62 @@ describe('CollectionDialog', () => {
     await user.click(screen.getByRole('button', { name: 'Close' }));
     expect(hideCollectionDialog).toHaveBeenCalled();
   });
+
+  it('fires correct showCollectionDialog when a child collection is selected', async () => {
+    const user = userEvent.setup();
+    const showCollectionDialog = vi.fn();
+
+    createWrapper({ manifest: parentCollection, showCollectionDialog });
+
+    expect(screen.getByRole('dialog')).toBeInTheDocument();
+    expect(screen.getAllByRole('menuitem')).toHaveLength(1);
+    expect(screen.getByRole('menuitem', { name: 'Collection of Test Cases - label for parent' })).toBeInTheDocument();
+
+    await user.click(screen.getByRole('menuitem', { name: 'Collection of Test Cases - label for parent' }));
+    expect(showCollectionDialog).toHaveBeenCalledWith(collection['@id'], [parentCollection['@id']], 'window');
+  });
+
+  it('fires correct showCollectionDialog when the parent collection is clicked', async () => {
+    const user = userEvent.setup();
+    const showCollectionDialog = vi.fn();
+
+    createWrapper({
+      collection: parentCollection,
+      dialogCollectionPath: [parentCollection['@id']],
+      manifest: collection,
+      showCollectionDialog,
+    });
+
+    expect(screen.getByRole('dialog')).toBeInTheDocument();
+    expect(screen.getByRole('button', { name: 'Parent Collection of Collection' })).toBeInTheDocument();
+
+    await user.click(screen.getByRole('button', { name: 'Parent Collection of Collection' }));
+    expect(showCollectionDialog).toHaveBeenCalledWith(parentCollection['@id'], [], 'window');
+  });
+
+  it('fires hideCollectionDialog, setWorkspaceAddVisibility, updateWindow when a manifest is selected', async () => {
+    const user = userEvent.setup();
+    const hideCollectionDialog = vi.fn();
+    const setWorkspaceAddVisibility = vi.fn();
+    const updateWindow = vi.fn();
+    const manifestId = 'http://iiif.io/api/presentation/2.1/example/fixtures/1/manifest.json';
+
+    createWrapper({
+      collection: parentCollection,
+      dialogCollectionPath: [parentCollection['@id']],
+      hideCollectionDialog,
+      manifest: collection,
+      setWorkspaceAddVisibility,
+      updateWindow,
+    });
+
+    expect(screen.getByRole('dialog')).toBeInTheDocument();
+
+    await user.click(screen.getByRole('menuitem', { name: 'Test 1 Manifest: Minimum Required Fields' }));
+    expect(hideCollectionDialog).toHaveBeenCalledWith('window');
+    expect(setWorkspaceAddVisibility).toHaveBeenCalledWith(false);
+    expect(updateWindow).toHaveBeenCalledWith('window', {
+      canvasId: null, collectionPath: [parentCollection['@id'], collection['@id']], manifestId,
+    });
+  });
 });
diff --git a/__tests__/src/reducers/windows.test.js b/__tests__/src/reducers/windows.test.js
index be79e54d..181fcc37 100644
--- a/__tests__/src/reducers/windows.test.js
+++ b/__tests__/src/reducers/windows.test.js
@@ -386,10 +386,10 @@ describe('windows reducer', () => {
     it('handles SHOW_COLLECTION_DIALOG by toggling the given window\'s collection dialog', () => {
       const beforeState = { abc123: { collectionDialogOn: false } };
       const action = {
-        collectionPath: [], manifestId: 'def456', type: ActionTypes.SHOW_COLLECTION_DIALOG, windowId: 'abc123',
+        dialogCollectionPath: [], manifestId: 'def456', type: ActionTypes.SHOW_COLLECTION_DIALOG, windowId: 'abc123',
       };
       const expectedState = {
-        abc123: { collectionDialogOn: true, collectionManifestId: 'def456', collectionPath: [] },
+        abc123: { collectionDialogOn: true, collectionManifestId: 'def456', dialogCollectionPath: [] },
       };
 
       expect(windowsReducer(beforeState, action)).toEqual(expectedState);
@@ -400,7 +400,7 @@ describe('windows reducer', () => {
     it('handles HIDE_COLLECTION_DIALOG by toggling the given window\'s collection dialog', () => {
       const beforeState = {
         abc123: {
-          collectionDialogOn: true, collectionManifestId: 'def456', collectionPath: [],
+          collectionDialogOn: true, collectionManifestId: 'def456', collectionPath: [], dialogCollectionPath: ['x'],
         },
       };
       const action = {
@@ -409,7 +409,9 @@ describe('windows reducer', () => {
       };
 
       const expectedState = {
-        abc123: { collectionDialogOn: false, collectionManifestId: 'def456', collectionPath: [] },
+        abc123: {
+          collectionDialogOn: false, collectionManifestId: 'def456', collectionPath: [], dialogCollectionPath: ['x'],
+        },
       };
 
       expect(windowsReducer(beforeState, action)).toEqual(expectedState);
diff --git a/__tests__/src/sagas/app.test.js b/__tests__/src/sagas/app.test.js
index 193f21cd..01129922 100644
--- a/__tests__/src/sagas/app.test.js
+++ b/__tests__/src/sagas/app.test.js
@@ -1,7 +1,7 @@
 import { call } from 'redux-saga/effects';
 import { expectSaga, testSaga } from 'redux-saga-test-plan';
 
-import { importConfig, importState } from '../../../src/state/sagas/app';
+import { fetchCollectionManifests, importConfig, importState } from '../../../src/state/sagas/app';
 import { fetchManifests } from '../../../src/state/sagas/iiif';
 import { fetchWindowManifest } from '../../../src/state/sagas/windows';
 import { addWindow } from '../../../src/state/actions';
@@ -80,4 +80,15 @@ describe('app-level sagas', () => {
         .run();
     });
   });
+
+  describe('fetchCollectionManifests', () => {
+    it('fetches ressources for manifestId and all collection path ids', () => {
+      const action = {
+        dialogCollectionPath: ['a', 'b'],
+        manifestId: 'c',
+        windowId: 'w',
+      };
+      testSaga(fetchCollectionManifests, action).next().call(fetchManifests, 'c', 'a', 'b');
+    });
+  });
 });
diff --git a/__tests__/src/sagas/windows.test.js b/__tests__/src/sagas/windows.test.js
index 126a3816..5426dd0f 100644
--- a/__tests__/src/sagas/windows.test.js
+++ b/__tests__/src/sagas/windows.test.js
@@ -507,7 +507,7 @@ describe('window-level sagas', () => {
         ])
         .put.like({
           action: {
-            collectionPath: [],
+            dialogCollectionPath: [],
             manifestId: 'manifest.json',
             type: 'mirador/SHOW_COLLECTION_DIALOG',
             windowId: 'x',
@@ -525,7 +525,7 @@ describe('window-level sagas', () => {
         ])
         .not.put.like({
           action: {
-            collectionPath: [],
+            dialogCollectionPath: [],
             manifestId: 'manifest.json',
             type: 'mirador/SHOW_COLLECTION_DIALOG',
             windowId: 'x',