Merge pull request #294 from edx/varshamenon4/fix-handle-dupe-course-…

…staff-mgmt-command

fix: handle dupe course staff mgmt command

edx_exams/apps/core/management/commands/bulk_add_course_staff.py

@@ -63,32 +63,34 @@ 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)
+
+        # bulk create course staff
+        # for i in range(0, len(reader), batch_size):
+        #     CourseStaffRole.objects.bulk_create(
+        #         (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]),
+        #         ignore_conflicts=True,
+        #     )
+        #     time.sleep(batch_delay)

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,[email protected],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,[email protected],staff,course-v1:edx+test+f20\n',
+                 'sam,[email protected],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,[email protected],staff,course-v1:edx+test+f20\n',
+                 'sam,[email protected],staff,course-v1:edx+test+f20\n'
+                 'tam,[email protected],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', '[email protected]'
         username2, email2 = 'cam', '[email protected]'
@@ -106,16 +125,16 @@ 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)
         """
         num_lines = 20
         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)