[APER-3229]
In a previous PR, I created a new utility function named `is_credentials_enabled()` that can be used to determine if use of the Credentials IDA is enabled by config in an Open edX instance.
This PR is some additional cleanup that replaces the direct import and use of the `CredentialsApiConfig` model with the new utility function.
I took some additional time to update some existing log messages to include more info while reducing our need to log PII. I've removed as much use of a learner's username as possible, replacing it with logging the learner's LMS User Id instead.
[APER-3229]
The monolith and the Credentials IDA keep independent records of a course runs certificate availability/visibility preferences. This PR aims to improve the communication between the monolith and the Credentiala IDA to keep the availability date/preference in sync with the monoliths records.
The current code is too strict and actually prevents valid updates in some configurations.
Additionally, the Credentials IDA doesn't understand the concept of "course pacing" (instructor-paced vs self-paced) and has troubles with courses with an availability date of "end". Instead of having to add the concept of course pacing (and syncing more data between the two systems), this PR proposes sending the end date of a course as the "certificate available date" to Credentials.
This way, the Credentials IDA can manage the visibility of awarded credentials in a course run with a display behavior of "end" using the existing feature set and models of the Credentials service.
* fixed one test to accommodate a slightly modified workflow
* allowed the automated linter to match current standards
Note: as part of this process, I realized a lot of the tests in `programs/tests/test_tasks.py` are somewhat problematic. Some aren't real unit tests (allow calls to something other than the system under test), at least one for that reason it won't run on local at all, and some mock the wrong part of the system or just don't match current flow. I'm not modifying them as part of this, but they should be looked at.
FIXES: APER-3146
To determine whether or not we need to revoke any program certificates, we first run get_revokable_program_uuids. This calls get_certified_programs, which calls get_credentials, which uses the OpenEdx Core utility method get_api_data. get_api_data makes the API call inside a try block, does raise_for_status also inside the try block, and then catches the exception, logs it, and returns an empty result to the caller.
This means that on a failure to call the credentials API, get_credentials can’t tell the difference between a failure to hit the API (because credentials is, as it sometimes is during a notify_programs run, overloaded), or a learner with no program certificates. In this particular case, this is absolute failure, incorrect behavior.
* Adds a new flag, `raise_on_error` which will make `get_api_data` log the exception and then re-raise the HTTPError, defaulting to false in order to avoid changing the behavior on any other callers
* Also: my editor reformatted all of the touched files to our modern code standards, and it seemed appropriate to let it do that.
* Also: added type hints in some cases, because they helped me write the code and debug. Our test suite definitely reports mypy results on type errors so we are verifying that hints are correct.
FIXES: APER-3146
From the warning: Django now detects this configuration automatically.
You can remove default_app_config.
This also removes two warnings from shell startup.
[APER-1941]
We are aware of a product issue that causes a `certificate_available_date` (CAD) to be set for self-paced courses (and thus copied to the course-run's (course) certificate configuration) that causes an issue with learners' Program Records to be inaccurate. The stored CAD in Credentials is causing these certificates to be marked as "unearned" on the Program Record in Credentials, as the IDA believes the learner should *not* have access to them yet (but these certificates are available in the LMS).
A management command was recently introduced in Studio that can be used to clean/remove the `certificate_available_date` data from a course-run in Mongo. These updates aren't making it to the Credentials IDA because of an issue with our logic in the `update_certificate_visible_date_on_course_update` Celery task. This task assumes that we only want to send updates for *Instructor-Paced* courses that have a Certificate Display Behavior set to `end_with_date`. In reality, we need updates to pass to Credentials for _some_ self-paced courses with bad data.
This PR hopes to update our infrastructure to allow these updates to flow to Credentials.
* Improve logging for failed requests to the Credentials IDA's `course_certificates` endpoint when updating a course certificate configuration.
* Update docstrings and comments where appropriate
* Split the logic of the update_certificate_visible_date_on_course_update task into two tasks. The former task will continue to _just_ handle visible_date attribute updates. The latter (new) task will be dedicated to making the REST API call that updates the `certificate_available_date` data in Credentials.
* Update the `handle_course_cert_date_change` function wqhen the COURSE_CERT_DATE_CHANGE signal is received to queue both the "visible_date" and "certificate available date" Celery tasks.
* Update existing tests for the task changes.
**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
This is a first stage for removing the LegacyWaffle* classes.
LegacyWaffleFlag usage replaced with WaffleFlag;
LegacyWaffleSwitche usage replaced with WaffleSwitch;
New CourseWaffleFlag added to the temporary module __future__ as FutureCourseWaffleFlag;
Updated all the imports to use CourseWaffleFlag from the __future__ module;
BREAKING CHANGE: A number of toggle related constants (e.g. ENABLE_ACCESSIBILITY_POLICY_PAGE)
changed types. They were strings, and are now toggle instances (e.g. WaffleSwitch). Although the entire
refactor should be self-contained in edx-platform, if any plugins or dependencies were directly
using these constants, they will break. If this is the case, try to find a better publicized way of
exposing those toggles.
It's long past time that the default test modulestore was Split,
instead of Old Mongo. This commit switches the default store and
fixes some tests that now fail:
- Tests that didn't expect MFE to be enabled (because we don't
enable MFE for Old Mongo) - opt out of MFE for those
- Tests that hardcoded old key string formats
- Lots of other random little differences
In many places, I didn't spend much time trying to figure out how to
properly fix the test, and instead just set the modulestore to Old
Mongo.
For those tests that I didn't spend time investigating, I've set
the modulestore to TEST_DATA_MONGO_AMNESTY_MODULESTORE - search for
that string to find further work.
When first building the Certificate Date Override feature, I set up the
CertificateDateOverride model to store the override dates as Dates
instead of DateTimes.
Turns out this is not how edX typically handles dates, and it’s causing
some minor headaches around needing to convert values. Also, using just
Dates causes timezone issues.
MICROBA-1488
When sending a GeneratedCertificate to Credentials, send the associated
CertificateDateOverride (if there is one), or else None. This
will be triggered after any save of a GeneratedCertificate, and after
any save or deletion of a single CertificateDateOverride.
Credentials will eventually store its own copy of this date override, or
edit or remove exiting date overrides.
* refactor: Merge the openedx certs app with lms one
Move the certs API from openedx into the lms certificates app.
Functionally, this is a no-op. Cleanup will happen in a subsequent
commit. This is simply a move.
feat: reimagine certificate display settings
The course settings `certificate_available_date` (CAD) and
`certificates_display_behavior` (CDB) were previously
acting indedependantly of one another. They now work in
tandem. This change:
- limits CDB to a dropdown
- removes "early_with_info" and adds "end_with_date"
- only takes CAD into account if "end_with_date" is selected
- Moves CDB to the main course schedule settings page
- updates CourseOverview model and CourseDetails objects to
validate these fields and choose sane defaults if they aren't
expected values
This work was previously done in bd9e7dd (complete with bugs), so this
version is toggleable via the ENABLE_V2_CERT_DISPLAY_SETTINGS setting
* [fix] Fix certificate available date sync
We were syncing the course available date to every course in
credentials. Since credentials doesn't understand "self-paced" courses,
or course end behaviors, some certificates were time gated incorrectly.
This check make sure to check if the course is not self-paced, and has a
CDB of 'end' before syncing the CA date.
The `may_certify` function is used to determine whether a learner should
be able to see that their certificate is ready. It is therefore entirely
in the certificates domain. This remove may_certifiy and
may_certify_for_course from the xmodule and course_overview
apps and into the certificates app. The xmodule may_certify was not called
outside of tests prior to this, and the other's were easily moved.
In order to avoid circular imports, this also moved
certificate_info_for_user out of the model code and into an API
The course settings `certificate_available_date` (CAD) and
`certificates_display_behavior` (CDB) were previously acting indedependantly
of one another. They now work in tandem. This change:
- limits CDB to a dropdown
- removes "early_with_info" and adds "end_with_date"
- only takes CAD into account if "end_with_date" is selected
- Moves CDB to the main course schedule settings page
- updates CourseOverview to validate these fields and choose sane
defaults if they aren't expected values
Certificates will now show under the following circumstances:
"Immediately upon passing"
certificate_availability_date = null
certificates_display_behavior = "early_no_info"
"End date of course"
certificate_availability_date = null
certificates_display_behavior = "end"
"A date after the course end date"
certificate_availability_date = <date>
certificates_display_behavior = "end_with_date"
[MICROBA-1307]
Before this change a user would not be auto refunded if they had a
certificate in a course with any status. This had unintended
consequences. This change updates the logic to only block auto refund
for statuses that we do not want to refund on such as downloadable.
Sometimes learners have certificates in old course runs which've been
deleted. When this happens loading the learner's program progress can
result in 500 errors. This corrects those by choosing to count any
non-existent course the learner has certificates for as counting
towards program completion, effectively assuming that availability
dates have passed for all such courses.
Also fixes an error with a condition related to how we determine
whether an enrolled course is considered "in progress". The previous
version of the code had a bug that would result in a lot more courses
being marked "in progress" for the purpose of program completion than
actually were.
JIRA:EDUCATOR-5787
* [feat] Management command to populate credentials availability date
This is a command to populate the new CredentialsCertificate model's
available_date for every existing course_run.
* [feat] calling course certificates api from LMS
Now that CourseCertificates in credentials have a field for the
available_date, we need to make sure we are always updating that field
when it changes in studio. This PR adds a call to a new Credentials API
that will update the field each time the date change signal fires.
