fix(webapp/sidenav): Fix bookmark reordering (#45632)

e2e/test/scenarios/organization/bookmarks-question.cy.spec.js

@@ -8,6 +8,8 @@ import {
   visitQuestion,
 } from "e2e/support/helpers";
 
+import { toggleQuestionBookmarkStatus } from "./helpers/bookmark-helpers";
+
 describe("scenarios > question > bookmarks", () => {
   beforeEach(() => {
     restore();
@@ -17,7 +19,7 @@ describe("scenarios > question > bookmarks", () => {
 
   it("should add, update bookmark name when question name is updated, then remove bookmark from question page", () => {
     visitQuestion(ORDERS_QUESTION_ID);
-    toggleBookmark();
+    toggleQuestionBookmarkStatus();
 
     openNavigationSidebar();
     navigationSidebar().within(() => {
@@ -64,7 +66,7 @@ describe("scenarios > question > bookmarks", () => {
     });
 
     // Remove bookmark
-    toggleBookmark({ wasSelected: true });
+    toggleQuestionBookmarkStatus({ wasSelected: true });
 
     navigationSidebar().within(() => {
       getSidebarSectionTitle(/Bookmarks/).should("not.exist");
@@ -72,11 +74,3 @@ describe("scenarios > question > bookmarks", () => {
     });
   });
 });
-
-function toggleBookmark({ wasSelected = false } = {}) {
-  const iconName = wasSelected ? "bookmark_filled" : "bookmark";
-  cy.findByTestId("qb-header-action-panel").within(() => {
-    cy.icon(iconName).click();
-  });
-  cy.wait("@toggleBookmark");
-}

e2e/test/scenarios/organization/bookmarks-reordering.cy.spec.ts

+import {
+  ORDERS_COUNT_QUESTION_ID,
+  ORDERS_QUESTION_ID,
+} from "e2e/support/cypress_sample_instance_data";
+import {
+  openNavigationSidebar,
+  restore,
+  visitQuestion,
+} from "e2e/support/helpers";
+
+import {
+  createAndBookmarkQuestion,
+  moveBookmark,
+  toggleQuestionBookmarkStatus,
+  verifyBookmarksOrder,
+} from "./helpers/bookmark-helpers";
+
+describe("scenarios > nav > bookmarks", () => {
+  beforeEach(() => {
+    restore();
+    cy.intercept("/api/bookmark/card/*").as("toggleBookmark");
+    cy.intercept("/api/bookmark/ordering").as("reorderBookmarks");
+    cy.signInAsAdmin();
+  });
+
+  it("should allow bookmarks to be reordered", () => {
+    cy.log("Create three bookmarks");
+    ["Question 1", "Question 2", "Question 3"].forEach(
+      createAndBookmarkQuestion,
+    );
+    openNavigationSidebar();
+    verifyBookmarksOrder(["Question 3", "Question 2", "Question 1"]);
+    moveBookmark("Question 1", -100);
+    verifyBookmarksOrder(["Question 1", "Question 3", "Question 2"]);
+    moveBookmark("Question 3", 100);
+    verifyBookmarksOrder(["Question 1", "Question 2", "Question 3"]);
+  });
+
+  it("should restore bookmarks order if PUT fails", () => {
+    cy.intercept(
+      { method: "PUT", url: "/api/bookmark/ordering" },
+      { statusCode: 500 },
+    ).as("failedToReorderBookmarks");
+    cy.log("Create two bookmarks");
+    visitQuestion(ORDERS_QUESTION_ID);
+    toggleQuestionBookmarkStatus();
+    visitQuestion(ORDERS_COUNT_QUESTION_ID);
+    toggleQuestionBookmarkStatus();
+    openNavigationSidebar();
+    const initialOrder = ["Orders, Count", "Orders"];
+    verifyBookmarksOrder(initialOrder);
+    moveBookmark("Orders, Count", 100, {
+      putAlias: "failedToReorderBookmarks",
+    });
+    verifyBookmarksOrder(initialOrder);
+  });
+});

e2e/test/scenarios/organization/helpers/bookmark-helpers.ts

+import { SAMPLE_DB_ID } from "e2e/support/cypress_data";
+import { SAMPLE_DATABASE } from "e2e/support/cypress_sample_database";
+import { moveDnDKitElement, navigationSidebar } from "e2e/support/helpers";
+
+export const toggleQuestionBookmarkStatus = ({ wasSelected = false } = {}) => {
+  const iconName = wasSelected ? "bookmark_filled" : "bookmark";
+  cy.findByTestId("qb-header-action-panel").within(() => {
+    cy.icon(iconName).click();
+  });
+  cy.wait("@toggleBookmark");
+};
+
+export const createAndBookmarkQuestion = (questionName: string) => {
+  createSimpleQuestion(questionName);
+  toggleQuestionBookmarkStatus();
+};
+
+export const createSimpleQuestion = (name: string) =>
+  cy.createQuestion(
+    {
+      name,
+      display: "table",
+      database: SAMPLE_DB_ID,
+      query: { "source-table": SAMPLE_DATABASE.ORDERS_ID },
+      visualization_settings: {},
+    },
+    { visitQuestion: true },
+  );
+
+export const verifyBookmarksOrder = (expectedOrder: string[]) => {
+  navigationSidebar()
+    .findByLabelText(/Bookmarks/)
+    .within(() => {
+      cy.get("li")
+        .should("have.length", expectedOrder.length)
+        .each((bookmark, index) => {
+          cy.wrap(bookmark).contains(expectedOrder[index]);
+        });
+    });
+};
+
+export const moveBookmark = (
+  name: string,
+  verticalDistance: number,
+  {
+    /** Alias for PUT request endpoint */
+    putAlias = "reorderBookmarks",
+  } = {},
+) => {
+  moveDnDKitElement(
+    navigationSidebar()
+      .findByLabelText(/Bookmarks/)
+      .findByText(name),
+    { vertical: verticalDistance },
+  );
+  cy.wait(`@${putAlias}`);
+};

frontend/src/metabase/entities/bookmarks.js

 import { createSelector } from "@reduxjs/toolkit";
 import { assoc, updateIn, dissoc, getIn } from "icepick";
+import { t } from "ttag";
 import _ from "underscore";
 
 import { bookmarkApi } from "metabase/api";
@@ -7,8 +8,8 @@ import Collections from "metabase/entities/collections";
 import Dashboards from "metabase/entities/dashboards";
 import Questions from "metabase/entities/questions";
 import { createEntity, entityCompatibleQuery } from "metabase/lib/entities";
+import { addUndo } from "metabase/redux/undo";
 import { BookmarkSchema } from "metabase/schema";
-
 const REORDER_ACTION = `metabase/entities/bookmarks/REORDER_ACTION`;
 
 /**
@@ -46,17 +47,29 @@ const Bookmarks = createEntity({
     REORDER: REORDER_ACTION,
   },
   actions: {
-    reorder: bookmarks => async dispatch => {
+    reorder: bookmarks => async (dispatch, getState) => {
+      const bookmarksBeforeReordering = getOrderedBookmarks(getState());
       const orderings = bookmarks.map(({ type, item_id }) => ({
         type,
         item_id,
       }));
-      await entityCompatibleQuery(
-        { orderings },
-        dispatch,
-        bookmarkApi.endpoints.reorderBookmarks,
-      );
-      return { type: REORDER_ACTION, payload: bookmarks };
+      dispatch({ type: REORDER_ACTION, payload: bookmarks });
+      try {
+        await entityCompatibleQuery(
+          { orderings },
+          dispatch,
+          bookmarkApi.endpoints.reorderBookmarks,
+        );
+      } catch (e) {
+        dispatch({ type: REORDER_ACTION, payload: bookmarksBeforeReordering });
+        dispatch(
+          addUndo({
+            icon: "warning",
+            toastColor: "error",
+            message: t`Something went wrong`,
+          }),
+        );
+      }
     },
   },
   objectSelectors: {

frontend/src/metabase/nav/containers/MainNavbar/MainNavbarContainer/BookmarkList/BookmarkList.tsx

@@ -44,7 +44,7 @@ interface CollectionSidebarBookmarksProps {
   }: {
     newIndex: number;
     oldIndex: number;
-  }) => void;
+  }) => Promise<any>;
   onToggle: (isExpanded: boolean) => void;
   initialState: "expanded" | "collapsed";
 }
@@ -135,12 +135,12 @@ const BookmarkList = ({
   }, []);
 
   const handleSortEnd = useCallback(
-    (input: DragEndEvent) => {
+    async (input: DragEndEvent) => {
       document.body.classList.remove(GrabberS.grabbing);
       setIsSorting(false);
       const newIndex = bookmarks.findIndex(b => b.id === input.over?.id);
       const oldIndex = bookmarks.findIndex(b => b.id === input.active.id);
-      reorderBookmarks({ newIndex, oldIndex });
+      await reorderBookmarks({ newIndex, oldIndex });
     },
     [reorderBookmarks, bookmarks],
   );

frontend/src/metabase/nav/containers/MainNavbar/MainNavbarContainer/MainNavbarContainer.tsx

@@ -61,7 +61,7 @@ interface Props extends MainNavbarProps {
   allError: boolean;
   allFetched: boolean;
   logout: () => void;
-  onReorderBookmarks: (bookmarks: Bookmark[]) => void;
+  onReorderBookmarks: (bookmarks: Bookmark[]) => Promise<any>;
   onChangeLocation: (location: LocationDescriptor) => void;
 }
 
@@ -133,14 +133,14 @@ function MainNavbarContainer({
   }, [rootCollection, trashCollection, collections, currentUser]);
 
   const reorderBookmarks = useCallback(
-    ({ newIndex, oldIndex }: { newIndex: number; oldIndex: number }) => {
+    async ({ newIndex, oldIndex }: { newIndex: number; oldIndex: number }) => {
       const newBookmarks = [...bookmarks];
       const movedBookmark = newBookmarks[oldIndex];
 
       newBookmarks.splice(oldIndex, 1);
       newBookmarks.splice(newIndex, 0, movedBookmark);
 
-      onReorderBookmarks(newBookmarks);
+      await onReorderBookmarks(newBookmarks);
     },
     [bookmarks, onReorderBookmarks],
   );

frontend/src/metabase/nav/containers/MainNavbar/MainNavbarContainer/MainNavbarView.tsx

@@ -60,7 +60,7 @@ type Props = {
   }: {
     newIndex: number;
     oldIndex: number;
-  }) => void;
+  }) => Promise<any>;
 };
 const OTHER_USERS_COLLECTIONS_URL = Urls.otherUsersPersonalCollections();
 const ADD_YOUR_OWN_DATA_URL = "/admin/databases/create";