Merge pull request #2043 from openedx/hamza/ENT-8540-address-content-…

…transmission-failures

ENT-8540 - feat: disable failed content transmissions for 24hrs

CHANGELOG.rst

@@ -15,6 +15,10 @@ Change Log
 
 Unreleased
 ----------
+[4.13.6]
+---------
+* feat: disable failing transmissions for 24hrs
+
 [4.13.5]
 ---------
 * feat: added autocomplete for enterprise customer in EnterpriseCustomerCatalogAdmin

enterprise/__init__.py

@@ -2,4 +2,4 @@
 Your project description goes here.
 """
 
-__version__ = "4.13.5"
+__version__ = "4.13.6"

integrated_channels/integrated_channel/exporters/content_metadata.py

@@ -12,6 +12,7 @@
 from django.apps import apps
 from django.conf import settings
 from django.db.models import Q
+from django.utils import timezone
 
 from enterprise.api_client.enterprise_catalog import EnterpriseCatalogApiClient
 from enterprise.constants import (
@@ -71,6 +72,7 @@ class ContentMetadataExporter(Exporter):
     # TODO: Move this to the EnterpriseCustomerPluginConfiguration model as a JSONField.
     DATA_TRANSFORM_MAPPING = {}
     SKIP_KEY_IF_NONE = False
+    LAST_24_HRS = timezone.now() - timezone.timedelta(hours=24)
 
     def __init__(self, user, enterprise_configuration):
         """
@@ -201,6 +203,8 @@ def _check_matched_content_updated_at(
                     remote_deleted_at__isnull=True,
                     remote_created_at__isnull=False,
                 )
+                content_query.add(Q(remote_errored_at__lt=self.LAST_24_HRS) | Q(
+                    remote_errored_at__isnull=True), Q.AND)
                 # If not force_retrieve_all_catalogs, filter content records where `content last changed` is less than
                 # the matched item's `date_updated`, otherwise select the row regardless of what the updated at time is.
                 if not force_retrieve_all_catalogs:
@@ -397,13 +401,17 @@ def _check_matched_content_to_delete(self, enterprise_customer_catalog, items):
                 incomplete_transmission.mark_for_delete()
                 items_to_delete[content_id] = incomplete_transmission
             else:
-                past_content = ContentMetadataItemTransmission.objects.filter(
+                past_content_query = Q(
                     enterprise_customer=self.enterprise_configuration.enterprise_customer,
                     integrated_channel_code=self.enterprise_configuration.channel_code(),
                     enterprise_customer_catalog_uuid=enterprise_customer_catalog.uuid,
                     plugin_configuration_id=self.enterprise_configuration.id,
                     content_id=content_id
-                ).first()
+                )
+                past_content_query.add(Q(remote_errored_at__lt=self.LAST_24_HRS) | Q(
+                    remote_errored_at__isnull=True), Q.AND)
+                past_content = ContentMetadataItemTransmission.objects.filter(
+                    past_content_query).first()
                 if past_content:
                     past_content.mark_for_delete()
                     items_to_delete[content_id] = past_content

integrated_channels/integrated_channel/migrations/0034_contentmetadataitemtransmission_remote_errored_at.py

@@ -0,0 +1,18 @@
+# Generated by Django 3.2.23 on 2024-03-05 09:49
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('integrated_channel', '0033_integratedchannelapirequestlogs_channel_name'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='contentmetadataitemtransmission',
+            name='remote_errored_at',
+            field=models.DateTimeField(blank=True, help_text='Date when the content transmission was failed in the remote API.', null=True),
+        ),
+    ]

integrated_channels/integrated_channel/models.py

@@ -12,6 +12,7 @@
 from django.db import models
 from django.db.models import Q
 from django.db.models.query import QuerySet
+from django.utils import timezone
 from django.utils.translation import gettext_lazy as _
 
 from model_utils.models import TimeStampedModel
@@ -27,6 +28,7 @@
 
 LOGGER = logging.getLogger(__name__)
 User = auth.get_user_model()
+LAST_24_HRS = timezone.now() - timezone.timedelta(hours=24)
 
 
 def set_default_display_name(*args, **kw):
