From 9c73c484a27880e5b969a832191569ac82c117a6 Mon Sep 17 00:00:00 2001 From: Alexander J Sheehan Date: Fri, 7 Jul 2023 16:19:32 +0000 Subject: [PATCH] fix: catching orphaned records without catalog uuids --- enterprise/constants.py | 3 + .../exporters/content_metadata.py | 37 ++++- .../integrated_channel/models.py | 6 +- .../transmitters/content_metadata.py | 2 +- .../exporters/content_metadata.py | 4 + .../test_exporters/test_content_metadata.py | 127 +++++++++++++++++- tests/test_management.py | 9 ++ 7 files changed, 177 insertions(+), 11 deletions(-) diff --git a/enterprise/constants.py b/enterprise/constants.py index 9415fbd1c9..586a271aa3 100644 --- a/enterprise/constants.py +++ b/enterprise/constants.py @@ -204,6 +204,9 @@ def json_serialized_course_modes(): 503: 'The server is temporarily unavailable.', } +IC_CREATE_ACTION = 'create' +IC_UPDATE_ACTION = 'update' +IC_DELETE_ACTION = 'delete' class FulfillmentTypes: LICENSE = 'license' diff --git a/integrated_channels/integrated_channel/exporters/content_metadata.py b/integrated_channels/integrated_channel/exporters/content_metadata.py index d156c74caa..7e6ff8c0d3 100644 --- a/integrated_channels/integrated_channel/exporters/content_metadata.py +++ b/integrated_channels/integrated_channel/exporters/content_metadata.py @@ -14,7 +14,12 @@ from django.db.models import Q from enterprise.api_client.enterprise_catalog import EnterpriseCatalogApiClient -from enterprise.constants import EXEC_ED_CONTENT_DESCRIPTION_TAG, EXEC_ED_COURSE_TYPE, TRANSMISSION_MARK_CREATE +from enterprise.constants import ( + EXEC_ED_CONTENT_DESCRIPTION_TAG, + EXEC_ED_COURSE_TYPE, + IC_DELETE_ACTION, + TRANSMISSION_MARK_CREATE, +) from enterprise.utils import get_content_metadata_item_id from integrated_channels.integrated_channel.exporters import Exporter from integrated_channels.utils import generate_formatted_log, truncate_item_dicts @@ -533,8 +538,23 @@ def export(self, **kwargs): existing_record.channel_metadata = transformed_item update_payload[key] = existing_record - for key, item in items_to_delete.items(): - delete_payload[key] = item + if items_to_delete: + delete_metadata_items = { get_content_metadata_item_id(item): item for item in self.enterprise_catalog_api.get_content_metadata( + self.enterprise_customer, + [enterprise_customer_catalog], + list(items_to_delete.keys()), + )} + for key, item in items_to_delete.items(): + delete_exec_ed_metadata = self._transform_exec_ed_content( + delete_metadata_items[item.content_id] + ) + transformed_delete_metadata = self._transform_item( + delete_exec_ed_metadata, + action=IC_DELETE_ACTION, + ) + item.channel_metadata = transformed_delete_metadata + item.save() + delete_payload[key] = item # If we're not at the max payload count, we can check for orphaned content and shove it in the delete payload current_payload_count = len(create_payload) + len(update_payload) + len(delete_payload) @@ -586,7 +606,7 @@ def _transform_exec_ed_content(self, content): content['full_description'] = EXEC_ED_CONTENT_DESCRIPTION_TAG + description return content - def _transform_item(self, content_metadata_item): + def _transform_item(self, content_metadata_item, action=None): """ Transform the provided content metadata item to the schema expected by the integrated channel. """ @@ -634,8 +654,17 @@ def _transform_item(self, content_metadata_item): continue transformed_item[integrated_channel_schema_key] = transformed_value + if action == IC_DELETE_ACTION: + self._transform_item_for_delete(transformed_item) + return transformed_item + def _transform_item_for_delete(self, content_metadata_item): + """ + Transform the provided content metadata item to the schema expected by the integrated channel. + """ + return content_metadata_item + def update_content_transmissions_catalog_uuids(self): """ Retrieve all content under the enterprise customer's catalog(s) and update all past transmission audits to have diff --git a/integrated_channels/integrated_channel/models.py b/integrated_channels/integrated_channel/models.py index 753ed24ec8..37e4cf833e 100644 --- a/integrated_channels/integrated_channel/models.py +++ b/integrated_channels/integrated_channel/models.py @@ -251,12 +251,16 @@ def fetch_orphaned_content_audits(self): self.enterprise_customer.enterprise_customer_catalogs.all() customer_catalog_uuids = enterprise_customer_catalogs.values_list('uuid', flat=True) + + non_existent_catalogs_filter = Q(enterprise_customer_catalog_uuid__in=customer_catalog_uuids) + null_catalogs_filter = Q(enterprise_customer_catalog_uuid__isnull=True) + return ContentMetadataItemTransmission.objects.filter( integrated_channel_code=self.channel_code(), enterprise_customer=self.enterprise_customer, remote_deleted_at__isnull=True, remote_created_at__isnull=False, - ).exclude(enterprise_customer_catalog_uuid__in=customer_catalog_uuids) + ).filter(~non_existent_catalogs_filter | null_catalogs_filter) def update_content_synced_at(self, action_happened_at, was_successful): """ diff --git a/integrated_channels/integrated_channel/transmitters/content_metadata.py b/integrated_channels/integrated_channel/transmitters/content_metadata.py index 6f3bca3701..49bb9a5d01 100644 --- a/integrated_channels/integrated_channel/transmitters/content_metadata.py +++ b/integrated_channels/integrated_channel/transmitters/content_metadata.py @@ -136,7 +136,7 @@ def _transmit_action(self, content_metadata_item_map, client_method, action_name self.enterprise_configuration.channel_code() ) - # If we're deleting, fetch all orphaned, uneresolved content transmissions + # If we're deleting, fetch all orphaned, unresolved content transmissions is_delete_action = action_name == 'delete' if is_delete_action: OrphanedContentTransmissions = apps.get_model( diff --git a/integrated_channels/sap_success_factors/exporters/content_metadata.py b/integrated_channels/sap_success_factors/exporters/content_metadata.py index 6c1b8e9e75..4796a68e44 100644 --- a/integrated_channels/sap_success_factors/exporters/content_metadata.py +++ b/integrated_channels/sap_success_factors/exporters/content_metadata.py @@ -46,6 +46,10 @@ class SapSuccessFactorsContentMetadataExporter(ContentMetadataExporter): 'price': 'price', } + def _transform_item_for_delete(self, content_metadata_item): + content_metadata_item['status'] = 'INACTIVE' + return content_metadata_item + def transform_provider_id(self, content_metadata_item): # pylint: disable=unused-argument """ Return the provider ID from the integrated channel configuration. diff --git a/tests/test_integrated_channels/test_integrated_channel/test_exporters/test_content_metadata.py b/tests/test_integrated_channels/test_integrated_channel/test_exporters/test_content_metadata.py index 74aa398c89..418a275b2e 100644 --- a/tests/test_integrated_channels/test_integrated_channel/test_exporters/test_content_metadata.py +++ b/tests/test_integrated_channels/test_integrated_channel/test_exporters/test_content_metadata.py @@ -16,6 +16,7 @@ from enterprise.constants import EXEC_ED_COURSE_TYPE from enterprise.utils import get_content_metadata_item_id from integrated_channels.integrated_channel.exporters.content_metadata import ContentMetadataExporter +from integrated_channels.sap_success_factors.exporters.content_metadata import SapSuccessFactorsContentMetadataExporter from integrated_channels.integrated_channel.models import ContentMetadataItemTransmission from test_utils import FAKE_UUIDS, factories from test_utils.factories import ContentMetadataItemTransmissionFactory @@ -58,11 +59,123 @@ def setUp(self): } super().setUp() + @mock.patch('enterprise.api_client.enterprise_catalog.EnterpriseCatalogApiClient.get_content_metadata') @mock.patch('enterprise.api_client.enterprise_catalog.EnterpriseCatalogApiClient.get_catalog_diff') - def test_exporter_get_catalog_diff_works_with_orphaned_content( - self, + def test_exporter_transforms_metadata_of_items_to_be_deleted( + self, mock_get_catalog_diff, + mock_get_content_metadata, ): + """ + Test that the exporter properly transforms the metadata of items that are to be deleted. + """ + sap_config = factories.SAPSuccessFactorsEnterpriseCustomerConfiguration( + enterprise_customer=self.enterprise_customer_catalog.enterprise_customer, + ) + + # Existing audit metadata, needs to be updated/transformed. + # According to the SAP channel, upon deletion we need to set each content metadata blob's status to `INACTIVE`. + # Additionally, according to the mocked catalog metadata, the transformed mappings of the record need ot be + # updated. We will assert that both of these transformations are applied by the exporter. + channel_metadata = { + "courseID": "NYIF+BOPC.4x", + "providerID": "EDX", + # This status should be transformed to `INACTIVE` by the exporter + "status": "ACTIVE", + "title": [{ + "locale": "English", + "value": "Brokerage Operations Professional Certificate Examination" + }], + "content": [ + { + "providerID": "EDX", + "contentTitle": "Brokerage Operations Professional Certificate Examination", + "contentID": "NYIF+BOPC.4x", + "launchType": 3, + "mobileEnabled": True, + } + ], + "schedule": [ + { + "startDate": "", + "endDate": "", + "active": False, + "duration": "0 days" + } + ], + "price": [ + { + "currency": "USD", + "value": 0.0 + } + ] + } + content_id = "NYIF+BOPC.4x" + mock_metadata = { + "aggregation_key": "course:NYIF+BOPC.4x", + "content_type": "course", + "full_description": "ayylmao", + "key": content_id, + "title": "Brokerage Operations Professional Certificate Examination", + "course_runs": [ + { + "key": "course-v1:NYIF+BOPC.4x+3T2017", + "uuid": "69b38e2f-e041-4634-b537-fc8d8e12c87e", + "title": "Brokerage Operations Professional Certificate Examination", + "short_description": "ayylmao", + "availability": "Archived", + "pacing_type": "self_paced", + "seats": [ + { + "type": "professional", + "price": "500.00", + "currency": "USD", + "upgrade_deadline": None, + "upgrade_deadline_override": None, + "credit_provider": None, + "credit_hours": None, + "sku": "0C1BF31", + "bulk_sku": "668527E" + } + ], + "start": "2017-09-07T00:00:00Z", + # A none value here will cause the sanitization to remove the schedule blob + "end": None, + } + ], + "uuid": "bbbf059e-b9fb-4ad7-a53e-4c6f85f47f4e", + "end_date": "2019-09-07T00:00:00Z", + "course_ends": "Future", + "entitlements": [], + "modified": "2023-07-10T04:29:38.934204Z", + "additional_information": None, + "course_run_keys": [ + "course-v1:NYIF+BOPC.4x+3T2017", + "course-v1:NYIF+BOPC.4x+1T2017" + ], + "enrollment_url": "https://foobar.com" + } + # Create the transmission audit with the out of date channel metadata + transmission_audit = factories.ContentMetadataItemTransmissionFactory( + content_id=content_id, + enterprise_customer=self.config.enterprise_customer, + plugin_configuration_id=sap_config.id, + integrated_channel_code=sap_config.channel_code(), + channel_metadata=channel_metadata, + remote_created_at=datetime.datetime.utcnow(), + enterprise_customer_catalog_uuid=self.enterprise_customer_catalog.uuid, + ) + # Mock the catalog service to return the metadata of the content to be deleted + mock_get_content_metadata.return_value = [mock_metadata] + # Mock the catalog service to return the content to be deleted in the delete payload + mock_get_catalog_diff.return_value = ([], [{'content_key': transmission_audit.content_id}], []) + exporter = SapSuccessFactorsContentMetadataExporter('fake-user', sap_config) + _, _, delete_payload = exporter.export() + assert delete_payload[transmission_audit.content_id].channel_metadata.get('schedule') == [] + assert delete_payload[transmission_audit.content_id].channel_metadata.get('status') == 'INACTIVE' + + @mock.patch('enterprise.api_client.enterprise_catalog.EnterpriseCatalogApiClient.get_catalog_diff') + def test_exporter_get_catalog_diff_works_with_orphaned_content(self, mock_get_catalog_diff): """ Test that the exporter _get_catalog_diff function properly marks orphaned content that is requested to be created by another, linked catalog as resolved. @@ -244,13 +357,12 @@ def test_content_exporter_delete_export(self, mock_get_catalog_diff, mock_get_co mock_delete_items = [{'content_key': FAKE_COURSE_RUN['key']}] mock_matched_items = [] mock_get_catalog_diff.return_value = mock_create_items, mock_delete_items, mock_matched_items + mock_get_content_metadata.return_value = [FAKE_COURSE_RUN] exporter = ContentMetadataExporter('fake-user', self.config) create_payload, update_payload, delete_payload = exporter.export() assert not create_payload assert not update_payload assert delete_payload.get(FAKE_COURSE_RUN['key']) == past_transmission - # Sanity check - mock_get_content_metadata.assert_not_called() @mock.patch('enterprise.api_client.enterprise_catalog.EnterpriseCatalogApiClient.get_content_metadata') @mock.patch('enterprise.api_client.enterprise_catalog.EnterpriseCatalogApiClient.get_catalog_diff') @@ -329,6 +441,9 @@ def test_content_exporter_truncation_bug_export(self, mock_api_client): [FAKE_COURSE_RUN['key']] ) + mock_api_client.return_value.get_catalog_diff.reset_mock() + mock_api_client.return_value.get_content_metadata.reset_mock() + past_transmission_to_update.content_last_changed = datetime.datetime.now() past_transmission_to_update.save() @@ -353,7 +468,9 @@ def test_content_exporter_truncation_bug_export(self, mock_api_client): assert not update_payload assert not create_payload assert delete_payload.get(FAKE_COURSE_RUN2['key']) == past_transmission_to_delete - assert mock_api_client.return_value.get_catalog_diff.call_count == 2 + # get_catalog_diff is called once per customer catalog + assert mock_api_client.return_value.get_catalog_diff.call_count == 1 + # get_content_metadata is called once for the single item to delete assert mock_api_client.return_value.get_content_metadata.call_count == 1 @mock.patch('integrated_channels.integrated_channel.exporters.content_metadata.EnterpriseCatalogApiClient') diff --git a/tests/test_management.py b/tests/test_management.py index ec6ad6b365..9e30713324 100644 --- a/tests/test_management.py +++ b/tests/test_management.py @@ -1932,6 +1932,15 @@ def test_normal_run(self): orphaned_content = OrphanedContentTransmissions.objects.first() assert orphaned_content.content_id == self.orphaned_content.content_id + @override_settings(ALLOW_ORPHANED_CONTENT_REMOVAL=True) + def test_orphaned_content_without_catalog_uuids(self): + self.orphaned_content.enterprise_customer_catalog_uuid = None + self.orphaned_content.save() + assert not OrphanedContentTransmissions.objects.all() + call_command('mark_orphaned_content_metadata_audits') + num_orphaned_content = OrphanedContentTransmissions.objects.count() + assert num_orphaned_content == 1 + @mark.django_db @ddt.ddt