From 8aa9d868bfaba4fc04b0595c8dc5b3fd124168b8 Mon Sep 17 00:00:00 2001 From: Arunmozhi Date: Wed, 1 Jan 2025 03:40:16 +1100 Subject: [PATCH 01/11] fix: use "price" in ecommerce data loader (#4450) (#4459) The Django Oscar upgrade of ecommerce changed the item's price field from `price_excl_tax` to just `price` causing the EcommerceApi data loader to fail. This commit checks for the `price_excl_tax` key and falls back to the 'price' value. Ref: https://github.com/openedx/ecommerce/pull/4050 --- .../apps/course_metadata/data_loaders/api.py | 12 +++++----- .../data_loaders/tests/mock_data.py | 22 +++++++++---------- .../data_loaders/tests/test_api.py | 6 ++--- 3 files changed, 21 insertions(+), 19 deletions(-) diff --git a/course_discovery/apps/course_metadata/data_loaders/api.py b/course_discovery/apps/course_metadata/data_loaders/api.py index eb5e44be73..25bfa4f534 100644 --- a/course_discovery/apps/course_metadata/data_loaders/api.py +++ b/course_discovery/apps/course_metadata/data_loaders/api.py @@ -573,7 +573,7 @@ def update_seats(self, body): def update_seat(self, course_run, product_body): stock_record = product_body['stockrecords'][0] currency_code = stock_record['price_currency'] - price = Decimal(stock_record['price_excl_tax']) + price = Decimal(stock_record.get('price_excl_tax', stock_record['price'])) sku = stock_record['partner_sku'] # For more context see ADR docs/decisions/0025-dont-sync-mobile-skus-on-discovery.rst @@ -644,9 +644,11 @@ def update_seat(self, course_run, product_body): def validate_stockrecord(self, stockrecords, title, product_class): """ Argument: - body (dict): product data from ecommerce, either entitlement or enrollment code + sockrecords (list): a list of stock records to validate from ecommerce + title (str): product title + product_class (str): either entitlement or enrollment code Returns: - product sku if no exceptions, else None + True when all validation checks pass, else None """ # Map product_class keys with how they should be displayed in the exception messages. product_classes = { @@ -681,7 +683,7 @@ def validate_stockrecord(self, stockrecords, title, product_class): try: currency_code = stock_record['price_currency'] - Decimal(stock_record['price_excl_tax']) + Decimal(stock_record.get('price_excl_tax', stock_record['price'])) sku = stock_record['partner_sku'] except (KeyError, ValueError): msg = 'A necessary stockrecord field is missing or incorrectly set for {product} {title}'.format( @@ -720,7 +722,7 @@ def update_entitlement(self, body): stock_record = stockrecords[0] currency_code = stock_record['price_currency'] - price = Decimal(stock_record['price_excl_tax']) + price = Decimal(stock_record.get('price_excl_tax', stock_record['price'])) sku = stock_record['partner_sku'] try: diff --git a/course_discovery/apps/course_metadata/data_loaders/tests/mock_data.py b/course_discovery/apps/course_metadata/data_loaders/tests/mock_data.py index 2afdb9a707..53b0981964 100644 --- a/course_discovery/apps/course_metadata/data_loaders/tests/mock_data.py +++ b/course_discovery/apps/course_metadata/data_loaders/tests/mock_data.py @@ -195,7 +195,7 @@ "stockrecords": [ { "price_currency": "USD", - "price_excl_tax": "0.00", + "price": "0.00", "partner_sku": "sku001", } ] @@ -225,7 +225,7 @@ "stockrecords": [ { "price_currency": "EUR", - "price_excl_tax": "0.00", + "price": "0.00", "partner_sku": "sku002", } ] @@ -242,7 +242,7 @@ "stockrecords": [ { "price_currency": "EUR", - "price_excl_tax": "25.00", + "price": "25.00", "partner_sku": "sku003", } ] @@ -260,7 +260,7 @@ "stockrecords": [ { "price_currency": "EUR", - "price_excl_tax": "250.00", + "price": "250.00", "partner_sku": "mobile.android.sku003", } ] @@ -277,7 +277,7 @@ "stockrecords": [ { "price_currency": "EUR", - "price_excl_tax": "25.00", + "price": "25.00", "partner_sku": "sku004" } ] @@ -304,7 +304,7 @@ "stockrecords": [ { "price_currency": "USD", - "price_excl_tax": "0.00", + "price": "0.00", "partner_sku": "sku005", } ] @@ -321,7 +321,7 @@ "stockrecords": [ { "price_currency": "USD", - "price_excl_tax": "25.00", + "price": "25.00", "partner_sku": "sku006", } ] @@ -350,7 +350,7 @@ "stockrecords": [ { "price_currency": "USD", - "price_excl_tax": "250.00", + "price": "250.00", "partner_sku": "sku007", } ] @@ -379,7 +379,7 @@ "stockrecords": [ { "price_currency": "USD", - "price_excl_tax": "250.00", + "price": "250.00", "partner_sku": "sku008", } ] @@ -404,7 +404,7 @@ "stockrecords": [ { "price_currency": "123", - "price_excl_tax": "0.00", + "price": "0.00", "partner_sku": "sku009", } ] @@ -429,7 +429,7 @@ "stockrecords": [ { "price_currency": "USD", - "price_excl_tax": "0.00", + "price": "0.00", "partner_sku": "sku010", } ] diff --git a/course_discovery/apps/course_metadata/data_loaders/tests/test_api.py b/course_discovery/apps/course_metadata/data_loaders/tests/test_api.py index 237ed17f11..7ea7ac400f 100644 --- a/course_discovery/apps/course_metadata/data_loaders/tests/test_api.py +++ b/course_discovery/apps/course_metadata/data_loaders/tests/test_api.py @@ -769,7 +769,7 @@ def mock_products_api(self, alt_course=None, alt_currency=None, alt_mode=None, h stockrecord = { "price_currency": alt_currency if alt_currency else "USD", - "price_excl_tax": "10.00", + "price": "10.00", } if valid_stockrecord: stockrecord.update({"partner_sku": "sku132"}) @@ -856,7 +856,7 @@ def assert_seats_loaded(self, body, mock_products): for product in products: stock_record = product['stockrecords'][0] price_currency = stock_record['price_currency'] - price = Decimal(stock_record['price_excl_tax']) + price = Decimal(stock_record['price']) sku = stock_record['partner_sku'] certificate_type = Seat.AUDIT credit_provider = None @@ -908,7 +908,7 @@ def assert_entitlements_loaded(self, body): course = Course.objects.get(uuid=attributes['UUID']) stock_record = datum['stockrecords'][0] price_currency = stock_record['price_currency'] - price = Decimal(stock_record['price_excl_tax']) + price = Decimal(stock_record['price']) sku = stock_record['partner_sku'] mode_name = attributes['certificate_type'] From d63d11931d18b57873d4256cd03a686e1b8bae15 Mon Sep 17 00:00:00 2001 From: edX requirements bot <49161187+edx-requirements-bot@users.noreply.github.com> Date: Mon, 6 Jan 2025 00:41:22 -0500 Subject: [PATCH 02/11] chore: python requirements update (#4532) --- requirements/docs.txt | 2 +- requirements/local.txt | 16 ++++++++-------- requirements/pip.txt | 2 +- requirements/production.txt | 10 +++++----- 4 files changed, 15 insertions(+), 15 deletions(-) diff --git a/requirements/docs.txt b/requirements/docs.txt index e70eba4799..f96d53bca0 100644 --- a/requirements/docs.txt +++ b/requirements/docs.txt @@ -49,7 +49,7 @@ packaging==24.2 # sphinx pydata-sphinx-theme==0.15.4 # via sphinx-book-theme -pygments==2.18.0 +pygments==2.19.0 # via # accessible-pygments # pydata-sphinx-theme diff --git a/requirements/local.txt b/requirements/local.txt index dffdbddfbf..975e077d05 100644 --- a/requirements/local.txt +++ b/requirements/local.txt @@ -64,9 +64,9 @@ boltons==21.0.0 # face # glom # semgrep -boto3==1.35.90 +boto3==1.35.92 # via django-ses -botocore==1.35.90 +botocore==1.35.92 # via # boto3 # s3transfer @@ -418,7 +418,7 @@ face==24.0.0 # via glom factory-boy==3.3.1 # via -r requirements/test.in -faker==33.1.0 +faker==33.3.0 # via factory-boy fastavro==1.10.0 # via openedx-events @@ -439,7 +439,7 @@ glom==22.1.0 # via semgrep google-api-core==2.24.0 # via google-api-python-client -google-api-python-client==2.156.0 +google-api-python-client==2.157.0 # via -r requirements/base.in google-auth==2.37.0 # via @@ -611,7 +611,7 @@ pycparser==2.22 # via cffi pydata-sphinx-theme==0.15.4 # via sphinx-book-theme -pygments==2.18.0 +pygments==2.19.0 # via # accessible-pygments # pydata-sphinx-theme @@ -649,7 +649,7 @@ pynacl==1.5.0 # via edx-django-utils pyopenssl==24.3.0 # via snowflake-connector-python -pyparsing==3.2.0 +pyparsing==3.2.1 # via # httplib2 # packaging @@ -843,7 +843,7 @@ stevedore==5.4.0 # code-annotations # edx-django-utils # edx-opaque-keys -taxonomy-connector==1.54.0 +taxonomy-connector==2.0.0 # via -r requirements/base.in testfixtures==8.3.0 # via -r requirements/test.in @@ -903,7 +903,7 @@ vine==5.1.0 # amqp # celery # kombu -virtualenv==20.28.0 +virtualenv==20.28.1 # via tox walrus==0.9.4 # via edx-event-bus-redis diff --git a/requirements/pip.txt b/requirements/pip.txt index 1225c42ef7..6248d33a4b 100644 --- a/requirements/pip.txt +++ b/requirements/pip.txt @@ -12,5 +12,5 @@ pip==24.2 # via # -c /home/runner/work/course-discovery/course-discovery/requirements/common_constraints.txt # -r requirements/pip.in -setuptools==75.6.0 +setuptools==75.7.0 # via -r requirements/pip.in diff --git a/requirements/production.txt b/requirements/production.txt index 768f28d14a..69cd230fc8 100644 --- a/requirements/production.txt +++ b/requirements/production.txt @@ -41,9 +41,9 @@ beautifulsoup4==4.12.3 # taxonomy-connector billiard==4.2.1 # via celery -boto3==1.35.90 +boto3==1.35.92 # via django-ses -botocore==1.35.90 +botocore==1.35.92 # via # boto3 # s3transfer @@ -356,7 +356,7 @@ gevent==24.11.1 # via -r requirements/production.in google-api-core==2.24.0 # via google-api-python-client -google-api-python-client==2.156.0 +google-api-python-client==2.157.0 # via -r requirements/base.in google-auth==2.37.0 # via @@ -507,7 +507,7 @@ pynacl==1.5.0 # via edx-django-utils pyopenssl==24.3.0 # via snowflake-connector-python -pyparsing==3.2.0 +pyparsing==3.2.1 # via httplib2 python-dateutil==2.9.0.post0 # via @@ -616,7 +616,7 @@ stevedore==5.4.0 # code-annotations # edx-django-utils # edx-opaque-keys -taxonomy-connector==1.54.0 +taxonomy-connector==2.0.0 # via -r requirements/base.in text-unidecode==1.3 # via python-slugify From 88b3ebf60df76330622ed0fa76bc09231310c41f Mon Sep 17 00:00:00 2001 From: zawan-ila <87228907+zawan-ila@users.noreply.github.com> Date: Mon, 6 Jan 2025 15:42:36 +0500 Subject: [PATCH 03/11] feat: exclude retired coursetypes and runtypes from api (#4525) --- course_discovery/apps/api/serializers.py | 5 +- .../v1/tests/test_views/test_course_runs.py | 38 +++++++++++++ .../api/v1/tests/test_views/test_courses.py | 55 +++++++++++++++++-- .../apps/api/v1/views/course_runs.py | 8 ++- course_discovery/apps/api/v1/views/courses.py | 5 ++ course_discovery/settings/base.py | 3 + course_discovery/settings/test.py | 3 + 7 files changed, 110 insertions(+), 7 deletions(-) diff --git a/course_discovery/apps/api/serializers.py b/course_discovery/apps/api/serializers.py index d186dbf4b6..2d6ea9a126 100644 --- a/course_discovery/apps/api/serializers.py +++ b/course_discovery/apps/api/serializers.py @@ -11,6 +11,7 @@ import pytz import waffle # lint-amnesty, pylint: disable=invalid-django-waffle-import +from django.conf import settings from django.contrib.auth import get_user_model from django.db.models import Count from django.db.models.query import Prefetch @@ -2742,7 +2743,9 @@ def create_type_options(self, info): ], } for course_type in CourseType.objects.prefetch_related( 'course_run_types__tracks__mode', 'entitlement_types', 'white_listed_orgs' - ).exclude(slug=CourseType.EMPTY) if not course_type.white_listed_orgs.exists() or user.is_staff or + ).exclude( + slug__in=[CourseType.EMPTY, *settings.RETIRED_COURSE_TYPES] + ) if not course_type.white_listed_orgs.exists() or user.is_staff or user.groups.filter(organization_extension__organization__in=course_type.white_listed_orgs.all()).exists()] return info diff --git a/course_discovery/apps/api/v1/tests/test_views/test_course_runs.py b/course_discovery/apps/api/v1/tests/test_views/test_course_runs.py index f9a07de3a3..d80cd73cbd 100644 --- a/course_discovery/apps/api/v1/tests/test_views/test_course_runs.py +++ b/course_discovery/apps/api/v1/tests/test_views/test_course_runs.py @@ -1,3 +1,4 @@ +import contextlib import datetime import urllib import uuid @@ -103,6 +104,25 @@ def test_get(self): assert response.status_code == 200 assert response.data == self.serialize_course_run(self.course_run) + @ddt.data( + [True, 200], + [False, 404] + ) + @ddt.unpack + def test_get_filters_retired(self, include_retired, status_code): + """ Verify the endpoint excludes retired run types by default. """ + bootcamp_type, _ = CourseRunType.objects.get_or_create(slug=CourseRunType.PAID_BOOTCAMP) + run = CourseRunFactory(course__partner=self.partner, type=bootcamp_type) + url = reverse('api:v1:course_run-detail', kwargs={'key': run.key}) + if include_retired: + url += '?include_retired_run_types=1' + + context = self.assertNumQueries(15, threshold=3) if include_retired else contextlib.nullcontext(0) + with context: + response = self.client.get(url) + + assert response.status_code == status_code + def test_get_exclude_deleted_programs(self): """ Verify the endpoint returns no associated deleted programs """ ProgramFactory(courses=[self.course_run.course], status=ProgramStatus.Deleted) @@ -1208,6 +1228,24 @@ def test_list(self): self.serialize_course_run(CourseRun.objects.all().order_by(Lower('key')), many=True) ) + @ddt.data( + [True, 3], + [False, 2] + ) + @ddt.unpack + def test_list_filters_retired(self, include_retired, expected_length): + """ Verify the endpoint excludes retired types by default. """ + bootcamp_type, _ = CourseRunType.objects.get_or_create(slug=CourseRunType.PAID_BOOTCAMP) + CourseRunFactory(course__partner=self.partner, type=bootcamp_type) + url = reverse('api:v1:course_run-list') + if include_retired: + url += '?include_retired_run_types=1' + + response = self.client.get(url) + + assert response.status_code == 200 + assert len(response.data['results']) == expected_length + def test_list_sorted_by_course_start_date(self): """ Verify the endpoint returns a list of all course runs sorted by start date. """ url = '{root}?ordering=start'.format(root=reverse('api:v1:course_run-list')) diff --git a/course_discovery/apps/api/v1/tests/test_views/test_courses.py b/course_discovery/apps/api/v1/tests/test_views/test_courses.py index 6f931722ea..904f37daba 100644 --- a/course_discovery/apps/api/v1/tests/test_views/test_courses.py +++ b/course_discovery/apps/api/v1/tests/test_views/test_courses.py @@ -110,11 +110,27 @@ def test_get(self): assert response.status_code == 200 assert response.data == self.serialize_course(self.course) + @ddt.data( + [True, 200], + [False, 404] + ) + @ddt.unpack + def test_get_filters_retired(self, include_retired, status_code): + """ Verify that retired courses types do not appear by default """ + bootcamp_type, _ = CourseType.objects.get_or_create(slug=CourseType.BOOTCAMP_2U) + bootcamp = CourseFactory(partner=self.partner, title='Fake Test', key='edX+bootcamp', type=bootcamp_type) + url = reverse('api:v1:course-detail', kwargs={'key': bootcamp.key}) + if include_retired: + url += '?include_retired_course_types=1' + + response = self.client.get(url) + assert response.status_code == status_code + def test_get_uuid(self): """ Verify the endpoint returns the details for a single course with UUID. """ url = reverse('api:v1:course-detail', kwargs={'key': self.course.uuid}) - with self.assertNumQueries(27): + with self.assertNumQueries(28): response = self.client.get(url) assert response.status_code == 200 assert response.data == self.serialize_course(self.course) @@ -354,6 +370,24 @@ def test_list(self): self.serialize_course(Course.objects.all(), many=True) ) + @ddt.data( + [True, 2], + [False, 1] + ) + @ddt.unpack + def test_list_filters_retired(self, include_retired, expected_length): + """ Verify that retired course types do not appear by default """ + bootcamp_type, _ = CourseType.objects.get_or_create(slug=CourseType.BOOTCAMP_2U) + CourseFactory(partner=self.partner, title='Fake Test', key='edX+bootcamp', type=bootcamp_type) + + url = reverse('api:v1:course-list') + if include_retired: + url += '?include_retired_course_types=1' + + response = self.client.get(url) + assert response.status_code == 200 + assert len(response.data['results']) == expected_length + def test_no_repeated_cache_calls_for_utm_calculation(self): """ Test that utm source calculation is done only once per request, and not per @@ -507,7 +541,8 @@ def test_list_courses_course_type_filter(self, course_type, expected_length): CourseFactory(partner=self.partner, title='Fake Test', key='edX+bootcamp', type=bootcamp_type) CourseFactory(partner=self.partner, title='Fake Test', key='edX+ver', type=self.verified_type) - url = reverse('api:v1:course-list') + '?editable=1&course_type={}'.format(course_type) + query_params = '?editable=1&include_retired_course_types=1&course_type={}'.format(course_type) + url = reverse('api:v1:course-list') + query_params response = self.client.get(url) assert response.status_code == 200 @@ -2589,13 +2624,20 @@ def test_update_geolocation__validation_error(self): assert GeoLocation.objects.count() == 1 @responses.activate - def test_options(self): + @ddt.data( + [['audit'], False], + [[], True] + ) + @ddt.unpack + def test_options(self, retired_course_types, is_retired_type_in_result): SubjectFactory(name='Subject1') CourseEntitlementFactory(course=self.course, mode=SeatTypeFactory.verified()) + audit_type, _ = CourseType.objects.get_or_create(slug=CourseType.AUDIT) url = reverse('api:v1:course-detail', kwargs={'key': self.course.uuid}) - with self.assertNumQueries(46, threshold=0): - response = self.client.options(url) + with override_settings(RETIRED_COURSE_TYPES=retired_course_types): + with self.assertNumQueries(46, threshold=1): + response = self.client.options(url) assert response.status_code == 200 data = response.json()['actions']['PUT'] @@ -2620,6 +2662,9 @@ def test_options(self): if options['uuid'] == str(credit_type.uuid): credit_options = options break + # Assert that retired course types do not appear in the result + type_uuids = [typ['uuid'] for typ in data['type']['type_options']] + assert (str(audit_type.uuid) in type_uuids) == is_retired_type_in_result assert credit_options is not None assert {t['mode']['slug'] for t in credit_options['tracks']} == {'verified', 'credit', 'audit'} diff --git a/course_discovery/apps/api/v1/views/course_runs.py b/course_discovery/apps/api/v1/views/course_runs.py index f9283b5dce..9a47e58fbd 100644 --- a/course_discovery/apps/api/v1/views/course_runs.py +++ b/course_discovery/apps/api/v1/views/course_runs.py @@ -1,5 +1,6 @@ import logging +from django.conf import settings from django.db import models, transaction from django.db.models.functions import Lower from django.http.response import Http404 @@ -27,7 +28,7 @@ from course_discovery.apps.course_metadata.choices import CourseRunStatus from course_discovery.apps.course_metadata.constants import COURSE_RUN_ID_REGEX from course_discovery.apps.course_metadata.exceptions import EcommerceSiteAPIClientException -from course_discovery.apps.course_metadata.models import Course, CourseEditor, CourseRun +from course_discovery.apps.course_metadata.models import Course, CourseEditor, CourseRun, CourseRunType from course_discovery.apps.course_metadata.utils import ensure_draft_world from course_discovery.apps.publisher.utils import is_publisher_user @@ -110,6 +111,11 @@ def get_queryset(self): ) else: queryset = queryset.filter(course__partner=partner) + if self.request.method == "GET" and not get_query_param(self.request, 'include_retired_run_types'): + retired_type_ids = list( + CourseRunType.objects.filter(slug__in=settings.RETIRED_RUN_TYPES).values_list('id', flat=True) + ) + queryset = queryset.exclude(type_id__in=retired_type_ids) return self.get_serializer_class().prefetch_queryset(queryset=queryset) diff --git a/course_discovery/apps/api/v1/views/courses.py b/course_discovery/apps/api/v1/views/courses.py index 50a2a5ec1e..3a41979fc8 100644 --- a/course_discovery/apps/api/v1/views/courses.py +++ b/course_discovery/apps/api/v1/views/courses.py @@ -162,6 +162,11 @@ def get_queryset(self): partner=partner, programs=programs, ) + if self.request.method == 'GET' and not get_query_param(self.request, 'include_retired_course_types'): + retired_type_ids = list( + CourseType.objects.filter(slug__in=settings.RETIRED_COURSE_TYPES).values_list('id', flat=True) + ) + queryset = queryset.exclude(type_id__in=retired_type_ids) if pub_q and edit_mode: return queryset.filter(Q(key__icontains=pub_q) | Q(title__icontains=pub_q)).order_by('key') diff --git a/course_discovery/settings/base.py b/course_discovery/settings/base.py index 2d578ac255..05eff58574 100644 --- a/course_discovery/settings/base.py +++ b/course_discovery/settings/base.py @@ -796,3 +796,6 @@ # If the keyword is found, the user has more lenient throttling limits. ENHANCED_THROTTLE_JWT_ROLE_KEYWORDS = [] ENHANCED_THROTTLE_LIMIT = '400/hour' + +RETIRED_RUN_TYPES = [] +RETIRED_COURSE_TYPES = [] diff --git a/course_discovery/settings/test.py b/course_discovery/settings/test.py index 4e6755cab2..e2246610be 100644 --- a/course_discovery/settings/test.py +++ b/course_discovery/settings/test.py @@ -163,3 +163,6 @@ ELASTICSEARCH_DSL_LOAD_PER_QUERY = 10000 ENHANCED_THROTTLE_JWT_ROLE_KEYWORDS = ['enterprise'] + +RETIRED_RUN_TYPES = ['paid-bootcamp', 'unpaid-bootcamp'] +RETIRED_COURSE_TYPES = ['bootcamp-2u'] From 445d177e0d2d821879a4ebd3ef3cbfa017c3f66e Mon Sep 17 00:00:00 2001 From: zawan-ila <87228907+zawan-ila@users.noreply.github.com> Date: Mon, 6 Jan 2025 16:12:47 +0500 Subject: [PATCH 04/11] feat: add course archival management cmd (#4524) --- course_discovery/apps/api/utils.py | 13 ++ .../apps/course_metadata/admin.py | 8 + .../apps/course_metadata/emails.py | 28 +++ .../management/commands/archive_courses.py | 175 ++++++++++++++++++ .../commands/tests/test_archive_courses.py | 158 ++++++++++++++++ .../migrations/0346_archivecoursesconfig.py | 33 ++++ .../apps/course_metadata/models.py | 14 ++ .../email/course_archival.html | 42 +++++ .../apps/course_metadata/tests/factories.py | 5 + course_discovery/settings/base.py | 1 + 10 files changed, 477 insertions(+) create mode 100644 course_discovery/apps/course_metadata/management/commands/archive_courses.py create mode 100644 course_discovery/apps/course_metadata/management/commands/tests/test_archive_courses.py create mode 100644 course_discovery/apps/course_metadata/migrations/0346_archivecoursesconfig.py create mode 100644 course_discovery/apps/course_metadata/templates/course_metadata/email/course_archival.html diff --git a/course_discovery/apps/api/utils.py b/course_discovery/apps/api/utils.py index 1c2cc518e3..77c3c96fa3 100644 --- a/course_discovery/apps/api/utils.py +++ b/course_discovery/apps/api/utils.py @@ -362,6 +362,19 @@ def push_to_studio(self, course_run, create=False, old_course_run_key=None, user else: self.update_course_run_details_in_studio(course_run) + def _update_end_date_in_studio(self, course_run): + """ + We do not update end dates in studio after creation. However, while archiving a course, we + may wish to set the end date in the past. This method serves that purpose. + """ + data = self.generate_data_for_studio_api(course_run, creating=False) + data.setdefault('schedule', {}) + for attr in ['end', 'enrollment_end']: + attr_val = getattr(course_run, attr) + if attr_val: + data['schedule'][attr] = serialize_datetime(attr_val) + self._request('patch', f'course_runs/{course_run.key}/', json=data) + def use_request_cache(cache_name, key_func): """ diff --git a/course_discovery/apps/course_metadata/admin.py b/course_discovery/apps/course_metadata/admin.py index 2fa25dbd30..8e40506356 100644 --- a/course_discovery/apps/course_metadata/admin.py +++ b/course_discovery/apps/course_metadata/admin.py @@ -1127,6 +1127,14 @@ class BulkUploadTagsConfigurationAdmin(admin.ModelAdmin): list_display = ('id', 'enabled', 'changed_by', 'change_date') +@admin.register(ArchiveCoursesConfig) +class ArchiveCoursesConfigurationAdmin(admin.ModelAdmin): + """ + Admin for ArchiveCoursesConfig model. + """ + list_display = ('id', 'enabled', 'changed_by', 'change_date') + + @admin.register(OrganizationMapping) class OrganizationMappingAdmin(admin.ModelAdmin): """ diff --git a/course_discovery/apps/course_metadata/emails.py b/course_discovery/apps/course_metadata/emails.py index 0389492913..bef0d532ea 100644 --- a/course_discovery/apps/course_metadata/emails.py +++ b/course_discovery/apps/course_metadata/emails.py @@ -6,6 +6,7 @@ import dateutil.parser from django.conf import settings +from django.core.mail import EmailMessage from django.core.mail.message import EmailMultiAlternatives from django.template.loader import get_template from django.utils.translation import gettext as _ @@ -416,3 +417,30 @@ def send_email_for_slug_updates(stats, to_users, subject=None): attachment.add_header('Content-Disposition', 'attachment', filename='slugs_update_summary.csv') email_msg.attach(attachment) email_msg.send() + + +def send_email_for_course_archival(report, csv_report, to_users): + """ + Send an overall report of a archive_courses mgmt command run + """ + success_count = len(report['successes']) + failure_count = len(report['failures']) + context = { + 'total_count': report['total_count'], + 'failure_count': failure_count, + 'success_count': success_count, + 'failures': report['failures'] + } + html_template = 'course_metadata/email/course_archival.html' + template = get_template(html_template) + html_content = template.render(context) + + email = EmailMessage( + "Course Archival Command Summary", + html_content, + settings.PUBLISHER_FROM_EMAIL, + to_users, + ) + email.attach("report.csv", csv_report, "text/csv") + email.content_subtype = "html" + email.send() diff --git a/course_discovery/apps/course_metadata/management/commands/archive_courses.py b/course_discovery/apps/course_metadata/management/commands/archive_courses.py new file mode 100644 index 0000000000..9ee13aae35 --- /dev/null +++ b/course_discovery/apps/course_metadata/management/commands/archive_courses.py @@ -0,0 +1,175 @@ +""" +Management command for archiving courses in bulk + +Example usage: + $ ./manage.py archive_courses --from-db + +Use ./manage.py archive_courses --help for more information on the available arguments and their behavior +""" +import csv +import io +import logging +from datetime import timedelta +from functools import reduce + +import unicodecsv +from django.conf import settings +from django.core.management import BaseCommand, CommandError +from django.db import transaction +from django.utils import timezone + +from course_discovery.apps.api.utils import StudioAPI +from course_discovery.apps.course_metadata.choices import ExternalProductStatus +from course_discovery.apps.course_metadata.emails import send_email_for_course_archival +from course_discovery.apps.course_metadata.models import ArchiveCoursesConfig, Course, CourseRunStatus, CourseType + +logger = logging.getLogger(__name__) + + +class Command(BaseCommand): + help = "Archive a collection of courses" + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self.report = { + 'failures': [], + 'successes': [], + 'total_count': 0 + } + + def add_arguments(self, parser): + parser.add_argument( + '--from-db', + help='Query the db for the uuids and command args. ' + 'The uuids and config is fetched from the ArchiveCoursesConfig model', + default=False, + action='store_true' + ) + parser.add_argument( + '--type', + type=str, + help="The course type to archive", + default="", + metavar="COURSE_TYPE" + ) + parser.add_argument( + '--mangle-end-date', + help="Set the end date and enrollment end date for the archived courses' runs in the past", + default=False, + action='store_true' + ) + parser.add_argument( + '--mangle-title', + help="Prepend 'DELETED' to the titles of the archived courses", + default=False, + action='store_true' + ) + + def handle(self, *args, **options): + from_db = options.get('from_db') + course_type = options.get('type') + mangle_end_date = options.get('mangle_end_date') + mangle_title = options.get('mangle_title') + + if from_db: + conf = ArchiveCoursesConfig.current() + mangle_end_date = conf.mangle_end_date + mangle_title = conf.mangle_title + courses = Course.objects.filter(uuid__in=self.get_uuids_from_database()) + elif course_type: + courses = Course.objects.filter( + type__in=[CourseType.objects.get(slug__in=[course_type, course_type.lower()])] + ) + else: + raise CommandError("Please provide one of --type or --from-db") + + self.report['total_count'] = courses.count() + + for course in courses: + # Store the original title in case we mangle it + course_title = course.title + try: + self.archive(course, mangle_end_date, mangle_title) + if course.draft_version: + self.archive(course.draft_version, mangle_end_date, mangle_title) + except Exception as exc: # pylint: disable=broad-exception-caught + self.report['failures'].append( + { + 'uuid': course.uuid, + 'title': course_title, + 'reason': repr(exc) + } + ) + logger.exception(f'Failed to archive course with uuid {course.uuid}') + else: + self.report['successes'].append( + { + 'uuid': course.uuid, + 'title': course_title + } + ) + logger.info(f"Successfully archived course with uuid: {course.uuid}") + + send_email_for_course_archival(self.report, self.get_csv_report(), settings.COURSE_ARCHIVAL_MAIL_RECIPIENTS) + + @transaction.atomic + def archive(self, course, mangle_end_date, mangle_title): + for course_run in course.course_runs.all(): + course_run.status = CourseRunStatus.Unpublished + course_run.save(update_fields=['status']) + + if mangle_end_date and course_run.end and course_run.end > timezone.now(): + course_run.end = timezone.now() + if mangle_end_date and course_run.enrollment_end and course_run.enrollment_end > timezone.now(): + course_run.enrollment_end = timezone.now() - timedelta(days=1) + self.verify_date_order(course_run) + course_run.save(update_fields=['end', 'enrollment_end']) + + # Push to studio to prevent RCM rewrite + if mangle_end_date: + api = StudioAPI(course_run.course.partner) + api._update_end_date_in_studio(course_run) # pylint: disable=protected-access + + if course.additional_metadata: + course.additional_metadata.product_status = ExternalProductStatus.Archived + if course.additional_metadata.end_date and course.additional_metadata.end_date > timezone.now(): + course.additional_metadata.end_date = timezone.now() + course.additional_metadata.save(update_fields=['product_status', 'end_date']) + + if mangle_title and not course.title.startswith('DELETED'): + course.title = f"DELETED - {course.title}" + course.save(update_fields=['title']) + + def get_uuids_from_database(self): + config = ArchiveCoursesConfig.current() + if not config.enabled: + raise CommandError('Configuration object is not enabled') + + if not config.csv_file: + raise CommandError('Configuration object does not have any input csv') + + reader = unicodecsv.DictReader(config.csv_file) + uuid_list = reduce(lambda uuid_list, row: uuid_list + list(row.values()), reader, []) + return uuid_list + + def get_csv_report(self): + output = io.StringIO() + writer = csv.writer(output) + writer.writerow(['course_uuid', 'title', 'status']) + for record in self.report['successes']: + writer.writerow([record['uuid'], record['title'], 'success']) + for record in self.report['failures']: + writer.writerow([record['uuid'], record['title'], 'failure']) + + return output.getvalue() + + def verify_date_order(self, run): + # Start date is <= End date + date_list = list(filter(bool, [run.start, run.end])) + if not sorted(date_list) == date_list: + raise ValueError(f"Start date is greater than end date for {run.key}") + + # Enrollment start <= enrollment end + date_list = list(filter(bool, [run.enrollment_start, run.enrollment_end])) + if not sorted(date_list) == date_list: + raise ValueError(f"Enrollment end date is greater than start date for {run.key}") diff --git a/course_discovery/apps/course_metadata/management/commands/tests/test_archive_courses.py b/course_discovery/apps/course_metadata/management/commands/tests/test_archive_courses.py new file mode 100644 index 0000000000..a3962ee545 --- /dev/null +++ b/course_discovery/apps/course_metadata/management/commands/tests/test_archive_courses.py @@ -0,0 +1,158 @@ +from datetime import timedelta +from itertools import product +from urllib.parse import urljoin + +import ddt +import pytest +import responses +from django.core.files.uploadedfile import SimpleUploadedFile +from django.core.management import CommandError, call_command +from django.test import TestCase +from django.utils import timezone + +from course_discovery.apps.api.v1.tests.test_views.mixins import OAuth2Mixin +from course_discovery.apps.course_metadata.choices import CourseRunStatus, ExternalProductStatus +from course_discovery.apps.course_metadata.models import Course, CourseRun +from course_discovery.apps.course_metadata.tests.factories import ( + AdditionalMetadataFactory, ArchiveCoursesConfigFactory, CourseFactory, CourseRunFactory +) +from course_discovery.apps.course_metadata.utils import ensure_draft_world + + +@ddt.ddt +class ArchiveCoursesCommandTests(TestCase, OAuth2Mixin): + """ + Test suite for archive_courses management command. + """ + def setUp(self): + super().setUp() + self.mock_access_token() + + end = timezone.now() + timedelta(days=5) + start = timezone.now() - timedelta(days=5) + self.course1 = CourseFactory(additional_metadata=AdditionalMetadataFactory(end_date=end)) + self.course2 = CourseFactory(additional_metadata=AdditionalMetadataFactory(end_date=end)) + self.courserun1 = CourseRunFactory(course=self.course1, end=end, enrollment_start=start) + self.courserun2 = CourseRunFactory(course=self.course2, end=end, enrollment_start=start) + self.courserun1_studio_url = urljoin( + self.course1.partner.studio_url, + f"api/v1/course_runs/{self.courserun1.key}/" + ) + self.courserun2_studio_url = urljoin( + self.course2.partner.studio_url, + f"api/v1/course_runs/{self.courserun2.key}/" + ) + + ensure_draft_world(Course.objects.get(pk=self.course1.pk)) + ensure_draft_world(Course.objects.get(pk=self.course2.pk)) + + self.csv_file_content = f"Uuids\n{self.course1.uuid}" + self.csv_file = SimpleUploadedFile( + name='test.csv', + content=self.csv_file_content.encode('utf-8'), + content_type='text/csv' + ) + ArchiveCoursesConfigFactory.create(csv_file=self.csv_file, enabled=True) + + def prepare_cmd_args(self, from_db, mangle_title, mangle_end_date): + args = [] + args.extend(['--from-db'] if from_db else ['--type', f'{self.course1.type.slug}']) + if mangle_title: + args.append('--mangle-title') + if mangle_end_date: + args.append('--mangle-end-date') + return args + + def verify_archived(self, course, mangle_title, mangle_end_date): + for c in [course, course.draft_version]: + assert c.additional_metadata.product_status == ExternalProductStatus.Archived + assert c.additional_metadata.end_date < timezone.now() + timedelta(minutes=1) + assert c.course_runs.last().status == CourseRunStatus.Unpublished + + is_title_mangled = c.title.startswith('DELETED') + assert is_title_mangled if mangle_title else not is_title_mangled + + is_end_date_mangled = c.course_runs.first().end < timezone.now() + timedelta(minutes=1) + assert is_end_date_mangled if mangle_end_date else not is_end_date_mangled + + def verify_not_archived(self, course): + for c in [course, course.draft_version]: + assert c.additional_metadata.product_status == ExternalProductStatus.Published + assert c.additional_metadata.end_date > timezone.now() + timedelta(minutes=1) + assert c.course_runs.last().status == CourseRunStatus.Published + + assert not c.title.startswith('DELETED') + assert not c.course_runs.first().end < timezone.now() + timedelta(minutes=1) + + @ddt.data( + *list(product([0, 1], repeat=3)) + ) + @ddt.unpack + @responses.activate + def test_success(self, from_db, mangle_title, mangle_end_date): + # Some sanity checks on counts + for model in [Course, CourseRun]: + assert model.objects.count() == 2 + assert model.everything.count() == 4 + + if from_db: + ArchiveCoursesConfigFactory.create( + csv_file=self.csv_file, + enabled=True, + mangle_end_date=mangle_end_date, + mangle_title=mangle_title + ) + + responses.add(responses.PATCH, self.courserun1_studio_url, status=200) + responses.add(responses.PATCH, self.courserun2_studio_url, status=200) + + if not from_db: + args = self.prepare_cmd_args(from_db, mangle_title, mangle_end_date) + else: + args = ['--from-db'] + call_command('archive_courses', *args) + + self.course1.refresh_from_db() + self.course2.refresh_from_db() + self.verify_archived(self.course1, mangle_title, mangle_end_date) + self.verify_not_archived(self.course2) + + if mangle_end_date: + assert responses.assert_call_count(self.courserun1_studio_url, 2) is True + else: + assert responses.assert_call_count(self.courserun1_studio_url, 0) is True + + @ddt.data( + True, + False + ) + @responses.activate + def test_failures(self, is_course1_archived): + "Test for the case when some courses are archived successfully and some are not" + responses.add(responses.PATCH, self.courserun1_studio_url, status=200 if is_course1_archived else 500) + responses.add(responses.PATCH, self.courserun2_studio_url, status=500 if is_course1_archived else 200) + self.csv_file_content = f"Uuids\n{self.course1.uuid}\n{self.course2.uuid}" + self.csv_file = SimpleUploadedFile( + name='test.csv', + content=self.csv_file_content.encode('utf-8'), + content_type='text/csv' + ) + ArchiveCoursesConfigFactory.create( + csv_file=self.csv_file, + enabled=True, + mangle_end_date=True, + mangle_title=True + ) + + call_command('archive_courses', '--from-db') + + self.course1.refresh_from_db() + self.course2.refresh_from_db() + archived_course = self.course1 if is_course1_archived else self.course2 + not_archived_course = self.course2 if is_course1_archived else self.course1 + self.verify_archived(archived_course, True, True) + self.verify_not_archived(not_archived_course) + + def test_raises_error_if_not_enough_arguments(self): + with pytest.raises(CommandError): + call_command("archive_courses") diff --git a/course_discovery/apps/course_metadata/migrations/0346_archivecoursesconfig.py b/course_discovery/apps/course_metadata/migrations/0346_archivecoursesconfig.py new file mode 100644 index 0000000000..bb45366939 --- /dev/null +++ b/course_discovery/apps/course_metadata/migrations/0346_archivecoursesconfig.py @@ -0,0 +1,33 @@ +# Generated by Django 4.2.17 on 2025-01-01 10:10 + +from django.conf import settings +import django.core.validators +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ('course_metadata', '0345_courserun_translation_languages_and_more'), + ] + + operations = [ + migrations.CreateModel( + name='ArchiveCoursesConfig', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('change_date', models.DateTimeField(auto_now_add=True, verbose_name='Change date')), + ('enabled', models.BooleanField(default=False, verbose_name='Enabled')), + ('csv_file', models.FileField(help_text='A csv file containing uuid of the courses to be archived', upload_to='', validators=[django.core.validators.FileExtensionValidator(allowed_extensions=['csv'])])), + ('mangle_end_date', models.BooleanField(default=False)), + ('mangle_title', models.BooleanField(default=False)), + ('changed_by', models.ForeignKey(editable=False, null=True, on_delete=django.db.models.deletion.PROTECT, to=settings.AUTH_USER_MODEL, verbose_name='Changed by')), + ], + options={ + 'ordering': ('-change_date',), + 'abstract': False, + }, + ), + ] diff --git a/course_discovery/apps/course_metadata/models.py b/course_discovery/apps/course_metadata/models.py index 6c00d55ab4..67bfc69b10 100644 --- a/course_discovery/apps/course_metadata/models.py +++ b/course_discovery/apps/course_metadata/models.py @@ -4442,6 +4442,20 @@ class BulkUploadTagsConfig(ConfigurationModel): ) +class ArchiveCoursesConfig(ConfigurationModel): + """ + Configuration to store a csv file for the archive_courses command + """ + # Timeout set to 0 so that the model does not read from cached config in case the config entry is deleted. + cache_timeout = 0 + csv_file = models.FileField( + validators=[FileExtensionValidator(allowed_extensions=['csv'])], + help_text=_("A csv file containing uuid of the courses to be archived") + ) + mangle_end_date = models.BooleanField(default=False) + mangle_title = models.BooleanField(default=False) + + class GeotargetingDataLoaderConfiguration(ConfigurationModel): """ Configuration to store a csv file that will be used in import_geotargeting_data. diff --git a/course_discovery/apps/course_metadata/templates/course_metadata/email/course_archival.html b/course_discovery/apps/course_metadata/templates/course_metadata/email/course_archival.html new file mode 100644 index 0000000000..aa97415791 --- /dev/null +++ b/course_discovery/apps/course_metadata/templates/course_metadata/email/course_archival.html @@ -0,0 +1,42 @@ +{% extends "course_metadata/email/email_base.html" %} +{% load django_markup %} +{% block body %} + +

+ The course archival process has completed. A summary is presented below. +

+
+ + + + + + + + + + + + + + + + +
+ Course Archival Summary +
Total Data Rows{{ total_count }}
Successfully Archived{{ success_count }}
Failures{{ failure_count }}
+
+ + +{% if failure_count > 0 %} +
+

Archival Failures

+ +
+{% endif %} + +{% endblock body %} diff --git a/course_discovery/apps/course_metadata/tests/factories.py b/course_discovery/apps/course_metadata/tests/factories.py index 76badebcaa..bfb6a08e41 100644 --- a/course_discovery/apps/course_metadata/tests/factories.py +++ b/course_discovery/apps/course_metadata/tests/factories.py @@ -936,6 +936,11 @@ class Meta: model = BulkUploadTagsConfig +class ArchiveCoursesConfigFactory(factory.django.DjangoModelFactory): + class Meta: + model = ArchiveCoursesConfig + + class GeotargetingDataLoaderConfigurationFactory(factory.django.DjangoModelFactory): class Meta: model = GeotargetingDataLoaderConfiguration diff --git a/course_discovery/settings/base.py b/course_discovery/settings/base.py index 05eff58574..08d842df06 100644 --- a/course_discovery/settings/base.py +++ b/course_discovery/settings/base.py @@ -799,3 +799,4 @@ RETIRED_RUN_TYPES = [] RETIRED_COURSE_TYPES = [] +COURSE_ARCHIVAL_MAIL_RECIPIENTS = ['phoenix@edx.org'] From 9f62270c7582e0890b34743433f0ad32c677f76e Mon Sep 17 00:00:00 2001 From: Muhammad Afaq Shuaib Date: Tue, 7 Jan 2025 12:39:43 +0500 Subject: [PATCH 05/11] Revert "temp: disable price update for published courses (#4520)" (#4530) This reverts commit de6f25b8e61028a2213e77ad7418798cd924af59. --- course_discovery/apps/api/v1/views/courses.py | 53 ++++++++----------- .../apps/course_metadata/models.py | 12 ++--- .../apps/course_metadata/toggles.py | 12 ----- 3 files changed, 25 insertions(+), 52 deletions(-) diff --git a/course_discovery/apps/api/v1/views/courses.py b/course_discovery/apps/api/v1/views/courses.py index 3a41979fc8..79b77ffff8 100644 --- a/course_discovery/apps/api/v1/views/courses.py +++ b/course_discovery/apps/api/v1/views/courses.py @@ -34,7 +34,6 @@ Collaborator, Course, CourseEditor, CourseEntitlement, CourseRun, CourseType, CourseUrlSlug, Organization, Program, Seat, Source, Video ) -from course_discovery.apps.course_metadata.toggles import IS_DISABLE_PRICE_UPDATES_FOR_PUBLISHED_RUNS from course_discovery.apps.course_metadata.utils import ( create_missing_entitlement, ensure_draft_world, validate_course_number, validate_slug_format ) @@ -344,36 +343,28 @@ def update_course(self, data, partial=False): # pylint: disable=too-many-statem self.log_request_subjects_and_prices(data, course) # First, update course entitlements - is_price_update_disabled = IS_DISABLE_PRICE_UPDATES_FOR_PUBLISHED_RUNS.is_enabled() - has_no_published_runs = not any( - course_run.status == CourseRunStatus.Published for course_run in course.active_course_runs - ) - if ( - course.is_external_course or not is_price_update_disabled or - (is_price_update_disabled and has_no_published_runs) - ): - if data.get('type') or data.get('prices'): - entitlements = [] - prices = data.get('prices', {}) - course_type = CourseType.objects.get(uuid=data.get('type')) if data.get('type') else course.type - entitlement_types = course_type.entitlement_types.all() - for entitlement_type in entitlement_types: - price = prices.get(entitlement_type.slug) - if price is None: - continue - entitlement, did_change = self.update_entitlement(course, entitlement_type, price, partial=partial) - entitlements.append(entitlement) - changed = changed or did_change - # Deleting entitlements here since they would be orphaned otherwise. - # One example of how this situation can happen is if a course team is switching between - # "Verified and Audit" and "Audit Only" before actually publishing their course run. - course.entitlements.exclude(mode__in=entitlement_types).delete() - course.entitlements.set(entitlements) - - # If entitlement has changed, get updated course object from DB that has new value for - # data modified timestamp. - if changed: - course.refresh_from_db() + if data.get('type') or data.get('prices'): + entitlements = [] + prices = data.get('prices', {}) + course_type = CourseType.objects.get(uuid=data.get('type')) if data.get('type') else course.type + entitlement_types = course_type.entitlement_types.all() + for entitlement_type in entitlement_types: + price = prices.get(entitlement_type.slug) + if price is None: + continue + entitlement, did_change = self.update_entitlement(course, entitlement_type, price, partial=partial) + entitlements.append(entitlement) + changed = changed or did_change + # Deleting entitlements here since they would be orphaned otherwise. + # One example of how this situation can happen is if a course team is switching between + # "Verified and Audit" and "Audit Only" before actually publishing their course run. + course.entitlements.exclude(mode__in=entitlement_types).delete() + course.entitlements.set(entitlements) + + # If entitlement has changed, get updated course object from DB that has new value for + # data modified timestamp. + if changed: + course.refresh_from_db() # Save video if a new video source is provided, also allow removing the video from course if video_data: diff --git a/course_discovery/apps/course_metadata/models.py b/course_discovery/apps/course_metadata/models.py index 67bfc69b10..b46a04da17 100644 --- a/course_discovery/apps/course_metadata/models.py +++ b/course_discovery/apps/course_metadata/models.py @@ -55,8 +55,8 @@ ) from course_discovery.apps.course_metadata.query import CourseQuerySet, CourseRunQuerySet, ProgramQuerySet from course_discovery.apps.course_metadata.toggles import ( - IS_DISABLE_PRICE_UPDATES_FOR_PUBLISHED_RUNS, IS_SUBDIRECTORY_SLUG_FORMAT_ENABLED, - IS_SUBDIRECTORY_SLUG_FORMAT_FOR_BOOTCAMP_ENABLED, IS_SUBDIRECTORY_SLUG_FORMAT_FOR_EXEC_ED_ENABLED + IS_SUBDIRECTORY_SLUG_FORMAT_ENABLED, IS_SUBDIRECTORY_SLUG_FORMAT_FOR_BOOTCAMP_ENABLED, + IS_SUBDIRECTORY_SLUG_FORMAT_FOR_EXEC_ED_ENABLED ) from course_discovery.apps.course_metadata.utils import ( UploadToFieldNamePath, clean_query, clear_slug_request_cache_for_course, custom_render_variations, @@ -2681,17 +2681,11 @@ def get_seat_upgrade_deadline(self, seat_type): return deadline def update_or_create_seat_helper(self, seat_type, prices, upgrade_deadline_override): - is_price_update_disabled = IS_DISABLE_PRICE_UPDATES_FOR_PUBLISHED_RUNS.is_enabled() defaults = { 'upgrade_deadline': self.get_seat_upgrade_deadline(seat_type), } if seat_type.slug in prices: - if ( - self.course.is_external_course or not is_price_update_disabled or ( - self.status != CourseRunStatus.Published and is_price_update_disabled - ) - ): - defaults['price'] = prices[seat_type.slug] + defaults['price'] = prices[seat_type.slug] if upgrade_deadline_override and seat_type.slug == Seat.VERIFIED: defaults['upgrade_deadline_override'] = upgrade_deadline_override diff --git a/course_discovery/apps/course_metadata/toggles.py b/course_discovery/apps/course_metadata/toggles.py index 09b5d64f8b..74d1ce8276 100644 --- a/course_discovery/apps/course_metadata/toggles.py +++ b/course_discovery/apps/course_metadata/toggles.py @@ -78,15 +78,3 @@ IS_COURSE_RUN_VARIANT_ID_ECOMMERCE_CONSUMABLE = WaffleSwitch( 'course_metadata.is_course_run_variant_id_ecommerce_consumable', __name__ ) -# .. toggle_name: course_metadata.is_disable_price_updates_for_published_runs -# .. toggle_implementation: WaffleSwitch -# .. toggle_default: False -# .. toggle_description: Temporary toggle to disable price updates for published course runs. -# .. toggle_use_cases: open_edx -# .. toggle_type: temporary -# .. toggle_creation_date: 2024-12-20 -# .. toggle_target_removal_date: 2025-01-05 -# .. toggle_tickets: PROD-4264 -IS_DISABLE_PRICE_UPDATES_FOR_PUBLISHED_RUNS = WaffleSwitch( - 'course_metadata.is_disable_price_updates_for_published_runs', __name__ -) From e80692fa5c02fea1ac226f24f92e320d2ecdd46f Mon Sep 17 00:00:00 2001 From: Syed Muhammad Dawoud Sheraz Ali <40599381+DawoudSheraz@users.noreply.github.com> Date: Tue, 7 Jan 2025 13:59:56 +0500 Subject: [PATCH 06/11] perf: fix some performance issues with program admin by converting fields to search async (#4534) --- course_discovery/apps/course_metadata/admin.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/course_discovery/apps/course_metadata/admin.py b/course_discovery/apps/course_metadata/admin.py index 8e40506356..20b668605f 100644 --- a/course_discovery/apps/course_metadata/admin.py +++ b/course_discovery/apps/course_metadata/admin.py @@ -388,6 +388,10 @@ class ProgramAdmin(DjangoObjectActions, SimpleHistoryAdmin): 'enterprise_subscription_inclusion', 'ofac_comment', 'data_modified_timestamp' ) raw_id_fields = ('video',) + autocomplete_fields = ( + 'corporate_endorsements', 'faq', 'individual_endorsements', 'job_outlook_items', + 'expected_learning_items', + ) search_fields = ('uuid', 'title', 'marketing_slug') exclude = ('card_image_url',) @@ -566,11 +570,14 @@ class SeatTypeAdmin(admin.ModelAdmin): @admin.register(Endorsement) class EndorsementAdmin(admin.ModelAdmin): list_display = ('endorser',) + search_fields = ('quote', 'endorser__given_name', 'endorser__family_name') + list_select_related = ['endorser', ] @admin.register(CorporateEndorsement) class CorporateEndorsementAdmin(admin.ModelAdmin): list_display = ('corporation_name',) + search_fields = ('corporation_name', 'statement',) @admin.register(FAQ) From 3bae84c9f61fabe313bd4b40b748de720531c448 Mon Sep 17 00:00:00 2001 From: Muhammad Afaq Shuaib Date: Tue, 7 Jan 2025 14:55:45 +0500 Subject: [PATCH 07/11] refactor: csv loader code implementation (#4529) --- .../data_loaders/csv_loader.py | 387 ++++++++++++------ 1 file changed, 259 insertions(+), 128 deletions(-) diff --git a/course_discovery/apps/course_metadata/data_loaders/csv_loader.py b/course_discovery/apps/course_metadata/data_loaders/csv_loader.py index 89feee5078..f240b9646d 100644 --- a/course_discovery/apps/course_metadata/data_loaders/csv_loader.py +++ b/course_discovery/apps/course_metadata/data_loaders/csv_loader.py @@ -4,6 +4,7 @@ """ import csv import logging +from functools import cache import unicodecsv from django.conf import settings @@ -75,8 +76,17 @@ def __init__( * product_source: slug of the external source that actually owns the product. """ super().__init__(partner, api_url, max_workers, is_threadsafe) - self.error_logs = {} - self.ingestion_summary = { + self.error_logs = {key: [] for key in CSV_LOADER_ERROR_LOG_SEQUENCE} + self.ingestion_summary = self._initialize_ingestion_summary() + self.course_uuids = {} # to show the discovery course ids for each processed course + self.product_type = product_type + self.product_source = self._get_product_source(product_source) + self.reader = self._initialize_csv_reader(csv_path, csv_file, use_gspread_client) + self.ingestion_summary['total_products_count'] = len(self.reader) + + def _initialize_ingestion_summary(self): + """Initialize the ingestion summary dictionary.""" + return { 'total_products_count': 0, 'success_count': 0, 'failure_count': 0, @@ -84,36 +94,35 @@ def __init__( 'created_products': [], 'archived_products': [] } - self.course_uuids = {} # to show the discovery course ids for each processed course - self.product_type = product_type + + def _get_product_source(self, product_source): + """ + Retrieve the product source or raise an exception if product source doesn't exist already + """ try: - self.product_source = Source.objects.get(slug=product_source) + return Source.objects.get(slug=product_source) except Source.DoesNotExist: - logger.exception(f"Unable to locate source with slug {product_source}") + logger.exception(f"Unable to locate source with slug '{product_source}'") raise - for error_log_key in CSV_LOADER_ERROR_LOG_SEQUENCE: - self.error_logs.setdefault(error_log_key, []) - + def _initialize_csv_reader(self, csv_path, csv_file, use_gspread_client): + """ + Initialize the CSV reader based on the input source (csv_path, csv_file or gspread_client) + """ try: if use_gspread_client: - # TODO: add unit tests - product_type_config = settings.PRODUCT_METADATA_MAPPING[product_type][self.product_source.slug] + product_type_config = settings.PRODUCT_METADATA_MAPPING[self.product_type][self.product_source.slug] gspread_client = GspreadClient() - self.reader = gspread_client.read_data(product_type_config) + return list(gspread_client.read_data(product_type_config)) else: - # Read file from the path if given. Otherwise, read from the file - # received from CSVDataLoaderConfiguration. - self.reader = csv.DictReader(open(csv_path, 'r')) if csv_path \ - else list(unicodecsv.DictReader(csv_file)) # lint-amnesty, pylint: disable=consider-using-with + # read the file from the provided path; otherwise, use the file received from CSVDataLoaderConfiguration + return list(csv.DictReader(open(csv_path, 'r'))) if csv_path else list(unicodecsv.DictReader(csv_file)) except FileNotFoundError: - logger.exception("Error opening csv file at path {}".format(csv_path)) # lint-amnesty, pylint: disable=logging-format-interpolation - raise # re-raising exception to avoid moving the code flow - except Exception: - logger.exception("Error reading the input data source") - raise # re-raising exception to avoid moving the code flow - self.reader = list(self.reader) - self.ingestion_summary['total_products_count'] = len(self.reader) + logger.exception(f"Error opening CSV file at path: {csv_path}") + raise + except Exception as e: + logger.exception(f"Error reading input data source: {e}") + raise def ingest(self): # pylint: disable=too-many-statements logger.info("Initiating CSV data loader flow.") @@ -128,41 +137,9 @@ def ingest(self): # pylint: disable=too-many-statements if 'external_identifier' in row: course_external_identifiers.add(row['external_identifier']) - logger.info('Starting data import flow for {}'.format(course_title)) # lint-amnesty, pylint: disable=logging-format-interpolation - if not Organization.objects.filter(key=org_key).exists(): - error_message = CSVIngestionErrorMessages.MISSING_ORGANIZATION.format( - org_key=org_key, - course_title=course_title, - ) - logger.error(error_message) - self._register_ingestion_error(CSVIngestionErrors.MISSING_ORGANIZATION, error_message) - continue - - try: - course_type = CourseType.objects.get(name=row['course_enrollment_track']) - course_run_type = CourseRunType.objects.get(name=row['course_run_enrollment_track']) - except CourseType.DoesNotExist: - error_message = CSVIngestionErrorMessages.MISSING_COURSE_TYPE.format( - course_title=course_title, course_type=row['course_enrollment_track'] - ) - logger.exception(error_message) - self._register_ingestion_error(CSVIngestionErrors.MISSING_COURSE_TYPE, error_message) - continue - except CourseRunType.DoesNotExist: - error_message = CSVIngestionErrorMessages.MISSING_COURSE_RUN_TYPE.format( - course_title=course_title, course_run_type=row['course_run_enrollment_track'] - ) - logger.exception(error_message) - self._register_ingestion_error(CSVIngestionErrors.MISSING_COURSE_RUN_TYPE, error_message) - continue - - missing_fields = self.validate_course_data(course_type, row) - if missing_fields: - error_message = CSVIngestionErrorMessages.MISSING_REQUIRED_DATA.format( - course_title=course_title, missing_data=missing_fields - ) - logger.error(error_message) - self._register_ingestion_error(CSVIngestionErrors.MISSING_REQUIRED_DATA, error_message) + logger.info(f'Starting data import flow for {course_title}') + is_valid, course_type, course_run_type = self._validate_and_process_row(row, course_title, org_key) + if not is_valid: continue course_key = self.get_course_key(org_key, row['number']) @@ -170,16 +147,12 @@ def ingest(self): # pylint: disable=too-many-statements is_course_already_ingested = bool(course) and str(course.uuid) in self.course_uuids is_course_created = False is_course_run_created = False - course_run_restriction = ( - None - if row.get('restriction_type', None) == 'None' - else row.get('restriction_type', None) - ) + course_run_restriction = self._get_course_run_restriction(row) is_future_variant = row.get('is_future_variant') == 'True' if course: try: - logger.info("Course {} is located in the database.".format(course_key)) # lint-amnesty, pylint: disable=logging-format-interpolation + logger.info(f"Course {course_key} is located in the database.") course_run, is_course_run_created = self._get_or_create_course_run( row, course, course_type, course_run_type.uuid ) @@ -187,19 +160,20 @@ def ingest(self): # pylint: disable=too-many-statements logger.exception(exc) continue else: - logger.info("Course key {} could not be found in database, creating the course.".format(course_key)) # lint-amnesty, pylint: disable=logging-format-interpolation + logger.info(f"Course key {course_key} could not be found in database, creating the course.") try: _ = self._create_course(row, course_type, course_run_type.uuid) except Exception as exc: # pylint: disable=broad-except exception_message = exc if hasattr(exc, 'response'): exception_message = exc.response.content.decode('utf-8') - error_message = CSVIngestionErrorMessages.COURSE_CREATE_ERROR.format( - course_title=course_title, - exception_message=exception_message + self._log_ingestion_error( + CSVIngestionErrors.COURSE_CREATE_ERROR, + CSVIngestionErrorMessages.COURSE_CREATE_ERROR.format( + course_title=course_title, + exception_message=exception_message + ) ) - logger.exception(error_message) - self._register_ingestion_error(CSVIngestionErrors.COURSE_CREATE_ERROR, error_message) continue course = Course.everything.select_related('type').get(key=course_key, partner=self.partner) @@ -216,9 +190,10 @@ def ingest(self): # pylint: disable=too-many-statements row['image'], headers=self.REQUEST_USER_AGENT_HEADERS) if not is_downloaded: - error_message = CSVIngestionErrorMessages.IMAGE_DOWNLOAD_FAILURE.format(course_title=course_title) - logger.error(error_message) - self._register_ingestion_error(CSVIngestionErrors.IMAGE_DOWNLOAD_FAILURE, error_message) + self._log_ingestion_error( + CSVIngestionErrors.IMAGE_DOWNLOAD_FAILURE, + CSVIngestionErrorMessages.IMAGE_DOWNLOAD_FAILURE.format(course_title=course_title) + ) continue if not is_course_created: self.add_product_source(course) @@ -229,12 +204,12 @@ def ingest(self): # pylint: disable=too-many-statements exception_message = exc if hasattr(exc, 'response'): exception_message = exc.response.content.decode('utf-8') - error_message = CSVIngestionErrorMessages.COURSE_UPDATE_ERROR.format( - course_title=course_title, - exception_message=exception_message + self._log_ingestion_error( + CSVIngestionErrors.COURSE_UPDATE_ERROR, + CSVIngestionErrorMessages.COURSE_UPDATE_ERROR.format( + course_title=course_title, exception_message=exception_message + ) ) - logger.exception(error_message) - self._register_ingestion_error(CSVIngestionErrors.COURSE_UPDATE_ERROR, error_message) continue if row.get('organization_logo_override'): @@ -246,11 +221,12 @@ def ingest(self): # pylint: disable=too-many-statements headers=self.REQUEST_USER_AGENT_HEADERS ) if not is_logo_downloaded: - error_message = CSVIngestionErrorMessages.LOGO_IMAGE_DOWNLOAD_FAILURE.format( - course_title=course_title + self._log_ingestion_error( + CSVIngestionErrors.LOGO_IMAGE_DOWNLOAD_FAILURE, + CSVIngestionErrorMessages.LOGO_IMAGE_DOWNLOAD_FAILURE.format( + course_title=course_title + ) ) - logger.error(error_message) - self._register_ingestion_error(CSVIngestionErrors.LOGO_IMAGE_DOWNLOAD_FAILURE, error_message) else: try: @@ -260,12 +236,12 @@ def ingest(self): # pylint: disable=too-many-statements except Exception as exc: # pylint: disable=broad-except exception_message = exc if hasattr(exc, 'response'): - error_message = CSVIngestionErrorMessages.COURSE_ENTITLEMENT_PRICE_UPDATE_ERROR.format( - course_title=course_title, - exception_message=exception_message + self._log_ingestion_error( + CSVIngestionErrors.COURSE_UPDATE_ERROR, + CSVIngestionErrorMessages.COURSE_ENTITLEMENT_PRICE_UPDATE_ERROR.format( + course_title=course_title, exception_message=exception_message + ) ) - logger.exception(error_message) - self._register_ingestion_error(CSVIngestionErrors.COURSE_UPDATE_ERROR, error_message) continue # No need to update the course run if the run is already in the review @@ -276,12 +252,12 @@ def ingest(self): # pylint: disable=too-many-statements exception_message = exc if hasattr(exc, 'response'): exception_message = exc.response.content.decode('utf-8') - error_message = CSVIngestionErrorMessages.COURSE_RUN_UPDATE_ERROR.format( - course_title=course_title, - exception_message=exception_message + self._log_ingestion_error( + CSVIngestionErrors.COURSE_RUN_UPDATE_ERROR, + CSVIngestionErrorMessages.COURSE_RUN_UPDATE_ERROR.format( + course_title=course_title, exception_message=exception_message + ) ) - logger.exception(error_message) - self._register_ingestion_error(CSVIngestionErrors.COURSE_RUN_UPDATE_ERROR, error_message) continue course_run.refresh_from_db() @@ -295,16 +271,9 @@ def ingest(self): # pylint: disable=too-many-statements course_run.save(update_fields=['status'], send_emails=False) self._complete_run_review(row, course_run) - logger.info("Course and course run updated successfully for course key {}".format(course_key)) # lint-amnesty, pylint: disable=logging-format-interpolation + logger.info(f"Course and course run updated successfully for course key {course_key}") - self.course_uuids[str(course.uuid)] = { - "title": course_title, - "price": ( - row.get("verified_price") if row.get("restriction_type", "None") != - CourseRunRestrictionType.CustomB2BEnterprise.value else - self.course_uuids.get(str(course.uuid), {}).get("price", None) - ), - } + self.course_uuids[str(course.uuid)] = {"title": course_title, "price": self._get_course_price(row, course)} self._register_successful_ingestion( str(course.uuid), str(course_run.variant_id), is_course_created, is_course_run_created, @@ -317,6 +286,179 @@ def ingest(self): # pylint: disable=too-many-statements self._render_error_logs() self._render_course_uuids() + self.clear_caches() + + def _get_course_price(self, row, course): + """ + Determine the price of the course based on the row data. + + Args: + row (dict): The data row containing course details. + course: The course instance. + + Returns: + float | None: The course price or None if unavailable. + """ + if row.get("restriction_type", "None") != CourseRunRestrictionType.CustomB2BEnterprise.value: + return row.get("verified_price") + return self.course_uuids.get(str(course.uuid), {}).get("price", None) + + def _get_course_run_restriction(self, row): + return None if row.get('restriction_type', None) == 'None' else row.get('restriction_type', None) + + @staticmethod + @cache + def _validate_organization(org_key): + """ + Helper method to validate the organization key + + Args: + org_key (str): Organization key + + Returns: + bool: True if the organization exists, False otherwise + """ + return Organization.objects.filter(key=org_key).exists() + + def validate_organization(self, org_key, course_title): + """ + Wrapper method to validate the organization key and log an error if the organization does not exist. + + Args: + org_key (str): Organization key + course_title (str): Course title + + Returns: + bool: True if the organization exists, False otherwise + """ + if not self._validate_organization(org_key): + self._log_ingestion_error( + CSVIngestionErrors.MISSING_ORGANIZATION, + CSVIngestionErrorMessages.MISSING_ORGANIZATION.format( + course_title=course_title, org_key=org_key + ) + ) + return False + return True + + @staticmethod + @cache + def get_course_type(course_type_name): + """ + Retrieve a CourseType object, using a cache to avoid redundant queries. + + Args: + course_type_name (str): Course type name + + Returns: + CourseType: CourseType object + """ + try: + return CourseType.objects.get(name=course_type_name) + except CourseType.DoesNotExist: + return None + + @staticmethod + @cache + def get_course_run_type(course_run_type_name): + """ + Retrieve a CourseRunType object, using a cache to avoid redundant queries. + + Args: + course_run_type_name (str): Course run type name + """ + try: + return CourseRunType.objects.get(name=course_run_type_name) + except CourseRunType.DoesNotExist: + return None + + def _validate_and_process_row(self, row, course_title, org_key): + """ + Validate the row data and process the row if it is valid. + + Args: + row (dict): course data row + course_title (str): Course title + org_key (str): Organization key + + Returns: + bool: True if the row is valid, False otherwise + CourseType: CourseType object + CourseRunType: CourseRunType object + """ + if not self.validate_organization(org_key, course_title): + return False, None, None + + def validate_course_and_course_run_types(row, course_title): + """ + Helper method to validate course and course run types. + + Args: + row (dict): Course data row + course_title (str): Course title + + Returns: + bool: True if course and course run types are valid, False otherwise + CourseType: CourseType object + CourseRunType: CourseRunType object + """ + course_type = self.get_course_type(row["course_enrollment_track"]) + if not course_type: + self._log_ingestion_error( + CSVIngestionErrors.MISSING_COURSE_TYPE, + CSVIngestionErrorMessages.MISSING_COURSE_TYPE.format( + course_title=course_title, course_type=row["course_enrollment_track"] + ), + ) + return False, None, None + + course_run_type = self.get_course_run_type(row["course_run_enrollment_track"]) + if not course_run_type: + self._log_ingestion_error( + CSVIngestionErrors.MISSING_COURSE_RUN_TYPE, + CSVIngestionErrorMessages.MISSING_COURSE_RUN_TYPE.format( + course_title=course_title, course_run_type=row["course_run_enrollment_track"] + ), + ) + return False, None, None + + return True, course_type, course_run_type + + is_valid, course_type, course_run_type = validate_course_and_course_run_types(row, course_title) + if not is_valid: + return False, course_type, course_run_type + + missing_fields = self.validate_course_data(course_type, row) + if missing_fields: + self._log_ingestion_error( + CSVIngestionErrors.MISSING_REQUIRED_DATA, + CSVIngestionErrorMessages.MISSING_REQUIRED_DATA.format( + course_title=course_title, missing_data=missing_fields + ) + ) + return False, course_type, course_run_type + + return True, course_type, course_run_type + + def _log_ingestion_error(self, error_code, message): + """ + Log the error message and continue the ingestion process. + + Args: + error_code: Error code + message (str): Error message + """ + logger.error(message) + self._register_ingestion_error(error_code, message) + + @classmethod + def clear_caches(cls): + """ + Clears all LRU caches associated with the class. + """ + cls.get_course_type.cache_clear() + cls.get_course_run_type.cache_clear() + cls._validate_organization.cache_clear() def _get_or_create_course_run(self, data, course, course_type, course_run_type_uuid): """ @@ -416,8 +558,7 @@ def _render_course_uuids(self): if self.course_uuids: logger.info("Course UUIDs:") for course_uuid, course_dict in self.course_uuids.items(): - logger.info( - "{}:{}".format(course_uuid, course_dict['title'])) # lint-amnesty, pylint: disable=logging-format-interpolation + logger.info(f"{course_uuid}:{course_dict['title']}") def _register_ingestion_error(self, error_key, error_message): """ @@ -669,7 +810,7 @@ def verify_and_get_language_tags(self, language_str): and return a list of language codes. """ languages_codes_list = [] - languages_list = language_str.split(',') + languages_list = language_str.split(",") for language in languages_list: language = language.strip() language_obj = LanguageTag.objects.filter( @@ -677,9 +818,7 @@ def verify_and_get_language_tags(self, language_str): ).first() if not language_obj: raise Exception( # pylint: disable=broad-exception-raised - 'Language {} from provided string {} is either missing or an invalid ietf language'.format( - language, language_str - ) + f"Language {language} from provided string {language_str} is either missing or an invalid ietf language" # pylint: disable=line-too-long ) languages_codes_list.append(language_obj.code) return languages_codes_list @@ -709,7 +848,7 @@ def _create_course(self, data, course_type, course_run_type_uuid): request_data = self._create_course_api_request_data(data, course_type, course_run_type_uuid) response = self._call_course_api('POST', url, request_data) if response.status_code not in (200, 201): - logger.info("Course creation response: {}".format(response.content)) # lint-amnesty, pylint: disable=logging-format-interpolation + logger.info(f"Course creation response: {response.content}") return response.json() def _update_course_entitlement_price(self, data, course_uuid, course_type, is_draft=False): @@ -751,7 +890,7 @@ def _create_course_run(self, data, course, course_type, course_run_type_uuid, re request_data = self._create_course_run_api_request_data(data, course, course_type, course_run_type_uuid, rerun) response = self._call_course_api('POST', url, request_data) if response.status_code not in (200, 201): - logger.info("Course run creation response: {}".format(response.content)) # lint-amnesty, pylint: disable=logging-format-interpolation + logger.info(f"Course run creation response: {response.content}") return response.json() def _update_course(self, data, course, is_draft): @@ -764,7 +903,7 @@ def _update_course(self, data, course, is_draft): response = self._call_course_api('PATCH', url, request_data) if response.status_code not in (200, 201): - logger.info("Course update response: {}".format(response.content)) # lint-amnesty, pylint: disable=logging-format-interpolation + logger.info(f"Course update response: {response.content}") return response.json() def _update_course_run(self, data, course_run, course_type, is_draft): @@ -776,7 +915,7 @@ def _update_course_run(self, data, course_run, course_type, is_draft): request_data = self._update_course_run_request_data(data, course_run, course_type, is_draft) response = self._call_course_api('PATCH', url, request_data) if response.status_code not in (200, 201): - logger.info("Course run update response: {}".format(response.content)) # lint-amnesty, pylint: disable=logging-format-interpolation + logger.info(f"Course run update response: {response.content}") return response.json() def _complete_run_review(self, data, course_run): @@ -833,9 +972,7 @@ def get_subject_slugs(self, *subjects): sub_obj = Subject.objects.get(translations__name=subject, translations__language_code='en') subject_slugs.append(sub_obj.slug) except Subject.DoesNotExist: - logger.exception("Unable to locate subject {} in the database. Skipping subject association".format( # lint-amnesty, pylint: disable=logging-format-interpolation - subject - )) + logger.exception(f"Unable to locate subject {subject} in the database. Skipping subject association") raise return subject_slugs @@ -857,7 +994,7 @@ def process_collaborators(self, collaborators, course_key): collaborator_obj, created = Collaborator.objects.get_or_create(name=collaborator) collaborator_uuids.append(str(collaborator_obj.uuid)) if created: - logger.info("Collaborator {} created for course {}".format(collaborator, course_key)) # lint-amnesty, pylint: disable=logging-format-interpolation + logger.info(f"Collaborator {collaborator} created for course {course_key}") return collaborator_uuids def process_staff_names(self, staff_names, course_run_key): @@ -884,10 +1021,7 @@ def process_staff_names(self, staff_names, course_run_key): ) staff_uuids.append(str(person.uuid)) if created: - logger.info("Staff with name {} has been created for course run {}".format( # lint-amnesty, pylint: disable=logging-format-interpolation - staff_name, - course_run_key - )) + logger.info(f"Staff with name {staff_name} has been created for course run {course_run_key}") return staff_uuids def process_heading_blurb(self, heading, blurb): @@ -906,15 +1040,12 @@ def process_stats(self, stat1, stat1_text, stat2, stat2_text): """ Return a list of stat/fact dicts if valid input values are provided. """ - stats = [] - stat1_dict = self.process_heading_blurb(stat1, stat1_text) - stat2_dict = self.process_heading_blurb(stat2, stat2_text) - - if stat1_dict: - stats.append(stat1_dict) - if stat2_dict: - stats.append(stat2_dict) - return stats + return [ + stat for stat in [ + self.process_heading_blurb(stat1, stat1_text), + self.process_heading_blurb(stat2, stat2_text), + ] if stat + ] def process_meta_information(self, meta_title, meta_description, meta_keywords): """ From 34494e7baec63c1332a2b1405f754ac28581cced Mon Sep 17 00:00:00 2001 From: zawan-ila <87228907+zawan-ila@users.noreply.github.com> Date: Tue, 7 Jan 2025 18:13:34 +0500 Subject: [PATCH 08/11] chore: prevent creation of retired product types from admin (#4533) --- .../apps/course_metadata/forms.py | 10 ++- .../apps/course_metadata/tests/test_admin.py | 62 ++++++++++++++++++- 2 files changed, 70 insertions(+), 2 deletions(-) diff --git a/course_discovery/apps/course_metadata/forms.py b/course_discovery/apps/course_metadata/forms.py index b64c967e2e..2d4f671228 100644 --- a/course_discovery/apps/course_metadata/forms.py +++ b/course_discovery/apps/course_metadata/forms.py @@ -1,10 +1,11 @@ from django import forms +from django.conf import settings from django.core.exceptions import ValidationError from django.forms.utils import ErrorList from django.utils.translation import gettext_lazy as _ from course_discovery.apps.course_metadata.choices import ProgramStatus -from course_discovery.apps.course_metadata.models import Course, CourseRun, Pathway, Program +from course_discovery.apps.course_metadata.models import Course, CourseRun, CourseRunType, CourseType, Pathway, Program from course_discovery.apps.course_metadata.widgets import SortedModelSelect2Multiple @@ -121,6 +122,8 @@ def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) if self.fields.get('product_source'): self.fields['product_source'].required = True + if not self.instance.pk: + self.fields['type'].queryset = CourseType.objects.exclude(slug__in=settings.RETIRED_COURSE_TYPES) class CourseRunAdminForm(forms.ModelForm): @@ -151,6 +154,11 @@ class Meta: ), } + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + if not self.instance.pk: + self.fields['type'].queryset = CourseRunType.objects.exclude(slug__in=settings.RETIRED_RUN_TYPES) + class PathwayAdminForm(forms.ModelForm): class Meta: diff --git a/course_discovery/apps/course_metadata/tests/test_admin.py b/course_discovery/apps/course_metadata/tests/test_admin.py index 625f523d5e..1a106ff8dd 100644 --- a/course_discovery/apps/course_metadata/tests/test_admin.py +++ b/course_discovery/apps/course_metadata/tests/test_admin.py @@ -7,6 +7,7 @@ from django.contrib.contenttypes.models import ContentType from django.http import HttpRequest from django.test import LiveServerTestCase, TestCase +from django.test.utils import override_settings from django.urls import reverse from rest_framework.status import HTTP_200_OK from selenium import webdriver @@ -25,7 +26,9 @@ from course_discovery.apps.course_metadata.choices import ProgramStatus from course_discovery.apps.course_metadata.constants import PathwayType from course_discovery.apps.course_metadata.forms import PathwayAdminForm, ProgramAdminForm -from course_discovery.apps.course_metadata.models import Degree, Person, Position, Program, ProgramType, Source +from course_discovery.apps.course_metadata.models import ( + CourseRunType, CourseType, Degree, Person, Position, Program, ProgramType, Source +) from course_discovery.apps.course_metadata.tests import factories @@ -222,6 +225,63 @@ def test_new_program_without_courses(self): response = self.client.get(reverse('admin:course_metadata_program_change', args=(program.id,))) assert response.status_code == 200 + @ddt.data( + [{'RETIRED_COURSE_TYPES': ['audit']}, False, CourseType], + [{'RETIRED_RUN_TYPES': ['audit']}, False, CourseRunType], + [{}, True, CourseType], + [{}, True, CourseRunType] + ) + @ddt.unpack + def test_retired_product_types_not_in_options(self, custom_settings, audit_in_options, type_model): + """ Verify that new objects (courses/courseruns) can not have a retired type""" + audit_type = type_model.objects.get(slug='audit') + url_name = ( + "admin:course_metadata_course_add" + if type_model is CourseType + else "admin:course_metadata_courserun_add" + ) + url = reverse(url_name) + with override_settings(**custom_settings): + response = self.client.get(url) + assert response.status_code == 200 + + soup = BeautifulSoup(response.content) + type_options = soup.find('select', {'name': 'type'}).find_all('option') + type_option_names = map(lambda opt: opt.get_text(), type_options) + assert (audit_type.name in type_option_names) == audit_in_options + + @ddt.data( + [{'RETIRED_COURSE_TYPES': ['audit']}, CourseType], + [{'RETIRED_RUN_TYPES': ['audit']}, CourseRunType], + [{}, CourseType], + [{}, CourseRunType] + ) + @ddt.unpack + def test_retired_product_types_in_options(self, custom_settings, type_model): + """ Verify that objects associated to retired types keep showing it in the type dropdown """ + audit_type = type_model.objects.get(slug='audit') + + url_name = ( + "admin:course_metadata_course_change" + if type_model is CourseType + else "admin:course_metadata_courserun_change" + ) + product = ( + factories.CourseFactory(type=audit_type) + if type_model is CourseType + else factories.CourseRunFactory(type=audit_type) + ) + + url = reverse(url_name, args=(product.id,)) + with override_settings(**custom_settings): + response = self.client.get(url) + assert response.status_code == 200 + + soup = BeautifulSoup(response.content) + type_options = soup.find('select', {'name': 'type'}).find_all('option') + type_option_names = map(lambda opt: opt.get_text(), type_options) + assert audit_type.name in type_option_names + class ProgramAdminFunctionalTests(SiteMixin, LiveServerTestCase): """ Functional Tests for Admin page.""" From 57a0e886aee7d6fb964f5b0066e93365d4c476d4 Mon Sep 17 00:00:00 2001 From: zawan-ila <87228907+zawan-ila@users.noreply.github.com> Date: Tue, 7 Jan 2025 23:12:03 +0500 Subject: [PATCH 09/11] feat: do not index retired products in elasticsearch and algolia (#4531) --- course_discovery/apps/api/utils.py | 17 +++++++++++- .../v1/tests/test_views/test_course_runs.py | 2 +- .../api/v1/tests/test_views/test_search.py | 27 ++++++++++++++++--- .../apps/course_metadata/algolia_models.py | 3 +++ .../search_indexes/documents/course.py | 4 ++- .../search_indexes/documents/course_run.py | 3 +++ .../tests/test_algolia_models.py | 10 +++++++ 7 files changed, 60 insertions(+), 6 deletions(-) diff --git a/course_discovery/apps/api/utils.py b/course_discovery/apps/api/utils.py index 77c3c96fa3..a35986ef56 100644 --- a/course_discovery/apps/api/utils.py +++ b/course_discovery/apps/api/utils.py @@ -4,6 +4,7 @@ import math from urllib.parse import parse_qsl, urlencode, urljoin +from django.conf import settings from django.core.files.base import ContentFile from django.db.models.fields.related import ManyToManyField from django.utils.translation import gettext as _ @@ -15,7 +16,7 @@ from course_discovery.apps.core.api_client.lms import LMSAPIClient from course_discovery.apps.core.utils import serialize_datetime from course_discovery.apps.course_metadata.choices import CourseRunRestrictionType -from course_discovery.apps.course_metadata.models import CourseRun +from course_discovery.apps.course_metadata.models import CourseRun, CourseRunType, CourseType logger = logging.getLogger(__name__) @@ -412,3 +413,17 @@ def wrapper(*args, **kwargs): return result return wrapper return inner + + +@use_request_cache("retired_run_types_cache", lambda: "retired_run_types") +def get_retired_run_type_ids(): + return list( + CourseRunType.objects.filter(slug__in=settings.RETIRED_RUN_TYPES).values_list('id', flat=True) + ) + + +@use_request_cache("retired_course_types_cache", lambda: "retired_course_types") +def get_retired_course_type_ids(): + return list( + CourseType.objects.filter(slug__in=settings.RETIRED_COURSE_TYPES).values_list('id', flat=True) + ) diff --git a/course_discovery/apps/api/v1/tests/test_views/test_course_runs.py b/course_discovery/apps/api/v1/tests/test_views/test_course_runs.py index d80cd73cbd..e2f5d2bcd9 100644 --- a/course_discovery/apps/api/v1/tests/test_views/test_course_runs.py +++ b/course_discovery/apps/api/v1/tests/test_views/test_course_runs.py @@ -1307,7 +1307,7 @@ def test_list_query(self): query = 'title:Some random title' url = '{root}?q={query}'.format(root=reverse('api:v1:course_run-list'), query=query) - with self.assertNumQueries(25, threshold=3): + with self.assertNumQueries(26, threshold=3): response = self.client.get(url) actual_sorted = sorted(response.data['results'], key=lambda course_run: course_run['key']) diff --git a/course_discovery/apps/api/v1/tests/test_views/test_search.py b/course_discovery/apps/api/v1/tests/test_views/test_search.py index ad4a93ed35..e17f269fce 100644 --- a/course_discovery/apps/api/v1/tests/test_views/test_search.py +++ b/course_discovery/apps/api/v1/tests/test_views/test_search.py @@ -2,6 +2,7 @@ import json import urllib.parse import uuid +from operator import itemgetter import ddt import factory @@ -9,6 +10,7 @@ from django.core.management import call_command from django.db.models import signals from django.test import TestCase +from django.test.utils import override_settings from django.urls import reverse from rest_framework.renderers import JSONRenderer @@ -207,10 +209,10 @@ def test_availability_faceting(self): ['results', 0, 'program_types', 0], ProgramStatus.Unpublished, 5), (detailed_path, CourseRunSearchModelSerializer, - ['results', 0, 'programs', 0, 'type'], ProgramStatus.Deleted, 21), + ['results', 0, 'programs', 0, 'type'], ProgramStatus.Deleted, 22), (detailed_path, CourseRunSearchModelSerializer, - ['results', 0, 'programs', 0, 'type'], ProgramStatus.Unpublished, 22), + ['results', 0, 'programs', 0, 'type'], ProgramStatus.Unpublished, 23), ) @ddt.unpack def test_exclude_unavailable_program_types(self, path, serializer, result_location_keys, program_status, @@ -597,7 +599,7 @@ def test_results_filtered_by_default_partner(self, short_code): self.serialize_program_search(other_program), ] - @ddt.data((True, 9), (False, 9)) + @ddt.data((True, 10), (False, 10)) @ddt.unpack def test_query_count_exclude_expired_course_run(self, exclude_expired, expected_queries): """ Verify that there is no query explosion when excluding expired course runs. """ @@ -697,6 +699,25 @@ def test_results_filtered_by_exclude_expired_course_run(self, request_method): expected_course_run_keys = [course_run.key for course_run in expected_course_runs] assert set(actual_course_run_keys) == set(expected_course_run_keys) + @ddt.data("course", "courserun") + def test_retired_courses_and_runs_not_indexed(self, content_type): + """ Verify that courses and runs corresponding to retired types are not indexed. """ + + run1, run2, run3 = CourseRunFactory.create_batch(3, course__partner=self.partner) + with override_settings(RETIRED_RUN_TYPES=[run1.type.slug], RETIRED_COURSE_TYPES=[run1.course.type.slug]): + call_command('search_index', '--rebuild', '-f') + + response = self.get_response(query={"content_type": content_type}, endpoint=self.list_path) + assert response.status_code == 200 + response_data = response.json() + assert response_data['count'] == 2 + if content_type == "course": + expected = [self.serialize_course_search(run2.course), self.serialize_course_search(run3.course)] + else: + expected = [self.serialize_course_run_search(run2), self.serialize_course_run_search(run3)] + + assert sorted(response_data['results'], key=itemgetter('key')) == sorted(expected, key=itemgetter('key')) + def test_empty_query(self): """ Verify, when the query (q) parameter is empty, the endpoint behaves as if the parameter was not provided. """ diff --git a/course_discovery/apps/course_metadata/algolia_models.py b/course_discovery/apps/course_metadata/algolia_models.py index ce37f61814..200b7659a9 100644 --- a/course_discovery/apps/course_metadata/algolia_models.py +++ b/course_discovery/apps/course_metadata/algolia_models.py @@ -422,6 +422,9 @@ def should_index(self): self.product_external_status == ExternalProductStatus.Archived: return False + if self.type.slug in settings.RETIRED_COURSE_TYPES: + return False + # WS-3723, Emeritus courses should be hidden until all features finished. if self.product_source and is_excluded_product_sources_check(self.product_source.slug): return False diff --git a/course_discovery/apps/course_metadata/search_indexes/documents/course.py b/course_discovery/apps/course_metadata/search_indexes/documents/course.py index 9db69bd062..bdf774f508 100644 --- a/course_discovery/apps/course_metadata/search_indexes/documents/course.py +++ b/course_discovery/apps/course_metadata/search_indexes/documents/course.py @@ -5,6 +5,7 @@ from taxonomy.choices import ProductTypes from taxonomy.utils import get_whitelisted_serialized_skills +from course_discovery.apps.api.utils import get_retired_course_type_ids from course_discovery.apps.course_metadata.models import Course, CourseRun from course_discovery.apps.course_metadata.utils import get_product_skill_names @@ -126,8 +127,9 @@ def prepare_prerequisites(self, obj): def get_queryset(self, excluded_restriction_types=None): if excluded_restriction_types is None: excluded_restriction_types = [] + retired_type_ids = get_retired_course_type_ids() - return super().get_queryset().prefetch_related( + return super().get_queryset().exclude(type_id__in=retired_type_ids).prefetch_related( Prefetch('course_runs', queryset=CourseRun.objects.exclude( restricted_run__restriction_type__in=excluded_restriction_types ).prefetch_related( diff --git a/course_discovery/apps/course_metadata/search_indexes/documents/course_run.py b/course_discovery/apps/course_metadata/search_indexes/documents/course_run.py index 01de13b9f6..cd5f11b16b 100644 --- a/course_discovery/apps/course_metadata/search_indexes/documents/course_run.py +++ b/course_discovery/apps/course_metadata/search_indexes/documents/course_run.py @@ -4,6 +4,7 @@ from taxonomy.choices import ProductTypes from taxonomy.utils import get_whitelisted_serialized_skills +from course_discovery.apps.api.utils import get_retired_run_type_ids from course_discovery.apps.course_metadata.choices import CourseRunStatus from course_discovery.apps.course_metadata.models import CourseRun from course_discovery.apps.course_metadata.utils import get_product_skill_names @@ -148,8 +149,10 @@ def prepare_transcript_languages(self, obj): ] def get_queryset(self, excluded_restriction_types=None): # pylint: disable=unused-argument + retired_type_ids = get_retired_run_type_ids() return filter_visible_runs( super().get_queryset() + .exclude(type_id__in=retired_type_ids) .select_related('course') .select_related('course__type') .prefetch_related('seats__type') diff --git a/course_discovery/apps/course_metadata/tests/test_algolia_models.py b/course_discovery/apps/course_metadata/tests/test_algolia_models.py index b5a669c6e0..1789087ba9 100644 --- a/course_discovery/apps/course_metadata/tests/test_algolia_models.py +++ b/course_discovery/apps/course_metadata/tests/test_algolia_models.py @@ -391,6 +391,16 @@ def test_product_external_status(self, external_status, should_index): course.additional_metadata = AdditionalMetadataFactory(product_status=external_status) assert course.should_index == should_index + @override_settings(RETIRED_COURSE_TYPES=['audit']) + def test_retired_course_indexing(self): + """ + Test that retired courses are not indexed + """ + course = self.create_course_with_basic_active_course_run() + course.authoring_organizations.add(OrganizationFactory()) + course.type = CourseTypeFactory(slug='audit') + assert not course.should_index + @ddt.data( (None, True), (CourseType.BOOTCAMP_2U, True), From 8275032484e5970b64e117c38265e074f64cbb6b Mon Sep 17 00:00:00 2001 From: "renovate[bot]" <29139614+renovate[bot]@users.noreply.github.com> Date: Wed, 8 Jan 2025 21:46:43 +0500 Subject: [PATCH 10/11] chore(deps): update memcached docker tag to v1.6.34 (#4432) --- docker-compose.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docker-compose.yml b/docker-compose.yml index a8617e2a14..504c8c1ec7 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -33,7 +33,7 @@ services: - "9300:9300" memcached: - image: memcached:1.6.29 + image: memcached:1.6.34 container_name: memcached course-discovery: From 8bf269cb22d53b7d7f0af5adcbc467a27d6ec7fc Mon Sep 17 00:00:00 2001 From: zawan-ila <87228907+zawan-ila@users.noreply.github.com> Date: Thu, 9 Jan 2025 15:12:16 +0500 Subject: [PATCH 11/11] perf: optimize loading of django admin program pages (#4537) --- course_discovery/apps/course_metadata/admin.py | 15 ++++++++++++--- .../apps/course_metadata/tests/test_widgets.py | 6 +++--- course_discovery/apps/course_metadata/widgets.py | 2 +- 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/course_discovery/apps/course_metadata/admin.py b/course_discovery/apps/course_metadata/admin.py index 20b668605f..8c7da6330f 100644 --- a/course_discovery/apps/course_metadata/admin.py +++ b/course_discovery/apps/course_metadata/admin.py @@ -7,8 +7,9 @@ from django.db.utils import IntegrityError from django.forms import CheckboxSelectMultiple, ModelForm from django.http import HttpResponseRedirect +from django.templatetags.static import static from django.urls import re_path, reverse -from django.utils.html import format_html +from django.utils.html import format_html, html_safe from django.utils.translation import gettext_lazy as _ from django_object_actions import DjangoObjectActions from parler.admin import TranslatableAdmin @@ -53,6 +54,13 @@ class Meta: } +@html_safe +class SortableSelectJSPath: + def __str__(self): + abs_path = static('js/sortable_select.js') + return f'' + + class ProgramEligibilityFilter(admin.SimpleListFilter): title = _('eligible for one-click purchase') parameter_name = 'eligible_for_one_click_purchase' @@ -125,6 +133,7 @@ class ProductValueAdmin(admin.ModelAdmin): list_display = [ 'id', 'per_click_usa', 'per_click_international', 'per_lead_usa', 'per_lead_international' ] + search_fields = ('id',) @admin.register(Course) @@ -390,7 +399,7 @@ class ProgramAdmin(DjangoObjectActions, SimpleHistoryAdmin): raw_id_fields = ('video',) autocomplete_fields = ( 'corporate_endorsements', 'faq', 'individual_endorsements', 'job_outlook_items', - 'expected_learning_items', + 'expected_learning_items', 'in_year_value' ) search_fields = ('uuid', 'title', 'marketing_slug') exclude = ('card_image_url',) @@ -523,7 +532,7 @@ class Media: js = ( 'bower_components/jquery-ui/ui/minified/jquery-ui.min.js', 'bower_components/jquery/dist/jquery.min.js', - 'js/sortable_select.js' + SortableSelectJSPath() ) diff --git a/course_discovery/apps/course_metadata/tests/test_widgets.py b/course_discovery/apps/course_metadata/tests/test_widgets.py index 6f121be99e..e1b7497206 100644 --- a/course_discovery/apps/course_metadata/tests/test_widgets.py +++ b/course_discovery/apps/course_metadata/tests/test_widgets.py @@ -7,9 +7,9 @@ @ddt.ddt class SortedModelSelect2MultipleTests(TestCase): @ddt.data( - (['1', '2'], [1, 2, 3]), - (['2', '1'], [2, 1, 3]), - (['3'], [3, 1, 2]), + (['1', '2'], [1, 2]), + (['2', '1'], [2, 1]), + (['3'], [3,]), ) @ddt.unpack def test_optgroups_are_sorted(self, value, result_order): diff --git a/course_discovery/apps/course_metadata/widgets.py b/course_discovery/apps/course_metadata/widgets.py index 8d789d47fb..38635ee4eb 100644 --- a/course_discovery/apps/course_metadata/widgets.py +++ b/course_discovery/apps/course_metadata/widgets.py @@ -29,4 +29,4 @@ def optgroups(self, name, value, attrs=None): ordered.append(item) break - return ordered + unselected + return ordered