fix: Correct an issue where cert available date was not sent to Crede…#28524
Merged
fix: Correct an issue where cert available date was not sent to Crede…#28524
Conversation
tuchfarber
approved these changes
Aug 23, 2021
Contributor
There was a problem hiding this comment.
We just do from xmodule.data import CertificatesDisplayBehaviors elsewhere
staubina
force-pushed
the
cert_available_date_sync_issue
branch
from
c6a87a0 to
5358a8b
Compare
|
Your PR has finished running tests. There were no failures. |
Contributor
|
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production. |
Contributor
|
EdX Release Notice: This PR has been deployed to the production environment. |
oliviaruizknott
added a commit
that referenced
this pull request
May 17, 2022
**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](#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`. 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](#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](#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](#26841) 2. [feat: Pass date in cert date update signal #26991](#26991) 3. [Fix certificate available date sync #28275](#28275) 4. [fix: Correct an issue where cert available date was not sent to Crede… #28524](#28524) MICROBA-1818
oliviaruizknott
added a commit
that referenced
this pull request
May 18, 2022
**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](#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](#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](#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](#26841) 2. [feat: Pass date in cert date update signal #26991](#26991) 3. [Fix certificate available date sync #28275](#28275) 4. [fix: Correct an issue where cert available date was not sent to Crede… #28524](#28524) MICROBA-1818
oliviaruizknott
added a commit
that referenced
this pull request
May 26, 2022
**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](#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](#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](#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](#26841) 2. [feat: Pass date in cert date update signal #26991](#26991) 3. [Fix certificate available date sync #28275](#28275) 4. [fix: Correct an issue where cert available date was not sent to Crede… #28524](#28524) MICROBA-1818
jawad-khan
pushed a commit
that referenced
this pull request
Jun 14, 2022
**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](#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](#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](#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](#26841) 2. [feat: Pass date in cert date update signal #26991](#26991) 3. [Fix certificate available date sync #28275](#28275) 4. [fix: Correct an issue where cert available date was not sent to Crede… #28524](#28524) MICROBA-1818
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Correct an issue where cert available date was not sent to Credentials