EDUCATOR-4916 - additional error handling

lms/djangoapps/teams/csv.py

@@ -125,6 +125,8 @@ class TeamMembershipImportManager(object):
         self.teamset_ids = reader.fieldnames[2:]
         row_dictionaries = []
         csv_usernames = set()
+        if not self.validate_header(reader.fieldnames):
+            return False
         if not self.validate_teamsets():
             return False
         self.load_user_ids_by_teamset_id()
@@ -132,6 +134,8 @@ class TeamMembershipImportManager(object):
         self.load_course_teams()
         # process student rows:
         for row in reader:
+            if not self.validate_teams_have_matching_teamsets(row):
+                return False
             username = row['user']
             if not username:
                 continue
@@ -141,7 +145,7 @@ class TeamMembershipImportManager(object):
             user = self.get_user(username)
             if user is None:
                 continue
-            if not self.validate_user_enrolled_in_course(user):
+            if not self.validate_user_enrollment_is_valid(user, row['mode']):
                 row['user'] = None
                 continue
             row['user'] = user
@@ -173,6 +177,19 @@ class TeamMembershipImportManager(object):
         for team in CourseTeam.objects.filter(course_id=self.course.id):
             self.existing_course_teams[(team.name, team.topic_id)] = team
 
+    def validate_header(self, header):
+        """
+        Validates header row to ensure that it contains at a minimum columns called 'user', 'mode'.
+        Teamset validation is handled separately
+        """
+        if 'user' not in header:
+            self.validation_errors.append("Header must contain column 'user'.")
+            return False
+        if 'mode' not in header:
+            self.validation_errors.append("Header must contain column 'mode'.")
+            return False
+        return True
+
     def validate_teamsets(self):
         """
         Validates team set ids. Returns true if there are no errors.
@@ -205,14 +222,19 @@ class TeamMembershipImportManager(object):
                 )
             }
 
-    def validate_user_enrolled_in_course(self, user):
+    def validate_user_enrollment_is_valid(self, user, supplied_enrollment):
         """
         Invalid states:
             user not enrolled in course
+            enrollment mode from csv doesn't match actual user enrollment
         """
-        if not CourseEnrollment.is_enrolled(user, self.course.id):
+        actual_enrollment_mode, user_enrolled = CourseEnrollment.enrollment_mode_for_user(user, self.course.id)
+        if not user_enrolled:
             self.validation_errors.append('User ' + user.username + ' is not enrolled in this course.')
             return False
+        if actual_enrollment_mode != supplied_enrollment.strip():
+            self.validation_errors.append('User ' + user.username + ' enrollment mismatch.')
+            return False
 
         return True
 
@@ -226,6 +248,23 @@ class TeamMembershipImportManager(object):
                 return False
         return True
 
+    def validate_teams_have_matching_teamsets(self, row):
+        """
+        It's possible for a user to create a row that has more team names in it
+        than there are teamset ids provided in the header.
+        In that case, `row` will have one or more null keys mapping to team names, for example:
+        {'teamset-1': 'team-a', 'teamset-2': 'team-beta', None: 'team-37'}
+
+        This method will add a validation error and return False if this is the case.
+        """
+        if None in row:
+            error_message = "Team(s) {0} don't have matching teamsets.".format(
+                row[None]
+            )
+            if self.add_error_and_check_if_max_exceeded(error_message):
+                return False
+        return True
+
     def validate_user_assignment_to_team_and_teamset(self, row):
         """
         Validates a user entry relative to an existing team.

lms/djangoapps/teams/tests/test_views.py

@@ -1712,7 +1712,7 @@ class TestBulkMembershipManagement(TeamAPITestCase):
     def test_upload_valid_csv_simple(self):
         self.create_and_enroll_student(username='a_user')
         csv_content = 'user,mode,topic_0' + '\n'
