From 629b7721724520a98ca66d00a962b63135fa3daa Mon Sep 17 00:00:00 2001 From: atesker Date: Thu, 20 Feb 2020 13:17:53 -0500 Subject: [PATCH] EDUCATOR-4916 - additional error handling --- lms/djangoapps/teams/csv.py | 45 ++++++++++++++-- lms/djangoapps/teams/tests/test_views.py | 68 ++++++++++++++++++++---- 2 files changed, 99 insertions(+), 14 deletions(-) diff --git a/lms/djangoapps/teams/csv.py b/lms/djangoapps/teams/csv.py index fa90d622fd..5e67358981 100644 --- a/lms/djangoapps/teams/csv.py +++ b/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. diff --git a/lms/djangoapps/teams/tests/test_views.py b/lms/djangoapps/teams/tests/test_views.py index e492a38d33..7a16e19eb2 100644 --- a/lms/djangoapps/teams/tests/test_views.py +++ b/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' + )