From 74780ad4c059113d340c13ddb965e3b248e9ad2c Mon Sep 17 00:00:00 2001 From: oliviaruizknott Date: Tue, 17 May 2022 16:17:04 -0400 Subject: [PATCH] fix: send COURSE_CERT_DATE_CHANGE signal on_commit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Previously** When a course administrator changed the `certificates_display_behavior` (presumably to `end_with_date`) AND set the `certificate_available_date` in Studio, the `certificate_available_date` was not syncing to Credentials. This was because we chose to send the `certificate_available_date` only if the course is self-paced and the `certificate_display_behavior` is set to `end_with_date`. [See PR #28275](https://github.com/openedx/edx-platform/pull/28275). However, we were checking those two conditions by looking at the relevant `CourseOverview`, which was not yet truly saved to reflect the updated display behavior at the time of the check due to atomic requests. [Read more about atomic requests and transactions here](https://docs.djangoproject.com/en/4.0/topics/db/transactions/#tying-transactions-to-http-requests-1); we have `ATOMIC_REQUESTS` set to `TRUE` in our codebase. Because the `certificate_display_behavior` was not (yet) `end_with_date`, the post to Credentials was not being fired. **Solution** To fix, this commit sends the `COURSE_CERT_DATE_CHANGE` signal `on_commit` instead, which waits until the transaction has completed and the update to the `CourseOverview` has been truly applied to the database. [Read more about `on_commit` here](https://docs.djangoproject.com/en/4.0/topics/db/transactions/#django.db.transaction.on_commit). Now, when the relevant `CourseOverview` is read, it will have the updated `certificate_display_behavior`. See the [Django docs for how to test on_commit callbacks here](https://docs.djangoproject.com/en/3.2/topics/testing/tools/#django.test.TestCase.captureOnCommitCallbacks); this seems to be our first time using the built-in method. This commit also cleans up some previous code that was meant to get around the problem caused by atomic requests, that is now unneccessary with this fix. It essentially reverses the work done in [PR #26991](https://github.com/openedx/edx-platform/pull/26991): we no longer need to explicitly pass the `certificate_available_date` since we can trust the `CourseOverview` to be properly updated. **Rejected Solutions** A. Simply publish the `COURSE_CERT_DATE_CHANGE` signal `on_commit`; no other changes. Rejected because: This would fix the problem, but leaves a lot of unnecessary code and some puzzling inconsistencies. I prefer the solution above because we are cleaning up behind ourselves. B. Pass the new `certificate_display_behavior` along with the `certificate_available_date`; read those direclty instead of checking the (not-yet-properly-updated) `CourseOverview`. Rejected because: The pattern of passing the new `certificate_available_date` down through all these methods was put in place to get around the atomic requests problem. I believe `on_commit` to be a better solution to getting around that problem. I’d like to move away from passing data down through several functions / methods. C. Start the celery task `on_commit` (rather than send the signal `on_commit`). Rejected because: The signal receiver basically only starts the celery task, and I find the break to be a bit more readable when sending the signal. No need to split hairs here. D. Remove the check for pacing and display behavior; send the updated `certificate_available_date` every time there is a change, no matter what the current display behavior is. Rejected because: We intentionally added this check in [PR #28275](https://github.com/openedx/edx-platform/pull/28275) because the task was not behaving as expected without it (specifically around self-paced courses). I assume this is still necessary. **Relevant Prior Work** The following PRs--in order--show how this section (and other relevant sections) of the code have been changed over time: 1. [Move cert date signals to avoid race conditions #26841](https://github.com/openedx/edx-platform/pull/26841) 2. [feat: Pass date in cert date update signal #26991](https://github.com/openedx/edx-platform/pull/26991) 3. [Fix certificate available date sync #28275](https://github.com/openedx/edx-platform/pull/28275) 4. [fix: Correct an issue where cert available date was not sent to Crede… #28524](https://github.com/openedx/edx-platform/pull/28524) MICROBA-1818 --- lms/djangoapps/certificates/api.py | 5 +- .../content/course_overviews/signals.py | 14 ++++-- .../course_overviews/tests/test_signals.py | 12 +++-- openedx/core/djangoapps/programs/signals.py | 5 +- openedx/core/djangoapps/programs/tasks.py | 49 +++++++++---------- openedx/core/djangoapps/signals/signals.py | 2 +- 6 files changed, 43 insertions(+), 44 deletions(-) diff --git a/lms/djangoapps/certificates/api.py b/lms/djangoapps/certificates/api.py index 5c0b1b4329..48fe0f7ef1 100644 --- a/lms/djangoapps/certificates/api.py +++ b/lms/djangoapps/certificates/api.py @@ -874,17 +874,16 @@ def _course_uses_available_date(course): ) -def available_date_for_certificate(course, certificate, certificate_available_date=None): +def available_date_for_certificate(course, certificate): """ Returns the available date to use with a certificate Arguments: course (CourseOverview or course descriptor): The course we're checking certificate (GeneratedCertificate): The certificate we're getting the date for - certificate_available_date (datetime): An optional date to override the from the course overview. """ if _course_uses_available_date(course): - return certificate_available_date or course.certificate_available_date + return course.certificate_available_date return certificate.modified_date diff --git a/openedx/core/djangoapps/content/course_overviews/signals.py b/openedx/core/djangoapps/content/course_overviews/signals.py index e0d59ab6fd..d415ad360d 100644 --- a/openedx/core/djangoapps/content/course_overviews/signals.py +++ b/openedx/core/djangoapps/content/course_overviews/signals.py @@ -5,6 +5,7 @@ Signal handler for invalidating cached course overviews import logging +from django.db import transaction from django.dispatch import Signal from django.dispatch.dispatcher import receiver @@ -92,8 +93,11 @@ def _check_for_cert_availability_date_changes(previous_course_overview, updated_ f"{previous_course_overview.certificate_available_date} to " + f"{updated_course_overview.certificate_available_date}. Sending COURSE_CERT_DATE_CHANGE signal." ) - COURSE_CERT_DATE_CHANGE.send_robust( - sender=None, - course_key=updated_course_overview.id, - available_date=updated_course_overview.certificate_available_date - ) + + def _send_course_cert_date_change_signal(): + COURSE_CERT_DATE_CHANGE.send_robust( + sender=None, + course_key=updated_course_overview.id, + ) + + transaction.on_commit(_send_course_cert_date_change_signal) 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 4b486f3616..9506a584bb 100644 --- a/openedx/core/djangoapps/content/course_overviews/tests/test_signals.py +++ b/openedx/core/djangoapps/content/course_overviews/tests/test_signals.py @@ -84,14 +84,16 @@ class CourseOverviewSignalsTestCase(ModuleStoreTestCase): ) # changing display name doesn't fire the signal - course.display_name = course.display_name + 'changed' - self.store.update_item(course, ModuleStoreEnum.UserID.test) + with self.captureOnCommitCallbacks(execute=True) as callbacks: + course.display_name = course.display_name + 'changed' + self.store.update_item(course, ModuleStoreEnum.UserID.test) assert not mock_signal.called # changing the given field fires the signal - for change in changes: - setattr(course, change.field_name, change.changed_value) - self.store.update_item(course, ModuleStoreEnum.UserID.test) + with self.captureOnCommitCallbacks(execute=True) as callbacks: + for change in changes: + setattr(course, change.field_name, change.changed_value) + self.store.update_item(course, ModuleStoreEnum.UserID.test) assert mock_signal.called @patch('openedx.core.djangoapps.content.course_overviews.signals.COURSE_START_DATE_CHANGED.send') diff --git a/openedx/core/djangoapps/programs/signals.py b/openedx/core/djangoapps/programs/signals.py index a548f597a6..ec9e3a5128 100644 --- a/openedx/core/djangoapps/programs/signals.py +++ b/openedx/core/djangoapps/programs/signals.py @@ -177,7 +177,7 @@ def handle_course_cert_revoked(sender, user, course_key, mode, status, **kwargs) @receiver(COURSE_CERT_DATE_CHANGE, dispatch_uid='course_certificate_date_change_handler') -def handle_course_cert_date_change(sender, course_key, available_date, **kwargs): # lint-amnesty, pylint: disable=unused-argument +def handle_course_cert_date_change(sender, course_key, **kwargs): # lint-amnesty, pylint: disable=unused-argument """ If course is updated and the certificate_available_date is changed, schedule a celery task to update visible_date for all certificates @@ -185,7 +185,6 @@ def handle_course_cert_date_change(sender, course_key, available_date, **kwargs) Args: course_key (CourseLocator): refers to the course whose certificate_available_date was updated. - available_date (datetime): the date to update the certificate's available date to Returns: None @@ -208,4 +207,4 @@ def handle_course_cert_date_change(sender, course_key, available_date, **kwargs) # 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(str(course_key), available_date) + 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 e6d1355ee7..38f95756d4 100644 --- a/openedx/core/djangoapps/programs/tasks.py +++ b/openedx/core/djangoapps/programs/tasks.py @@ -1,7 +1,6 @@ """ This file contains celery tasks for programs-related functionality. """ -from datetime import datetime from urllib.parse import urljoin from celery import shared_task @@ -347,14 +346,16 @@ def update_credentials_course_certificate_configuration_available_date( certificate_available_date=None ): """ - This task will update the course certificate configuration's available date. This is different from the - "visable_date" attribute. This date will always either be the available date that is set in studio for - a given course, or it will be None. + This task will update the CourseCertificate configuration's available date + in Credentials. This is different from the "visible_date" attribute. This + date will always either be the available date that is set in Studio for a + given course, or it will be None. Arguments: course_run_key (str): The course run key to award the certificate for - certificate_available_date (str): A string representation of the datetime for when to make the certificate - available to the user. If not provided, it will be none. + certificate_available_date (str): A string representation of the + datetime for when to make the certificate available to the user. If + not provided, it will be None. """ LOGGER.info( f"Running task update_credentials_course_certificate_configuration_available_date for course {course_key} \ @@ -386,7 +387,7 @@ def update_credentials_course_certificate_configuration_available_date( @shared_task(bind=True, ignore_result=True) @set_code_owner_attribute -def award_course_certificate(self, username, course_run_key, certificate_available_date=None): +def award_course_certificate(self, username, course_run_key): """ This task is designed to be called whenever a student GeneratedCertificate is updated. It can be called independently for a username and a course_run, but is invoked on each GeneratedCertificate.save. @@ -397,9 +398,6 @@ def award_course_certificate(self, username, course_run_key, certificate_availab Arguments: username (str): The user to award the Credentials course cert to course_run_key (str): The course run key to award the certificate for - certificate_available_date (str): A string representation of the datetime for when to make the certificate - available to the user. If not provided, it will calculate the date. - """ def _retry_with_custom_exception(username, course_run_key, reason, countdown): exception = MaxRetriesExceededError( @@ -460,23 +458,16 @@ def award_course_certificate(self, username, course_run_key, certificate_availab f"Task award_course_certificate was called without course overview data for course {course_key}" ) return + credentials_client = get_credentials_api_client( User.objects.get(username=settings.CREDENTIALS_SERVICE_USERNAME), ) - # Date is being passed via JSON and is encoded in the EMCA date time string format. The rest of the code - # expects a datetime. - if certificate_available_date: - certificate_available_date = datetime.strptime(certificate_available_date, DATE_FORMAT) - - # Even in the cases where this task is called with a certificate_available_date, we still need to retrieve - # the course overview because it's required to determine if we should use the certificate_available_date or - # the certs modified date visible_date = available_date_for_certificate( course_overview, certificate, - certificate_available_date=certificate_available_date ) + LOGGER.info( "Task award_course_certificate will award certificate for course " f"{course_key} with a visible date of {visible_date}" @@ -713,7 +704,7 @@ def revoke_program_certificates(self, username, course_key): # lint-amnesty, py @shared_task(bind=True, ignore_result=True) @set_code_owner_attribute -def update_certificate_visible_date_on_course_update(self, course_key, certificate_available_date): +def update_certificate_visible_date_on_course_update(self, course_key): """ This task is designed to be called whenever a course is updated with certificate_available_date so that visible_date is updated on credential @@ -728,8 +719,6 @@ def update_certificate_visible_date_on_course_update(self, course_key, certifica Arguments: course_key (str): The course identifier - certificate_available_date (str): The date to update the certificate availablity date to. It's a string - representation of a datetime object because task parameters must be JSON-able. Returns: None @@ -750,9 +739,11 @@ def update_certificate_visible_date_on_course_update(self, course_key, certifica f"Failed to update certificate availability date for course {course_key}. Reason: {error_msg}" ) raise self.retry(exc=exception, countdown=countdown, max_retries=MAX_RETRIES) - # update the course certificate with the new certificate available date if: - # - The course is not self paced - # - The certificates_display_behavior is not "end_with_date" + + # Update the CourseCertificate configuration in Credentials with the new + # certificate_available_date if: + # - The course is not self paced, AND + # - The certificates_display_behavior is "end_with_date" course_overview = CourseOverview.get_from_id(course_key) if ( course_overview.self_paced is False and @@ -760,8 +751,12 @@ def update_certificate_visible_date_on_course_update(self, course_key, certifica ): update_credentials_course_certificate_configuration_available_date.delay( str(course_key), - certificate_available_date + str(course_overview.certificate_available_date) ) + + # This code will update the visible_date in Credentials; we have moved away + # from relying on visible_date in favor of the above, but this still runs + # and visible_date is still updated users_with_certificates_in_course = GeneratedCertificate.eligible_available_certificates.filter( course_id=course_key ).values_list('user__username', flat=True) @@ -771,4 +766,4 @@ def update_certificate_visible_date_on_course_update(self, course_key, certifica 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), certificate_available_date=certificate_available_date) + award_course_certificate.delay(user, str(course_key)) diff --git a/openedx/core/djangoapps/signals/signals.py b/openedx/core/djangoapps/signals/signals.py index f995fd91e8..d11caaa24d 100644 --- a/openedx/core/djangoapps/signals/signals.py +++ b/openedx/core/djangoapps/signals/signals.py @@ -16,7 +16,7 @@ COURSE_GRADE_CHANGED = Signal() COURSE_CERT_CHANGED = Signal() COURSE_CERT_AWARDED = Signal() COURSE_CERT_REVOKED = Signal() -# providing_args=["course_key", "available_date"] +# providing_args=["course_key",] COURSE_CERT_DATE_CHANGE = Signal() # providing_args=['user', 'course_id', 'subsection_id', 'subsection_grade', ]