From 6c97dfe1e5ad199adc238320e5384a2501a4b725 Mon Sep 17 00:00:00 2001 From: Matt Tuchfarber Date: Wed, 3 Mar 2021 17:32:47 -0500 Subject: [PATCH] Move cert date signals to avoid race conditions COURSE_CERT_DATE_CHANGE was being called before saving the new data in the course overview. The listeners were expecting to pull the data out of the course overview, and thus were only right about half the time. This moves the signal to trigger after the course publish signals are handled. --- .../djangoapps/content/course_overviews/signals.py | 7 +++++++ .../content/course_overviews/tests/test_signals.py | 4 ++++ openedx/core/djangoapps/courseware_api/views.py | 1 - openedx/core/djangoapps/models/course_details.py | 5 ----- openedx/core/djangoapps/programs/signals.py | 11 ++++++++--- openedx/core/djangoapps/programs/tasks.py | 5 ++++- .../core/djangoapps/programs/tests/test_signals.py | 10 +++++++--- 7 files changed, 30 insertions(+), 13 deletions(-) diff --git a/openedx/core/djangoapps/content/course_overviews/signals.py b/openedx/core/djangoapps/content/course_overviews/signals.py index 474391c4eb..65d9e617b1 100644 --- a/openedx/core/djangoapps/content/course_overviews/signals.py +++ b/openedx/core/djangoapps/content/course_overviews/signals.py @@ -8,6 +8,7 @@ import logging from django.dispatch import Signal from django.dispatch.dispatcher import receiver +from openedx.core.djangoapps.signals.signals import COURSE_CERT_DATE_CHANGE from xmodule.modulestore.django import SignalHandler from .models import CourseOverview @@ -50,6 +51,7 @@ def _check_for_course_changes(previous_course_overview, updated_course_overview) if previous_course_overview: _check_for_course_date_changes(previous_course_overview, updated_course_overview) _check_for_pacing_changes(previous_course_overview, updated_course_overview) + _check_for_cert_availability_date_changes(previous_course_overview, updated_course_overview) def _check_for_course_date_changes(previous_course_overview, updated_course_overview): @@ -83,3 +85,8 @@ def _check_for_pacing_changes(previous_course_overview, updated_course_overview) updated_course_overview=updated_course_overview, previous_self_paced=previous_course_overview.self_paced, ) + + +def _check_for_cert_availability_date_changes(previous_course_overview, updated_course_overview): + if previous_course_overview.certificate_available_date != updated_course_overview.certificate_available_date: + COURSE_CERT_DATE_CHANGE.send_robust(sender=None, course_key=updated_course_overview.id) diff --git a/openedx/core/djangoapps/content/course_overviews/tests/test_signals.py b/openedx/core/djangoapps/content/course_overviews/tests/test_signals.py index a83a85dcb0..ee1696b1b4 100644 --- a/openedx/core/djangoapps/content/course_overviews/tests/test_signals.py +++ b/openedx/core/djangoapps/content/course_overviews/tests/test_signals.py @@ -90,3 +90,7 @@ class CourseOverviewSignalsTestCase(ModuleStoreTestCase): @patch('openedx.core.djangoapps.content.course_overviews.signals.COURSE_PACING_CHANGED.send') def test_pacing_changed(self, mock_signal): self.assert_changed_signal_sent('self_paced', True, False, mock_signal) + + @patch('openedx.core.djangoapps.content.course_overviews.signals.COURSE_CERT_DATE_CHANGE.send_robust') + def test_cert_date_changed(self, mock_signal): + self.assert_changed_signal_sent('certificate_available_date', self.TODAY, self.NEXT_WEEK, mock_signal) diff --git a/openedx/core/djangoapps/courseware_api/views.py b/openedx/core/djangoapps/courseware_api/views.py index ed7fff4a30..0594840b94 100644 --- a/openedx/core/djangoapps/courseware_api/views.py +++ b/openedx/core/djangoapps/courseware_api/views.py @@ -6,7 +6,6 @@ import json from completion.exceptions import UnavailableCompletionData from completion.utilities import get_key_to_last_completed_block -from django.conf import settings from django.urls import reverse from django.utils.translation import ugettext as _ from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication diff --git a/openedx/core/djangoapps/models/course_details.py b/openedx/core/djangoapps/models/course_details.py index 1aa0713263..0d6fd3d543 100644 --- a/openedx/core/djangoapps/models/course_details.py +++ b/openedx/core/djangoapps/models/course_details.py @@ -6,11 +6,8 @@ CourseDetails import logging import re -import six from django.conf import settings -from openedx.core.djangoapps.models.config.waffle import enable_course_detail_update_certificate_date -from openedx.core.djangoapps.signals.signals import COURSE_CERT_DATE_CHANGE from openedx.core.djangolib.markup import HTML from openedx.core.lib.courses import course_image_url from xmodule.fields import Date @@ -246,8 +243,6 @@ class CourseDetails(object): if converted != descriptor.certificate_available_date: dirty = True descriptor.certificate_available_date = converted - if enable_course_detail_update_certificate_date(course_key): - COURSE_CERT_DATE_CHANGE.send_robust(sender=cls, course_key=six.text_type(course_key)) if 'course_image_name' in jsondict and jsondict['course_image_name'] != descriptor.course_image: descriptor.course_image = jsondict['course_image_name'] diff --git a/openedx/core/djangoapps/programs/signals.py b/openedx/core/djangoapps/programs/signals.py index a5f566c79b..48d804b93d 100644 --- a/openedx/core/djangoapps/programs/signals.py +++ b/openedx/core/djangoapps/programs/signals.py @@ -8,6 +8,7 @@ import logging from django.dispatch import receiver from openedx.core.djangoapps.credentials.helpers import is_learner_records_enabled_for_org +from openedx.core.djangoapps.models.config.waffle import enable_course_detail_update_certificate_date from openedx.core.djangoapps.signals.signals import ( COURSE_CERT_AWARDED, COURSE_CERT_CHANGED, @@ -82,8 +83,7 @@ def handle_course_cert_changed(sender, user, course_key, mode, status, **kwargs) Returns: None - - """ + """ # Import here instead of top of file since this module gets imported before # the credentials app is loaded, resulting in a Django deprecation warning. from openedx.core.djangoapps.credentials.models import CredentialsApiConfig @@ -196,6 +196,10 @@ def handle_course_cert_date_change(sender, course_key, **kwargs): # lint-amnest None """ + # Stop if cert date updating isn't in effect for the course + if not enable_course_detail_update_certificate_date(course_key): + return + # Import here instead of top of file since this module gets imported before # the credentials app is loaded, resulting in a Django deprecation warning. from openedx.core.djangoapps.credentials.models import CredentialsApiConfig @@ -209,6 +213,7 @@ def handle_course_cert_date_change(sender, course_key, **kwargs): # lint-amnest 'handling COURSE_CERT_DATE_CHANGE for course %s', course_key, ) + # import here, because signal is registered at startup, but items in tasks are not yet loaded from openedx.core.djangoapps.programs.tasks import update_certificate_visible_date_on_course_update - update_certificate_visible_date_on_course_update.delay(course_key) + update_certificate_visible_date_on_course_update.delay(str(course_key)) diff --git a/openedx/core/djangoapps/programs/tasks.py b/openedx/core/djangoapps/programs/tasks.py index 8183dc469a..9af43dbdde 100644 --- a/openedx/core/djangoapps/programs/tasks.py +++ b/openedx/core/djangoapps/programs/tasks.py @@ -606,7 +606,6 @@ def update_certificate_visible_date_on_course_update(self, course_key): # feature, it may indicate a condition where processing of such tasks # has been temporarily disabled. Since this is a recoverable situation, # mark this task for retry instead of failing it altogether. - if not CredentialsApiConfig.current().is_learner_issuance_enabled: error_msg = ( "Task update_certificate_visible_date_on_course_update cannot be executed when credentials issuance is " @@ -622,5 +621,9 @@ def update_certificate_visible_date_on_course_update(self, course_key): course_id=course_key ).values_list('user__username', flat=True) + LOGGER.info( + "Task update_certificate_visible_date_on_course_update resending course certificates" + f"for {len(users_with_certificates_in_course)} users in course {course_key}." + ) for user in users_with_certificates_in_course: award_course_certificate.delay(user, str(course_key)) diff --git a/openedx/core/djangoapps/programs/tests/test_signals.py b/openedx/core/djangoapps/programs/tests/test_signals.py index 07e7379269..506dedfd52 100644 --- a/openedx/core/djangoapps/programs/tests/test_signals.py +++ b/openedx/core/djangoapps/programs/tests/test_signals.py @@ -238,6 +238,7 @@ class CertRevokedReceiverTest(TestCase): new_callable=mock.PropertyMock, return_value=False, ) +@mock.patch("openedx.core.djangoapps.programs.signals.enable_course_detail_update_certificate_date") class CourseCertAvailableDateChangedReceiverTest(TestCase): """ Tests for the `handle_course_cert_date_change` signal handler function. @@ -253,7 +254,7 @@ class CourseCertAvailableDateChangedReceiverTest(TestCase): 'course_key': TEST_COURSE_KEY, } - def test_signal_received(self, mock_is_learner_issuance_enabled, mock_task): # pylint: disable=unused-argument + def test_signal_received(self, mock_enable_update, mock_is_learner_issuance_enabled, mock_task): # pylint: disable=unused-argument """ Ensures the receiver function is invoked when COURSE_CERT_DATE_CHANGE is sent. @@ -262,23 +263,26 @@ class CourseCertAvailableDateChangedReceiverTest(TestCase): to the way django signals work), we mock a configuration call that is known to take place inside the function. """ + mock_enable_update.return_value = True COURSE_CERT_DATE_CHANGE.send(**self.signal_kwargs) assert mock_is_learner_issuance_enabled.call_count == 1 - def test_programs_disabled(self, mock_is_learner_issuance_enabled, mock_task): + def test_programs_disabled(self, mock_enable_update, mock_is_learner_issuance_enabled, mock_task): """ Ensures that the receiver function does nothing when the credentials API configuration is not enabled. """ + mock_enable_update.return_value = True handle_course_cert_date_change(**self.signal_kwargs) assert mock_is_learner_issuance_enabled.call_count == 1 assert mock_task.call_count == 0 - def test_programs_enabled(self, mock_is_learner_issuance_enabled, mock_task): + def test_programs_enabled(self, mock_enable_update, mock_is_learner_issuance_enabled, mock_task): """ Ensures that the receiver function invokes the expected celery task when the credentials API configuration is enabled. """ + mock_enable_update.return_value = True mock_is_learner_issuance_enabled.return_value = True handle_course_cert_date_change(**self.signal_kwargs)