Fix featured list channel sorting #2559
Conversation
| }) | ||
| Object.keys(resultsByPosition) | ||
| .sort( | ||
| (a, b) => Number(a) - Number(b), // Sort positions numerically |
There was a problem hiding this comment.
This is the only functional change vs the old implementation. But I decided to move it to a separate file and have copilot add some tests.
| const obj = { id: "test", position: 5 } | ||
| expect(hasPosition(obj)).toBe(true) |
There was a problem hiding this comment.
copilot really wanted to add this test......
ChristopherChudzicki
changed the title
| resultsByPosition[result?.position ?? ""]?.push(result) | ||
| }) | ||
| Object.keys(resultsByPosition) | ||
| .sort() |
There was a problem hiding this comment.
Explanation: Object keys are strings, and these were stringified numbers, so they were being sorted like 0, 1, 10, 11, 2, 3, 4, ....
This wasn't a visible problem on the homepage because that carusel uses items from every offeror... to get up to 12 items, that carousel only needs positions 0, 1, 2 from all five offerors.
The featured list query is also used on channel pages (We don't query the learning path directly because, at least right now, those are private; only visible to non-staff via /featured endpoint). And on the channel pages, 12 items are shown, so the sorting was getting messed up.
ChristopherChudzicki
added
the
Needs Review
| .mockImplementation(() => faker.number.float({ min: 0, max: 1 })) | ||
|
|
||
| describe("randomizeGroups", () => { | ||
| it("should group by position and randomize within groups with duplicates", () => { |
There was a problem hiding this comment.
Technically does not test that items are randomized within their groups (though not very testable!)
| const hasPosition = <T extends { position: number | null }>( | ||
| r: T, | ||
| ): r is T & { position: number } => r.position !== null |
There was a problem hiding this comment.
Type guard+narrowing... nice!
What are the relevant tickets?
Closes https://github.com/mitodl/hq/issues/6278
Description (What does it do?)
Fixes some sorting we did on the frontend to handle numbers properly.
How can this be tested?
./manage.py populate_featured_listsmanagement command (I changed the default to 12, which is better for our carousels in general). Note: This will override your featured lists. If you want to keep them, manually add at least 12 items to one of them.curl http://api.learn.odl.local:8065/api/v1/featured/\?offered_by\=ocw\&limit\=12 | jq '.results.[].title')