From 0ee5a64939da2b9fc608fa89b65180378dce52a4 Mon Sep 17 00:00:00 2001 From: Christie Rice <8483753+crice100@users.noreply.github.com> Date: Mon, 14 Jun 2021 12:07:28 -0400 Subject: [PATCH] feat: Save updates to the allowlist model, and add history to the allowlist model (#27904) MICROBA-982 --- lms/djangoapps/certificates/api.py | 35 +++- .../0027_historicalcertificateallowlist.py | 43 +++++ lms/djangoapps/certificates/models.py | 6 + lms/djangoapps/certificates/tests/test_api.py | 178 +++++++++++------- 4 files changed, 191 insertions(+), 71 deletions(-) create mode 100644 lms/djangoapps/certificates/migrations/0027_historicalcertificateallowlist.py diff --git a/lms/djangoapps/certificates/api.py b/lms/djangoapps/certificates/api.py index f0aa00688b..3e43f0499d 100644 --- a/lms/djangoapps/certificates/api.py +++ b/lms/djangoapps/certificates/api.py @@ -32,6 +32,7 @@ from lms.djangoapps.certificates.generation_handler import ( ) from lms.djangoapps.certificates.data import CertificateStatuses from lms.djangoapps.certificates.models import ( + CertificateAllowlist, CertificateGenerationConfiguration, CertificateGenerationCourseSetting, CertificateInvalidation, @@ -628,6 +629,15 @@ def create_or_update_certificate_allowlist_entry(user, course_key, notes, enable } ) + __, __ = CertificateAllowlist.objects.update_or_create( + user=user, + course_id=course_key, + defaults={ + 'allowlist': enabled, + 'notes': notes, + } + ) + log.info(f"Updated the allowlist of course {course_key} with student {user.id} and enabled={enabled}") return certificate_allowlist, created @@ -641,6 +651,7 @@ def remove_allowlist_entry(user, course_key): Returns the result of the removal operation as a bool. """ log.info(f"Removing student {user.id} from the allowlist in course {course_key}") + deleted = False allowlist_entry = get_allowlist_entry(user, course_key) if allowlist_entry: @@ -651,9 +662,15 @@ def remove_allowlist_entry(user, course_key): log.info(f"Removing student {user.id} from the allowlist in course {course_key}.") allowlist_entry.delete() - return True + deleted = True - return False + 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 def get_allowlist_entry(user, course_key): @@ -670,6 +687,20 @@ def get_allowlist_entry(user, course_key): 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: + return None + + return allowlist_entry + + def is_on_allowlist(user, course_key): """ Determines if a learner has an active allowlist entry for a given course-run. diff --git a/lms/djangoapps/certificates/migrations/0027_historicalcertificateallowlist.py b/lms/djangoapps/certificates/migrations/0027_historicalcertificateallowlist.py new file mode 100644 index 0000000000..8531a5d214 --- /dev/null +++ b/lms/djangoapps/certificates/migrations/0027_historicalcertificateallowlist.py @@ -0,0 +1,43 @@ +# Generated by Django 2.2.20 on 2021-06-09 19:24 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion +import django.utils.timezone +import model_utils.fields +import opaque_keys.edx.django.models +import simple_history.models + + +class Migration(migrations.Migration): + + dependencies = [ + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ('certificates', '0026_certificateallowlist'), + ] + + operations = [ + migrations.CreateModel( + name='HistoricalCertificateAllowlist', + fields=[ + ('id', models.IntegerField(auto_created=True, blank=True, db_index=True, verbose_name='ID')), + ('created', model_utils.fields.AutoCreatedField(default=django.utils.timezone.now, editable=False, verbose_name='created')), + ('modified', model_utils.fields.AutoLastModifiedField(default=django.utils.timezone.now, editable=False, verbose_name='modified')), + ('course_id', opaque_keys.edx.django.models.CourseKeyField(blank=True, default=None, max_length=255)), + ('allowlist', models.BooleanField(default=0)), + ('notes', models.TextField(default=None, null=True)), + ('history_id', models.AutoField(primary_key=True, serialize=False)), + ('history_date', models.DateTimeField()), + ('history_change_reason', models.CharField(max_length=100, null=True)), + ('history_type', models.CharField(choices=[('+', 'Created'), ('~', 'Changed'), ('-', 'Deleted')], max_length=1)), + ('history_user', models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='+', to=settings.AUTH_USER_MODEL)), + ('user', models.ForeignKey(blank=True, db_constraint=False, null=True, on_delete=django.db.models.deletion.DO_NOTHING, related_name='+', to=settings.AUTH_USER_MODEL)), + ], + options={ + 'verbose_name': 'historical certificate allowlist', + 'ordering': ('-history_date', '-history_id'), + 'get_latest_by': 'history_date', + }, + bases=(simple_history.models.HistoricalChanges, models.Model), + ), + ] diff --git a/lms/djangoapps/certificates/models.py b/lms/djangoapps/certificates/models.py index ebd2b68ce5..5320f64c79 100644 --- a/lms/djangoapps/certificates/models.py +++ b/lms/djangoapps/certificates/models.py @@ -132,6 +132,12 @@ class CertificateAllowlist(TimeStampedModel): allowlist = models.BooleanField(default=0) notes = models.TextField(default=None, null=True) + # This is necessary because CMS does not install the certificates app, but it + # imports this model's code. Simple History will attempt to connect to the installed + # model in the certificates app, which will fail. + if 'certificates' in apps.app_configs: + history = HistoricalRecords() + @classmethod def get_certificate_allowlist(cls, course_id, student=None): """ diff --git a/lms/djangoapps/certificates/tests/test_api.py b/lms/djangoapps/certificates/tests/test_api.py index 9512069929..93b49bcaf0 100644 --- a/lms/djangoapps/certificates/tests/test_api.py +++ b/lms/djangoapps/certificates/tests/test_api.py @@ -31,6 +31,7 @@ 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, @@ -801,75 +802,6 @@ class CertificatesBrandingTest(ModuleStoreTestCase): assert self.configuration['urls']['TOS_AND_HONOR'] in data['company_tos_url'] -class AllowlistTests(ModuleStoreTestCase): - """ - Tests for handling allowlist certificates - """ - - def setUp(self): - super().setUp() - - # Create users, a course run, and enrollments - self.user = UserFactory() - self.user2 = UserFactory() - self.user3 = UserFactory() - self.user4 = UserFactory() - - self.course_run = CourseFactory() - self.course_run_key = self.course_run.id # pylint: disable=no-member - self.second_course_run = CourseFactory() - self.second_course_run_key = self.second_course_run.id # pylint: disable=no-member - self.third_course_run = CourseFactory() - self.third_course_run_key = self.third_course_run.id # pylint: disable=no-member - - CourseEnrollmentFactory( - user=self.user, - course_id=self.course_run_key, - is_active=True, - mode="verified", - ) - CourseEnrollmentFactory( - user=self.user2, - course_id=self.course_run_key, - is_active=True, - mode="verified", - ) - CourseEnrollmentFactory( - user=self.user3, - course_id=self.course_run_key, - is_active=True, - mode="verified", - ) - CourseEnrollmentFactory( - user=self.user4, - course_id=self.second_course_run_key, - is_active=True, - mode="verified", - ) - - # Add user to the allowlist - CertificateAllowlistFactory.create(course_id=self.course_run_key, user=self.user) - # Add user to the allowlist, but set whitelist to false - CertificateAllowlistFactory.create(course_id=self.course_run_key, user=self.user2, whitelist=False) - # Add user to the allowlist in the other course - CertificateAllowlistFactory.create(course_id=self.second_course_run_key, user=self.user4) - - def test_get_users_allowlist(self): - """ - Test that allowlisted users are returned correctly - """ - users = get_allowlisted_users(self.course_run_key) - assert 1 == users.count() - assert users[0].id == self.user.id - - users = get_allowlisted_users(self.second_course_run_key) - assert 1 == users.count() - assert users[0].id == self.user4.id - - users = get_allowlisted_users(self.third_course_run_key) - assert 0 == users.count() - - class CertificateAllowlistTests(ModuleStoreTestCase): """ Tests for allowlist functionality. @@ -1039,6 +971,114 @@ class CertificateAllowlistTests(ModuleStoreTestCase): assert not can_be_added_to_allowlist(self.user, self.course_run_key) + def test_get_users_allowlist(self): + """ + Test that allowlisted users are returned correctly + """ + u1 = UserFactory() + u2 = UserFactory() + u3 = UserFactory() + u4 = UserFactory() + + cr1 = CourseFactory() + key1 = cr1.id # pylint: disable=no-member + cr2 = CourseFactory() + key2 = cr2.id # pylint: disable=no-member + cr3 = CourseFactory() + key3 = cr3.id # pylint: disable=no-member + + CourseEnrollmentFactory( + user=u1, + course_id=key1, + is_active=True, + mode="verified", + ) + CourseEnrollmentFactory( + user=u2, + course_id=key1, + is_active=True, + mode="verified", + ) + CourseEnrollmentFactory( + user=u3, + course_id=key1, + is_active=True, + mode="verified", + ) + CourseEnrollmentFactory( + user=u4, + course_id=key2, + is_active=True, + mode="verified", + ) + + # 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 in the other course + CertificateAllowlistFactory.create(course_id=key2, user=u4) + + users = get_allowlisted_users(key1) + assert 1 == users.count() + assert users[0].id == u1.id + + users = get_allowlisted_users(key2) + assert 1 == users.count() + assert users[0].id == u4.id + + users = get_allowlisted_users(key3) + assert 0 == users.count() + + def test_add_and_update(self): + """ + Test add and update of the allowlist + """ + u1 = UserFactory() + 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 + + # 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 + + # 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 + + def test_remove(self): + """ + Test removal from the allowlist + """ + u1 = UserFactory() + notes = 'I had a thought....' + + # 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 + + # 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 + class CertificateInvalidationTests(ModuleStoreTestCase): """