From 973b522f341fb21b1474327d234bbedf67de21fa Mon Sep 17 00:00:00 2001 From: Varsha Menon Date: Wed, 24 Jul 2024 10:41:55 -0400 Subject: [PATCH] fix: handle dupes course staff mgmt command --- .../commands/bulk_add_course_staff.py | 24 +++----- .../test/test_bulk_add_course_staff.py | 58 +++++++++++++++++-- 2 files changed, 62 insertions(+), 20 deletions(-) diff --git a/edx_exams/apps/core/management/commands/bulk_add_course_staff.py b/edx_exams/apps/core/management/commands/bulk_add_course_staff.py index 23204ea0..2f3c3b71 100644 --- a/edx_exams/apps/core/management/commands/bulk_add_course_staff.py +++ b/edx_exams/apps/core/management/commands/bulk_add_course_staff.py @@ -63,32 +63,26 @@ def add_course_staff_from_csv(self, csv_file, batch_size, batch_delay): Add the given set of course staff provided in csv """ reader = list(csv.DictReader(csv_file)) - users_to_create = [] - users_existing = {u.username for u in User.objects.filter(username__in=[r.get('username') for r in reader])} - for row in reader: - if row.get('username') not in users_existing: - users_to_create.append(row) - users_existing.add(row.get('username')) # bulk create users - for i in range(0, len(users_to_create), batch_size): + for i in range(0, len(reader), batch_size): User.objects.bulk_create( - User( - username=user.get('username'), - email=user.get('email'), - ) - for user in users_to_create[i:i + batch_size] + (User( + username=row.get('username'), + email=row.get('email'), + ) for row in reader[i:i + batch_size]), + ignore_conflicts=True, ) time.sleep(batch_delay) # bulk create course staff for i in range(0, len(reader), batch_size): CourseStaffRole.objects.bulk_create( - CourseStaffRole( + (CourseStaffRole( user=User.objects.get(username=row.get('username')), course_id=row.get('course_id'), role=row.get('role'), - ) - for row in reader[i:i + batch_size] + ) for row in reader[i:i + batch_size]), + ignore_conflicts=True, ) time.sleep(batch_delay) diff --git a/edx_exams/apps/core/management/commands/test/test_bulk_add_course_staff.py b/edx_exams/apps/core/management/commands/test/test_bulk_add_course_staff.py index 0b6e80e7..023974df 100644 --- a/edx_exams/apps/core/management/commands/test/test_bulk_add_course_staff.py +++ b/edx_exams/apps/core/management/commands/test/test_bulk_add_course_staff.py @@ -5,7 +5,7 @@ from django.test import TestCase from edx_exams.apps.core.models import CourseStaffRole, User -from edx_exams.apps.core.test_utils.factories import UserFactory +from edx_exams.apps.core.test_utils.factories import CourseStaffRoleFactory, UserFactory class TestBulkAddCourseStaff(TestCase): @@ -90,9 +90,28 @@ def test_add_course_staff_with_not_default_batch_size(self): 'sam,sam@pond.com,staff,course-v1:edx+test+f20\n'] with NamedTemporaryFile() as csv: csv = self._write_test_csv(csv, lines) - with self.assertNumQueries(9): + with self.assertNumQueries(8): call_command(self.command, f'--csv_path={csv.name}', '--batch_size=1') + def test_add_course_staff_with_batch_size_larger_than_list(self): + """ Assert that the number of queries is correct given batch size larger than lines """ + lines = ['pam,pam@pond.com,staff,course-v1:edx+test+f20\n', + 'sam,sam@pond.com,staff,course-v1:edx+test+f20\n'] + with NamedTemporaryFile() as csv: + csv = self._write_test_csv(csv, lines) + with self.assertNumQueries(6): + call_command(self.command, f'--csv_path={csv.name}', '--batch_size=3') + + def test_add_course_staff_with_batch_size_smaller_than_list(self): + """ Assert that the number of queries is correct given batch size smaller than lines """ + lines = ['pam,pam@pond.com,staff,course-v1:edx+test+f20\n', + 'sam,sam@pond.com,staff,course-v1:edx+test+f20\n' + 'tam,tam@pond.com,staff,course-v1:edx+test+f20\n'] + with NamedTemporaryFile() as csv: + csv = self._write_test_csv(csv, lines) + with self.assertNumQueries(9): + call_command(self.command, f'--csv_path={csv.name}', '--batch_size=2') + def test_add_course_staff_with_not_default_batch_delay(self): username, email = 'pam', 'pam@pond.com' username2, email2 = 'cam', 'cam@pond.com' @@ -106,8 +125,8 @@ def test_add_course_staff_with_not_default_batch_delay(self): def test_num_queries_correct(self): """ - Assert the number of queries to be 5 + 1 * number of lines: - 2 for savepoint/release savepoint, 1 to get existing usernames, + Assert the number of queries to be 4 + 1 * number of lines: + 2 for savepoint/release savepoint 1 to bulk create users, 1 to bulk create course role 1 for each user (to get user) """ @@ -115,7 +134,7 @@ def test_num_queries_correct(self): lines = [f'pam{i},pam{i}@pond.com,staff,course-v1:edx+test+f20\n' for i in range(num_lines)] with NamedTemporaryFile() as csv: csv = self._write_test_csv(csv, lines) - with self.assertNumQueries(5 + num_lines): + with self.assertNumQueries(4 + num_lines): call_command(self.command, f'--csv_path={csv.name}') def test_dupe_user_csv(self): @@ -130,3 +149,32 @@ def test_dupe_user_csv(self): call_command(self.command, f'--csv_path={csv.name}') self._assert_user_and_role(username, email, self.course_role, self.course_id) self._assert_user_and_role(username, email, self.course_role, course_id_2) + + def test_existing_course_staff_csv(self): + """ Assert that the course staff role are correctly created given already existing course staff roles in csv """ + course_existing = 'course-v1:edx+test+f24' + CourseStaffRoleFactory.create( + user=self.user, + course_id=course_existing, + role=self.course_role, + ) + lines = [f'{self.user.username},{self.user.email},{self.course_role},{course_existing}\n'] + with NamedTemporaryFile() as csv: + csv = self._write_test_csv(csv, lines) + call_command(self.command, f'--csv_path={csv.name}') + self._assert_user_and_role(self.user.username, self.user.email, self.course_role, course_existing) + + def test_dupe_course_staff_csv(self): + """ Assert that the course staff role are correctly created given dupe course staff roles in csv """ + course_existing = 'course-v1:edx+test+f24' + CourseStaffRoleFactory.create( + user=self.user, + course_id=course_existing, + role=self.course_role, + ) + lines = [f'{self.user.username},{self.user.email},{self.course_role},{course_existing}\n', + f'{self.user.username},{self.user.email},{self.course_role},{course_existing}\n'] + with NamedTemporaryFile() as csv: + csv = self._write_test_csv(csv, lines) + call_command(self.command, f'--csv_path={csv.name}') + self._assert_user_and_role(self.user.username, self.user.email, self.course_role, course_existing)