From c73e5ba518b636bb23e2f16892dfc8cc818380b7 Mon Sep 17 00:00:00 2001 From: Diana Huang Date: Fri, 24 Feb 2023 10:06:00 -0500 Subject: [PATCH] docs: add CertificateModel ADR. (#31825) Adds an ADR explaining that historical fields used by CertificateModel were not removed. Adds comments to point to ADR. --- .../008-certificate-model-remnants.rst | 44 +++++++++++++++++++ lms/djangoapps/certificates/models.py | 7 ++- 2 files changed, 49 insertions(+), 2 deletions(-) create mode 100644 lms/djangoapps/certificates/docs/decisions/008-certificate-model-remnants.rst diff --git a/lms/djangoapps/certificates/docs/decisions/008-certificate-model-remnants.rst b/lms/djangoapps/certificates/docs/decisions/008-certificate-model-remnants.rst new file mode 100644 index 0000000000..a934dc3761 --- /dev/null +++ b/lms/djangoapps/certificates/docs/decisions/008-certificate-model-remnants.rst @@ -0,0 +1,44 @@ +Leaving PDF Certificate Fields in Certificates Model +==================================================== + +Status +------ + +Accepted + +Context +------- + +As part of the `deprecation of PDF certificates`_, we had to make a decision about the future of several existing fields in +the ``GeneratedCertificate`` model that would no longer be used by the UI code. The replacement for PDF certificates is +web/HTML certificates, which have been outlined in a `previous ADR`_. + + +.. _deprecation of PDF certificates: https://github.com/openedx/public-engineering/issues/27 +.. _previous ADR: https://github.com/openedx/edx-platform/blob/master/lms/djangoapps/certificates/docs/decisions/003-web-certs.rst + +Decision +-------- + +We decided to leave the existing fields ``download_url``, ``download_uuid``, and ``error_reason`` within the ``GeneratedCertificate`` +model and did not remove them as part of our deprecation process where we removed the UI to display them. + + +Consequences +------------ + +A consequence of this decision is that all Open edX operators, including edx.org, will continue to store this data in their databases, +even though it does not have any functional use. + +Several pieces of code still reference these fields, and there are events that still pass around the download_url, even though this field +should no longer be populated or used any longer. + + +Alternatives Considered +----------------------- + +We considered removing these fields and all associated code, but it was felt as though it was going to be too complex, because it involved +migrations and several API changes in order to support the removal. + +We also felt as though it would be useful for operators that did have historical PDF certificates to continue to have a historical record of +where the PDF certificates were stored. diff --git a/lms/djangoapps/certificates/models.py b/lms/djangoapps/certificates/models.py index dd6be3ccbd..dc43b0fd35 100644 --- a/lms/djangoapps/certificates/models.py +++ b/lms/djangoapps/certificates/models.py @@ -224,8 +224,6 @@ class GeneratedCertificate(models.Model): user = models.ForeignKey(User, on_delete=models.CASCADE) course_id = CourseKeyField(max_length=255, blank=True, default=None) verify_uuid = models.CharField(max_length=32, blank=True, default='', db_index=True) - download_uuid = models.CharField(max_length=32, blank=True, default='') - download_url = models.CharField(max_length=128, blank=True, default='') grade = models.CharField(max_length=5, blank=True, default='') key = models.CharField(max_length=32, blank=True, default='') distinction = models.BooleanField(default=False) @@ -234,6 +232,11 @@ class GeneratedCertificate(models.Model): name = models.CharField(blank=True, max_length=255) created_date = models.DateTimeField(auto_now_add=True) modified_date = models.DateTimeField(auto_now=True) + + # These fields have been kept around even though they are not used. + # See lms/djangoapps/certificates/docs/decisions/008-certificate-model-remnants.rst for the ADR. + download_uuid = models.CharField(max_length=32, blank=True, default='') + download_url = models.CharField(max_length=128, blank=True, default='') error_reason = models.CharField(max_length=512, blank=True, default='') # This is necessary because CMS does not install the certificates app, but it