Fix featured list channel sorting (#2559)

* create channels with 12 resources by default

* fix featured list sorting

Author: ChristopherChudzicki

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

@@ -20,6 +20,7 @@ import type {
   LearningResourcesSearchResponse,
 } from "../../generated/v1"
 import { queryOptions } from "@tanstack/react-query"
+import { hasPosition, randomizeGroups } from "./util"
 
 /* List memberships were previously determined in the learningResourcesApi
  * from user_list_parents and learning_path_parents on each resource.
@@ -37,35 +38,6 @@ export const clearListMemberships = (
   learning_path_parents: [],
 })
 
-const shuffle = ([...arr]) => {
-  let m = arr.length
-  while (m) {
-    const i = Math.floor(Math.random() * m--)
-    ;[arr[m], arr[i]] = [arr[i], arr[m]]
-  }
-  return arr
-}
-
-const randomizeResults = ([...results]) => {
-  const resultsByPosition: {
-    [position: string]: (LearningResource & { position?: string })[] | undefined
-  } = {}
-  const randomizedResults: LearningResource[] = []
-  results.forEach((result) => {
-    if (!resultsByPosition[result?.position]) {
-      resultsByPosition[result?.position] = []
-    }
-    resultsByPosition[result?.position ?? ""]?.push(result)
-  })
-  Object.keys(resultsByPosition)
-    .sort()
-    .forEach((position) => {
-      const shuffled = shuffle(resultsByPosition[position] ?? [])
-      randomizedResults.push(...shuffled)
-    })
-  return randomizedResults
-}
-
 const learningResourceKeys = {
   root: ["learning_resources"],
   // list
@@ -183,11 +155,17 @@ const learningResourceQueries = {
       queryKey: learningResourceKeys.featured(params),
       queryFn: () =>
         featuredApi.featuredList(params).then((res) => {
+          const results = res.data.results
+          const withPosition = results.filter(hasPosition)
+          if (withPosition.length !== results.length) {
+            // Should not happen. The featured API always sets position.
+            console.warn(
+              "Some featured results are missing position information.",
+            )
+          }
           return {
             ...res.data,
-            results: randomizeResults(
-              res.data.results.map(clearListMemberships),
-            ),
+            results: randomizeGroups(withPosition),
           }
         }),
     }),

frontends/api/src/hooks/learningResources/util.test.ts

@@ -0,0 +1,97 @@
+import { faker } from "@faker-js/faker/locale/en"
+import { randomizeGroups, hasPosition } from "./util"
+
+faker.seed(12345) // Seed faker for consistent test results
+jest
+  .spyOn(Math, "random")
+  .mockImplementation(() => faker.number.float({ min: 0, max: 1 }))
+
+describe("randomizeGroups", () => {
+  it("should group by position and randomize within groups with duplicates", () => {
+    const items = [
+      { id: "a1", position: 1 },
+      { id: "a2", position: 1 },
+      { id: "b1", position: 12 },
+      { id: "b2", position: 12 },
+      { id: "c1", position: 2 },
+      { id: "c2", position: 2 },
+      { id: "d1", position: 3 },
+    ]
+
+    const result = randomizeGroups(items)
+
+    // Should be grouped by position in numerical order
+    expect(result[0].position).toBe(1)
+    expect(result[1].position).toBe(1)
+    expect(result[2].position).toBe(2)
+    expect(result[3].position).toBe(2)
+    expect(result[4].position).toBe(3)
+    expect(result[5].position).toBe(12)
+    expect(result[6].position).toBe(12)
+
+    // Should contain all items
+    expect(result).toHaveLength(7)
+    expect(result.map((item) => item.id).sort()).toEqual([
+      "a1",
+      "a2",
+      "b1",
+      "b2",
+      "c1",
+      "c2",
+      "d1",
+    ])
+  })
+
+  it("should handle positions greater than 10 correctly (avoid lexicographical sorting)", () => {
+    const items = [
+      { id: "item15", position: 15 },
+      { id: "item2", position: 2 },
+      { id: "item11", position: 11 },
+      { id: "item1", position: 1 },
+      { id: "item20", position: 20 },
+    ]
+
+    const result = randomizeGroups(items)
+
+    // Should be numerically sorted: 1, 2, 11, 15, 20
+    // NOT lexicographically sorted: 1, 11, 15, 2, 20
+    const positions = result.map((item) => item.position)
+    expect(positions).toEqual([1, 2, 11, 15, 20])
+  })
+
+  it("should handle empty array", () => {
+    const items: Array<{ id: string; position: number }> = []
+    const result = randomizeGroups(items)
+    expect(result).toEqual([])
+  })
+})
+
+describe("hasPosition", () => {
+  it("should return true for objects with non-null position", () => {
+    const obj = { id: "test", position: 5 }
+    expect(hasPosition(obj)).toBe(true)
+  })
+
+  it("should return false for objects with null position", () => {
+    const obj = { id: "test", position: null }
+    expect(hasPosition(obj)).toBe(false)
+  })
+
+  it("should return true for objects with position 0", () => {
+    const obj = { id: "test", position: 0 }
+    expect(hasPosition(obj)).toBe(true)
+  })
+
+  it("should act as a type guard", () => {
+    const obj: { id: string; position: number | null } = {
+      id: "test",
+      position: 5,
+    }
+
+    if (hasPosition(obj)) {
+      // TypeScript should now know that obj.position is number, not number | null
+      const position: number = obj.position
+      expect(position).toBe(5)
+    }
+  })
+})

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

@@ -0,0 +1,58 @@
+const hasPosition = <T extends { position: number | null }>(
+  r: T,
+): r is T & { position: number } => r.position !== null
+
+const shuffle = ([...arr]) => {
+  let m = arr.length
+  while (m) {
+    const i = Math.floor(Math.random() * m--)
+    ;[arr[m], arr[i]] = [arr[i], arr[m]]
+  }
+  return arr
+}
+
+/**
+ * Randomize a group of ordered items, where item positions might be duplicated.
+ * Ordering is preserved between groups, but randomized within groups.
+ *
+ * E.g., given the items
+ *  [
+ *  { id: 1, position: 1 },
+ *  { id: 2, position: 1 },
+ *  { id: 3, position: 2 },
+ *  { id: 4, position: 3 },
+ *  { id: 5, position: 3 },
+ *  { id: 6, position: 3 },
+ * ]
+ *
+ * The results would be:
+ * [
+ *   ...items with position 1 in random order...,
+ *   ...items with position 2 in random order...,
+ *   ...items with position 3 in random order...
+ * ]
+ */
+const randomizeGroups = <T extends { position: number }>(results: T[]): T[] => {
+  const resultsByPosition: {
+    [position: string]: T[] | undefined
+  } = {}
+  const randomizedResults: T[] = []
+  results.forEach((result) => {
+    const pos = result?.position
+    if (!resultsByPosition[pos]) {
+      resultsByPosition[pos] = []
+    }
+    resultsByPosition[pos]?.push(result)
+  })
+  Object.keys(resultsByPosition)
+    .sort(
+      (a, b) => Number(a) - Number(b), // Sort positions numerically
+    )
+    .forEach((position) => {
+      const shuffled = shuffle(resultsByPosition[position] ?? [])
+      randomizedResults.push(...shuffled)
+    })
+  return randomizedResults
+}
+
+export { randomizeGroups, hasPosition }

learning_resources/management/commands/populate_featured_lists.py

@@ -31,7 +31,7 @@ def add_arguments(self, parser):
             "resource_count",
             nargs="?",
             type=int,
-            default=10,
+            default=12,
             help="Set the number of courses per featured list (default is 10)",
         )
 
@@ -44,7 +44,7 @@ def handle(self, *args, **options):  # noqa: ARG002
         self.stdout.write("Creating featured list for each featured offeror channel")
 
         start = now_in_utc()
-        resource_count = options.get("resource_count", 10)
+        resource_count = options["resource_count"]
         for offeror in LearningResourceOfferor.objects.all():
             self.stdout.write(f"Creating featured list for {offeror.name} channel")