@@ -671,6 +673,11 @@ class Meta:
         blank=True,
         null=True
     )
+    remote_errored_at = models.DateTimeField(
+        help_text='Date when the content transmission was failed in the remote API.',
+        blank=True,
+        null=True
+    )
     remote_updated_at = models.DateTimeField(
         help_text='Date when the content transmission was last updated in the remote API',
         blank=True,
@@ -707,13 +714,16 @@ def deleted_transmissions(cls, enterprise_customer, plugin_configuration_id, int
         """
         Return any pre-existing records for this customer/plugin/content which was previously deleted
         """
-        return ContentMetadataItemTransmission.objects.filter(
+        query = Q(
             enterprise_customer=enterprise_customer,
             plugin_configuration_id=plugin_configuration_id,
             content_id=content_id,
             integrated_channel_code=integrated_channel_code,
             remote_deleted_at__isnull=False,
         )
+        query.add(Q(remote_errored_at__lt=LAST_24_HRS) |
+                  Q(remote_errored_at__isnull=True), Q.AND)
+        return ContentMetadataItemTransmission.objects.filter(query)
 
     @classmethod
     def incomplete_create_transmissions(
@@ -734,6 +744,7 @@ def incomplete_create_transmissions(
             remote_created_at__isnull=True,
             remote_updated_at__isnull=True,
             remote_deleted_at__isnull=True,
+            remote_errored_at__isnull=True,
         )
         in_db_but_failed_to_send_query = Q(
             enterprise_customer=enterprise_customer,
@@ -745,6 +756,8 @@ def incomplete_create_transmissions(
             remote_deleted_at__isnull=True,
             api_response_status_code__gte=400,
         )
+        in_db_but_failed_to_send_query.add(
+            Q(remote_errored_at__lt=LAST_24_HRS) | Q(remote_errored_at__isnull=True), Q.AND)
         in_db_but_unsent_query.add(in_db_but_failed_to_send_query, Q.OR)
         return ContentMetadataItemTransmission.objects.filter(in_db_but_unsent_query)
 
@@ -769,6 +782,8 @@ def incomplete_update_transmissions(
             remote_deleted_at__isnull=True,
             api_response_status_code__gte=400,
         )
+        in_db_but_failed_to_send_query.add(
+            Q(remote_errored_at__lt=LAST_24_HRS) | Q(remote_errored_at__isnull=True), Q.AND)
         return ContentMetadataItemTransmission.objects.filter(in_db_but_failed_to_send_query)
 
     @classmethod
@@ -791,6 +806,8 @@ def incomplete_delete_transmissions(
             remote_deleted_at__isnull=False,
             api_response_status_code__gte=400,
         )
+        in_db_but_failed_to_send_query.add(
+            Q(remote_errored_at__lt=LAST_24_HRS) | Q(remote_errored_at__isnull=True), Q.AND)
         return ContentMetadataItemTransmission.objects.filter(in_db_but_failed_to_send_query)
 
     def _mark_transmission(self, mark_for):

integrated_channels/integrated_channel/transmitters/content_metadata.py

@@ -249,9 +249,12 @@ def _transmit_action(self, content_metadata_item_map, client_method, action_name
                         transmission.remote_deleted_at = action_happened_at
                         if was_successful:
                             successfully_removed_content_keys.append(transmission.content_id)
-                    transmission.save()
                     if was_successful:
                         transmission.remove_marked_for()
+                        transmission.remote_errored_at = None
+                    else:
+                        transmission.remote_errored_at = action_happened_at
+                    transmission.save()
                     self.enterprise_configuration.update_content_synced_at(action_happened_at, was_successful)
                     results.append(transmission)
 

tests/test_integrated_channels/test_integrated_channel/test_exporters/test_content_metadata.py

@@ -12,6 +12,7 @@
 from testfixtures import LogCapture
 
 from django.test.utils import override_settings
+from django.utils import timezone
 
 from enterprise.constants import EXEC_ED_COURSE_TYPE
 from enterprise.utils import get_content_metadata_item_id
@@ -111,6 +112,7 @@ def test_exporter_transforms_metadata_of_items_to_be_deleted(
             ]
         }
         content_id = "NYIF+BOPC.4x"
+        content_id_to_skip = "CTIF+BOPC.4x"
         mock_metadata = {
             "aggregation_key": "course:NYIF+BOPC.4x",
             "content_type": "course",
@@ -164,7 +166,20 @@ def test_exporter_transforms_metadata_of_items_to_be_deleted(
             channel_metadata=channel_metadata,
             remote_created_at=datetime.datetime.utcnow(),
             enterprise_customer_catalog_uuid=self.enterprise_customer_catalog.uuid,
+            remote_errored_at=None,
         )
+        transmission_audit_to_skip = factories.ContentMetadataItemTransmissionFactory(
+            content_id=content_id_to_skip,
+            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,
+            # failed within last 24hrs
+            remote_errored_at=timezone.now() - timezone.timedelta(hours=23),
+        )
+
         # 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
@@ -174,6 +189,14 @@ def test_exporter_transforms_metadata_of_items_to_be_deleted(
         assert delete_payload[transmission_audit.content_id].channel_metadata.get('schedule') == []
         assert delete_payload[transmission_audit.content_id].channel_metadata.get('status') == 'INACTIVE'
 
+        # if transmission was attempted in last 24hrs, it shouldn't be reattempted
+        mock_get_catalog_diff.return_value = (
+            [], [{'content_key': transmission_audit_to_skip.content_id}], [])
+        exporter = SapSuccessFactorsContentMetadataExporter(
+            'fake-user', sap_config)
+        _, _, delete_payload = exporter.export()
+        assert not delete_payload
+
     @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):
         """
@@ -226,6 +249,8 @@ def test_exporter_considers_failed_updates_as_existing_content(
             remote_updated_at=datetime.datetime.utcnow(),
             content_last_changed=None,
             api_response_status_code=500,
+            # didn't failed within last 24hrs
+            remote_errored_at=timezone.now() - timezone.timedelta(hours=25),
         )
         mock_metadata = get_fake_content_metadata()[:1]
         mock_metadata[0]['key'] = test_failed_updated_content.content_id
@@ -256,6 +281,29 @@ def test_exporter_considers_failed_updates_as_existing_content(
         # The exporter should now properly include the content in the update payload.
         assert update_payload == {test_failed_updated_content.content_id: test_failed_updated_content}
 
+        # shouldn't export if it errored in last 24hrs
+        exporter = ContentMetadataExporter('fake-user', self.config)
+        content_id_to_skip = "CTIF+BOPC.4x"
+        test_failed_updated_content_to_skip = ContentMetadataItemTransmissionFactory(
+            content_id=content_id_to_skip,
+            enterprise_customer=self.enterprise_customer_catalog.enterprise_customer,
+            enterprise_customer_catalog_uuid=self.enterprise_customer_catalog.uuid,
+            integrated_channel_code=self.config.channel_code(),
+            plugin_configuration_id=self.config.id,
+            remote_created_at=datetime.datetime.utcnow(),
+            remote_updated_at=datetime.datetime.utcnow(),
+            content_last_changed=None,
+            api_response_status_code=500,
+            # failed within last 24hrs
+            remote_errored_at=timezone.now() - timezone.timedelta(hours=23),
+        )
+        mock_get_catalog_diff.return_value = (
+            [], [], [{'content_key': test_failed_updated_content_to_skip.content_id,
+                      'date_updated': datetime.datetime.now()}]
+        )
+        _, update_payload, __ = exporter.export()
+        assert not update_payload
+
     @mock.patch('enterprise.api_client.enterprise_catalog.EnterpriseCatalogApiClient.get_content_metadata')
     @mock.patch('enterprise.api_client.enterprise_catalog.EnterpriseCatalogApiClient.get_catalog_diff')
     def test_content_exporter_create_export(self, mock_get_catalog_diff, mock_get_content_metadata):
@@ -389,6 +437,7 @@ def test_content_exporter_failed_create_export(self, mock_get_catalog_diff, mock
             remote_created_at=None,
             remote_updated_at=None,
             remote_deleted_at=None,
+            remote_errored_at=None,
         )
         past_transmission.save()
         mock_get_content_metadata.return_value = get_fake_content_metadata()