From a9fc13e06cc8c6427e7701d0f0dfca2641a2e137 Mon Sep 17 00:00:00 2001 From: Muhammad Afaq Shuaib Date: Mon, 29 Apr 2024 18:03:57 +0500 Subject: [PATCH] feat: add Loader updates for custom presentations and support multiple varaints change (#4326) --- .../data_loaders/csv_loader.py | 26 ++++- .../data_loaders/tests/test_csv_loader.py | 12 ++- .../commands/import_course_metadata.py | 1 + .../populate_executive_education_data_csv.py | 50 +++++++--- ...t_populate_executive_education_data_csv.py | 94 ++++++++++++++++++- .../email/loader_ingestion.html | 67 +++++++++++-- .../email/loader_ingestion.txt | 2 +- .../apps/course_metadata/tests/test_emails.py | 21 +++-- 8 files changed, 232 insertions(+), 41 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 8e86d0dd2d..f45fa60e59 100644 --- a/course_discovery/apps/course_metadata/data_loaders/csv_loader.py +++ b/course_discovery/apps/course_metadata/data_loaders/csv_loader.py @@ -169,6 +169,7 @@ def ingest(self): # pylint: disable=too-many-statements course = Course.objects.filter_drafts(key=course_key, partner=self.partner).first() is_course_created = False is_course_run_created = False + course_run_restriction = None if course: try: @@ -199,6 +200,11 @@ def ingest(self): # pylint: disable=too-many-statements course_run = CourseRun.everything.filter(course=course).first() is_course_created = True is_course_run_created = True + course_run_restriction = ( + None + if row.get('restriction_type', None) == 'None' + else row.get('restriction_type', None) + ) is_downloaded = download_and_save_course_image( course, @@ -271,8 +277,8 @@ def ingest(self): # pylint: disable=too-many-statements logger.info("Course and course run updated successfully for course key {}".format(course_key)) # lint-amnesty, pylint: disable=logging-format-interpolation self.course_uuids[str(course.uuid)] = course_title self._register_successful_ingestion( - str(course.uuid), is_course_created, is_course_run_created, course.active_url_slug, - row.get('external_course_marketing_type', None)) + str(course.uuid), str(course_run.variant_id), is_course_created, is_course_run_created, + course_run_restriction, course.active_url_slug, row.get('external_course_marketing_type', None)) self._archive_stale_products(course_external_identifiers) logger.info("CSV loader ingest pipeline has completed.") @@ -305,8 +311,11 @@ def _get_or_create_course_run(self, data, course, course_type, course_run_type_u start_datetime = self.get_formatted_datetime_string(f"{data['start_date']} {data['start_time']}") end_datetime = self.get_formatted_datetime_string(f'{data["end_date"]} {data["end_time"]}') + # Added a sanity check (variant_id__isnull=True) to ensure that a wrong course run with the same schedule is not + # incorrectly updated. It is possible the runs with same schedule but different restriction types can exist. filtered_course_runs = course_runs.filter( - Q(variant_id=variant_id) | (Q(start=start_datetime) & Q(end=end_datetime)) + Q(variant_id=variant_id) | + (Q(start=start_datetime) & Q(end=end_datetime) & Q(variant_id__isnull=True)) ).order_by('created') course_run = filtered_course_runs.last() @@ -406,9 +415,11 @@ def get_ingestion_stats(self): def _register_successful_ingestion( self, course_uuid, + course_run_variant_id, is_course_created, is_course_run_created, - active_url_slug, + course_run_restriction='None', + active_url_slug='', external_course_marketing_type=None ): """ @@ -421,7 +432,9 @@ def _register_successful_ingestion( 'uuid': course_uuid, 'external_course_marketing_type': external_course_marketing_type, 'url_slug': active_url_slug, - 'rerun': is_course_run_created + 'rerun': is_course_run_created, + 'course_run_variant_id': course_run_variant_id, + 'restriction_type': course_run_restriction, } ) else: @@ -555,6 +568,7 @@ def _update_course_run_request_data(self, data, course_run, course_type, is_draf transcript_language = self.verify_and_get_language_tags(data['transcript_language']) registration_deadline = data.get('reg_close_date', '') variant_id = data.get('variant_id', '') + restriction_type = data.get('restriction_type', None) update_course_run_data = { 'run_type': str(course_run.type.uuid), @@ -583,6 +597,8 @@ def _update_course_run_request_data(self, data, course_run, course_type, is_draf )}) if variant_id: update_course_run_data.update({'variant_id': variant_id}) + if restriction_type and restriction_type != 'None': + update_course_run_data.update({'restriction_type': restriction_type}) return update_course_run_data def get_formatted_datetime_string(self, date_string): diff --git a/course_discovery/apps/course_metadata/data_loaders/tests/test_csv_loader.py b/course_discovery/apps/course_metadata/data_loaders/tests/test_csv_loader.py index e188ef23b8..abc066ce47 100644 --- a/course_discovery/apps/course_metadata/data_loaders/tests/test_csv_loader.py +++ b/course_discovery/apps/course_metadata/data_loaders/tests/test_csv_loader.py @@ -288,7 +288,9 @@ def test_single_valid_row(self, csv_slug, expected_slug, jwt_decode_patch): # p 'uuid': str(course.uuid), 'external_course_marketing_type': 'short_course', 'url_slug': expected_slug, - 'rerun': True + 'rerun': True, + 'course_run_variant_id': str(course.course_runs.last().variant_id), + 'restriction_type': None, }], 'archived_products_count': 0, 'archived_products': [], @@ -370,7 +372,9 @@ def test_archived_flow_published_course(self, jwt_decode_patch): # pylint: disa 'uuid': str(course.uuid), 'external_course_marketing_type': 'short_course', 'url_slug': 'csv-course', - 'rerun': True + 'rerun': True, + 'course_run_variant_id': str(course.course_runs.last().variant_id), + 'restriction_type': None, }], 'archived_products_count': 2, 'errors': loader.error_logs @@ -579,7 +583,9 @@ def test_ingest_flow_for_preexisting_published_course_with_new_run_creation(self 'external_course_marketing_type': course.additional_metadata.external_course_marketing_type, 'url_slug': course.active_url_slug, - 'rerun': True + 'rerun': True, + 'course_run_variant_id': str(course_run.variant_id), + 'restriction_type': None, }], 'archived_products_count': 0, 'archived_products': [], diff --git a/course_discovery/apps/course_metadata/management/commands/import_course_metadata.py b/course_discovery/apps/course_metadata/management/commands/import_course_metadata.py index 4fce47bd11..32eec271d9 100644 --- a/course_discovery/apps/course_metadata/management/commands/import_course_metadata.py +++ b/course_discovery/apps/course_metadata/management/commands/import_course_metadata.py @@ -93,6 +93,7 @@ def handle(self, *args, **options): for model in apps.get_app_config('course_metadata').get_models(): for signal in (post_save, post_delete): signal.disconnect(receiver=api_change_receiver, sender=model) + products_json = [] try: loader = CSVDataLoader( diff --git a/course_discovery/apps/course_metadata/management/commands/populate_executive_education_data_csv.py b/course_discovery/apps/course_metadata/management/commands/populate_executive_education_data_csv.py index 992468bdee..ea0c068bb3 100644 --- a/course_discovery/apps/course_metadata/management/commands/populate_executive_education_data_csv.py +++ b/course_discovery/apps/course_metadata/management/commands/populate_executive_education_data_csv.py @@ -16,6 +16,7 @@ from django.conf import settings from django.core.management import BaseCommand, CommandError +from course_discovery.apps.course_metadata.choices import CourseRunRestrictionType from course_discovery.apps.course_metadata.data_loaders import utils from course_discovery.apps.course_metadata.data_loaders.utils import map_external_org_code_to_internal_org_code from course_discovery.apps.course_metadata.utils import fetch_getsmarter_products @@ -40,7 +41,7 @@ class Command(BaseCommand): 'upgrade_deadline_override_date', 'upgrade_deadline_override_time', 'redirect_url', 'external_identifier', 'lead_capture_form_url', 'certificate_header', 'certificate_text', 'stat1', 'stat1_text', 'stat2', 'stat2_text', 'organic_url', 'organization_short_code_override', 'organization_logo_override', 'variant_id', - 'meta_title', 'meta_description', 'meta_keywords', 'slug', 'external_course_marketing_type' + 'meta_title', 'meta_description', 'meta_keywords', 'slug', 'external_course_marketing_type', 'restriction_type', ] # Mapping English and Spanish languages to IETF equivalent variants @@ -150,8 +151,15 @@ def handle(self, *args, **options): if getsmarter_flag: product['organization'] = map_external_org_code_to_internal_org_code( product['universityAbbreviation'], product_source) - output_dict = self.get_transformed_data(row, product) - output_writer = self.write_csv_row(output_writer, output_dict) + if product.get('variants'): + variants = product.pop('variants') + for variant in variants: + product.update({'variant': variant}) + output_dict = self.get_transformed_data(row, product) + output_writer = self.write_csv_row(output_writer, output_dict) + else: + output_dict = self.get_transformed_data(row, product) + output_writer = self.write_csv_row(output_writer, output_dict) logger.info(self.SUCCESS_MESSAGE.format(product['name'])) # lint-amnesty, pylint: disable=logging-format-interpolation logger.info("Data Transformation has completed. Warnings raised during the transformation:") @@ -301,7 +309,9 @@ def get_transformed_data(self, partially_filled_csv_dict, product_dict): return { **default_values, 'organization': product_dict.get('organization', ''), - 'organization_short_code_override': product_dict['altUniversityAbbreviation'], + 'organization_short_code_override': product_dict[ + 'altUniversityAbbreviation' + ], '2u_organization_code': product_dict['universityAbbreviation'], 'number': product_dict['abbreviation'], 'alternate_number': product_dict['altAbbreviation'], @@ -310,28 +320,40 @@ def get_transformed_data(self, partially_filled_csv_dict, product_dict): '2u_primary_subject': product_dict['subjectMatter'], 'subject_subcategory': product_dict['altSubjectMatter1'], 'syllabus': utils.format_curriculum(product_dict['curriculum']), - 'learner_testimonials': utils.format_testimonials(product_dict['testimonials']), + 'learner_testimonials': utils.format_testimonials( + product_dict['testimonials'] + ), 'frequently_asked_questions': utils.format_faqs(product_dict['faqs']), 'about_video_link': utils.format_base64_strings(product_dict['videoURL']), 'variant_id': product_dict['variant']['id'], 'end_date': product_dict['variant']['endDate'], + 'restriction_type': ( + CourseRunRestrictionType.CustomB2BEnterprise.value + if product_dict['variant'].get('websiteVisibility', None) == 'private' + else None + ), 'length': product_dict['durationWeeks'], - 'redirect_url': utils.format_base64_strings(product_dict.get('edxPlpUrl', '')), + 'redirect_url': utils.format_base64_strings( + product_dict.get('edxPlpUrl', '') + ), 'external_identifier': product_dict['id'], 'long_description': f"{product_dict['introduction']}{product_dict['isThisCourseForYou']}", - 'lead_capture_form_url': utils.format_base64_strings(product_dict['lcfURL']), + 'lead_capture_form_url': utils.format_base64_strings( + product_dict['lcfURL'] + ), 'certificate_header': product_dict['certificate'].get('headline', ''), 'certificate_text': product_dict['certificate'].get('blurb', ''), 'stat1': stats.get('stat1', ''), 'stat1_text': stats.get('stat1Blurb', ''), 'stat2': stats.get('stat2', ''), 'stat2_text': stats.get('stat2Blurb', ''), - 'organic_url': utils.format_base64_strings(product_dict.get('edxRedirectUrl', '')), + 'organic_url': utils.format_base64_strings( + product_dict.get('edxRedirectUrl', '') + ), 'meta_title': product_dict.get('metaTitle', ''), 'meta_description': product_dict.get('metaDescription', ''), 'meta_keywords': product_dict.get('metaKeywords', ''), 'slug': product_dict.get('slug', ''), - 'title': partially_filled_csv_dict.get('title') or product_dict['altName'] or product_dict['name'], '2u_title': product_dict['name'], 'edx_title': product_dict['altName'], @@ -341,8 +363,10 @@ def get_transformed_data(self, partially_filled_csv_dict, product_dict): ), 'verified_price': partially_filled_csv_dict.get('verified_price') or product_dict['variant']['finalPrice'], 'collaborators': partially_filled_csv_dict.get('collaborators', ''), - 'prerequisites': partially_filled_csv_dict.get('prerequisites', ''), - 'additional_information': partially_filled_csv_dict.get('additional_information', ''), + "prerequisites": partially_filled_csv_dict.get("prerequisites", ""), + 'additional_information': partially_filled_csv_dict.get( + 'additional_information', '' + ), 'secondary_subject': partially_filled_csv_dict.get('secondary_subject', ''), 'tertiary_subject': partially_filled_csv_dict.get('tertiary_subject', ''), 'start_date': partially_filled_csv_dict.get('start_date') or product_dict['variant']['startDate'], @@ -351,6 +375,8 @@ def get_transformed_data(self, partially_filled_csv_dict, product_dict): ) or product_dict['variant']['finalRegCloseDate'], 'minimum_effort': minimum_effort, 'maximum_effort': maximum_effort, - 'organization_logo_override': utils.format_base64_strings(product_dict['logoUrl']), + 'organization_logo_override': utils.format_base64_strings( + product_dict['logoUrl'] + ), 'external_course_marketing_type': product_dict['productType'], } diff --git a/course_discovery/apps/course_metadata/management/commands/tests/test_populate_executive_education_data_csv.py b/course_discovery/apps/course_metadata/management/commands/tests/test_populate_executive_education_data_csv.py index 3789897d0b..99aed347c6 100644 --- a/course_discovery/apps/course_metadata/management/commands/tests/test_populate_executive_education_data_csv.py +++ b/course_discovery/apps/course_metadata/management/commands/tests/test_populate_executive_education_data_csv.py @@ -1,6 +1,7 @@ """ Unit tests for populate_executive_education_data_csv management command. """ +import copy import csv import json from datetime import date @@ -106,21 +107,58 @@ class TestPopulateExecutiveEducationDataCsv(CSVLoaderMixin, TestCase): }, ]} - def mock_product_api_call(self): + variant_1 = { + "id": "00000000-0000-0000-0000-000000000000", + "course": "Test Organisations Programme 2024-01-31", + "currency": "USD", + "normalPrice": 36991.0, + "discount": 4000.0, + "finalPrice": 32991.0, + "regCloseDate": "2024-03-12", + "startDate": "2024-03-20", + "endDate": "2024-04-28", + "finalRegCloseDate": "2024-03-26", + "websiteVisibility": "private", + } + + variant_2 = { + "id": "11111111-1111-1111-1111-111111111111", + "course": "Test Organisations Programme 2024-02-06", + "currency": "USD", + "normalPrice": 36991.0, + "discount": 4000.0, + "finalPrice": 32991.0, + "regCloseDate": "2024-03-12", + "startDate": "2024-03-20", + "endDate": "2024-04-28", + "finalRegCloseDate": "2024-03-26", + "websiteVisibility": "public", + } + + SUCCESS_API_RESPONSE_V2 = copy.deepcopy(SUCCESS_API_RESPONSE) + SUCCESS_API_RESPONSE_V2['products'][0].pop('variant') + SUCCESS_API_RESPONSE_V2["products"][0].update({"variants": [variant_1, variant_2,]}) + + def mock_product_api_call(self, override_product_api_response=None): """ Mock product api with success response. """ + api_response = self.SUCCESS_API_RESPONSE + if override_product_api_response: + api_response = override_product_api_response responses.add( responses.GET, settings.PRODUCT_API_URL + '/?detail=2', - body=json.dumps(self.SUCCESS_API_RESPONSE), + body=json.dumps(api_response), status=200, ) - def mock_get_smarter_client_response(self): + def mock_get_smarter_client_response(self, override_get_smarter_client_response=None): """ Mock get_smarter_client response with success response. """ + if override_get_smarter_client_response: + return override_get_smarter_client_response return self.SUCCESS_API_RESPONSE @mock.patch('course_discovery.apps.course_metadata.utils.GetSmarterEnterpriseApiClient') @@ -140,6 +178,55 @@ def test_successful_file_data_population_with_getsmarter_flag(self, mock_get_sma reader = csv.DictReader(open(output_csv.name, 'r')) # lint-amnesty, pylint: disable=consider-using-with data_row = next(reader) self._assert_api_response(data_row) + log_capture.check_present( + ( + LOGGER_PATH, + 'INFO', + 'Data population and transformation completed for CSV row title CSV Course' + ), + ) + + @mock.patch('course_discovery.apps.course_metadata.utils.GetSmarterEnterpriseApiClient') + def test_successful_file_data_population_with_getsmarter_flag_with_multiple_variants(self, mock_get_smarter_client): + """ + Verify the successful population has data from API response if getsmarter flag is provided and + the product can have multiple variants + """ + mock_get_smarter_client.return_value.request.return_value.json.return_value = ( + self.mock_get_smarter_client_response(override_get_smarter_client_response=self.SUCCESS_API_RESPONSE_V2) + ) + with NamedTemporaryFile() as output_csv: + with LogCapture(LOGGER_PATH) as log_capture: + call_command( + 'populate_executive_education_data_csv', + '--output_csv', output_csv.name, + '--use_getsmarter_api_client', True, + ) + + output_csv.seek(0) + with open(output_csv.name, 'r') as csv_file: + reader = csv.DictReader(csv_file) + data_row = next(reader) + assert data_row['Variant Id'] == self.variant_1['id'] + assert data_row['Start Time'] == '00:00:00' + assert data_row['Start Date'] == self.variant_1['startDate'] + assert data_row['End Time'] == '00:00:00' + assert data_row['End Date'] == self.variant_1['endDate'] + assert data_row['Reg Close Date'] == self.variant_1['finalRegCloseDate'] + assert data_row['Reg Close Time'] == '00:00:00' + assert data_row['Verified Price'] == str(self.variant_1['finalPrice']) + assert data_row['Restriction Type'] == 'custom-b2b-enterprise' + + data_row = next(reader) + assert data_row['Variant Id'] == self.variant_2['id'] + assert data_row['Start Time'] == '00:00:00' + assert data_row['Start Date'] == self.variant_2['startDate'] + assert data_row['End Time'] == '00:00:00' + assert data_row['End Date'] == self.variant_2['endDate'] + assert data_row['Reg Close Date'] == self.variant_2['finalRegCloseDate'] + assert data_row['Reg Close Time'] == '00:00:00' + assert data_row['Verified Price'] == str(self.variant_2['finalPrice']) + assert data_row['Restriction Type'] == 'None' log_capture.check_present( ( @@ -201,6 +288,7 @@ def test_successful_file_data_population_with_input_csv(self): assert data_row['Learner Testimonials'] == '

" This is a good course"

-Lorem ' \ 'Ipsum (Gibberish)

' assert str(date.today().year) in data_row['Publish Date'] + assert data_row['Restriction Type'] == 'None' log_capture.check_present( ( diff --git a/course_discovery/apps/course_metadata/templates/course_metadata/email/loader_ingestion.html b/course_discovery/apps/course_metadata/templates/course_metadata/email/loader_ingestion.html index b1d360537f..bd4452a29b 100644 --- a/course_discovery/apps/course_metadata/templates/course_metadata/email/loader_ingestion.html +++ b/course_discovery/apps/course_metadata/templates/course_metadata/email/loader_ingestion.html @@ -47,21 +47,70 @@

New Products

+ {% endif %}
{% endif %} diff --git a/course_discovery/apps/course_metadata/templates/course_metadata/email/loader_ingestion.txt b/course_discovery/apps/course_metadata/templates/course_metadata/email/loader_ingestion.txt index d13f0fd9ea..023ce3348f 100644 --- a/course_discovery/apps/course_metadata/templates/course_metadata/email/loader_ingestion.txt +++ b/course_discovery/apps/course_metadata/templates/course_metadata/email/loader_ingestion.txt @@ -12,7 +12,7 @@ Archived Products: {{ archived_products_count }} {% if created_products_count > 0 %} New Products {% for new_product in created_products %} -{{ new_product.uuid }} - {{ new_product.url_slug}} {% if new_product.external_course_marketing_type %} ({{ new_product.external_course_marketing_type }}) {% endif %} {% if new_product.rerun %} A new run has been created {% endif %} +{{ new_product.uuid }} - {{ new_product.url_slug}} {% if new_product.external_course_marketing_type %} ({{ new_product.external_course_marketing_type }}) {% endif %} {% if new_product.rerun %} (rerun: True) {% endif %} {% if new_product.course_run_variant_id %} (variant: {{ new_product.course_run_variant_id }}) {% endif %} {% if new_product.restriction_type %} (restriction_type: {{ new_product.restriction_type }}) {% endif %} {% endfor %} {% endif %} {% if created_products_count > 0 %} diff --git a/course_discovery/apps/course_metadata/tests/test_emails.py b/course_discovery/apps/course_metadata/tests/test_emails.py index 304b21d8bc..6aabe4d236 100644 --- a/course_discovery/apps/course_metadata/tests/test_emails.py +++ b/course_discovery/apps/course_metadata/tests/test_emails.py @@ -557,6 +557,7 @@ def test_email_new_products(self): Verify the email content for new products. """ uuid = str(uuid4()) + variant_id = str(uuid4()) url_slug = 'course-slug-1' emails.send_ingestion_email( self.partner, self.EMAIL_SUBJECT, self.USER_EMAILS, self.EXEC_ED_PRODUCT, self.source, @@ -571,11 +572,14 @@ def test_email_new_products(self): 'external_course_marketing_type': None, 'url_slug': url_slug, 'rerun': True, + 'course_run_variant_id': variant_id, + 'restriction_type': 'None', } ], } ) + # pylint: disable=line-too-long self._assert_email_content( self.EMAIL_SUBJECT, [ @@ -584,10 +588,11 @@ def test_email_new_products(self): "New Products 1 ", "Updated Products 0 ", "

New Products

", - f"
  • {uuid} - {url_slug} " - f"A new run has been created
    • " + "Course UUIDURL SlugExternal Course Marketing TypeVariant IDRestriction TypeRerun", + f"{uuid}{url_slug}{variant_id}NoneYes", ] ) + # pylint: enable=line-too-long def test_email_new_exec_ed_products(self): """ @@ -625,6 +630,7 @@ def test_email_new_exec_ed_products(self): } ) + # pylint: disable=line-too-long self._assert_email_content( self.EMAIL_SUBJECT, [ @@ -633,14 +639,13 @@ def test_email_new_exec_ed_products(self): "New Products 3 ", "Updated Products 0 ", "

    New Products

    ", - f"
  • {uuid} - {url_slug} " - f"(sprint) A new run has been created
    • " - f"
  • {uuid} - {url_slug} " - f"(course_stack) A new run has been created
    • " - f"
  • {uuid} - {url_slug} " - f"(short_course) A new run has been created
    • " + "Course UUIDURL SlugExternal Course Marketing TypeVariant IDRestriction TypeRerun", + f"{uuid}{url_slug}sprintYes", + f"{uuid}{url_slug}course_stackYes", + f"{uuid}{url_slug}short_courseYes", ] ) + # pylint: enable=line-too-long def test_email_ingestion_failures(self): """