Filled vs Unfilled Bookmarks

frontends/api/src/hooks/learningResources/index.ts

@@ -16,7 +16,6 @@ import type {
   LearningPathResource,
   LearningPathRelationshipRequest,
   MicroLearningPathRelationship,
-  LearningResource,
   LearningResourcesSearchApiLearningResourcesSearchRetrieveRequest as LRSearchRequest,
   UserlistsApiUserlistsListRequest as ULListRequest,
   UserlistsApiUserlistsCreateRequest as ULCreateRequest,
@@ -28,10 +27,13 @@ import type {
   MicroUserListRelationship,
   PlatformsApiPlatformsListRequest,
   FeaturedApiFeaturedListRequest as FeaturedListParams,
+  PaginatedLearningResourceList,
 } from "../../generated/v1"
 import learningResources, {
   invalidateResourceQueries,
   invalidateUserListQueries,
+  updateListParentsOnAdd,
+  updateListParentsOnDestroy,
 } from "./keyFactory"
 import { ListType } from "../../common/constants"
 
@@ -168,14 +170,15 @@ const useLearningpathRelationshipCreate = () => {
         learning_resource_id: params.parent,
         LearningPathRelationshipRequest: params,
       }),
-    onSuccess: (response, _vars) => {
-      queryClient.setQueryData(
-        learningResources.detail(response.data.child).queryKey,
-        response.data.resource,
+    onSettled: (_response, _err, vars) => {
+      invalidateResourceQueries(
+        queryClient,
+        vars.child,
+        // do NOT skip invalidating the /featured/ lists,
+        // Changing a learning path might change the members of the featured
+        // lists.
+        { skipFeatured: false },
       )
-    },
-    onSettled: (response, _err, vars) => {
-      invalidateResourceQueries(queryClient, vars.child)
       invalidateResourceQueries(queryClient, vars.parent)
     },
   })
@@ -189,22 +192,15 @@ const useLearningpathRelationshipDestroy = () => {
         id: params.id,
         learning_resource_id: params.parent,
       }),
-    onSuccess: (_response, vars) => {
-      queryClient.setQueryData(
-        learningResources.detail(vars.child).queryKey,
-        (old: LearningResource | undefined) => {
-          if (!old) return
-          const parents =
-            old.learning_path_parents?.filter(({ id }) => vars.id !== id) ?? []
-          return {
-            ...old,
-            learning_path_parents: parents,
-          }
-        },
-      )
-    },
     onSettled: (_response, _err, vars) => {
-      invalidateResourceQueries(queryClient, vars.child)
+      invalidateResourceQueries(
+        queryClient,
+        vars.child,
+        // do NOT skip invalidating the /featured/ lists,
+        // Changing a learning path might change the members of the featured
+        // lists.
+        { skipFeatured: false },
+      )
       invalidateResourceQueries(queryClient, vars.parent)
     },
   })
@@ -298,13 +294,21 @@ const useUserListRelationshipCreate = () => {
         UserListRelationshipRequest: params,
       }),
     onSuccess: (response, _vars) => {
-      queryClient.setQueryData(
-        learningResources.detail(response.data.child).queryKey,
-        response.data.resource,
+      queryClient.setQueriesData<PaginatedLearningResourceList>(
+        learningResources.featured({}).queryKey,
+        (old) => updateListParentsOnAdd(response.data, old),
       )
     },
-    onSettled: (response, _err, vars) => {
-      invalidateResourceQueries(queryClient, vars.child)
+    onSettled: (_response, _err, vars) => {
+      invalidateResourceQueries(
+        queryClient,
+        vars.child,
+        // Do NOT invalidate the featured lists. Re-fetching the featured list
+        // data will cause the order to change, since the /featured API returns
+        // at random order.
+        // Instead, `onSuccess` hook will manually update the data.
+        { skipFeatured: true },
+      )
       invalidateUserListQueries(queryClient, vars.parent)
     },
   })
@@ -319,21 +323,21 @@ const useUserListRelationshipDestroy = () => {
         userlist_id: params.parent,
       }),
     onSuccess: (_response, vars) => {
-      queryClient.setQueryData(
-        learningResources.detail(vars.child).queryKey,
-        (old: LearningResource | undefined) => {
-          if (!old) return
-          const parents =
-            old.user_list_parents?.filter(({ id }) => vars.id !== id) ?? []
-          return {
-            ...old,
-            user_list_parents: parents,
-          }
-        },
+      queryClient.setQueriesData<PaginatedLearningResourceList>(
+        learningResources.featured({}).queryKey,
+        (old) => updateListParentsOnDestroy(vars, old),
       )
     },
     onSettled: (_response, _err, vars) => {
-      invalidateResourceQueries(queryClient, vars.child)
+      invalidateResourceQueries(
+        queryClient,
+        vars.child,
+        // Do NOT invalidate the featured lists. Re-fetching the featured list
+        // data will cause the order to change, since the /featured API returns
+        // at random order.
+        // Instead, `onSuccess` hook will manually update the data.
+        { skipFeatured: true },
+      )
       invalidateUserListQueries(queryClient, vars.parent)
     },
   })

