From 1181fb343ee754faa41f9ef2429782fa69951e1f Mon Sep 17 00:00:00 2001 From: Christie Rice <8483753+crice100@users.noreply.github.com> Date: Mon, 5 Apr 2021 14:00:07 -0400 Subject: [PATCH] feat!: Remove allow_certificate checks from course certificates (#27206) DEPR-140 MICROBA-985 --- .../management/commands/cert_restriction.py | 94 ------------------- common/djangoapps/student/tests/factories.py | 1 - .../db_fixtures/certificates_web_view.json | 3 +- .../management/commands/cert_restriction.py | 8 -- .../management/commands/cert_restriction.py | 8 -- lms/djangoapps/certificates/models.py | 13 +-- lms/djangoapps/certificates/queue.py | 24 ----- lms/djangoapps/certificates/tests/tests.py | 45 ++++----- .../tests/test_tasks_helper.py | 25 ++--- 9 files changed, 31 insertions(+), 190 deletions(-) delete mode 100644 common/djangoapps/student/management/commands/cert_restriction.py delete mode 100644 import_shims/lms/student/management/commands/cert_restriction.py delete mode 100644 import_shims/studio/student/management/commands/cert_restriction.py diff --git a/common/djangoapps/student/management/commands/cert_restriction.py b/common/djangoapps/student/management/commands/cert_restriction.py deleted file mode 100644 index 594b6e5fb6..0000000000 --- a/common/djangoapps/student/management/commands/cert_restriction.py +++ /dev/null @@ -1,94 +0,0 @@ -# lint-amnesty, pylint: disable=missing-module-docstring - -import csv -import os - -from django.core.management.base import BaseCommand, CommandError - -from common.djangoapps.student.models import UserProfile - - -class Command(BaseCommand): # lint-amnesty, pylint: disable=missing-class-docstring - help = """ - Sets or gets certificate restrictions for users - from embargoed countries. (allow_certificate in - userprofile) - - CSV should be comma delimited with double quoted entries. - - $ ... cert_restriction --import path/to/userlist.csv - - Export a list of students who have "allow_certificate" in - userprofile set to True - - $ ... cert_restriction --output path/to/export.csv - - Enable a single user so she is not on the restricted list - - $ ... cert_restriction -e user - - Disable a single user so she is on the restricted list - - $ ... cert_restriction -d user - - """ - - def add_arguments(self, parser): - # This command can only take one of these arguments per run, this enforces that. - group = parser.add_mutually_exclusive_group(required=True) - group.add_argument('-i', '--import', - metavar='IMPORT_FILE', - nargs='?', - help='CSV file to import, comma delimitted file with double-quoted entries') - group.add_argument('-o', '--output', - metavar='EXPORT_FILE', - nargs='?', - help='CSV file to export') - group.add_argument('-e', '--enable', - metavar='STUDENT', - nargs='?', - help='Enable a certificate for a single student') - group.add_argument('-d', '--disable', - metavar='STUDENT', - nargs='?', - help='Disable a certificate for a single student') - - def handle(self, *args, **options): - if options['output']: - if os.path.exists(options['output']): - raise CommandError("File {} already exists".format(options['output'])) - disabled_users = UserProfile.objects.filter(allow_certificate=False) - - with open(options['output'], 'w') as csvfile: - csvwriter = csv.writer(csvfile, delimiter=',', quotechar='"', quoting=csv.QUOTE_MINIMAL) - for user in disabled_users: - csvwriter.writerow([user.user.username]) - print('{} disabled users written'.format(len(disabled_users))) - - elif options['import']: - if not os.path.exists(options['import']): - raise CommandError("File {} does not exist".format(options['import'])) - - print("Importing students from {}".format(options['import'])) - - with open(options['import']) as csvfile: - student_list = csv.reader(csvfile, delimiter=',', quotechar='"') - students = [student[0] for student in student_list] - - if not students: - raise CommandError("Unable to read student data from {}".format(options['import'])) - - update_cnt = UserProfile.objects.filter(user__username__in=students).update(allow_certificate=False) - print('{} user(s) disabled out of {} in CSV file'.format(update_cnt, len(students))) - - elif options['enable']: - print("Enabling {} for certificate download".format(options['enable'])) - cert_allow = UserProfile.objects.get(user__username=options['enable']) - cert_allow.allow_certificate = True - cert_allow.save() - - elif options['disable']: - print("Disabling {} for certificate download".format(options['disable'])) - cert_allow = UserProfile.objects.get(user__username=options['disable']) - cert_allow.allow_certificate = False - cert_allow.save() diff --git a/common/djangoapps/student/tests/factories.py b/common/djangoapps/student/tests/factories.py index ecffdf63cd..27e762cbd1 100644 --- a/common/djangoapps/student/tests/factories.py +++ b/common/djangoapps/student/tests/factories.py @@ -59,7 +59,6 @@ class UserProfileFactory(DjangoModelFactory): # lint-amnesty, pylint: disable=m gender = 'm' mailing_address = None goals = 'Learn a lot' - allow_certificate = True class RegistrationFactory(DjangoModelFactory): # lint-amnesty, pylint: disable=missing-class-docstring diff --git a/common/test/db_fixtures/certificates_web_view.json b/common/test/db_fixtures/certificates_web_view.json index 8f8e244932..b11899849f 100644 --- a/common/test/db_fixtures/certificates_web_view.json +++ b/common/test/db_fixtures/certificates_web_view.json @@ -37,8 +37,7 @@ "fields": { "user": 99, "name": "test cert", - "courseware": "course.xml", - "allow_certificate": true + "courseware": "course.xml" } }, { diff --git a/import_shims/lms/student/management/commands/cert_restriction.py b/import_shims/lms/student/management/commands/cert_restriction.py deleted file mode 100644 index 78e0b2acb5..0000000000 --- a/import_shims/lms/student/management/commands/cert_restriction.py +++ /dev/null @@ -1,8 +0,0 @@ -"""Deprecated import support. Auto-generated by import_shims/generate_shims.sh.""" -# pylint: disable=redefined-builtin,wrong-import-position,wildcard-import,useless-suppression,line-too-long - -from import_shims.warn import warn_deprecated_import - -warn_deprecated_import('student.management.commands.cert_restriction', 'common.djangoapps.student.management.commands.cert_restriction') - -from common.djangoapps.student.management.commands.cert_restriction import * diff --git a/import_shims/studio/student/management/commands/cert_restriction.py b/import_shims/studio/student/management/commands/cert_restriction.py deleted file mode 100644 index 78e0b2acb5..0000000000 --- a/import_shims/studio/student/management/commands/cert_restriction.py +++ /dev/null @@ -1,8 +0,0 @@ -"""Deprecated import support. Auto-generated by import_shims/generate_shims.sh.""" -# pylint: disable=redefined-builtin,wrong-import-position,wildcard-import,useless-suppression,line-too-long - -from import_shims.warn import warn_deprecated_import - -warn_deprecated_import('student.management.commands.cert_restriction', 'common.djangoapps.student.management.commands.cert_restriction') - -from common.djangoapps.student.management.commands.cert_restriction import * diff --git a/lms/djangoapps/certificates/models.py b/lms/djangoapps/certificates/models.py index 7b610cc333..4555bbd6a9 100644 --- a/lms/djangoapps/certificates/models.py +++ b/lms/djangoapps/certificates/models.py @@ -89,9 +89,7 @@ class CertificateWhitelist(models.Model): """ Tracks students who are whitelisted, all users in this table will always qualify for a certificate - regardless of their grade unless they are on the - embargoed country restriction list - (allow_certificate set to False in userprofile). + regardless of their grade. .. no_pii: """ @@ -584,10 +582,9 @@ def certificate_status(generated_certificate): deleted - The certificate has been deleted. downloadable - The certificate is available for download. notpassing - The student was graded but is not passing - restricted - The student is on the restricted embargo list and - should not be issued a certificate. This will - be set if allow_certificate is set to False in - the userprofile table + restricted - The student is on the restricted list. This status was + previously set if allow_certificate was set to False in + the userprofile table. unverified - The student is in verified enrollment track and the student did not have their identity verified, even though they should be eligible for the cert otherwise. @@ -643,7 +640,7 @@ def certificate_info_for_user(user, course_id, grade, user_is_whitelisted, user_ user_is_verified = grade is not None and mode_is_verified eligible_for_certificate = 'Y' if (user_is_whitelisted or user_is_verified or certificate_generated) \ - and user.profile.allow_certificate else 'N' + else 'N' if certificate_generated and can_have_certificate: certificate_is_delivered = 'Y' diff --git a/lms/djangoapps/certificates/queue.py b/lms/djangoapps/certificates/queue.py index d84ad71b8c..8d044751fb 100644 --- a/lms/djangoapps/certificates/queue.py +++ b/lms/djangoapps/certificates/queue.py @@ -104,7 +104,6 @@ class XQueueCertInterface: requests_auth, ) self.whitelist = CertificateWhitelist.objects.all() - self.restricted = UserProfile.objects.filter(allow_certificate=False) self.use_https = True def regen_cert(self, student, course_id, course=None, forced_grade=None, template_file=None, generate_pdf=True): @@ -212,9 +211,6 @@ class XQueueCertInterface: If a student has a passing grade or is in the whitelist table for the course a request will be made for a new cert. - If a student has allow_certificate set to False in the - userprofile table the status will change to 'restricted' - If a student does not have a passing grade the status will change to status.notpassing @@ -402,26 +398,6 @@ class XQueueCertInterface: ) return cert - # Check to see whether the student is on the the embargoed - # country restricted list. If so, they should not receive a - # certificate -- set their status to restricted and log it. - if self.restricted.filter(user=student).exists(): - cert.status = status.restricted - cert.save() - - LOGGER.info( - ( - "Student %s is in the embargoed country restricted " - "list, so their certificate status has been set to '%s' " - "for the course '%s'. " - "No certificate generation task was sent to the XQueue." - ), - student.id, - cert.status, - str(course_id) - ) - return cert - if unverified: cert.status = status.unverified cert.save() diff --git a/lms/djangoapps/certificates/tests/tests.py b/lms/djangoapps/certificates/tests/tests.py index 7d8ee2f7ed..afb2df0605 100644 --- a/lms/djangoapps/certificates/tests/tests.py +++ b/lms/djangoapps/certificates/tests/tests.py @@ -58,19 +58,17 @@ class CertificatesModelTest(ModuleStoreTestCase, MilestonesTestCaseMixin): @unpack @data( - {'allow_certificate': False, 'whitelisted': False, 'grade': None, 'output': ['N', 'N', 'N/A']}, - {'allow_certificate': True, 'whitelisted': True, 'grade': None, 'output': ['Y', 'N', 'N/A']}, - {'allow_certificate': True, 'whitelisted': False, 'grade': 0.9, 'output': ['N', 'N', 'N/A']}, - {'allow_certificate': False, 'whitelisted': True, 'grade': 0.8, 'output': ['N', 'N', 'N/A']}, - {'allow_certificate': False, 'whitelisted': None, 'grade': 0.8, 'output': ['N', 'N', 'N/A']} + {'whitelisted': False, 'grade': None, 'output': ['N', 'N', 'N/A']}, + {'whitelisted': True, 'grade': None, 'output': ['Y', 'N', 'N/A']}, + {'whitelisted': False, 'grade': 0.9, 'output': ['N', 'N', 'N/A']}, + {'whitelisted': True, 'grade': 0.8, 'output': ['Y', 'N', 'N/A']}, + {'whitelisted': None, 'grade': 0.8, 'output': ['N', 'N', 'N/A']} ) - def test_certificate_info_for_user(self, allow_certificate, whitelisted, grade, output): + def test_certificate_info_for_user(self, whitelisted, grade, output): """ Verify that certificate_info_for_user works. """ student = UserFactory() - student.profile.allow_certificate = allow_certificate - student.profile.save() # pylint: disable=no-member # for instructor paced course certificate_info = certificate_info_for_user( @@ -88,15 +86,15 @@ class CertificatesModelTest(ModuleStoreTestCase, MilestonesTestCaseMixin): @unpack @data( - {'allow_certificate': False, 'whitelisted': False, 'grade': None, 'output': ['N', 'Y', 'honor']}, - {'allow_certificate': True, 'whitelisted': True, 'grade': None, 'output': ['Y', 'Y', 'honor']}, - {'allow_certificate': True, 'whitelisted': False, 'grade': 0.9, 'output': ['Y', 'Y', 'honor']}, - {'allow_certificate': False, 'whitelisted': True, 'grade': 0.8, 'output': ['N', 'Y', 'honor']}, - {'allow_certificate': False, 'whitelisted': None, 'grade': 0.8, 'output': ['N', 'Y', 'honor']}, - {'allow_certificate': True, 'whitelisted': None, 'grade': None, 'output': ['Y', 'Y', 'honor']}, - {'allow_certificate': False, 'whitelisted': True, 'grade': None, 'output': ['N', 'Y', 'honor']} + {'whitelisted': False, 'grade': None, 'output': ['Y', 'Y', 'honor']}, + {'whitelisted': True, 'grade': None, 'output': ['Y', 'Y', 'honor']}, + {'whitelisted': False, 'grade': 0.9, 'output': ['Y', 'Y', 'honor']}, + {'whitelisted': True, 'grade': 0.8, 'output': ['Y', 'Y', 'honor']}, + {'whitelisted': None, 'grade': 0.8, 'output': ['Y', 'Y', 'honor']}, + {'whitelisted': None, 'grade': None, 'output': ['Y', 'Y', 'honor']}, + {'whitelisted': True, 'grade': None, 'output': ['Y', 'Y', 'honor']} ) - def test_certificate_info_for_user_when_grade_changes(self, allow_certificate, whitelisted, grade, output): + def test_certificate_info_for_user_when_grade_changes(self, whitelisted, grade, output): """ Verify that certificate_info_for_user works as expect in scenario when grading of problems changes after certificates already generated. In such scenario `Certificate delivered` should not depend @@ -104,8 +102,6 @@ class CertificatesModelTest(ModuleStoreTestCase, MilestonesTestCaseMixin): of time. """ student = UserFactory() - student.profile.allow_certificate = allow_certificate - student.profile.save() # pylint: disable=no-member certificate1 = GeneratedCertificateFactory.create( user=student, @@ -137,17 +133,16 @@ class CertificatesModelTest(ModuleStoreTestCase, MilestonesTestCaseMixin): @unpack @data( - {'allow_certificate': True, 'whitelisted': False, 'grade': 0.8, 'mode': 'audit', 'output': ['N', 'N', 'N/A']}, - {'allow_certificate': True, 'whitelisted': True, 'grade': 0.8, 'mode': 'audit', 'output': ['Y', 'N', 'N/A']}, - {'allow_certificate': True, 'whitelisted': False, 'grade': 0.8, 'mode': 'verified', 'output': ['Y', 'N', 'N/A']} + {'whitelisted': False, 'grade': 0.8, 'mode': 'audit', 'output': ['N', 'N', 'N/A']}, + {'whitelisted': True, 'grade': 0.8, 'mode': 'audit', 'output': ['Y', 'N', 'N/A']}, + {'whitelisted': False, 'grade': 0.8, 'mode': 'verified', 'output': ['Y', 'N', 'N/A']} ) - def test_certificate_info_for_user_with_course_modes(self, allow_certificate, whitelisted, grade, mode, output): + def test_certificate_info_for_user_with_course_modes(self, whitelisted, grade, mode, output): """ Verify that certificate_info_for_user works with course modes. """ user = UserFactory.create() - user.profile.allow_certificate = allow_certificate - user.profile.save() + _ = CourseEnrollment.enroll(user, self.instructor_paced_course.id, mode) certificate_info = certificate_info_for_user( user, self.instructor_paced_course.id, grade, @@ -159,8 +154,6 @@ class CertificatesModelTest(ModuleStoreTestCase, MilestonesTestCaseMixin): # Create one user with certs and one without student_no_certs = UserFactory() student_with_certs = UserFactory() - student_with_certs.profile.allow_certificate = True - student_with_certs.profile.save() # pylint: disable=no-member # Set up a couple of courses course_1 = CourseFactory.create() diff --git a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py index 2ac8cbd58b..c437071c17 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py @@ -1889,14 +1889,6 @@ class TestGradeReportEnrollmentAndCertificateInfo(TestReportMixin, InstructorTas data=problem_xml ) - def user_is_embargoed(self, user, is_embargoed): - """ - Set a users embargoed state. - """ - user_profile = UserFactory(username=user.username, email=user.email).profile - user_profile.allow_certificate = not is_embargoed - user_profile.save() # pylint: disable=no-member - def _verify_csv_data(self, username, expected_data): """ Verify grade report data. @@ -1919,7 +1911,6 @@ class TestGradeReportEnrollmentAndCertificateInfo(TestReportMixin, InstructorTas user_enroll_mode, has_passed, whitelisted, - is_embargoed, verification_status, certificate_status, certificate_mode): @@ -1934,8 +1925,6 @@ class TestGradeReportEnrollmentAndCertificateInfo(TestReportMixin, InstructorTas CertificateWhitelistFactory.create(user=user, course_id=self.course.id, whitelist=whitelisted) - self.user_is_embargoed(user, is_embargoed) - if user_enroll_mode in CourseMode.VERIFIED_MODES: SoftwareSecurePhotoVerificationFactory.create(user=user, status=verification_status) @@ -1950,19 +1939,19 @@ class TestGradeReportEnrollmentAndCertificateInfo(TestReportMixin, InstructorTas @ddt.data( ( - 'verified', False, False, False, 'approved', 'notpassing', 'honor', + 'verified', False, False, 'approved', 'notpassing', 'honor', ['verified', 'ID Verified', 'N', 'N', 'N/A'] ), ( - 'verified', False, True, False, 'approved', 'downloadable', 'verified', + 'verified', False, True, 'approved', 'downloadable', 'verified', ['verified', 'ID Verified', 'Y', 'Y', 'verified'] ), ( - 'honor', True, True, True, 'approved', 'restricted', 'honor', - ['honor', 'N/A', 'N', 'N', 'N/A'] + 'honor', True, True, 'approved', 'restricted', 'honor', + ['honor', 'N/A', 'Y', 'N', 'N/A'] ), ( - 'verified', True, True, False, 'must_retry', 'downloadable', 'honor', + 'verified', True, True, 'must_retry', 'downloadable', 'honor', ['verified', 'Not ID Verified', 'Y', 'Y', 'honor'] ), ) @@ -1972,7 +1961,6 @@ class TestGradeReportEnrollmentAndCertificateInfo(TestReportMixin, InstructorTas user_enroll_mode, has_passed, whitelisted, - is_embargoed, verification_status, certificate_status, certificate_mode, @@ -1983,7 +1971,6 @@ class TestGradeReportEnrollmentAndCertificateInfo(TestReportMixin, InstructorTas user_enroll_mode, has_passed, whitelisted, - is_embargoed, verification_status, certificate_status, certificate_mode @@ -2034,7 +2021,7 @@ class TestCertificateGeneration(InstructorTaskModuleTestCase): 'failed': 0, 'skipped': 2 } - with self.assertNumQueries(174): + with self.assertNumQueries(169): self.assertCertificatesGenerated(task_input, expected_results) expected_results = {