From 69cb68e65a504013577332bc3bba93512d4ebc31 Mon Sep 17 00:00:00 2001 From: Matt Bertrand Date: Mon, 7 Oct 2024 12:09:59 -0400 Subject: [PATCH 1/6] Ignore NotFoundErrors in switch_indices function (#1654) --- learning_resources_search/indexing_api.py | 9 ++++++--- learning_resources_search/indexing_api_test.py | 12 ++++++++++-- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/learning_resources_search/indexing_api.py b/learning_resources_search/indexing_api.py index e037a40c79..0b8803dcdd 100644 --- a/learning_resources_search/indexing_api.py +++ b/learning_resources_search/indexing_api.py @@ -560,9 +560,12 @@ def switch_indices(backing_index, object_type): conn.indices.delete(index) # Finally, remove the link to the reindexing alias - conn.indices.delete_alias( - name=get_reindexing_alias_name(object_type), index=backing_index - ) + try: + conn.indices.delete_alias( + name=get_reindexing_alias_name(object_type), index=backing_index + ) + except NotFoundError: + log.warning("Reindex alias not found for %s", object_type) def delete_orphaned_indexes(obj_types, delete_reindexing_tags): diff --git a/learning_resources_search/indexing_api_test.py b/learning_resources_search/indexing_api_test.py index 50c1943028..cfae97d13e 100644 --- a/learning_resources_search/indexing_api_test.py +++ b/learning_resources_search/indexing_api_test.py @@ -108,11 +108,13 @@ def test_clear_and_create_index(mocked_es, object_type, skip_mapping, already_ex @pytest.mark.parametrize("object_type", [COURSE_TYPE, PROGRAM_TYPE]) @pytest.mark.parametrize("default_exists", [True, False]) -def test_switch_indices(mocked_es, mocker, default_exists, object_type): +@pytest.mark.parametrize("alias_exists", [True, False]) +def test_switch_indices(mocked_es, mocker, default_exists, alias_exists, object_type): """ switch_indices should atomically remove the old backing index for the default alias and replace it with the new one """ + mock_log = mocker.patch("learning_resources_search.indexing_api.log.warning") refresh_mock = mocker.patch( "learning_resources_search.indexing_api.refresh_index", autospec=True ) @@ -120,7 +122,9 @@ def test_switch_indices(mocked_es, mocker, default_exists, object_type): conn_mock.indices.exists_alias.return_value = default_exists old_backing_index = "old_backing" conn_mock.indices.get_alias.return_value.keys.return_value = [old_backing_index] - + conn_mock.indices.delete_alias.side_effect = ( + None if alias_exists else NotFoundError() + ) backing_index = "backing" switch_indices(backing_index, object_type) @@ -155,6 +159,10 @@ def test_switch_indices(mocked_es, mocker, default_exists, object_type): conn_mock.indices.delete_alias.assert_called_once_with( name=get_reindexing_alias_name(object_type), index=backing_index ) + if not alias_exists: + mock_log.assert_called_once_with("Reindex alias not found for %s", object_type) + else: + mock_log.assert_not_called() @pytest.mark.parametrize("temp_alias_exists", [True, False]) From d9ed99aa0f15f528f2ddbf9a0f963cdccfafd9a6 Mon Sep 17 00:00:00 2001 From: Shankar Ambady Date: Tue, 8 Oct 2024 09:27:47 -0400 Subject: [PATCH 2/6] Shanbady/randomize featured resources (#1653) * removing backend randomization * removing results randomization * moiving shuffle to frontend * fixing tests * test fix * refactoring to old randomization method on frontend * updating spec * fixing typechecks * updating spec and adding position field to api * adding position to factory * fixing test --- frontends/api/src/generated/v1/api.ts | 42 +++++++++++++++++++ .../src/hooks/learningResources/index.test.ts | 18 ++++---- .../src/hooks/learningResources/keyFactory.ts | 37 +++++++++++++++- .../test-utils/factories/learningResources.ts | 1 + learning_resources/serializers.py | 1 + learning_resources/serializers_test.py | 1 + learning_resources/views.py | 19 +-------- learning_resources/views_test.py | 1 - openapi/specs/v1.yaml | 35 ++++++++++++++++ 9 files changed, 127 insertions(+), 28 deletions(-) diff --git a/frontends/api/src/generated/v1/api.ts b/frontends/api/src/generated/v1/api.ts index 62385b9658..b230201908 100644 --- a/frontends/api/src/generated/v1/api.ts +++ b/frontends/api/src/generated/v1/api.ts @@ -596,6 +596,12 @@ export interface CourseResource { * @memberof CourseResource */ topics?: Array + /** + * + * @type {number} + * @memberof CourseResource + */ + position: number | null /** * * @type {LearningResourceOfferor} @@ -1366,6 +1372,12 @@ export interface LearningPathResource { * @memberof LearningPathResource */ topics?: Array + /** + * + * @type {number} + * @memberof LearningPathResource + */ + position: number | null /** * * @type {LearningResourceOfferor} @@ -4093,6 +4105,12 @@ export interface PodcastEpisodeResource { * @memberof PodcastEpisodeResource */ topics?: Array + /** + * + * @type {number} + * @memberof PodcastEpisodeResource + */ + position: number | null /** * * @type {LearningResourceOfferor} @@ -4463,6 +4481,12 @@ export interface PodcastResource { * @memberof PodcastResource */ topics?: Array + /** + * + * @type {number} + * @memberof PodcastResource + */ + position: number | null /** * * @type {LearningResourceOfferor} @@ -5053,6 +5077,12 @@ export interface ProgramResource { * @memberof ProgramResource */ topics?: Array + /** + * + * @type {number} + * @memberof ProgramResource + */ + position: number | null /** * * @type {LearningResourceOfferor} @@ -5902,6 +5932,12 @@ export interface VideoPlaylistResource { * @memberof VideoPlaylistResource */ topics?: Array + /** + * + * @type {number} + * @memberof VideoPlaylistResource + */ + position: number | null /** * * @type {LearningResourceOfferor} @@ -6260,6 +6296,12 @@ export interface VideoResource { * @memberof VideoResource */ topics?: Array + /** + * + * @type {number} + * @memberof VideoResource + */ + position: number | null /** * * @type {LearningResourceOfferor} diff --git a/frontends/api/src/hooks/learningResources/index.test.ts b/frontends/api/src/hooks/learningResources/index.test.ts index f9785a3266..5c4c6364f4 100644 --- a/frontends/api/src/hooks/learningResources/index.test.ts +++ b/frontends/api/src/hooks/learningResources/index.test.ts @@ -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) } @@ -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 }) @@ -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) } diff --git a/frontends/api/src/hooks/learningResources/keyFactory.ts b/frontends/api/src/hooks/learningResources/keyFactory.ts index d2fcf3f262..4732b3909a 100644 --- a/frontends/api/src/hooks/learningResources/keyFactory.ts +++ b/frontends/api/src/hooks/learningResources/keyFactory.ts @@ -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 +} + +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], @@ -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], diff --git a/frontends/api/src/test-utils/factories/learningResources.ts b/frontends/api/src/test-utils/factories/learningResources.ts index f37c0092fb..3ed4fb3be2 100644 --- a/frontends/api/src/test-utils/factories/learningResources.ts +++ b/frontends/api/src/test-utils/factories/learningResources.ts @@ -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, diff --git a/learning_resources/serializers.py b/learning_resources/serializers.py index c52a40ebc6..96e5576a7b 100644 --- a/learning_resources/serializers.py +++ b/learning_resources/serializers.py @@ -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( diff --git a/learning_resources/serializers_test.py b/learning_resources/serializers_test.py index b454de5ee7..eb7a43e0b9 100644 --- a/learning_resources/serializers_test.py +++ b/learning_resources/serializers_test.py @@ -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, diff --git a/learning_resources/views.py b/learning_resources/views.py index aa1fdad051..cc8234c22e 100644 --- a/learning_resources/views.py +++ b/learning_resources/views.py @@ -2,7 +2,6 @@ import logging from hmac import compare_digest -from random import shuffle import rapidjson from django.conf import settings @@ -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 @@ -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) diff --git a/learning_resources/views_test.py b/learning_resources/views_test.py index ddf8ad4a3b..baeda8e39b 100644 --- a/learning_resources/views_test.py +++ b/learning_resources/views_test.py @@ -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]: diff --git a/openapi/specs/v1.yaml b/openapi/specs/v1.yaml index 1fe6d76a0e..3ea879b09b 100644 --- a/openapi/specs/v1.yaml +++ b/openapi/specs/v1.yaml @@ -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' @@ -8106,6 +8110,7 @@ components: - offered_by - pace - platform + - position - prices - professional - readable_id @@ -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' @@ -8590,6 +8599,7 @@ components: - offered_by - pace - platform + - position - prices - readable_id - resource_category @@ -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' @@ -10689,6 +10703,7 @@ components: - pace - platform - podcast_episode + - position - prices - professional - readable_id @@ -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' @@ -11014,6 +11033,7 @@ components: - pace - platform - podcast + - position - prices - professional - readable_id @@ -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' @@ -11468,6 +11492,7 @@ components: - offered_by - pace - platform + - position - prices - professional - program @@ -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' @@ -12065,6 +12094,7 @@ components: - offered_by - pace - platform + - position - prices - professional - readable_id @@ -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' @@ -12376,6 +12410,7 @@ components: - offered_by - pace - platform + - position - prices - professional - readable_id From b78f67824608e8809a85b43a8e3394e15214801f Mon Sep 17 00:00:00 2001 From: Shankar Ambady Date: Tue, 8 Oct 2024 11:30:17 -0400 Subject: [PATCH 3/6] Cached department and topic page counts (#1661) * adding counts to department page * removing log statement * adding cached counts to topic page * fixing typecheck * removing unrelated (but stray) console.log * test fixes * removing old aggregation requests --- frontends/mit-learn/src/common/utils.ts | 25 +++++++++- .../DepartmentListingPage.test.tsx | 42 +++++++++++++++++ .../DepartmentListingPage.tsx | 47 +++++-------------- .../pages/OnboardingPage/OnboardingPage.tsx | 1 - .../TopicsListingPage.test.tsx | 19 ++++++++ .../TopicListingPage/TopicsListingPage.tsx | 41 +++++----------- .../UnitsListingPage/UnitsListingPage.tsx | 28 ++--------- 7 files changed, 114 insertions(+), 89 deletions(-) diff --git a/frontends/mit-learn/src/common/utils.ts b/frontends/mit-learn/src/common/utils.ts index 462ac1d428..e72cc6f266 100644 --- a/frontends/mit-learn/src/common/utils.ts +++ b/frontends/mit-learn/src/common/utils.ts @@ -1,3 +1,4 @@ +import { ChannelCounts } from "api/v0" const getSearchParamMap = (urlParams: URLSearchParams) => { const params: Record = {} for (const [key] of urlParams.entries()) { @@ -6,4 +7,26 @@ const getSearchParamMap = (urlParams: URLSearchParams) => { return params } -export { getSearchParamMap } +const aggregateProgramCounts = ( + groupBy: string, + data: Array, +): Record => { + return Object.fromEntries( + Object.entries(data).map(([_key, value]) => { + return [value[groupBy as keyof ChannelCounts], value.counts.programs] + }), + ) +} + +const aggregateCourseCounts = ( + groupBy: string, + data: Array, +): Record => { + return Object.fromEntries( + Object.entries(data).map(([_key, value]) => { + return [value[groupBy as keyof ChannelCounts], value.counts.courses] + }), + ) +} + +export { getSearchParamMap, aggregateProgramCounts, aggregateCourseCounts } diff --git a/frontends/mit-learn/src/pages/DepartmentListingPage/DepartmentListingPage.test.tsx b/frontends/mit-learn/src/pages/DepartmentListingPage/DepartmentListingPage.test.tsx index 8138a4bb73..c70eb0ee36 100644 --- a/frontends/mit-learn/src/pages/DepartmentListingPage/DepartmentListingPage.test.tsx +++ b/frontends/mit-learn/src/pages/DepartmentListingPage/DepartmentListingPage.test.tsx @@ -62,6 +62,48 @@ describe("DepartmentListingPage", () => { } setMockResponse.get(urls.schools.list(), schools) + setMockResponse.get(urls.channels.counts("department"), [ + { + name: department1.name, + title: department1.name, + counts: { + programs: 1, + courses: 10, + }, + }, + { + name: department2.name, + title: department2.name, + counts: { + programs: 2, + courses: 20, + }, + }, + { + name: department3.name, + title: department3.name, + counts: { + programs: 0, + courses: 1, + }, + }, + { + name: department4.name, + title: department4.name, + counts: { + programs: 4, + courses: 40, + }, + }, + { + name: department5.name, + title: department5.name, + counts: { + programs: 5, + courses: 50, + }, + }, + ]) setMockResponse.get( urls.search.resources({ resource_type: ["course"], diff --git a/frontends/mit-learn/src/pages/DepartmentListingPage/DepartmentListingPage.tsx b/frontends/mit-learn/src/pages/DepartmentListingPage/DepartmentListingPage.tsx index b7165eb714..7606c5742e 100644 --- a/frontends/mit-learn/src/pages/DepartmentListingPage/DepartmentListingPage.tsx +++ b/frontends/mit-learn/src/pages/DepartmentListingPage/DepartmentListingPage.tsx @@ -12,14 +12,8 @@ import { Breadcrumbs, } from "ol-components" import { pluralize } from "ol-utilities" -import type { - LearningResourceSchool, - LearningResourcesSearchResponse, -} from "api" -import { - useLearningResourcesSearch, - useSchoolsList, -} from "api/hooks/learningResources" +import type { LearningResourceSchool } from "api" +import { useSchoolsList } from "api/hooks/learningResources" import { RiPaletteLine, RiArrowRightSLine, @@ -32,6 +26,8 @@ import { } from "@remixicon/react" import { HOME } from "@/common/urls" import MetaTags from "@/page-components/MetaTags/MetaTags" +import { aggregateProgramCounts, aggregateCourseCounts } from "@/common/utils" +import { useChannelCounts } from "api/hooks/channels" const SCHOOL_ICONS: Record = { // School of Architecture and Planning @@ -140,8 +136,8 @@ const SchoolDepartments: React.FC = ({ {school.departments.map((department) => { - const courses = courseCounts[department.department_id] ?? 0 - const programs = programCounts[department.department_id] ?? 0 + const courses = courseCounts[department.name] ?? 0 + const programs = programCounts[department.name] ?? 0 const counts = [ { count: courses, label: pluralize("Course", courses) }, { count: programs, label: pluralize("Program", programs) }, @@ -183,34 +179,17 @@ const SchoolList = styled.div(({ theme }) => ({ }, })) -const aggregateByDepartment = ( - data: LearningResourcesSearchResponse, -): Record => { - const buckets = data.metadata.aggregations["department"] ?? [] - return Object.fromEntries( - buckets.map((bucket) => { - return [bucket.key, bucket.doc_count] - }), - ) -} - const DepartmentListingPage: React.FC = () => { - const schoolsQuery = useSchoolsList() - const courseQuery = useLearningResourcesSearch({ - resource_type: ["course"], - aggregations: ["department"], - }) - const programQuery = useLearningResourcesSearch({ - resource_type: ["program"], - aggregations: ["department"], - }) - const courseCounts = courseQuery.data - ? aggregateByDepartment(courseQuery.data) + const channelCountQuery = useChannelCounts("department") + const courseCounts = channelCountQuery.data + ? aggregateCourseCounts("title", channelCountQuery.data) : {} - const programCounts = programQuery.data - ? aggregateByDepartment(programQuery.data) + const programCounts = channelCountQuery.data + ? aggregateProgramCounts("title", channelCountQuery.data) : {} + const schoolsQuery = useSchoolsList() + return ( <> diff --git a/frontends/mit-learn/src/pages/OnboardingPage/OnboardingPage.tsx b/frontends/mit-learn/src/pages/OnboardingPage/OnboardingPage.tsx index 8ff211063c..4395489f79 100644 --- a/frontends/mit-learn/src/pages/OnboardingPage/OnboardingPage.tsx +++ b/frontends/mit-learn/src/pages/OnboardingPage/OnboardingPage.tsx @@ -229,7 +229,6 @@ const OnboardingPage: React.FC = () => { } values={formik.values.goals} onChange={(event) => { - console.log(event) formik.handleChange(event) }} /> diff --git a/frontends/mit-learn/src/pages/TopicListingPage/TopicsListingPage.test.tsx b/frontends/mit-learn/src/pages/TopicListingPage/TopicsListingPage.test.tsx index 91802d56df..b9d157000b 100644 --- a/frontends/mit-learn/src/pages/TopicListingPage/TopicsListingPage.test.tsx +++ b/frontends/mit-learn/src/pages/TopicListingPage/TopicsListingPage.test.tsx @@ -51,6 +51,25 @@ describe("TopicsListingPage", () => { [t2.name]: 20, } + setMockResponse.get(urls.channels.counts("topic"), [ + { + name: t1.name, + title: t1.name, + counts: { + programs: 10, + courses: 100, + }, + }, + { + name: t2.name, + title: t2.name, + counts: { + programs: 20, + courses: 200, + }, + }, + ]) + setMockResponse.get(urls.topics.list(), { count: Object.values(topics).length, next: null, diff --git a/frontends/mit-learn/src/pages/TopicListingPage/TopicsListingPage.tsx b/frontends/mit-learn/src/pages/TopicListingPage/TopicsListingPage.tsx index 9e535856b2..207a424f58 100644 --- a/frontends/mit-learn/src/pages/TopicListingPage/TopicsListingPage.tsx +++ b/frontends/mit-learn/src/pages/TopicListingPage/TopicsListingPage.tsx @@ -15,13 +15,12 @@ import { Link } from "react-router-dom" import { propsNotNil } from "ol-utilities" import MetaTags from "@/page-components/MetaTags/MetaTags" -import { - useLearningResourceTopics, - useLearningResourcesSearch, -} from "api/hooks/learningResources" -import { LearningResourcesSearchResponse, LearningResourceTopic } from "api" +import { useLearningResourceTopics } from "api/hooks/learningResources" +import { LearningResourceTopic } from "api" import RootTopicIcon from "@/components/RootTopicIcon/RootTopicIcon" import { HOME } from "@/common/urls" +import { aggregateProgramCounts, aggregateCourseCounts } from "@/common/utils" +import { useChannelCounts } from "api/hooks/channels" const TOPICS_BANNER_IMAGE = "/static/images/background_steps.jpeg" @@ -182,17 +181,6 @@ const TopicBoxLoading = () => { const Page = styled.div({}) -const aggregateByTopic = ( - data: LearningResourcesSearchResponse, -): Record => { - const buckets = data.metadata.aggregations["topic"] ?? [] - return Object.fromEntries( - buckets.map((bucket) => { - return [bucket.key, bucket.doc_count] - }), - ) -} - type TopicGroup = { id: number title: string @@ -258,28 +246,23 @@ const RootTopicList = styled(PlainList)(({ theme }) => ({ })) const ToopicsListingPage: React.FC = () => { + const channelCountQuery = useChannelCounts("topic") const topicsQuery = useLearningResourceTopics() - const courseQuery = useLearningResourcesSearch({ - resource_type: ["course"], - aggregations: ["topic"], - }) - const programQuery = useLearningResourcesSearch({ - resource_type: ["program"], - aggregations: ["topic"], - }) + const channelsGroups = useMemo(() => { - const courseCounts = courseQuery.data - ? aggregateByTopic(courseQuery.data) + const courseCounts = channelCountQuery.data + ? aggregateCourseCounts("title", channelCountQuery.data) : {} - const programCounts = programQuery.data - ? aggregateByTopic(programQuery.data) + const programCounts = channelCountQuery.data + ? aggregateProgramCounts("title", channelCountQuery.data) : {} return groupTopics( topicsQuery.data?.results ?? [], courseCounts, programCounts, ) - }, [topicsQuery.data?.results, courseQuery.data, programQuery.data]) + }, [topicsQuery.data?.results, channelCountQuery.data]) + return ( diff --git a/frontends/mit-learn/src/pages/UnitsListingPage/UnitsListingPage.tsx b/frontends/mit-learn/src/pages/UnitsListingPage/UnitsListingPage.tsx index c594d7a442..a981904808 100644 --- a/frontends/mit-learn/src/pages/UnitsListingPage/UnitsListingPage.tsx +++ b/frontends/mit-learn/src/pages/UnitsListingPage/UnitsListingPage.tsx @@ -15,31 +15,12 @@ import { LearningResourceOfferorDetail } from "api" import { HOME } from "@/common/urls" import { UnitCards, UnitCardLoading } from "./UnitCard" import MetaTags from "@/page-components/MetaTags/MetaTags" -import { ChannelCounts } from "api/v0" + +import { aggregateProgramCounts, aggregateCourseCounts } from "@/common/utils" const UNITS_BANNER_IMAGE = "/static/images/background_steps.jpeg" const DESKTOP_WIDTH = "1056px" -const aggregateProgramCounts = ( - data: Array, -): Record => { - return Object.fromEntries( - Object.entries(data).map(([_key, value]) => { - return [value.name, value.counts.programs] - }), - ) -} - -const aggregateCourseCounts = ( - data: Array, -): Record => { - return Object.fromEntries( - Object.entries(data).map(([_key, value]) => { - return [value.name, value.counts.courses] - }), - ) -} - const sortUnits = ( units: Array | undefined, courseCounts: Record, @@ -221,12 +202,11 @@ const UnitsListingPage: React.FC = () => { const channelCountQuery = useChannelCounts("unit") const courseCounts = channelCountQuery.data - ? aggregateCourseCounts(channelCountQuery.data) + ? aggregateCourseCounts("name", channelCountQuery.data) : {} const programCounts = channelCountQuery.data - ? aggregateProgramCounts(channelCountQuery.data) + ? aggregateProgramCounts("name", channelCountQuery.data) : {} - const academicUnits = sortUnits( units?.filter((unit) => unit.professional === false), courseCounts, From bdb534e1de854762d261b8f47a516520af33b8ed Mon Sep 17 00:00:00 2001 From: Matt Bertrand Date: Tue, 8 Oct 2024 11:58:00 -0400 Subject: [PATCH 4/6] Add location field to LearningResource and LearningResourceRun models (#1604) --- frontends/api/src/generated/v1/api.ts | 102 ++++++++++++++++++ learning_resources/etl/constants.py | 1 + learning_resources/etl/loaders.py | 15 +-- learning_resources/etl/loaders_test.py | 3 + learning_resources/etl/sloan.py | 16 +++ learning_resources/etl/sloan_test.py | 21 ++++ .../0070_learningresource_location.py | 22 ++++ learning_resources/models.py | 4 +- learning_resources/serializers_test.py | 1 + learning_resources_search/constants.py | 2 + openapi/specs/v1.yaml | 51 +++++++++ 11 files changed, 231 insertions(+), 7 deletions(-) create mode 100644 learning_resources/migrations/0070_learningresource_location.py diff --git a/frontends/api/src/generated/v1/api.ts b/frontends/api/src/generated/v1/api.ts index b230201908..0b518da320 100644 --- a/frontends/api/src/generated/v1/api.ts +++ b/frontends/api/src/generated/v1/api.ts @@ -806,6 +806,12 @@ export interface CourseResource { * @memberof CourseResource */ continuing_ed_credits?: string | null + /** + * + * @type {string} + * @memberof CourseResource + */ + location?: string } /** @@ -1020,6 +1026,12 @@ export interface CourseResourceRequest { * @memberof CourseResourceRequest */ continuing_ed_credits?: string | null + /** + * + * @type {string} + * @memberof CourseResourceRequest + */ + location?: string } /** @@ -1582,6 +1594,12 @@ export interface LearningPathResource { * @memberof LearningPathResource */ continuing_ed_credits?: string | null + /** + * + * @type {string} + * @memberof LearningPathResource + */ + location?: string } /** @@ -1680,6 +1698,12 @@ export interface LearningPathResourceRequest { * @memberof LearningPathResourceRequest */ continuing_ed_credits?: string | null + /** + * + * @type {string} + * @memberof LearningPathResourceRequest + */ + location?: string } /** @@ -2336,6 +2360,12 @@ export interface LearningResourceRun { * @memberof LearningResourceRun */ availability?: AvailabilityEnum | null + /** + * + * @type {string} + * @memberof LearningResourceRun + */ + location?: string } /** @@ -2491,6 +2521,12 @@ export interface LearningResourceRunRequest { * @memberof LearningResourceRunRequest */ availability?: AvailabilityEnum | null + /** + * + * @type {string} + * @memberof LearningResourceRunRequest + */ + location?: string } /** @@ -3517,6 +3553,12 @@ export interface PatchedLearningPathResourceRequest { * @memberof PatchedLearningPathResourceRequest */ continuing_ed_credits?: string | null + /** + * + * @type {string} + * @memberof PatchedLearningPathResourceRequest + */ + location?: string } /** @@ -4315,6 +4357,12 @@ export interface PodcastEpisodeResource { * @memberof PodcastEpisodeResource */ continuing_ed_credits?: string | null + /** + * + * @type {string} + * @memberof PodcastEpisodeResource + */ + location?: string } /** @@ -4413,6 +4461,12 @@ export interface PodcastEpisodeResourceRequest { * @memberof PodcastEpisodeResourceRequest */ continuing_ed_credits?: string | null + /** + * + * @type {string} + * @memberof PodcastEpisodeResourceRequest + */ + location?: string } /** @@ -4691,6 +4745,12 @@ export interface PodcastResource { * @memberof PodcastResource */ continuing_ed_credits?: string | null + /** + * + * @type {string} + * @memberof PodcastResource + */ + location?: string } /** @@ -4789,6 +4849,12 @@ export interface PodcastResourceRequest { * @memberof PodcastResourceRequest */ continuing_ed_credits?: string | null + /** + * + * @type {string} + * @memberof PodcastResourceRequest + */ + location?: string } /** @@ -5287,6 +5353,12 @@ export interface ProgramResource { * @memberof ProgramResource */ continuing_ed_credits?: string | null + /** + * + * @type {string} + * @memberof ProgramResource + */ + location?: string } /** @@ -5385,6 +5457,12 @@ export interface ProgramResourceRequest { * @memberof ProgramResourceRequest */ continuing_ed_credits?: string | null + /** + * + * @type {string} + * @memberof ProgramResourceRequest + */ + location?: string } /** @@ -6142,6 +6220,12 @@ export interface VideoPlaylistResource { * @memberof VideoPlaylistResource */ continuing_ed_credits?: string | null + /** + * + * @type {string} + * @memberof VideoPlaylistResource + */ + location?: string } /** @@ -6240,6 +6324,12 @@ export interface VideoPlaylistResourceRequest { * @memberof VideoPlaylistResourceRequest */ continuing_ed_credits?: string | null + /** + * + * @type {string} + * @memberof VideoPlaylistResourceRequest + */ + location?: string } /** @@ -6506,6 +6596,12 @@ export interface VideoResource { * @memberof VideoResource */ continuing_ed_credits?: string | null + /** + * + * @type {string} + * @memberof VideoResource + */ + location?: string } /** @@ -6604,6 +6700,12 @@ export interface VideoResourceRequest { * @memberof VideoResourceRequest */ continuing_ed_credits?: string | null + /** + * + * @type {string} + * @memberof VideoResourceRequest + */ + location?: string } /** diff --git a/learning_resources/etl/constants.py b/learning_resources/etl/constants.py index fd1e7fcc6a..339e83c291 100644 --- a/learning_resources/etl/constants.py +++ b/learning_resources/etl/constants.py @@ -116,3 +116,4 @@ class ResourceNextRunConfig: next_start_date: datetime = None prices: list[Decimal] = field(default_factory=list) availability: str = None + location: str = None diff --git a/learning_resources/etl/loaders.py b/learning_resources/etl/loaders.py index be472cce26..238ef8b40b 100644 --- a/learning_resources/etl/loaders.py +++ b/learning_resources/etl/loaders.py @@ -138,17 +138,20 @@ def load_run_dependent_values( next_upcoming_run or resource.runs.filter(published=True).order_by("-start_date").first() ) - resource.availability = best_run.availability if best_run else resource.availability - resource.prices = ( - best_run.prices - if resource.certification and best_run and best_run.prices - else [] - ) + if best_run: + resource.availability = best_run.availability + resource.prices = ( + best_run.prices + if resource.certification and best_run and best_run.prices + else [] + ) + resource.location = best_run.location resource.save() return ResourceNextRunConfig( next_start_date=resource.next_start_date, prices=resource.prices, availability=resource.availability, + location=resource.location, ) diff --git a/learning_resources/etl/loaders_test.py b/learning_resources/etl/loaders_test.py index 6a57a47261..1aabe6e9e9 100644 --- a/learning_resources/etl/loaders_test.py +++ b/learning_resources/etl/loaders_test.py @@ -1463,6 +1463,7 @@ def test_load_run_dependent_values(certification): availability=Availability.dated.name, prices=[Decimal("0.00"), Decimal("20.00")], start_date=closest_date, + location="Portland, ME", ) LearningResourceRunFactory.create( learning_resource=course, @@ -1470,11 +1471,13 @@ def test_load_run_dependent_values(certification): availability=Availability.dated.name, prices=[Decimal("0.00"), Decimal("50.00")], start_date=furthest_date, + location="Portland, OR", ) result = load_run_dependent_values(course) assert result.next_start_date == course.next_start_date == closest_date assert result.prices == course.prices == ([] if not certification else run.prices) assert result.availability == course.availability == Availability.dated.name + assert result.location == course.location == run.location @pytest.mark.parametrize( diff --git a/learning_resources/etl/sloan.py b/learning_resources/etl/sloan.py index a58b4e311e..deb90008ce 100644 --- a/learning_resources/etl/sloan.py +++ b/learning_resources/etl/sloan.py @@ -170,6 +170,21 @@ def parse_format(run_data: dict) -> str: return default_format() +def parse_location(run_data: dict) -> str: + """ + Parse location from run data + + Args: + run_data (list): the run data + + Returns: + str: the location + """ + if not run_data or run_data["Delivery"] == "Online": + return "" + return run_data["Location"] or "" + + def extract(): """ Extract Sloan Executive Education data @@ -232,6 +247,7 @@ def transform_run(run_data, course_data): "instructors": [{"full_name": name.strip()} for name in faculty_names], "pace": [parse_pace(run_data)], "format": parse_format(run_data), + "location": parse_location(run_data), } diff --git a/learning_resources/etl/sloan_test.py b/learning_resources/etl/sloan_test.py index 85c472d15e..b22a09ef7d 100644 --- a/learning_resources/etl/sloan_test.py +++ b/learning_resources/etl/sloan_test.py @@ -17,6 +17,7 @@ parse_datetime, parse_format, parse_image, + parse_location, parse_pace, transform_course, transform_delivery, @@ -130,6 +131,7 @@ def test_transform_run( "instructors": [{"full_name": name.strip()} for name in faculty_names], "pace": [Pace.instructor_paced.name], "format": [Format.synchronous.name], + "location": run_data["Location"], } @@ -280,3 +282,22 @@ def test_parse_format(delivery, run_format, expected_format): } assert parse_format(run_data) == expected_format assert parse_format(None) == [Format.asynchronous.name] + + +@pytest.mark.parametrize( + ("delivery", "location", "result"), + [ + ("Online", "Online", ""), + ("In Person", "Cambridge, MA", "Cambridge, MA"), + ("Blended", "Boston, MA", "Boston, MA"), + ("Online", None, ""), + ], +) +def test_parse_location(delivery, location, result): + """Test that the location is parsed correctly""" + run_data = { + "Delivery": delivery, + "Location": location, + } + assert parse_location(run_data) == result + assert parse_location(None) == "" diff --git a/learning_resources/migrations/0070_learningresource_location.py b/learning_resources/migrations/0070_learningresource_location.py new file mode 100644 index 0000000000..3d6ba2cec0 --- /dev/null +++ b/learning_resources/migrations/0070_learningresource_location.py @@ -0,0 +1,22 @@ +# Generated by Django 4.2.16 on 2024-09-24 15:28 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("learning_resources", "0069_learningresource_ocw_topics"), + ] + + operations = [ + migrations.AddField( + model_name="learningresource", + name="location", + field=models.CharField(blank=True, max_length=256), + ), + migrations.AddField( + model_name="learningresourcerun", + name="location", + field=models.CharField(blank=True, max_length=256), + ), + ] diff --git a/learning_resources/models.py b/learning_resources/models.py index e3c3bf3d75..b5f11e2981 100644 --- a/learning_resources/models.py +++ b/learning_resources/models.py @@ -8,7 +8,7 @@ from django.contrib.auth.models import User from django.contrib.postgres.fields import ArrayField from django.db import models -from django.db.models import Count, JSONField, OuterRef, Prefetch, Q +from django.db.models import CharField, Count, JSONField, OuterRef, Prefetch, Q from django.db.models.functions import Lower from django.utils import timezone @@ -455,6 +455,7 @@ class LearningResource(TimestampedModel): ), default=default_format, ) + location = models.CharField(max_length=256, blank=True) @property def audience(self) -> str | None: @@ -606,6 +607,7 @@ class LearningResourceRun(TimestampedModel): ), default=default_format, ) + location = CharField(max_length=256, blank=True) def __str__(self): return f"LearningResourceRun platform={self.learning_resource.platform} run_id={self.run_id}" # noqa: E501 diff --git a/learning_resources/serializers_test.py b/learning_resources/serializers_test.py index eb7a43e0b9..960c0bc622 100644 --- a/learning_resources/serializers_test.py +++ b/learning_resources/serializers_test.py @@ -273,6 +273,7 @@ def test_learning_resource_serializer( # noqa: PLR0913 "pace": [ {"code": lr_pace, "name": Pace[lr_pace].value} for lr_pace in resource.pace ], + "location": resource.location, "next_start_date": resource.next_start_date, "availability": resource.availability, "completeness": 1.0, diff --git a/learning_resources_search/constants.py b/learning_resources_search/constants.py index 31b9b8fa8a..d64ac12ada 100644 --- a/learning_resources_search/constants.py +++ b/learning_resources_search/constants.py @@ -285,6 +285,7 @@ class FilterConfig: }, }, "prices": {"type": "scaled_float", "scaling_factor": 100}, + "location": {"type": "keyword"}, }, }, "next_start_date": {"type": "date"}, @@ -293,6 +294,7 @@ class FilterConfig: "completeness": {"type": "float"}, "license_cc": {"type": "boolean"}, "continuing_ed_credits": {"type": "float"}, + "location": {"type": "keyword"}, } diff --git a/openapi/specs/v1.yaml b/openapi/specs/v1.yaml index 3ea879b09b..e0a8571a55 100644 --- a/openapi/specs/v1.yaml +++ b/openapi/specs/v1.yaml @@ -8095,6 +8095,9 @@ components: format: decimal pattern: ^-?\d{0,3}(?:\.\d{0,2})?$ nullable: true + location: + type: string + maxLength: 256 required: - certification - certification_type @@ -8186,6 +8189,9 @@ components: format: decimal pattern: ^-?\d{0,3}(?:\.\d{0,2})?$ nullable: true + location: + type: string + maxLength: 256 required: - readable_id - title @@ -8584,6 +8590,9 @@ components: format: decimal pattern: ^-?\d{0,3}(?:\.\d{0,2})?$ nullable: true + location: + type: string + maxLength: 256 required: - certification - certification_type @@ -8672,6 +8681,9 @@ components: format: decimal pattern: ^-?\d{0,3}(?:\.\d{0,2})?$ nullable: true + location: + type: string + maxLength: 256 required: - title LearningPathResourceResourceTypeEnum: @@ -9204,6 +9216,9 @@ components: oneOf: - $ref: '#/components/schemas/AvailabilityEnum' - $ref: '#/components/schemas/NullEnum' + location: + type: string + maxLength: 256 required: - delivery - format @@ -9315,6 +9330,9 @@ components: oneOf: - $ref: '#/components/schemas/AvailabilityEnum' - $ref: '#/components/schemas/NullEnum' + location: + type: string + maxLength: 256 required: - level - run_id @@ -10057,6 +10075,9 @@ components: format: decimal pattern: ^-?\d{0,3}(?:\.\d{0,2})?$ nullable: true + location: + type: string + maxLength: 256 PatchedLearningResourceRelationshipRequest: type: object description: CRUD serializer for LearningResourceRelationship @@ -10688,6 +10709,9 @@ components: format: decimal pattern: ^-?\d{0,3}(?:\.\d{0,2})?$ nullable: true + location: + type: string + maxLength: 256 required: - certification - certification_type @@ -10779,6 +10803,9 @@ components: format: decimal pattern: ^-?\d{0,3}(?:\.\d{0,2})?$ nullable: true + location: + type: string + maxLength: 256 required: - readable_id - title @@ -11018,6 +11045,9 @@ components: format: decimal pattern: ^-?\d{0,3}(?:\.\d{0,2})?$ nullable: true + location: + type: string + maxLength: 256 required: - certification - certification_type @@ -11109,6 +11139,9 @@ components: format: decimal pattern: ^-?\d{0,3}(?:\.\d{0,2})?$ nullable: true + location: + type: string + maxLength: 256 required: - readable_id - title @@ -11478,6 +11511,9 @@ components: format: decimal pattern: ^-?\d{0,3}(?:\.\d{0,2})?$ nullable: true + location: + type: string + maxLength: 256 required: - certification - certification_type @@ -11569,6 +11605,9 @@ components: format: decimal pattern: ^-?\d{0,3}(?:\.\d{0,2})?$ nullable: true + location: + type: string + maxLength: 256 required: - readable_id - title @@ -12080,6 +12119,9 @@ components: format: decimal pattern: ^-?\d{0,3}(?:\.\d{0,2})?$ nullable: true + location: + type: string + maxLength: 256 required: - certification - certification_type @@ -12171,6 +12213,9 @@ components: format: decimal pattern: ^-?\d{0,3}(?:\.\d{0,2})?$ nullable: true + location: + type: string + maxLength: 256 required: - readable_id - title @@ -12396,6 +12441,9 @@ components: format: decimal pattern: ^-?\d{0,3}(?:\.\d{0,2})?$ nullable: true + location: + type: string + maxLength: 256 required: - certification - certification_type @@ -12487,6 +12535,9 @@ components: format: decimal pattern: ^-?\d{0,3}(?:\.\d{0,2})?$ nullable: true + location: + type: string + maxLength: 256 required: - readable_id - title From 061d29b489b111dbb0cc84c7534b5af970938717 Mon Sep 17 00:00:00 2001 From: Matt Bertrand Date: Tue, 8 Oct 2024 12:16:10 -0400 Subject: [PATCH 5/6] Always delete past events during ETL, and filter them out from api results too just in case (#1660) --- news_events/etl/conftest.py | 6 ++--- news_events/etl/loaders.py | 10 +++++--- news_events/etl/loaders_test.py | 43 ++++++++++++++++++++++++--------- news_events/factories.py | 2 +- news_events/views.py | 9 +++++-- news_events/views_test.py | 7 ++++++ 6 files changed, 57 insertions(+), 20 deletions(-) diff --git a/news_events/etl/conftest.py b/news_events/etl/conftest.py index af351d1408..badc4352c9 100644 --- a/news_events/etl/conftest.py +++ b/news_events/etl/conftest.py @@ -65,7 +65,7 @@ def sources_data() -> SimpleNamespace: } if feed_type == FeedType.news.name else { - "event_datetime": "2024-03-15T13:42:36Z", + "event_datetime": "2124-03-15T13:42:36Z", **event_details, }, }, @@ -86,7 +86,7 @@ def sources_data() -> SimpleNamespace: } if feed_type == FeedType.news.name else { - "event_datetime": "2024-03-13T15:57:53Z", + "event_datetime": "2124-03-13T15:57:53Z", **event_details, }, }, @@ -111,7 +111,7 @@ def sources_data() -> SimpleNamespace: } if feed_type == FeedType.news.name else { - "event_datetime": "2024-02-15T13:42:36Z", + "event_datetime": "2124-02-15T13:42:36Z", **event_details, }, }, diff --git a/news_events/etl/loaders.py b/news_events/etl/loaders.py index 15ea7dcc08..4f01a50f88 100644 --- a/news_events/etl/loaders.py +++ b/news_events/etl/loaders.py @@ -2,6 +2,7 @@ import logging +from main.utils import now_in_utc from news_events.constants import FeedType from news_events.models import ( FeedEventDetail, @@ -153,10 +154,13 @@ def load_feed_source( FeedItem.objects.filter(source=source).exclude( pk__in=[item.pk for item in items if item] ).delete() - FeedImage.objects.filter( - feeditem__isnull=True, feedsource__isnull=True + # Always delete past events and orphaned images + FeedImage.objects.filter(feeditem__isnull=True, feedsource__isnull=True).delete() + if source.feed_type == FeedType.events.name: + FeedItem.objects.filter( + source=source, + event_details__event_datetime__lt=now_in_utc(), ).delete() - return source, items diff --git a/news_events/etl/loaders_test.py b/news_events/etl/loaders_test.py index 3903b97553..06eab486c1 100644 --- a/news_events/etl/loaders_test.py +++ b/news_events/etl/loaders_test.py @@ -7,7 +7,11 @@ from news_events.constants import FeedType from news_events.etl import loaders -from news_events.factories import FeedImageFactory, FeedItemFactory, FeedSourceFactory +from news_events.factories import ( + FeedImageFactory, + FeedItemFactory, + FeedSourceFactory, +) from news_events.models import FeedImage, FeedItem, FeedSource pytestmark = [pytest.mark.django_db] @@ -65,24 +69,41 @@ def load_feed_sources_bad_item(mocker, sources_data): ) -def test_load_feed_sources_delete_old_items(sources_data): +@pytest.mark.parametrize("empty_data", [True, False]) +def test_load_feed_sources_delete_old_items(sources_data, empty_data): """Tests that load_sources deletes old items and images""" - source_data = sources_data.news + source_data = sources_data.events source = FeedSourceFactory.create( - url=source_data[0]["url"], feed_type=FeedType.news.name + url=source_data[0]["url"], feed_type=FeedType.events.name ) - old_source_item = FeedItemFactory(source=source, is_news=True) - other_source_item = FeedItemFactory.create(is_news=True) + omitted_event_item = FeedItemFactory.create(is_event=True, source=source) + + expired_event_item = FeedItemFactory.create(is_event=True, source=source) + expired_event_item.event_details.event_datetime = "2000-01-01T00:00:00Z" + expired_event_item.event_details.save() + + other_source_item = FeedItemFactory.create(is_event=True) orphaned_image = FeedImageFactory.create() # no source or item - loaders.load_feed_sources(FeedType.news.name, source_data) + if empty_data: + source_data[0]["items"] = [] + loaders.load_feed_sources(FeedType.events.name, source_data) + + # Existing feed items with future dates should be removed only if source data exists + assert FeedItem.objects.filter(pk=omitted_event_item.pk).exists() is empty_data + assert ( + FeedImage.objects.filter(pk=omitted_event_item.image.pk).exists() is empty_data + ) - assert FeedItem.objects.filter(pk=old_source_item.pk).exists() is False - assert FeedImage.objects.filter(pk=old_source_item.image.pk).exists() is False + # Events from other sources should be unaffected assert FeedItem.objects.filter(pk=other_source_item.pk).exists() is True - assert FeedImage.objects.filter(pk=other_source_item.image.pk).exists() is True + + # Old events and orphaned images should always be removed + assert FeedItem.objects.filter(pk=expired_event_item.pk).exists() is False + assert FeedImage.objects.filter(pk=expired_event_item.image.pk).exists() is False assert FeedImage.objects.filter(pk=orphaned_image.pk).exists() is False - assert FeedItem.objects.filter(source=source).count() == 2 + + assert FeedItem.objects.filter(source=source).count() == 1 if empty_data else 2 def test_load_item_null_data(): diff --git a/news_events/factories.py b/news_events/factories.py index c6f7683dd3..6b70e39051 100644 --- a/news_events/factories.py +++ b/news_events/factories.py @@ -113,7 +113,7 @@ class FeedEventDetailFactory(factory.django.DjangoModelFactory): audience = factory.List(random.choices(["Faculty", "Public", "Students"])) # noqa: S311 location = factory.List(random.choices(["Online", "MIT Campus"])) # noqa: S311 event_type = factory.List(random.choices(["Webinar", "Concert", "Conference"])) # noqa: S311 - event_datetime = factory.Faker("date_time", tzinfo=UTC) + event_datetime = factory.Faker("future_datetime", tzinfo=UTC) class Meta: model = models.FeedEventDetail diff --git a/news_events/views.py b/news_events/views.py index ebe7882519..5ec5c27635 100644 --- a/news_events/views.py +++ b/news_events/views.py @@ -1,6 +1,7 @@ """Views for news_events""" from django.conf import settings +from django.db.models import Q from django.utils.decorators import method_decorator from drf_spectacular.utils import extend_schema, extend_schema_view from rest_framework import viewsets @@ -8,7 +9,8 @@ from main.filters import MultipleOptionsFilterBackend from main.permissions import AnonymousAccessReadonlyPermission -from main.utils import cache_page_for_all_users +from main.utils import cache_page_for_all_users, now_in_utc +from news_events.constants import FeedType from news_events.filters import FeedItemFilter, FeedSourceFilter from news_events.models import FeedItem, FeedSource from news_events.serializers import FeedItemSerializer, FeedSourceSerializer @@ -45,7 +47,10 @@ class FeedItemViewSet(viewsets.ReadOnlyModelViewSet): filterset_class = FeedItemFilter queryset = ( FeedItem.objects.select_related(*FeedItem.related_selects) - .all() + .filter( + Q(source__feed_type=FeedType.news.name) + | Q(event_details__event_datetime__gte=now_in_utc()) + ) .order_by( "-news_details__publish_date", "-event_details__event_datetime", diff --git a/news_events/views_test.py b/news_events/views_test.py index da79578a6e..0e6396dff4 100644 --- a/news_events/views_test.py +++ b/news_events/views_test.py @@ -1,11 +1,14 @@ """Tests for news_events views""" +from datetime import UTC, datetime + import pytest from django.urls import reverse from main.test_utils import assert_json_equal from news_events import factories, serializers from news_events.constants import FeedType +from news_events.factories import FeedEventDetailFactory def test_feed_source_viewset_list(client): @@ -71,6 +74,9 @@ def test_feed_item_viewset_list(client, is_news): else x.event_details.event_datetime, reverse=True, ) + past_event = FeedEventDetailFactory( + event_datetime=datetime(2020, 1, 1, tzinfo=UTC) + ).feed_item results = ( client.get(reverse("news_events:v0:news_events_items_api-list")) .json() @@ -79,6 +85,7 @@ def test_feed_item_viewset_list(client, is_news): assert_json_equal( [serializers.FeedItemSerializer(instance=item).data for item in items], results ) + assert past_event.id not in [item["id"] for item in results] @pytest.mark.parametrize("feed_type", FeedType.names()) From 9d7897b6da555f7bc6a9dd92b418138e3784a57c Mon Sep 17 00:00:00 2001 From: Doof Date: Tue, 8 Oct 2024 16:35:11 +0000 Subject: [PATCH 6/6] Release 0.21.1 --- RELEASE.rst | 9 +++++++++ main/settings.py | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/RELEASE.rst b/RELEASE.rst index 2bd8d68605..3bcb8bddfd 100644 --- a/RELEASE.rst +++ b/RELEASE.rst @@ -1,6 +1,15 @@ Release Notes ============= +Version 0.21.1 +-------------- + +- Always delete past events during ETL, and filter them out from api results too just in case (#1660) +- Add location field to LearningResource and LearningResourceRun models (#1604) +- Cached department and topic page counts (#1661) +- Shanbady/randomize featured resources (#1653) +- Ignore NotFoundErrors in switch_indices function (#1654) + Version 0.21.0 (Released October 07, 2024) -------------- diff --git a/main/settings.py b/main/settings.py index 7636321eb9..78d5fa679e 100644 --- a/main/settings.py +++ b/main/settings.py @@ -33,7 +33,7 @@ from main.settings_pluggy import * # noqa: F403 from openapi.settings_spectacular import open_spectacular_settings -VERSION = "0.21.0" +VERSION = "0.21.1" log = logging.getLogger()