From 1f3decf7a0bb42bc8c7cd0ec588a3b7f2ca06d5f Mon Sep 17 00:00:00 2001 From: Matt Bertrand Date: Tue, 23 Jul 2024 11:46:43 -0400 Subject: [PATCH 1/3] No prices for archived runs --- learning_resources/etl/loaders.py | 11 +++++++++-- learning_resources/etl/loaders_test.py | 18 ++++++++++++++++-- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/learning_resources/etl/loaders.py b/learning_resources/etl/loaders.py index 8acbc54137..6d8e8dc8a7 100644 --- a/learning_resources/etl/loaders.py +++ b/learning_resources/etl/loaders.py @@ -9,6 +9,7 @@ from django.db import transaction from learning_resources.constants import ( + AvailabilityType, LearningResourceFormat, LearningResourceRelationTypes, LearningResourceType, @@ -227,8 +228,14 @@ def load_run( image_data = run_data.pop("image", None) instructors_data = run_data.pop("instructors", []) - # Make sure any prices are unique and sorted in ascending order - run_data["prices"] = sorted(set(run_data.get("prices", [])), key=lambda x: float(x)) + if run_data.get("availability") == AvailabilityType.archived.value: + # Archived runs should not have prices + run_data["prices"] = [] + else: + # Make sure any prices are unique and sorted in ascending order + run_data["prices"] = sorted( + set(run_data.get("prices", [])), key=lambda x: float(x) + ) with transaction.atomic(): ( diff --git a/learning_resources/etl/loaders_test.py b/learning_resources/etl/loaders_test.py index 08494aaf2c..5e444831e8 100644 --- a/learning_resources/etl/loaders_test.py +++ b/learning_resources/etl/loaders_test.py @@ -12,6 +12,7 @@ from django.utils import timezone from learning_resources.constants import ( + AvailabilityType, LearningResourceFormat, LearningResourceRelationTypes, LearningResourceType, @@ -593,7 +594,10 @@ def test_load_course_dupe_urls(unique_url): @pytest.mark.parametrize("run_exists", [True, False]) -def test_load_run(run_exists): +@pytest.mark.parametrize( + "availability", [AvailabilityType.archived.value, AvailabilityType.current.value] +) +def test_load_run(run_exists, availability): """Test that load_run loads the course run""" course = CourseFactory.create(learning_resource__runs=[]) learning_resource_run = ( @@ -602,7 +606,11 @@ def test_load_run(run_exists): else LearningResourceRunFactory.build() ) props = model_to_dict( - LearningResourceRunFactory.build(run_id=learning_resource_run.run_id) + LearningResourceRunFactory.build( + run_id=learning_resource_run.run_id, + availability=availability, + prices=["70.00", "20.00"], + ) ) del props["id"] @@ -618,6 +626,12 @@ def test_load_run(run_exists): assert isinstance(result, LearningResourceRun) + assert result.prices == ( + [] + if availability == AvailabilityType.archived.value + else sorted(props["prices"]) + ) + props.pop("prices") for key, value in props.items(): assert getattr(result, key) == value, f"Property {key} should equal {value}" From 7d7efe7fd9a56cbe3b61d44b6788b03c2407b1c9 Mon Sep 17 00:00:00 2001 From: Matt Bertrand Date: Tue, 23 Jul 2024 12:33:39 -0400 Subject: [PATCH 2/3] Also set prices = [] if certification == False --- learning_resources/etl/loaders.py | 7 +++++-- learning_resources/etl/loaders_test.py | 20 +++++++++++++------- 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/learning_resources/etl/loaders.py b/learning_resources/etl/loaders.py index 6d8e8dc8a7..3e990a94a7 100644 --- a/learning_resources/etl/loaders.py +++ b/learning_resources/etl/loaders.py @@ -228,8 +228,11 @@ def load_run( image_data = run_data.pop("image", None) instructors_data = run_data.pop("instructors", []) - if run_data.get("availability") == AvailabilityType.archived.value: - # Archived runs should not have prices + if ( + run_data.get("availability") == AvailabilityType.archived.value + or learning_resource.certification is False + ): + # Archived runs or runs of resources w/out certificates should not have prices run_data["prices"] = [] else: # Make sure any prices are unique and sorted in ascending order diff --git a/learning_resources/etl/loaders_test.py b/learning_resources/etl/loaders_test.py index 5e444831e8..4cbf40eab3 100644 --- a/learning_resources/etl/loaders_test.py +++ b/learning_resources/etl/loaders_test.py @@ -392,7 +392,9 @@ def test_load_course( # noqa: PLR0913 ) assert result.next_start_date == expected_next_start_date assert result.prices == ( - [Decimal(0.00), Decimal(49.00)] if is_run_published else [] + [Decimal(0.00), Decimal(49.00)] + if is_run_published and result.certification + else [] ) if course_exists and ((not is_published or not is_run_published) or blocklisted): @@ -597,11 +599,14 @@ def test_load_course_dupe_urls(unique_url): @pytest.mark.parametrize( "availability", [AvailabilityType.archived.value, AvailabilityType.current.value] ) -def test_load_run(run_exists, availability): +@pytest.mark.parametrize("certification", [True, False]) +def test_load_run(run_exists, availability, certification): """Test that load_run loads the course run""" - course = CourseFactory.create(learning_resource__runs=[]) + course = LearningResourceFactory.create( + is_course=True, runs=[], certification=certification + ) learning_resource_run = ( - LearningResourceRunFactory.create(learning_resource=course.learning_resource) + LearningResourceRunFactory.create(learning_resource=course) if run_exists else LearningResourceRunFactory.build() ) @@ -617,18 +622,19 @@ def test_load_run(run_exists, availability): del props["learning_resource"] assert LearningResourceRun.objects.count() == (1 if run_exists else 0) + assert course.certification == certification - result = load_run(course.learning_resource, props) + result = load_run(course, props) assert LearningResourceRun.objects.count() == 1 - assert result.learning_resource == course.learning_resource + assert result.learning_resource == course assert isinstance(result, LearningResourceRun) assert result.prices == ( [] - if availability == AvailabilityType.archived.value + if (availability == AvailabilityType.archived.value or certification is False) else sorted(props["prices"]) ) props.pop("prices") From 9b12c25e2cdc87105c3c6cdfdd191e609cccba41 Mon Sep 17 00:00:00 2001 From: Matt Bertrand Date: Tue, 23 Jul 2024 13:11:15 -0400 Subject: [PATCH 3/3] Additional test --- learning_resources/etl/loaders.py | 6 +++++- learning_resources/etl/loaders_test.py | 17 +++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/learning_resources/etl/loaders.py b/learning_resources/etl/loaders.py index 3e990a94a7..021900bb29 100644 --- a/learning_resources/etl/loaders.py +++ b/learning_resources/etl/loaders.py @@ -126,7 +126,11 @@ def load_next_start_date_and_prices( next_upcoming_run or resource.runs.filter(published=True).order_by("-start_date").first() ) - resource.prices = best_run.prices if best_run and best_run.prices else [] + resource.prices = ( + best_run.prices + if resource.certification and best_run and best_run.prices + else [] + ) resource.save() return resource.next_start_date, resource.prices diff --git a/learning_resources/etl/loaders_test.py b/learning_resources/etl/loaders_test.py index 4cbf40eab3..5bba504525 100644 --- a/learning_resources/etl/loaders_test.py +++ b/learning_resources/etl/loaders_test.py @@ -32,6 +32,7 @@ load_courses, load_image, load_instructors, + load_next_start_date_and_prices, load_offered_by, load_playlist, load_playlists, @@ -1385,3 +1386,19 @@ def test_load_course_percolation( mock_upsert_tasks.upsert_learning_resource_immutable_signature.assert_called_with( result.id ) + + +@pytest.mark.parametrize("certification", [True, False]) +def test_load_prices_by_certificate(certification): + """Prices should be empty for a course without certificates, else equal to only published run""" + course = LearningResourceFactory.create( + is_course=True, certification=certification, runs=[] + ) + run = LearningResourceRunFactory.create( + learning_resource=course, + published=True, + availability=AvailabilityType.current.value, + prices=[Decimal("0.00"), Decimal("20.00")], + ) + load_next_start_date_and_prices(course) + assert course.prices == ([] if not certification else run.prices)