From 007abaf5b10707607d47a9f9d89572b36d18b8e2 Mon Sep 17 00:00:00 2001 From: Alex Dusenbery Date: Thu, 13 Jun 2024 15:05:14 -0400 Subject: [PATCH] fix: submit the create_enterprise_enrollment task with a configurable countdown Also removes python 3.8 from CI matrix. --- .github/workflows/ci.yml | 3 +-- CHANGELOG.rst | 8 ++++++-- enterprise/__init__.py | 2 +- enterprise/admin/forms.py | 4 ++-- enterprise/rules.py | 1 - enterprise/signals.py | 18 ++++++++++++++---- .../exporters/content_metadata.py | 4 ++-- .../integrated_channel/models.py | 10 +++++----- tests/test_enterprise/test_signals.py | 9 +++++---- 9 files changed, 36 insertions(+), 23 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 21c8eb9b79..f7be57ec72 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -17,7 +17,6 @@ jobs: fail-fast: false matrix: python-version: - - '3.8' - '3.11' toxenv: [quality, docs, django42-celery53, django42-pii-annotations] env: @@ -39,7 +38,7 @@ jobs: TOXENV: ${{ matrix.toxenv }} run: tox - name: Run code coverage - if: matrix.python-version == '3.8' && matrix.toxenv == 'django42-celery53' + if: matrix.python-version == '3.11' && matrix.toxenv == 'django42-celery53' uses: codecov/codecov-action@v4 with: flags: unittests diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 3693f082eb..677b186ece 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -17,12 +17,16 @@ Unreleased ---------- * nothing unreleased +[4.20.5] +-------- +* fix: submit the ``create_enterprise_enrollment`` task with a configurable countdown + [4.20.4] ---------- +-------- * feat: Populates ``learner_portal_sidebar_content`` in ``EnterpriseCustomer`` and removes references to old fields. [4.20.3] ---------- +-------- * feat: Makes ``career_engagement_network_message`` field nullable in ``EnterpriseCustomer``. [4.20.2] diff --git a/enterprise/__init__.py b/enterprise/__init__.py index cd5f9828e3..704fdd1c15 100644 --- a/enterprise/__init__.py +++ b/enterprise/__init__.py @@ -2,4 +2,4 @@ Your project description goes here. """ -__version__ = "4.20.4" +__version__ = "4.20.5" diff --git a/enterprise/admin/forms.py b/enterprise/admin/forms.py index 6efd122a8f..8f3c39d5ec 100644 --- a/enterprise/admin/forms.py +++ b/enterprise/admin/forms.py @@ -770,8 +770,8 @@ def clean(self): # `start_date` must always come before the `expiration_date` raise ValidationError({'expiration_date': ['Expiration date should be after start date.']}) admin_notification = AdminNotification.objects.filter( - Q(start_date__range=(start_date, expiration_date)) | # pylint: disable=unsupported-binary-operation - Q(expiration_date__range=(start_date, expiration_date)) | # pylint: disable=unsupported-binary-operation + Q(start_date__range=(start_date, expiration_date)) | + Q(expiration_date__range=(start_date, expiration_date)) | Q(start_date__lt=start_date, expiration_date__gt=expiration_date) ).exclude( pk=self.instance.id diff --git a/enterprise/rules.py b/enterprise/rules.py index 2f127873c1..87fcf3c327 100644 --- a/enterprise/rules.py +++ b/enterprise/rules.py @@ -201,7 +201,6 @@ def has_explicit_access_to_reporting_api(user, obj): ) -# pylint: disable=unsupported-binary-operation rules.add_perm('enterprise.can_access_admin_dashboard', has_implicit_access_to_dashboard | has_explicit_access_to_dashboard) diff --git a/enterprise/signals.py b/enterprise/signals.py index d5649aaa5d..251671e84d 100644 --- a/enterprise/signals.py +++ b/enterprise/signals.py @@ -4,6 +4,7 @@ from logging import getLogger +from django.conf import settings from django.db import transaction from django.db.models.signals import post_delete, post_save, pre_save from django.dispatch import receiver @@ -49,6 +50,10 @@ SAPSuccessFactorsEnterpriseCustomerConfiguration, ] +# Default number of seconds to use as task countdown +# if not otherwise specified via Django settings. +DEFAULT_COUNTDOWN = 3 + @disable_for_loaddata def handle_user_post_save(sender, **kwargs): # pylint: disable=unused-argument @@ -385,6 +390,10 @@ def create_enterprise_enrollment_receiver(sender, instance, **kwargs): # pyl instance.course_id, ) + # Number of seconds to tell celery to wait before the `create_enterprise_enrollment` + # task should begin execution. + countdown = getattr(settings, 'CREATE_ENTERPRISE_ENROLLMENT_TASK_COUNTDOWN', DEFAULT_COUNTDOWN) + def submit_task(): """ In-line helper to run the create_enterprise_enrollment task on commit. @@ -393,10 +402,11 @@ def submit_task(): "User %s is an EnterpriseCustomerUser. Spinning off task to check if course is within User's " "Enterprise's EnterpriseCustomerCatalog." ), user_id) - create_enterprise_enrollment.delay( - str(instance.course_id), - ecu.id, - ) + task_args = (str(instance.course_id), ecu.id) + # Submit the task with a countdown to help avoid possible race-conditions/deadlocks + # due to external processes that read or write the same + # records the task tries to read or write. + create_enterprise_enrollment.apply_async(task_args, countdown=countdown) # This receiver might be executed within a transaction that creates an ECE record. # Ensure that the task is only submitted after a commit tasks place, because diff --git a/integrated_channels/integrated_channel/exporters/content_metadata.py b/integrated_channels/integrated_channel/exporters/content_metadata.py index 9603a5000f..fc2f4e076d 100644 --- a/integrated_channels/integrated_channel/exporters/content_metadata.py +++ b/integrated_channels/integrated_channel/exporters/content_metadata.py @@ -205,7 +205,7 @@ def _check_matched_content_updated_at( ) content_query.add( - Q(remote_errored_at__lt=self.LAST_24_HRS) | Q(remote_errored_at__isnull=True) | # pylint: disable=unsupported-binary-operation + Q(remote_errored_at__lt=self.LAST_24_HRS) | Q(remote_errored_at__isnull=True) | Q(remote_errored_at__lt=self.enterprise_customer.modified), Q.AND ) @@ -414,7 +414,7 @@ def _check_matched_content_to_delete(self, enterprise_customer_catalog, items): ) past_content_query.add( - Q(remote_errored_at__lt=self.LAST_24_HRS) | Q(remote_errored_at__isnull=True) | # pylint: disable=unsupported-binary-operation + Q(remote_errored_at__lt=self.LAST_24_HRS) | Q(remote_errored_at__isnull=True) | Q(remote_errored_at__lt=self.enterprise_customer.modified), Q.AND) past_content = ContentMetadataItemTransmission.objects.filter( past_content_query).first() diff --git a/integrated_channels/integrated_channel/models.py b/integrated_channels/integrated_channel/models.py index a8faabcae6..37e8999f5b 100644 --- a/integrated_channels/integrated_channel/models.py +++ b/integrated_channels/integrated_channel/models.py @@ -269,7 +269,7 @@ def fetch_orphaned_content_audits(self): enterprise_customer=self.enterprise_customer, remote_deleted_at__isnull=True, remote_created_at__isnull=False, - ).filter(~non_existent_catalogs_filter | null_catalogs_filter) # pylint: disable=unsupported-binary-operation + ).filter(~non_existent_catalogs_filter | null_catalogs_filter) def update_content_synced_at(self, action_happened_at, was_successful): """ @@ -723,7 +723,7 @@ def deleted_transmissions(cls, enterprise_customer, plugin_configuration_id, int remote_deleted_at__isnull=False, ) query.add( - Q(remote_errored_at__lt=LAST_24_HRS) # pylint: disable=unsupported-binary-operation + Q(remote_errored_at__lt=LAST_24_HRS) | Q(remote_errored_at__isnull=True) | Q(remote_errored_at__lt=enterprise_customer.modified), Q.AND) return ContentMetadataItemTransmission.objects.filter(query) @@ -760,7 +760,7 @@ def incomplete_create_transmissions( 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) | # pylint: disable=unsupported-binary-operation + Q(remote_errored_at__lt=LAST_24_HRS) | Q(remote_errored_at__isnull=True) | Q(remote_errored_at__lt=enterprise_customer.modified), 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) @@ -787,7 +787,7 @@ def incomplete_update_transmissions( 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) | # pylint: disable=unsupported-binary-operation + Q(remote_errored_at__lt=LAST_24_HRS) | Q(remote_errored_at__isnull=True) | Q(remote_errored_at__lt=enterprise_customer.modified), Q.AND) return ContentMetadataItemTransmission.objects.filter(in_db_but_failed_to_send_query) @@ -812,7 +812,7 @@ def incomplete_delete_transmissions( api_response_status_code__gte=400, ) in_db_but_failed_to_send_query.add( - Q(remote_errored_at__lt=LAST_24_HRS) # pylint: disable=unsupported-binary-operation + Q(remote_errored_at__lt=LAST_24_HRS) | Q(remote_errored_at__isnull=True) | Q(remote_errored_at__lt=enterprise_customer.modified), Q.AND) return ContentMetadataItemTransmission.objects.filter(in_db_but_failed_to_send_query) diff --git a/tests/test_enterprise/test_signals.py b/tests/test_enterprise/test_signals.py index fb8dca79a9..f36ebf8f1b 100644 --- a/tests/test_enterprise/test_signals.py +++ b/tests/test_enterprise/test_signals.py @@ -843,7 +843,7 @@ def setUp(self): self.non_enterprise_user = UserFactory(id=999, email='user999@example.com') super().setUp() - @mock.patch('enterprise.tasks.create_enterprise_enrollment.delay') + @mock.patch('enterprise.tasks.create_enterprise_enrollment.apply_async') def test_receiver_calls_task_if_ecu_exists(self, mock_task): """ Receiver should call a task @@ -863,11 +863,12 @@ def test_receiver_calls_task_if_ecu_exists(self, mock_task): 'created': True, } - with self.captureOnCommitCallbacks(execute=True): + with self.captureOnCommitCallbacks(execute=True), \ + override_settings(CREATE_ENTERPRISE_ENROLLMENT_TASK_COUNTDOWN=42): create_enterprise_enrollment_receiver(sender, instance, **kwargs) - mock_task.assert_called_once_with(str(instance.course_id), self.enterprise_customer_user.id) + mock_task.assert_called_once_with((str(instance.course_id), self.enterprise_customer_user.id), countdown=42) - @mock.patch('enterprise.tasks.create_enterprise_enrollment.delay') + @mock.patch('enterprise.tasks.create_enterprise_enrollment.apply_async') def test_receiver_does_not_call_task_if_ecu_not_exists(self, mock_task): """ Receiver should NOT call a task