Merge pull request #26622 from edx/tuchfarber/remove_pii_from_mgmt_cmd_cfg

Update mgmt cmds to use user_ids instead of PII

lms/djangoapps/certificates/api.py

@@ -160,7 +160,7 @@ def get_certificates_for_user_by_course_keys(user, course_keys):
     }
 
 
-def get_recently_modified_certificates(course_keys=None, start_date=None, end_date=None, usernames=None):
+def get_recently_modified_certificates(course_keys=None, start_date=None, end_date=None, user_ids=None):
     """
     Returns a QuerySet of GeneratedCertificate objects filtered by the input
     parameters and ordered by modified_date.
@@ -176,8 +176,8 @@ def get_recently_modified_certificates(course_keys=None, start_date=None, end_da
     if end_date:
         cert_filter_args['modified_date__lte'] = end_date
 
-    if usernames:
-        cert_filter_args['user__username__in'] = usernames
+    if user_ids:
+        cert_filter_args['user__id__in'] = user_ids
 
     return GeneratedCertificate.objects.filter(**cert_filter_args).order_by('modified_date')
 

lms/djangoapps/certificates/management/commands/cert_allowlist_generation.py

@@ -22,7 +22,7 @@ class Command(BaseCommand):
     Management command to generate allowlist certificates for one or more users in a given course run.
 
     Example usage:
-    ./manage.py lms cert_allowlist_generation -u edx verified -c course-v1:edX+DemoX+Demo_Course
+    ./manage.py lms cert_allowlist_generation -u 123 verified -c course-v1:edX+DemoX+Demo_Course
     """
 
     help = """
@@ -35,7 +35,7 @@ class Command(BaseCommand):
             nargs="+",
             metavar='USER',
             dest='user',
-            help='user or space-separated list of users for whom to generate allowlist certificates'
+            help='user_id or space-separated list of user_ids for whom to generate allowlist certificates'
         )
         parser.add_argument(
             '-c', '--course-key',
@@ -80,8 +80,12 @@ class Command(BaseCommand):
 
         # Loop over each user, and ask that a cert be generated for them
         users_str = options['user']
-        for user_identifier in users_str:
-            user = _get_user_from_identifier(user_identifier)
+        for user_id in users_str:
+            user = None
+            try:
+                user = User.objects.get(id=user_id)
+            except User.DoesNotExist:
+                log.warning('User {user} could not be found'.format(user=user_id))
             if user is not None:
                 log.info(
                     'Calling generate_allowlist_certificate_task for {user} : {course}'.format(
@@ -89,18 +93,3 @@ class Command(BaseCommand):
                         course=course_key
                     ))
                 generate_allowlist_certificate_task(user, course_key)
-
-
-def _get_user_from_identifier(identifier):
-    """
-    Using the string identifier, fetch the relevant user object from database
-    """
-    try:
-        if '@' in identifier:
-            user = User.objects.get(email=identifier)
-        else:
-            user = User.objects.get(username=identifier)
-        return user
-    except User.DoesNotExist:
-        log.warning('User {user} could not be found'.format(user=identifier))
-        return None

lms/djangoapps/certificates/management/commands/tests/test_cert_allowlist_generation.py

@@ -71,7 +71,7 @@ class CertAllowlistGenerationTests(ModuleStoreTestCase):
         """
         Test generation for 1 user
         """
-        call_command("cert_allowlist_generation", "--u", self.user.username, "--c", self.course_run_key)
+        call_command("cert_allowlist_generation", "--u", self.user.id, "--c", self.course_run_key)
 
     def test_successful_generation_multiple_users(self):
         """
