Fix crash on restoring bookmarked items, update bookmarks on restore (#44414)

* Fix crash on restoring bookmarked items, update bookmarks on restore

* adding bookmark assertions to e2e tests

* removing unintended createDashboard change

e2e/test/scenarios/collections/trash.cy.spec.js

@@ -111,11 +111,18 @@ describe("scenarios > collections > trash", () => {
 
   it("should be able to trash & restore dashboards/collections/questions on entity page and from parent collection", () => {
     cy.log("create test resources");
-    createCollection({ name: "Collection A" });
-    createDashboard({ name: "Dashboard A" });
+    cy.log("Bookmark the resources to test metabase#44224");
+    createCollection({ name: "Collection A" }).then(collection => {
+      cy.request("POST", `/api/bookmark/collection/${collection.id}`);
+    });
+    createDashboard({ name: "Dashboard A" }).then(dashboard => {
+      cy.request("POST", `/api/bookmark/dashboard/${dashboard.id}`);
+    });
     createNativeQuestion({
       name: "Question A",
       native: { query: "select 1;" },
+    }).then(question => {
+      cy.request("POST", `/api/bookmark/card/${question.id}`);
     });
 
     visitRootCollection();
@@ -137,12 +144,15 @@ describe("scenarios > collections > trash", () => {
 
     toggleEllipsisMenuFor(/Collection A/);
     popover().findByText("Restore").click();
+    ensureBookmarkVisible(/Collection A/);
 
     toggleEllipsisMenuFor("Dashboard A");
     popover().findByText("Restore").click();
+    ensureBookmarkVisible("Dashboard A");
 
     toggleEllipsisMenuFor("Question A");
     popover().findByText("Restore").click();
+    ensureBookmarkVisible("Question A");
 
     cy.log("should be able to archive entities from their own views");
     visitRootCollection();
@@ -158,6 +168,7 @@ describe("scenarios > collections > trash", () => {
       cy.findByText("Move to trash").click();
     });
     ensureCanRestoreFromPage("Collection A");
+    ensureBookmarkVisible("Collection A");
 
     // dashboard
     collectionTable().within(() => {
@@ -174,6 +185,7 @@ describe("scenarios > collections > trash", () => {
       cy.findByText("Dashboard A").should("not.exist");
     });
     ensureCanRestoreFromPage("Dashboard A");
+    ensureBookmarkVisible("Dashboard A");
 
     // question
     collectionTable().within(() => {
@@ -190,6 +202,7 @@ describe("scenarios > collections > trash", () => {
       cy.findByText("Question A").should("not.exist");
     });
     ensureCanRestoreFromPage("Question A");
+    ensureBookmarkVisible("Question A");
   });
 
   it("should not show restore option if entity is within nested in an archived collection list", () => {
@@ -626,6 +639,8 @@ describe("scenarios > collections > trash", () => {
   });
 });
 
+describe("Restoring items", () => {});
+
 function toggleEllipsisMenuFor(item) {
   collectionTable().within(() => {
     cy.findByText(item)
@@ -638,9 +653,12 @@ function toggleEllipsisMenuFor(item) {
 function createCollection(collectionInfo, archive) {
   return cy
     .createCollection(collectionInfo)
-    .then(({ body: collection }) =>
-      Promise.all([collection, archive && cy.archiveCollection(collection.id)]),
-    )
+    .then(({ body: collection }) => {
+      return Promise.all([
+        collection,
+        archive && cy.archiveCollection(collection.id),
+      ]);
+    })
     .then(([collection]) => collection);
 }
 
@@ -701,3 +719,9 @@ function selectItem(name) {
     .closest("tr")
     .within(() => cy.findByRole("checkbox").click());
 }
+
+function ensureBookmarkVisible(bookmark) {
+  cy.findByRole("tab", { name: /bookmarks/i })
+    .findByText(bookmark)
+    .should("be.visible");
+}

frontend/src/metabase/collections/components/ActionMenu/ActionMenu.tsx

@@ -21,6 +21,7 @@ import {
 } from "metabase/collections/utils";
 import { ConfirmDeleteModal } from "metabase/components/ConfirmDeleteModal";
 import EventSandbox from "metabase/components/EventSandbox";
+import { bookmarks as BookmarkEntity } from "metabase/entities";
 import { useDispatch } from "metabase/lib/redux";
 import { entityForObject } from "metabase/lib/schema";
 import * as Urls from "metabase/lib/urls";
@@ -140,6 +141,7 @@ function ActionMenu({
     const result = await dispatch(
       Entity.actions.update({ id: item.id, archived: false }),
     );
+    await dispatch(BookmarkEntity.actions.invalidateLists());
     const parent = HACK_getParentCollectionFromEntityUpdateAction(item, result);
     const redirect = parent ? Urls.collection(parent) : `/collection/root`;
 

frontend/src/metabase/collections/components/CollectionContent/CollectionContentView.tsx

@@ -21,6 +21,7 @@ import {
   isTrashedCollection,
 } from "metabase/collections/utils";
 import ItemsDragLayer from "metabase/containers/dnd/ItemsDragLayer";
+import Bookmarks from "metabase/entities/bookmarks";
 import Collections from "metabase/entities/collections";
 import Search from "metabase/entities/search";
 import { useListSelect } from "metabase/hooks/use-list-select";
@@ -220,9 +221,10 @@ export const CollectionContentView = ({
                 canWrite={collection.can_write}
                 canRestore={collection.can_restore}
                 canDelete={collection.can_delete}
-                onUnarchive={() => {
+                onUnarchive={async () => {
                   const input = { ...actionId, name: collection.name };
-                  dispatch(Collections.actions.setArchived(input, false));
+                  await dispatch(Collections.actions.setArchived(input, false));
+                  await dispatch(Bookmarks.actions.invalidateLists());
                 }}
                 onMove={({ id }) =>
                   dispatch(Collections.actions.setCollection(actionId, { id }))

frontend/src/metabase/dashboard/components/Dashboard/Dashboard.tsx

@@ -21,6 +21,7 @@ import type {
   FetchDashboardResult,
   SuccessfulFetchDashboardResult,
 } from "metabase/dashboard/types";
+import Bookmarks from "metabase/entities/bookmarks";
 import Dashboards from "metabase/entities/dashboards";
 import { getMainElement } from "metabase/lib/dom";
 import { useDispatch } from "metabase/lib/redux";
@@ -436,7 +437,10 @@ function DashboardInner(props: DashboardProps) {
                 canWrite={canWrite}
                 canRestore={canRestore}
                 canDelete={canDelete}
-                onUnarchive={() => dispatch(setArchivedDashboard(false))}
+                onUnarchive={async () => {
+                  await dispatch(setArchivedDashboard(false));
+                  await dispatch(Bookmarks.actions.invalidateLists());
+                }}
                 onMove={({ id }) => dispatch(moveDashboardToCollection({ id }))}
                 onDeletePermanently={() => {
                   const { id } = dashboard;

frontend/src/metabase/entities/bookmarks.js

 import { createSelector } from "@reduxjs/toolkit";
-import { assoc, updateIn, dissoc } from "icepick";
+import { assoc, updateIn, dissoc, getIn } from "icepick";
 import _ from "underscore";
 
 import { bookmarkApi } from "metabase/api";
@@ -62,10 +62,14 @@ const Bookmarks = createEntity({
   objectSelectors: {
     getIcon,
   },
+
   reducer: (state = {}, { type, payload, error }) => {
     if (type === Questions.actionTypes.UPDATE && payload?.object) {
       const { archived, type, id, name } = payload.object;
       const key = `card-${id}`;
+      if (!getIn(state, [key])) {
+        return state;
+      }
       if (archived) {
         return dissoc(state, key);
       } else {
@@ -80,6 +84,9 @@ const Bookmarks = createEntity({
     if (type === Dashboards.actionTypes.UPDATE && payload?.object) {
       const { archived, id, name } = payload.object;
       const key = `dashboard-${id}`;
+      if (!getIn(state, [key])) {
+        return state;
+      }
       if (archived) {
         return dissoc(state, key);
       } else {
@@ -91,6 +98,9 @@ const Bookmarks = createEntity({
       const { id, authority_level, name } = payload.object;
       const key = `collection-${id}`;
 
+      if (!getIn(state, [key])) {
+        return state;
+      }
       if (payload.object.archived) {
         return dissoc(state, key);
       } else {

frontend/src/metabase/query_builder/components/view/View.jsx

@@ -11,6 +11,7 @@ import LoadingAndErrorWrapper from "metabase/components/LoadingAndErrorWrapper";
 import Toaster from "metabase/components/Toaster";
 import CS from "metabase/css/core/index.css";
 import QueryBuilderS from "metabase/css/query_builder.module.css";
+import Bookmarks from "metabase/entities/bookmarks";
 import Questions from "metabase/entities/questions";
 import {
   rememberLastUsedDatabase,
@@ -457,7 +458,10 @@ class View extends Component {
 
 const mapDispatchToProps = dispatch => ({
   onSetDatabaseId: id => dispatch(rememberLastUsedDatabase(id)),
-  onUnarchive: question => dispatch(setArchivedQuestion(question, false)),
+  onUnarchive: async question => {
+    await dispatch(setArchivedQuestion(question, false));
+    await dispatch(Bookmarks.actions.invalidateLists());
+  },
   onMove: (question, newCollection) =>
     dispatch(
       Questions.actions.setCollection({ id: question.id() }, newCollection, {