-        csv_content += 'a_user, masters, team wind power'
+        csv_content += 'a_user,audit,team wind power'
         csv_file = SimpleUploadedFile('test_file.csv', csv_content.encode('utf8'), content_type='text/csv')
         self.client.login(username=self.users['course_staff'].username, password=self.users['course_staff'].password)
         self.make_call(reverse('team_membership_bulk_management', args=[self.good_course_id]),
@@ -1723,7 +1723,7 @@ class TestBulkMembershipManagement(TeamAPITestCase):
     def test_upload_invalid_teamset(self):
         self.create_and_enroll_student(username='a_user')
         csv_content = 'user,mode,topic_0_bad' + '\n'
-        csv_content += 'a_user, masters, team wind power'
+        csv_content += 'a_user,audit,team wind power'
         csv_file = SimpleUploadedFile('test_file.csv', csv_content.encode('utf8'), content_type='text/csv')
         self.client.login(username=self.users['course_staff'].username, password=self.users['course_staff'].password)
         self.make_call(reverse('team_membership_bulk_management', args=[self.good_course_id]),
@@ -1746,9 +1746,9 @@ class TestBulkMembershipManagement(TeamAPITestCase):
         self.create_and_enroll_student(username='b_user')
         self.create_and_enroll_student(username='c_user')
         csv_content = 'user,mode,topic_0,topic_1,topic_2' + '\n'
-        csv_content += 'a_user, masters,team wind power,team 2' + '\n'
-        csv_content += 'b_user, masters,,team 2' + '\n'
-        csv_content += 'c_user, masters,,,team 3'
+        csv_content += 'a_user,audit,team wind power,team 2' + '\n'
+        csv_content += 'b_user,audit,,team 2' + '\n'
+        csv_content += 'c_user,audit,,,team 3'
         csv_file = SimpleUploadedFile('test_file.csv', csv_content.encode('utf8'), content_type='text/csv')
         self.client.login(username=self.users['course_staff'].username, password=self.users['course_staff'].password)
         self.make_call(reverse('team_membership_bulk_management', args=[self.good_course_id]),
@@ -1771,17 +1771,25 @@ class TestBulkMembershipManagement(TeamAPITestCase):
                        )
 
     def test_upload_only_existing_courses(self):
-        self.create_and_enroll_student(username='a_user')
-        self.create_and_enroll_student(username='b_user')
-        existing_team_1 = CourseTeamFactory.create(course_id=self.test_course_1.id, topic_id='topic_1')
-        existing_team_2 = CourseTeamFactory.create(course_id=self.test_course_1.id, topic_id='topic_2')
+        self.create_and_enroll_student(username='a_user', mode=CourseMode.MASTERS)
+        self.create_and_enroll_student(username='b_user', mode=CourseMode.MASTERS)
+        existing_team_1 = CourseTeamFactory.create(
+            course_id=self.test_course_1.id,
+            topic_id='topic_1',
+            organization_protected=True
+        )
+        existing_team_2 = CourseTeamFactory.create(
+            course_id=self.test_course_1.id,
+            topic_id='topic_2',
+            organization_protected=True
+        )
 
         csv_content = 'user,mode,topic_1,topic_2' + '\n'
-        csv_content += 'a_user, masters,{},{}'.format(
+        csv_content += 'a_user,masters,{},{}'.format(
             existing_team_1.name,
             existing_team_2.name
         ) + '\n'
-        csv_content += 'b_user, masters,{},{}'.format(
+        csv_content += 'b_user,masters,{},{}'.format(
             existing_team_1.name,
             existing_team_2.name
         ) + '\n'
@@ -1794,3 +1802,41 @@ class TestBulkMembershipManagement(TeamAPITestCase):
             data={'csv': csv_file},
             user='staff'
         )
+
+    def test_upload_invalid_header(self):
+        self.create_and_enroll_student(username='a_user')
+        csv_content = 'mode,topic_1' + '\n'
+        csv_content += 'a_user,audit, team wind power'
+        csv_file = SimpleUploadedFile('test_file.csv', csv_content.encode('utf8'), content_type='text/csv')
+        self.client.login(username=self.users['course_staff'].username, password=self.users['course_staff'].password)
+        self.make_call(reverse(
+            'team_membership_bulk_management', args=[self.good_course_id]),
+            400, method='post',
+            data={'csv': csv_file}, user='staff'
+        )
+
+    def test_upload_invalid_more_teams_than_teamsets(self):
+        self.create_and_enroll_student(username='a_user')
+        csv_content = 'user,mode,topic_1' + '\n'
+        csv_content += 'a_user, masters, team wind power, extra1, extra2'
+        csv_file = SimpleUploadedFile('test_file.csv', csv_content.encode('utf8'), content_type='text/csv')
+        self.client.login(username=self.users['course_staff'].username, password=self.users['course_staff'].password)
+        self.make_call(reverse(
+            'team_membership_bulk_management',
+            args=[self.good_course_id]),
+            400, method='post',
+            data={'csv': csv_file}, user='staff'
+        )
+
+    def test_upload_invalid_student_enrollment_mismatch(self):
+        self.create_and_enroll_student(username='a_user', mode=CourseMode.AUDIT)
+        csv_content = 'user,mode,topic_1' + '\n'
+        csv_content += 'a_user,masters,team wind power'
+        csv_file = SimpleUploadedFile('test_file.csv', csv_content.encode('utf8'), content_type='text/csv')
+        self.client.login(username=self.users['course_staff'].username, password=self.users['course_staff'].password)
+        self.make_call(reverse(
+            'team_membership_bulk_management',
+            args=[self.good_course_id]),
+            400, method='post',
+            data={'csv': csv_file}, user='staff'
+        )