@@ -79,9 +79,8 @@ class CertAllowlistGenerationTests(ModuleStoreTestCase):
         """
         call_command("cert_allowlist_generation",
                      "--u",
-                     self.user.username,
-                     self.user2.email,
-                     "invalid_user",
-                     " ",
+                     self.user.id,
+                     self.user2.id,
+                     "999999",  # non-existant userid
                      "--c",
                      self.course_run_key)

lms/djangoapps/certificates/migrations/0020_remove_existing_mgmt_cmd_args.py

@@ -0,0 +1,21 @@
+# Generated by Django 2.2.18 on 2021-02-18 22:05
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+    """
+    Delete all existing AllowListGenerationConfiguration entries because they may contain PII prior to this change
+    """
+
+    def remove_existing_configs(apps, schema_editor):
+        AllowListGenerationConfiguration = apps.get_model('certificates', 'AllowListGenerationConfiguration')
+        AllowListGenerationConfiguration.objects.all().delete()
+
+    dependencies = [
+        ('certificates', '0019_allowlistgenerationconfiguration'),
+    ]
+
+    operations = [
+        migrations.RunPython(remove_existing_configs)
+    ]

openedx/core/djangoapps/credentials/management/commands/notify_credentials.py

@@ -162,7 +162,7 @@ class Command(BaseCommand):
             help='Send program award notifications with course notification tasks',
         )
         parser.add_argument(
-            '--usernames',
+            '--user_ids',
             default=None,
             nargs='+',
             help='Run the command for the given user or list of users',
@@ -190,7 +190,7 @@ class Command(BaseCommand):
 
         log.info(
             u"notify_credentials starting, dry-run=%s, site=%s, delay=%d seconds, page_size=%d, "
-            u"from=%s, to=%s, notify_programs=%s, usernames=%s, execution=%s",
+            u"from=%s, to=%s, notify_programs=%s, user_ids=%s, execution=%s",
             options['dry_run'],
             options['site'],
             options['delay'],
@@ -198,7 +198,7 @@ class Command(BaseCommand):
             options['start_date'] if options['start_date'] else 'NA',
             options['end_date'] if options['end_date'] else 'NA',
             options['notify_programs'],
-            options['usernames'],
+            options['user_ids'],
             'auto' if options['auto'] else 'manual',
         )
 
@@ -208,16 +208,16 @@ class Command(BaseCommand):
             log.error(u'No site configuration found for site %s', options['site'])
 
         course_keys = self.get_course_keys(options['courses'])
-        if not (course_keys or options['start_date'] or options['end_date'] or options['usernames']):
-            raise CommandError('You must specify a filter (e.g. --courses= or --start-date or --usernames)')
+        if not (course_keys or options['start_date'] or options['end_date'] or options['user_ids']):
+            raise CommandError('You must specify a filter (e.g. --courses= or --start-date or --user_ids)')
 
         certs = get_recently_modified_certificates(
-            course_keys, options['start_date'], options['end_date'], options['usernames']
+            course_keys, options['start_date'], options['end_date'], options['user_ids']
         )
 
         users = None
-        if options['usernames']:
-            users = User.objects.filter(username__in=options['usernames'])
+        if options['user_ids']:
+            users = User.objects.filter(id__in=options['user_ids'])
         grades = get_recently_modified_grades(
             course_keys, options['start_date'], options['end_date'], users
         )

openedx/core/djangoapps/credentials/management/commands/tests/test_notify_credentials.py

@@ -126,7 +126,7 @@ class TestNotifyCredentials(TestCase):
     @mock.patch(COMMAND_MODULE + '.Command.send_notifications')
     def test_username_arg(self, mock_send):
         call_command(
-            Command(), '--start-date', '2017-02-01', '--end-date', '2017-02-02', '--usernames', self.user2.username
+            Command(), '--start-date', '2017-02-01', '--end-date', '2017-02-02', '--user_ids', self.user2.id
         )
         assert mock_send.called
         self.assertListEqual(list(mock_send.call_args[0][0]), [self.cert4])
@@ -134,7 +134,7 @@ class TestNotifyCredentials(TestCase):
         mock_send.reset_mock()
 
         call_command(
-            Command(), '--usernames', self.user2.username
+            Command(), '--user_ids', self.user2.id
         )
         assert mock_send.called
         self.assertListEqual(list(mock_send.call_args[0][0]), [self.cert4])
@@ -142,7 +142,7 @@ class TestNotifyCredentials(TestCase):
         mock_send.reset_mock()
 
         call_command(
-            Command(), '--start-date', '2017-02-01', '--end-date', '2017-02-02', '--usernames', self.user.username
+            Command(), '--start-date', '2017-02-01', '--end-date', '2017-02-02', '--user_ids', self.user.id
         )
         assert mock_send.called
         self.assertListEqual(list(mock_send.call_args[0][0]), [self.cert2])
@@ -150,7 +150,7 @@ class TestNotifyCredentials(TestCase):
         mock_send.reset_mock()
 
         call_command(
-            Command(), '--usernames', self.user.username
+            Command(), '--user_ids', self.user.id
         )
         assert mock_send.called
         self.assertListEqual(list(mock_send.call_args[0][0]), [self.cert1, self.cert2, self.cert3])
@@ -158,7 +158,7 @@ class TestNotifyCredentials(TestCase):
         mock_send.reset_mock()
 
         call_command(
-            Command(), '--usernames', self.user.username, self.user2.username
+            Command(), '--user_ids', self.user.id, self.user2.id
         )
         assert mock_send.called
         self.assertListEqual(list(mock_send.call_args[0][0]), [self.cert1, self.cert2, self.cert4, self.cert3])

openedx/core/djangoapps/credentials/migrations/0005_remove_existing_mgmt_cmd_args.py

@@ -0,0 +1,20 @@
+# Generated by Django 2.2.18 on 2021-02-18 22:01
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+    """
+    Delete all existing NotifyCredentialsConfig entries because they may contain PII prior to this change
+    """
+    def remove_existing_configs(apps, schema_editor):
+        NotifyCredentialsConfig = apps.get_model('credentials', 'NotifyCredentialsConfig')
+        NotifyCredentialsConfig.objects.all().delete()
+
+    dependencies = [
+        ('credentials', '0004_notifycredentialsconfig'),
+    ]
+
+    operations = [
+        migrations.RunPython(remove_existing_configs)
+    ]