frontends/api/src/hooks/learningResources/keyFactory.ts

@@ -27,6 +27,8 @@ import type {
   OfferorsApiOfferorsListRequest,
   PlatformsApiPlatformsListRequest,
   FeaturedApiFeaturedListRequest as FeaturedListParams,
+  UserListRelationship,
+  MicroUserListRelationship,
 } from "../../generated/v1"
 import { createQueryKeys } from "@lukemorales/query-key-factory"
 
@@ -45,7 +47,7 @@ const learningResources = createQueryKeys("learningResources", {
         .learningResourcesList(params)
         .then((res) => res.data),
   }),
-  featured: (params: FeaturedListParams) => ({
+  featured: (params: FeaturedListParams = {}) => ({
     queryKey: [params],
     queryFn: () => featuredApi.featuredList(params).then((res) => res.data),
   }),
@@ -143,35 +145,33 @@ const learningResources = createQueryKeys("learningResources", {
   },
 })
 
-const learningPathHasResource =
+const listHasResource =
   (resourceId: number) =>
   (query: Query): boolean => {
     // eslint-disable-next-line @typescript-eslint/no-explicit-any
     const data = query.state.data as any
-    const resources: LearningResource[] = data.pages
-      ? data.pages.flatMap(
-          (page: PaginatedLearningResourceList) => page.results,
-        )
-      : data.results
-    return resources.some((res) => res.id === resourceId)
-  }
-
-const userListHasResource =
-  (resourceId: number) =>
-  (query: Query): boolean => {
-    // eslint-disable-next-line @typescript-eslint/no-explicit-any
-    const data = query.state.data as any
-    const resources: UserList[] = data.pages
+    const resources: LearningResource[] | UserList[] = data.pages
       ? data.pages.flatMap(
           (page: PaginatedLearningResourceList) => page.results,
         )
       : data.results
     return resources.some((res) => res.id === resourceId)
   }
 
+/**
+ * Invalidate Resource queries that a specific resource appears in.
+ *
+ * By default, this will invalidate featured list queries. This can result in
+ * odd behavior because the featured list item order is randomized: when the
+ * featured list cache is invalidated, the newly fetched data may be in a
+ * different order. To maintain the order, use skipFeatured to skip invalidation
+ * of featured lists and instead manually update the cached data via
+ * `updateListParentsOnAdd`.
+ */
 const invalidateResourceQueries = (
   queryClient: QueryClient,
   resourceId: LearningResource["id"],
+  { skipFeatured = false } = {},
 ) => {
   /**
    * Invalidate details queries.
@@ -191,17 +191,13 @@ const invalidateResourceQueries = (
   const lists = [
     learningResources.list._def,
     learningResources.learningpaths._ctx.list._def,
-    learningResources.userlists._ctx.list._def,
-    learningResources.featured._def,
+    learningResources.search._def,
+    ...(skipFeatured ? [] : [learningResources.featured._def]),
   ]
   lists.forEach((queryKey) => {
     queryClient.invalidateQueries({
       queryKey,
-      predicate: learningPathHasResource(resourceId),
-    })
-    queryClient.invalidateQueries({
-      queryKey,
-      predicate: userListHasResource(resourceId),
+      predicate: listHasResource(resourceId),
     })
   })
 }
@@ -213,7 +209,72 @@ const invalidateUserListQueries = (
   queryClient.invalidateQueries(
     learningResources.userlists._ctx.detail(userListId).queryKey,
   )
+  const lists = [learningResources.userlists._ctx.list._def]
+  lists.forEach((queryKey) => {
+    queryClient.invalidateQueries({
+      queryKey,
+      predicate: listHasResource(userListId),
+    })
+  })
+}
+
+/**
+ * Given
+ *  - a list of learning resources L
+ *  - a new relationship between learningpath/userlist and a resource R
+ * Update the list L so that it includes the updated resource R. (If the list
+ * did not contain R to begin with, no change is made)
+ */
+const updateListParentsOnAdd = (
+  relationship: UserListRelationship,
+  oldList?: PaginatedLearningResourceList,
+) => {
+  if (!oldList) return oldList
+  const matchIndex = oldList.results.findIndex(
+    (res) => res.id === relationship.child,
+  )
+  if (matchIndex === -1) return oldList
+  const updatesResults = [...oldList.results]
+  updatesResults[matchIndex] = relationship.resource
+  return {
+    ...oldList,
+    results: updatesResults,
+  }
+}
+
+/**
+ * Given
+ *  - a list of learning resources L
+ *  - a destroyed relationship between learningpath/userlist and a resource R
+ * Update the list L so that it includes the updated resource R. (If the list
+ * did not contain R to begin with, no change is made)
+ */
+const updateListParentsOnDestroy = (
+  relationship: MicroUserListRelationship,
+  list?: PaginatedLearningResourceList,
+) => {
+  if (!list) return list
+  if (!relationship) return list
+  const matchIndex = list.results.findIndex(
+    (res) => res.id === relationship.child,
+  )
+  if (matchIndex === -1) return list
+  const updatedResults = [...list.results]
+  const newResource = { ...updatedResults[matchIndex] }
+  newResource.user_list_parents =
+    newResource.user_list_parents?.filter((m) => m.id !== relationship.id) ??
+    null
+  updatedResults[matchIndex] = newResource
+  return {
+    ...list,
+    results: updatedResults,
+  }
 }
 
 export default learningResources
-export { invalidateResourceQueries, invalidateUserListQueries }
+export {
+  invalidateResourceQueries,
+  invalidateUserListQueries,
+  updateListParentsOnAdd,
+  updateListParentsOnDestroy,
+}

frontends/mit-open/src/page-components/ResourceCard/ResourceCard.test.tsx

@@ -1,6 +1,11 @@
 import React from "react"
 import * as NiceModal from "@ebay/nice-modal-react"
-import { renderWithProviders, user, screen } from "../../test-utils"
+import {
+  renderWithProviders,
+  user,
+  screen,
+  expectProps,
+} from "../../test-utils"
 import type { User } from "../../test-utils"
 import { ResourceCard, ResourceListCard } from "./ResourceCard"
 import {
@@ -11,6 +16,17 @@ import type { ResourceCardProps } from "./ResourceCard"
 import { urls, factories, setMockResponse } from "api/test-utils"
 import { RESOURCE_DRAWER_QUERY_PARAM } from "@/common/urls"
 import invariant from "tiny-invariant"
+import { LearningResourceCard, LearningResourceListCard } from "ol-components"
+
+jest.mock("ol-components", () => {
+  const actual = jest.requireActual("ol-components")
+  return {
+    __esModule: true,
+    ...actual,
+    LearningResourceCard: jest.fn(actual.LearningResourceCard),
+    LearningResourceListCard: jest.fn(actual.LearningResourceListCard),
+  }
+})
 
 jest.mock("@ebay/nice-modal-react", () => {
   const actual = jest.requireActual("@ebay/nice-modal-react")
@@ -24,11 +40,13 @@ jest.mock("@ebay/nice-modal-react", () => {
 describe.each([
   {
     CardComponent: ResourceCard,
+    BaseComponent: LearningResourceCard,
   },
   {
     CardComponent: ResourceListCard,
+    BaseComponent: LearningResourceListCard,
   },
-])("$CardComponent", ({ CardComponent }) => {
+])("$CardComponent", ({ CardComponent, BaseComponent }) => {
   const makeResource = factories.learningResources.resource
   type SetupOptions = {
     user?: Partial<User>
@@ -84,6 +102,47 @@ describe.each([
     },
   )
 
+  test.each([
+    {
+      userlist: { count: 1, inList: true },
+      learningpath: { count: 1, inList: true },
+    },
+    {
+      userlist: { count: 0, inList: false },
+      learningpath: { count: 1, inList: true },
+    },
+    {
+      userlist: { count: 1, inList: true },
+      learningpath: { count: 0, inList: false },
+    },
+    {
+      userlist: { count: 0, inList: false },
+      learningpath: { count: 0, inList: false },
+    },
+  ])(
+    "'Add to ...' buttons are filled based on membership in list",
+    ({ userlist, learningpath }) => {
+      const resource = makeResource()
+      const { microLearningPathRelationship } = factories.learningResources
+      const { microUserListRelationship } = factories.userLists
+      resource.learning_path_parents = Array.from(
+        { length: learningpath.count },
+        () => microLearningPathRelationship({ child: resource.id }),
+      )
+      resource.user_list_parents = Array.from({ length: userlist.count }, () =>
+        microUserListRelationship({ child: resource.id }),
+      )
+
+      setup({ user: { is_authenticated: true }, props: { resource } })
+
+      expectProps(BaseComponent, {
+        resource,
+        inLearningPath: learningpath.inList,
+        inUserList: userlist.inList,
+      })
+    },
+  )
+
   test("Clicking add to list button opens AddToListDialog when authenticated", async () => {
     const showModal = jest.mocked(NiceModal.show)