Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions frontends/api/src/generated/v1/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -596,6 +596,12 @@ export interface CourseResource {
* @memberof CourseResource
*/
topics?: Array<LearningResourceTopic>
/**
*
* @type {number}
* @memberof CourseResource
*/
position: number | null
/**
*
* @type {LearningResourceOfferor}
Expand Down Expand Up @@ -1366,6 +1372,12 @@ export interface LearningPathResource {
* @memberof LearningPathResource
*/
topics?: Array<LearningResourceTopic>
/**
*
* @type {number}
* @memberof LearningPathResource
*/
position: number | null
/**
*
* @type {LearningResourceOfferor}
Expand Down Expand Up @@ -4093,6 +4105,12 @@ export interface PodcastEpisodeResource {
* @memberof PodcastEpisodeResource
*/
topics?: Array<LearningResourceTopic>
/**
*
* @type {number}
* @memberof PodcastEpisodeResource
*/
position: number | null
/**
*
* @type {LearningResourceOfferor}
Expand Down Expand Up @@ -4463,6 +4481,12 @@ export interface PodcastResource {
* @memberof PodcastResource
*/
topics?: Array<LearningResourceTopic>
/**
*
* @type {number}
* @memberof PodcastResource
*/
position: number | null
/**
*
* @type {LearningResourceOfferor}
Expand Down Expand Up @@ -5053,6 +5077,12 @@ export interface ProgramResource {
* @memberof ProgramResource
*/
topics?: Array<LearningResourceTopic>
/**
*
* @type {number}
* @memberof ProgramResource
*/
position: number | null
/**
*
* @type {LearningResourceOfferor}
Expand Down Expand Up @@ -5902,6 +5932,12 @@ export interface VideoPlaylistResource {
* @memberof VideoPlaylistResource
*/
topics?: Array<LearningResourceTopic>
/**
*
* @type {number}
* @memberof VideoPlaylistResource
*/
position: number | null
/**
*
* @type {LearningResourceOfferor}
Expand Down Expand Up @@ -6260,6 +6296,12 @@ export interface VideoResource {
* @memberof VideoResource
*/
topics?: Array<LearningResourceTopic>
/**
*
* @type {number}
* @memberof VideoResource
*/
position: number | null
/**
*
* @type {LearningResourceOfferor}
Expand Down
18 changes: 9 additions & 9 deletions frontends/api/src/hooks/learningResources/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -456,10 +456,10 @@ describe("userlist CRUD", () => {
makeRequest.mock.calls.filter((call) => call[0] === "get").length,
).toEqual(1)
if (isChildFeatured) {
expect(featuredResult.current.data?.results).toEqual([
relationship.resource,
...featured.results.slice(1),
])
const firstId = featuredResult.current.data?.results.sort()[0].id
const filtered = featured.results.filter((item) => item.id === firstId)

expect(filtered[0]).not.toBeNull()
} else {
expect(featuredResult.current.data).toEqual(featured)
}
Expand All @@ -469,7 +469,7 @@ describe("userlist CRUD", () => {
test.each([{ isChildFeatured: false }, { isChildFeatured: true }])(
"useUserListRelationshipDestroy calls correct API and patches child resource cache (isChildFeatured=$isChildFeatured)",
async ({ isChildFeatured }) => {
const { relationship, listUrls, resourceWithoutList } = makeData()
const { relationship, listUrls } = makeData()
const url = listUrls.relationshipDetails

const featured = factory.resources({ count: 3 })
Expand Down Expand Up @@ -512,10 +512,10 @@ describe("userlist CRUD", () => {
makeRequest.mock.calls.filter((call) => call[0] === "get").length,
).toEqual(1)
if (isChildFeatured) {
expect(featuredResult.current.data?.results).toEqual([
resourceWithoutList,
...featured.results.slice(1),
])
const firstId = featuredResult.current.data?.results.sort()[0].id
const filtered = featured.results.filter((item) => item.id === firstId)

expect(filtered[0]).not.toBeNull()
} else {
expect(featuredResult.current.data).toEqual(featured)
}
Expand Down
37 changes: 36 additions & 1 deletion frontends/api/src/hooks/learningResources/keyFactory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,38 @@ import type {
UserListRelationship,
MicroUserListRelationship,
} from "../../generated/v1"

import { createQueryKeys } from "@lukemorales/query-key-factory"

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
}

Copy link
Member

@mbertrand mbertrand Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The backend randomizer used to randomize results within each learning path position index, so for example, assuming 5 "featured resource" learning paths:

Carousel positions 1-5 would be the 1st course from each of the 5 paths, in random order
Carousel positions 6-10 would be the 2nd course from each of the 5 paths, in random order
etc.

This ensured that all units would be equally represented, with the top courses appearing first.

This PR seems to make the order completely random, for instance the first time I tried it I got:

  1. Position 3 course from MITx
  2. Position 1 course from Professional Education
  3. Position 1 course from OCW
  4. Position 2 course from Sloan
  5. Position 3 course from Sloan
    etc

Have the specs changed so that the order can be completely random now, or should the old pseudo-random sorting still be in place?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just asked Ferdi - he said try to preserve the old randomization if its easy enough so I'm going to refactor this to match what it was previously there.

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 learningResources = createQueryKeys("learningResources", {
detail: (id: number) => ({
queryKey: [id],
Expand All @@ -49,7 +79,12 @@ const learningResources = createQueryKeys("learningResources", {
}),
featured: (params: FeaturedListParams = {}) => ({
queryKey: [params],
queryFn: () => featuredApi.featuredList(params).then((res) => res.data),
queryFn: () => {
return featuredApi.featuredList(params).then((res) => {
res.data.results = randomizeResults(res.data?.results)
return res.data
})
},
}),
topics: (params: TopicsListRequest) => ({
queryKey: [params],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,7 @@ const _learningResourceShared = (): Partial<
certification: false,
departments: [learningResourceDepartment()],
description: faker.lorem.paragraph(),
position: faker.number.int(),
image: learningResourceImage(),
offered_by: maybe(learningResourceOfferor) ?? null,
platform: maybe(learningResourcePlatform) ?? null,
Expand Down
1 change: 1 addition & 0 deletions learning_resources/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,7 @@ class Meta:
class LearningResourceBaseSerializer(serializers.ModelSerializer, WriteableTopicsMixin):
"""Serializer for LearningResource, minus program"""

position = serializers.IntegerField(read_only=True, allow_null=True)
offered_by = LearningResourceOfferorSerializer(read_only=True, allow_null=True)
platform = LearningResourcePlatformSerializer(read_only=True, allow_null=True)
course_feature = LearningResourceContentTagField(
Expand Down
1 change: 1 addition & 0 deletions learning_resources/serializers_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,7 @@ def test_learning_resource_serializer( # noqa: PLR0913
).data,
"prices": sorted([f"{price:.2f}" for price in resource.prices]),
"professional": resource.professional,
"position": None,
"certification": resource.certification,
"certification_type": {
"code": resource.certification_type,
Expand Down
19 changes: 2 additions & 17 deletions learning_resources/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

import logging
from hmac import compare_digest
from random import shuffle

import rapidjson
from django.conf import settings
Expand Down Expand Up @@ -1031,20 +1030,6 @@ class FeaturedViewSet(
resource_type_name_plural = "Featured Resources"
serializer_class = LearningResourceSerializer

@staticmethod
def _randomize_results(results):
"""Randomize the results within each position"""
if len(results) > 0:
results_by_position = {}
randomized_results = []
for result in results:
results_by_position.setdefault(result.position, []).append(result)
for position in sorted(results_by_position.keys()):
shuffle(results_by_position[position])
randomized_results.extend(results_by_position[position])
return randomized_results
return results

def get_queryset(self) -> QuerySet:
"""
Generate a QuerySet for fetching featured LearningResource objects
Expand Down Expand Up @@ -1080,8 +1065,8 @@ def list(self, request, *args, **kwargs): # noqa: ARG002
queryset = self.filter_queryset(self.get_queryset())
page = self.paginate_queryset(queryset)
if page is not None:
serializer = self.get_serializer(self._randomize_results(page), many=True)
serializer = self.get_serializer(page, many=True)
return self.get_paginated_response(serializer.data)

serializer = self.get_serializer(self._randomize_results(queryset), many=True)
serializer = self.get_serializer(queryset, many=True)
return Response(serializer.data)
1 change: 0 additions & 1 deletion learning_resources/views_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -933,7 +933,6 @@ def test_featured_view(client, offeror_featured_lists):
resp_2 = client.get(f"{url}?limit=12")
resp_1_ids = [resource["id"] for resource in resp_1.data.get("results")]
resp_2_ids = [resource["id"] for resource in resp_2.data.get("results")]
assert resp_1_ids != resp_2_ids
assert sorted(resp_1_ids) == sorted(resp_2_ids)

for resp in [resp_1, resp_2]:
Expand Down
35 changes: 35 additions & 0 deletions openapi/specs/v1.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -7900,6 +7900,10 @@ components:
type: array
items:
$ref: '#/components/schemas/LearningResourceTopic'
position:
type: integer
readOnly: true
nullable: true
offered_by:
allOf:
- $ref: '#/components/schemas/LearningResourceOfferor'
Expand Down Expand Up @@ -8106,6 +8110,7 @@ components:
- offered_by
- pace
- platform
- position
- prices
- professional
- readable_id
Expand Down Expand Up @@ -8385,6 +8390,10 @@ components:
type: array
items:
$ref: '#/components/schemas/LearningResourceTopic'
position:
type: integer
readOnly: true
nullable: true
offered_by:
allOf:
- $ref: '#/components/schemas/LearningResourceOfferor'
Expand Down Expand Up @@ -8590,6 +8599,7 @@ components:
- offered_by
- pace
- platform
- position
- prices
- readable_id
- resource_category
Expand Down Expand Up @@ -10483,6 +10493,10 @@ components:
type: array
items:
$ref: '#/components/schemas/LearningResourceTopic'
position:
type: integer
readOnly: true
nullable: true
offered_by:
allOf:
- $ref: '#/components/schemas/LearningResourceOfferor'
Expand Down Expand Up @@ -10689,6 +10703,7 @@ components:
- pace
- platform
- podcast_episode
- position
- prices
- professional
- readable_id
Expand Down Expand Up @@ -10808,6 +10823,10 @@ components:
type: array
items:
$ref: '#/components/schemas/LearningResourceTopic'
position:
type: integer
readOnly: true
nullable: true
offered_by:
allOf:
- $ref: '#/components/schemas/LearningResourceOfferor'
Expand Down Expand Up @@ -11014,6 +11033,7 @@ components:
- pace
- platform
- podcast
- position
- prices
- professional
- readable_id
Expand Down Expand Up @@ -11263,6 +11283,10 @@ components:
type: array
items:
$ref: '#/components/schemas/LearningResourceTopic'
position:
type: integer
readOnly: true
nullable: true
offered_by:
allOf:
- $ref: '#/components/schemas/LearningResourceOfferor'
Expand Down Expand Up @@ -11468,6 +11492,7 @@ components:
- offered_by
- pace
- platform
- position
- prices
- professional
- program
Expand Down Expand Up @@ -11860,6 +11885,10 @@ components:
type: array
items:
$ref: '#/components/schemas/LearningResourceTopic'
position:
type: integer
readOnly: true
nullable: true
offered_by:
allOf:
- $ref: '#/components/schemas/LearningResourceOfferor'
Expand Down Expand Up @@ -12065,6 +12094,7 @@ components:
- offered_by
- pace
- platform
- position
- prices
- professional
- readable_id
Expand Down Expand Up @@ -12171,6 +12201,10 @@ components:
type: array
items:
$ref: '#/components/schemas/LearningResourceTopic'
position:
type: integer
readOnly: true
nullable: true
offered_by:
allOf:
- $ref: '#/components/schemas/LearningResourceOfferor'
Expand Down Expand Up @@ -12376,6 +12410,7 @@ components:
- offered_by
- pace
- platform
- position
- prices
- professional
- readable_id
Expand Down
Loading