Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

feat!: Read from the allowlist model, instead of from the whitelist model. Only write to the allowlist model. (#27974)

Browse Source 
MICROBA-982
This commit is contained in:
Christie Rice
2021-06-25 09:47:32 -04:00
committed by GitHub
parent 44e8ea4e0b
commit 69580aa592
12 changed files with 45 additions and 261 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
lms/djangoapps/certificates/README.rst
View File

@@ -2,7 +2,7 @@ Status: Maintenance
Responsibilities
================
The Certificates app is responsible for creating and managing Course-Run certificates, including any relevant data models for invalidating and white-listing users' certificate eligibilities.
The Certificates app is responsible for creating and managing course run certificates, including relevant data models for invalidating certificates and managing the allowlist.
Direction: Move and Extract
===========================

46
lms/djangoapps/certificates/api.py
View File

@@ -38,7 +38,6 @@ from lms.djangoapps.certificates.models import (
CertificateInvalidation,
CertificateTemplate,
CertificateTemplateAsset,
CertificateWhitelist,
ExampleCertificateSet,
GeneratedCertificate,
certificate_status_for_student
@@ -113,7 +112,7 @@ def get_certificates_for_user(username):
"course_key": CourseLocator('edX', 'DemoX', 'Demo_Course', None, None),
"type": "verified",
"status": "downloadable",
"download_url": "http://www.example.com/cert.pdf",
"download_url": "https://www.example.com/cert.pdf",
"grade": "0.98",
"created": 2015-07-31T00:00:00Z,
"modified": 2015-07-31T00:00:00Z
@@ -411,7 +410,7 @@ def example_certificates_status(course_key):
{
'description': 'honor',
'status': 'success',
'download_url': 'http://www.example.com/abcd/honor_cert.pdf'
'download_url': 'https://www.example.com/abcd/honor_cert.pdf'
},
{
'description': 'verified',
@@ -613,23 +612,14 @@ def get_allowlisted_users(course_key):
"""
Return the users who are on the allowlist for this course run
"""
return User.objects.filter(certificatewhitelist__course_id=course_key, certificatewhitelist__whitelist=True)
return User.objects.filter(certificateallowlist__course_id=course_key, certificateallowlist__allowlist=True)
def create_or_update_certificate_allowlist_entry(user, course_key, notes, enabled=True):
"""
Update-or-create an allowlist entry for a student in a given course-run.
"""
certificate_allowlist, created = CertificateWhitelist.objects.update_or_create(
user=user,
course_id=course_key,
defaults={
'whitelist': enabled,
'notes': notes,
}
)
__, __ = CertificateAllowlist.objects.update_or_create(
certificate_allowlist, created = CertificateAllowlist.objects.update_or_create(
user=user,
course_id=course_key,
defaults={
@@ -664,12 +654,6 @@ def remove_allowlist_entry(user, course_key):
allowlist_entry.delete()
deleted = True
new_model_entry = _get_allowlist_entry_from_new_model(user, course_key)
if new_model_entry:
log.info(f"Allowlist entry exists in the new allowlist model for student {user.id} in course {course_key}, "
f"and will be removed.")
new_model_entry.delete()
return deleted
@@ -678,24 +662,10 @@ def get_allowlist_entry(user, course_key):
Retrieves and returns an allowlist entry for a given learner and course-run.
"""
log.info(f"Attempting to retrieve an allowlist entry for student {user.id} in course {course_key}.")
try:
allowlist_entry = CertificateWhitelist.objects.get(user=user, course_id=course_key)
except ObjectDoesNotExist:
log.warning(f"No allowlist entry found for student {user.id} in course {course_key}.")
return None
return allowlist_entry
def _get_allowlist_entry_from_new_model(user, course_key):
"""
Retrieves and returns an allowlist entry from the allowlist model.
This is temporary code until we switch completely from the CertificateWhitelist to the CertificateAllowlist.
"""
try:
allowlist_entry = CertificateAllowlist.objects.get(user=user, course_id=course_key)
except ObjectDoesNotExist:
log.warning(f"No allowlist entry found for student {user.id} in course {course_key}.")
return None
return allowlist_entry
@@ -772,7 +742,7 @@ def get_allowlist(course_key):
"""
Return the certificate allowlist for the given course run
"""
return CertificateWhitelist.get_certificate_white_list(course_key)
return CertificateAllowlist.get_certificate_allowlist(course_key)
def get_enrolled_allowlisted_users(course_key):
@@ -783,8 +753,8 @@ def get_enrolled_allowlisted_users(course_key):
"""
users = CourseEnrollment.objects.users_enrolled_in(course_key)
return users.filter(
certificatewhitelist__course_id=course_key,
certificatewhitelist__whitelist=True
certificateallowlist__course_id=course_key,
certificateallowlist__allowlist=True
)

10
lms/djangoapps/certificates/docs/decisions/001-allowlist-cert-requirements.rst
View File

@@ -12,9 +12,11 @@ This doc covers requirements for allowlist course certificates.
Users can earn a course certificate in a particular course run (the certificate
is stored in the *GeneratedCertificate* model). If a user has not earned a certificate
but the course staff would like them to have a certificate anyway, the user can
be added to the certificate allowlist for the course run. The allowlist is currently
stored in the *CertificateWhitelist* model, and was previously referred to as the
certificate whitelist.
be added to the certificate allowlist for the course run.
The allowlist is stored in the *CertificateAllowlist* model. It was previously
referred to as the certificate whitelist, and was previously stored in the
*CertificateWhitelist* model.
Requirements
------------
@@ -32,4 +34,4 @@ the time the certificate is generated:
* The user must have an approved, unexpired, ID verification
* The user must not have an invalidated certificate for the course run (see the *CertificateInvalidation* model)
* HTML (web) certificates must be globally enabled, and also enabled for the course run
* The user must be on the allowlist for the course run (see the *CertificateWhitelist* model)
* The user must be on the allowlist for the course run (see the *CertificateAllowlist* model)

4
lms/djangoapps/certificates/generation_handler.py
View File

@@ -14,8 +14,8 @@ from common.djangoapps.course_modes import api as modes_api
from common.djangoapps.student.models import CourseEnrollment
from lms.djangoapps.certificates.data import CertificateStatuses
from lms.djangoapps.certificates.models import (
CertificateAllowlist,
CertificateInvalidation,
CertificateWhitelist,
GeneratedCertificate
)
from lms.djangoapps.certificates.queue import XQueueCertInterface
@@ -356,7 +356,7 @@ def is_on_certificate_allowlist(user, course_key):
"""
Check if the user is on the allowlist, and is enabled for the allowlist, for this course run
"""
return CertificateWhitelist.objects.filter(user=user, course_id=course_key, whitelist=True).exists()
return CertificateAllowlist.objects.filter(user=user, course_id=course_key, allowlist=True).exists()
def _can_generate_certificate_for_status(user, course_key):

45
lms/djangoapps/certificates/models.py
View File

@@ -69,51 +69,6 @@ class CertificateWhitelist(models.Model):
created = AutoCreatedField(_('created'))
notes = models.TextField(default=None, null=True)
@classmethod
def get_certificate_white_list(cls, course_id, student=None):
"""
Return certificate white list for the given course as dict object,
returned dictionary will have the following key-value pairs
[{
id: 'id (pk) of CertificateWhitelist item'
user_id: 'User Id of the student'
user_name: 'name of the student'
user_email: 'email of the student'
course_id: 'Course key of the course to whom certificate exception belongs'
created: 'Creation date of the certificate exception'
notes: 'Additional notes for the certificate exception'
}, {...}, ...]
"""
white_list = cls.objects.filter(course_id=course_id, whitelist=True)
if student:
white_list = white_list.filter(user=student)
result = []
generated_certificates = GeneratedCertificate.eligible_certificates.filter(
course_id=course_id,
user__in=[exception.user for exception in white_list],
status=CertificateStatuses.downloadable
)
generated_certificates = {
certificate['user']: certificate['created_date']
for certificate in generated_certificates.values('user', 'created_date')
}
for item in white_list:
certificate_generated = generated_certificates.get(item.user.id, '')
result.append({
'id': item.id,
'user_id': item.user.id,
'user_name': str(item.user.username),
'user_email': str(item.user.email),
'course_id': str(item.course_id),
'created': item.created.strftime("%B %d, %Y"),
'certificate_generated': certificate_generated and certificate_generated.strftime("%B %d, %Y"),
'notes': str(item.notes or ''),
})
return result
class CertificateAllowlist(TimeStampedModel):
"""

6
lms/djangoapps/certificates/queue.py
View File

@@ -20,7 +20,7 @@ from common.djangoapps.course_modes.models import CourseMode
from common.djangoapps.student.models import CourseEnrollment, UserProfile
from lms.djangoapps.certificates.data import CertificateStatuses as status
from lms.djangoapps.certificates.models import (
CertificateWhitelist,
CertificateAllowlist,
ExampleCertificate,
GeneratedCertificate,
certificate_status_for_student
@@ -103,7 +103,7 @@ class XQueueCertInterface:
settings.XQUEUE_INTERFACE['django_auth'],
requests_auth,
)
self.allowlist = CertificateWhitelist.objects.all()
self.allowlist = CertificateAllowlist.objects.all()
self.use_https = True
def regen_cert(self, student, course_id, forced_grade=None, template_file=None, generate_pdf=True):
@@ -259,7 +259,7 @@ class XQueueCertInterface:
self.request.user = student
self.request.session = {}
is_allowlisted = self.allowlist.filter(user=student, course_id=course_id, whitelist=True).exists()
is_allowlisted = self.allowlist.filter(user=student, course_id=course_id, allowlist=True).exists()
course_grade = CourseGradeFactory().read(student, course_key=course_id)
enrollment_mode, __ = CourseEnrollment.enrollment_mode_for_user(student, course_id)
mode_is_verified = enrollment_mode in GeneratedCertificate.VERIFIED_CERTS_MODES

4
lms/djangoapps/certificates/signals.py
View File

@@ -18,9 +18,9 @@ from lms.djangoapps.certificates.generation_handler import (
is_on_certificate_allowlist
)
from lms.djangoapps.certificates.models import (
CertificateAllowlist,
CertificateGenerationCourseSetting,
CertificateStatuses,
CertificateWhitelist,
GeneratedCertificate
)
from lms.djangoapps.certificates.tasks import CERTIFICATE_DELAY_SECONDS, generate_certificate
@@ -52,7 +52,7 @@ def _update_cert_settings_on_pacing_change(sender, updated_course_overview, **kw
))
@receiver(post_save, sender=CertificateWhitelist, dispatch_uid="append_certificate_allowlist")
@receiver(post_save, sender=CertificateAllowlist, dispatch_uid="append_certificate_allowlist")
def _listen_for_certificate_allowlist_append(sender, instance, **kwargs): # pylint: disable=unused-argument
"""
Listen for a user being added to or modified on the allowlist

16
lms/djangoapps/certificates/tests/factories.py
View File

@@ -13,7 +13,6 @@ from lms.djangoapps.certificates.models import (
CertificateHtmlViewConfiguration,
CertificateInvalidation,
CertificateStatuses,
CertificateWhitelist,
GeneratedCertificate
)
@@ -38,21 +37,6 @@ class CertificateAllowlistFactory(DjangoModelFactory):
Certificate allowlist factory
"""
class Meta:
model = CertificateWhitelist
course_id = None
whitelist = True
notes = 'Test Notes'
class TemporaryCertificateAllowlistFactory(DjangoModelFactory):
"""
Temporary certificate allowlist factory.
This will be removed once the CertificateAllowlistFactory uses the CertificateAllowlist as its model.
"""
class Meta:
model = CertificateAllowlist

39
lms/djangoapps/certificates/tests/test_api.py
View File

@@ -32,7 +32,6 @@ from common.djangoapps.student.tests.factories import (
)
from common.djangoapps.util.testing import EventTestMixin
from lms.djangoapps.certificates.api import (
_get_allowlist_entry_from_new_model,
can_be_added_to_allowlist,
cert_generation_enabled,
certificate_downloadable_status,
@@ -790,7 +789,7 @@ class CertificateAllowlistTests(ModuleStoreTestCase):
result, __ = create_or_update_certificate_allowlist_entry(self.user, self.course_run_key, "New test", False)
assert result.notes == "New test"
assert not result.whitelist
assert not result.allowlist
def test_remove_allowlist_entry(self):
"""
@@ -879,7 +878,7 @@ class CertificateAllowlistTests(ModuleStoreTestCase):
"""
Test to verify that we will return False when the allowlist entry if it is disabled.
"""
CertificateAllowlistFactory.create(course_id=self.course_run_key, user=self.user, whitelist=False)
CertificateAllowlistFactory.create(course_id=self.course_run_key, user=self.user, allowlist=False)
result = is_on_allowlist(self.user, self.course_run_key)
assert not result
@@ -970,8 +969,8 @@ class CertificateAllowlistTests(ModuleStoreTestCase):
# Add user to the allowlist
CertificateAllowlistFactory.create(course_id=key1, user=u1)
# Add user to the allowlist, but set whitelist to false
CertificateAllowlistFactory.create(course_id=key1, user=u2, whitelist=False)
# Add user to the allowlist, but set allowlist to false
CertificateAllowlistFactory.create(course_id=key1, user=u2, allowlist=False)
# Add user to the allowlist in the other course
CertificateAllowlistFactory.create(course_id=key2, user=u4)
@@ -994,25 +993,19 @@ class CertificateAllowlistTests(ModuleStoreTestCase):
notes = 'blah'
# Check before adding user
old_entry = get_allowlist_entry(u1, self.course_run_key)
assert old_entry is None
new_entry = _get_allowlist_entry_from_new_model(u1, self.course_run_key)
assert new_entry is None
entry = get_allowlist_entry(u1, self.course_run_key)
assert entry is None
# Add user
create_or_update_certificate_allowlist_entry(u1, self.course_run_key, notes)
old_entry = get_allowlist_entry(u1, self.course_run_key)
assert old_entry.notes == notes
new_entry = _get_allowlist_entry_from_new_model(u1, self.course_run_key)
assert new_entry.notes == notes
entry = get_allowlist_entry(u1, self.course_run_key)
assert entry.notes == notes
# Update user
new_notes = 'really useful info'
create_or_update_certificate_allowlist_entry(u1, self.course_run_key, new_notes)
old_entry = get_allowlist_entry(u1, self.course_run_key)
assert old_entry.notes == new_notes
new_entry = _get_allowlist_entry_from_new_model(u1, self.course_run_key)
assert new_entry.notes == new_notes
entry = get_allowlist_entry(u1, self.course_run_key)
assert entry.notes == new_notes
def test_remove(self):
"""
@@ -1023,17 +1016,13 @@ class CertificateAllowlistTests(ModuleStoreTestCase):
# Add user
create_or_update_certificate_allowlist_entry(u1, self.course_run_key, notes)
old_entry = get_allowlist_entry(u1, self.course_run_key)
assert old_entry.notes == notes
new_entry = _get_allowlist_entry_from_new_model(u1, self.course_run_key)
assert new_entry.notes == notes
entry = get_allowlist_entry(u1, self.course_run_key)
assert entry.notes == notes
# Remove user
remove_allowlist_entry(u1, self.course_run_key)
old_entry = get_allowlist_entry(u1, self.course_run_key)
assert old_entry is None
new_entry = _get_allowlist_entry_from_new_model(u1, self.course_run_key)
assert new_entry is None
entry = get_allowlist_entry(u1, self.course_run_key)
assert entry is None
class CertificateInvalidationTests(ModuleStoreTestCase):

2
lms/djangoapps/certificates/tests/test_generation_handler.py
View File

@@ -84,7 +84,7 @@ class AllowlistTests(ModuleStoreTestCase):
is_active=True,
mode="verified",
)
CertificateAllowlistFactory.create(course_id=self.course_run_key, user=u, whitelist=False)
CertificateAllowlistFactory.create(course_id=self.course_run_key, user=u, allowlist=False)
assert not is_on_certificate_allowlist(u, self.course_run_key)
@ddt.data(

130
lms/djangoapps/certificates/tests/test_models.py
View File

@@ -9,7 +9,6 @@ import pytest
from django.conf import settings
from django.core.exceptions import ValidationError
from django.core.files.uploadedfile import SimpleUploadedFile
from django.core.paginator import Paginator
from django.test import TestCase
from django.test.utils import override_settings
from opaque_keys.edx.locator import CourseKey, CourseLocator
@@ -23,22 +22,18 @@ from lms.djangoapps.certificates.models import (
CertificateInvalidation,
CertificateStatuses,
CertificateTemplateAsset,
CertificateWhitelist,
ExampleCertificate,
ExampleCertificateSet,
GeneratedCertificate
)
from lms.djangoapps.certificates.tests.factories import (
CertificateAllowlistFactory
)
from lms.djangoapps.certificates.tests.factories import (
CertificateInvalidationFactory,
GeneratedCertificateFactory,
TemporaryCertificateAllowlistFactory,
CertificateAllowlistFactory,
)
from lms.djangoapps.instructor_task.tests.factories import InstructorTaskFactory
from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory
from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, SharedModuleStoreTestCase
from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase
from xmodule.modulestore.tests.factories import CourseFactory
FEATURES_INVALID_FILE_PATH = settings.FEATURES.copy()
@@ -482,15 +477,15 @@ class CertificateAllowlistTest(SharedModuleStoreTestCase):
assert len(ret) == 0
def test_get_allowlist_multiple_users(self):
TemporaryCertificateAllowlistFactory.create(course_id=self.course_run_key, user=self.user)
TemporaryCertificateAllowlistFactory.create(course_id=self.course_run_key, user=self.second_user)
CertificateAllowlistFactory.create(course_id=self.course_run_key, user=self.user)
CertificateAllowlistFactory.create(course_id=self.course_run_key, user=self.second_user)
ret = CertificateAllowlist.get_certificate_allowlist(course_id=self.course_run_key)
assert len(ret) == 2
def test_get_allowlist_no_cert(self):
allowlist_item = TemporaryCertificateAllowlistFactory.create(course_id=self.course_run_key, user=self.user)
TemporaryCertificateAllowlistFactory.create(course_id=self.course_run_key, user=self.second_user)
allowlist_item = CertificateAllowlistFactory.create(course_id=self.course_run_key, user=self.user)
CertificateAllowlistFactory.create(course_id=self.course_run_key, user=self.second_user)
ret = CertificateAllowlist.get_certificate_allowlist(course_id=self.course_run_key, student=self.user)
assert len(ret) == 1
@@ -506,7 +501,7 @@ class CertificateAllowlistTest(SharedModuleStoreTestCase):
assert item['notes'] == allowlist_item.notes
def test_get_allowlist_cert(self):
allowlist_item = TemporaryCertificateAllowlistFactory.create(course_id=self.course_run_key, user=self.user)
allowlist_item = CertificateAllowlistFactory.create(course_id=self.course_run_key, user=self.user)
cert = GeneratedCertificateFactory.create(
status=CertificateStatuses.downloadable,
user=self.user,
@@ -519,114 +514,3 @@ class CertificateAllowlistTest(SharedModuleStoreTestCase):
item = ret[0]
assert item['id'] == allowlist_item.id
assert item['certificate_generated'] == cert.created_date.strftime("%B %d, %Y")
class TemporaryMigrationTests(ModuleStoreTestCase):
"""
Temporary tests for the allowlist data migration
"""
def setUp(self):
super().setUp()
self.course = CourseFactory.create(
org='edx',
number='verified',
display_name='Verified Course'
)
self.key = self.course.id
self.u1 = UserFactory.create()
self.u2 = UserFactory.create()
self.u3 = UserFactory.create()
def _migrate(self):
"""
Migrate the data
"""
whitelisted = CertificateWhitelist.objects.all().order_by('id')
paginator = Paginator(whitelisted, 1)
for page_num in paginator.page_range:
page = paginator.page(page_num)
for w in page.object_list:
CertificateAllowlist.objects.update_or_create(
user_id=w.user_id,
course_id=w.course_id,
defaults={
'allowlist': w.whitelist,
'notes': w.notes,
'created': w.created,
}
)
def test_migration(self):
"""
Temporary test for the allowlist data migration
"""
TemporaryCertificateAllowlistFactory.create(
user=self.u1,
course_id=self.key,
)
CertificateAllowlistFactory.create(
user=self.u1,
course_id=self.key,
whitelist=False,
notes='nope'
)
CertificateAllowlistFactory.create(
user=self.u2,
course_id=self.key,
)
self._migrate()
whitelisted = CertificateWhitelist.objects.all()
allowlisted = CertificateAllowlist.objects.all()
assert len(whitelisted) == len(allowlisted)
a = CertificateAllowlist.objects.get(user_id=self.u1.id)
assert a.allowlist is False
assert a.notes == 'nope'
a = CertificateAllowlist.objects.get(user_id=self.u2.id)
assert a.allowlist is True
def test_migration_empty(self):
"""
Temporary test for the allowlist data migration when the allowlist is empty
"""
CertificateAllowlistFactory.create(
user=self.u1,
course_id=self.key,
whitelist=False
)
CertificateAllowlistFactory.create(
user=self.u2,
course_id=self.key,
)
CertificateAllowlistFactory.create(
user=self.u3,
course_id=self.key,
)
self._migrate()
whitelisted = CertificateWhitelist.objects.all()
allowlisted = CertificateAllowlist.objects.all()
assert len(whitelisted) == len(allowlisted)
def test_migration_both_empty(self):
"""
Temporary test for the allowlist data migration when both models are empty
"""
whitelisted = CertificateWhitelist.objects.all()
assert len(whitelisted) == 0
self._migrate()
whitelisted = CertificateWhitelist.objects.all()
allowlisted = CertificateAllowlist.objects.all()
assert len(whitelisted) == len(allowlisted)

2
lms/djangoapps/instructor/views/api.py
View File

@@ -3025,7 +3025,7 @@ def add_certificate_exception(course_key, student, certificate_exception):
'user_name': student.username,
'user_id': student.id,
'certificate_generated': generated_certificate and generated_certificate.created_date.strftime("%B %d, %Y"),
'created': certificate_allowlist_entry.created.strftime("%A, %B %d, %Y"),
'created': certificate_allowlist_entry.created.strftime("%B %d, %Y"),
})
return exception