Add error reporting to UI for CSV team management (#23035)

* Add error banner for upload memberships errors

* Edit error message language

* Fix linter warnings

common/djangoapps/third_party_auth/management/commands/tests/test_saml.py

@@ -11,7 +11,6 @@ import mock
 from django.conf import settings
 from django.core.management import call_command
 from django.core.management.base import CommandError
-from django.test import TestCase
 from django.utils.six import StringIO
 from requests import exceptions
 from requests.models import Response

lms/djangoapps/edxnotes/decorators.py

@@ -22,7 +22,9 @@ def edxnotes(cls):
         Returns raw html for the component.
         """
         # Import is placed here to avoid model import at project startup.
-        from edxnotes.helpers import generate_uid, get_edxnotes_id_token, get_public_endpoint, get_token_url, is_feature_enabled
+        from edxnotes.helpers import (
+            generate_uid, get_edxnotes_id_token, get_public_endpoint, get_token_url, is_feature_enabled
+        )
 
         runtime = getattr(self, 'descriptor', self).runtime
         if not hasattr(runtime, 'modulestore'):

lms/djangoapps/teams/csv.py

@@ -70,7 +70,7 @@ def _lookup_team_membership_data(course):
 def _group_teamset_memberships_by_user(course_team_memberships):
     """
     Parameters:
-        - course_team_memberships: a collection of CourseTeamMemberships
+        - course_team_memberships: a collection of CourseTeamMemberships.
 
     Returns:
         {
@@ -192,6 +192,10 @@ class TeamMembershipImportManager(object):
         return True
 
     def load_user_ids_by_teamset_id(self):
+        """
+        Get users associations with each teamset in a course and
+        save to `self.user_ids_by_teamset_id`
+        """
         for teamset_id in self.teamset_ids:
             self.user_ids_by_teamset_id[teamset_id] = {
                 membership.user_id for membership in
@@ -216,7 +220,7 @@ class TeamMembershipImportManager(object):
         Ensures that username exists only once in an input file
         """
         if username in usernames_found_so_far:
-            error_message = 'Username {} was found more than once in input file.'.format(username)
+            error_message = 'Username {} listed more than once in file.'.format(username)
             if self.add_error_and_check_if_max_exceeded(error_message):
                 return False
         return True
@@ -240,7 +244,7 @@ class TeamMembershipImportManager(object):
             except KeyError:
                 # if a team doesn't exists, the validation doesn't apply to it.
                 all_teamset_user_ids = self.user_ids_by_teamset_id[teamset_id]
-                error_message = 'The user {0} is already a member of a team inside teamset {1} in this course.'.format(
+                error_message = 'User {0} is already on a team in teamset {1}.'.format(
                     user.username, teamset_id
                 )
                 if user.id in all_teamset_user_ids and self.add_error_and_check_if_max_exceeded(error_message):
@@ -250,11 +254,11 @@ class TeamMembershipImportManager(object):
                     continue
             max_team_size = self.course.teams_configuration.default_max_team_size
             if max_team_size is not None and team.users.count() >= max_team_size:
-                if self.add_error_and_check_if_max_exceeded('Team ' + team.team_id + ' is already full.'):
+                if self.add_error_and_check_if_max_exceeded('Team ' + team.team_id + ' is full.'):
                     return False
 
             if (user.id, team.topic_id) in self.existing_course_team_memberships:
-                error_message = 'The user {0} is already a member of a team inside teamset {1} in this course.'.format(
+                error_message = 'User {0} is already on a team in teamset {1}.'.format(
                     user.username, team.topic_id
                 )
                 if self.add_error_and_check_if_max_exceeded(error_message):
@@ -318,6 +322,6 @@ class TeamMembershipImportManager(object):
             try:
                 return User.objects.get(email=user_name)
             except User.DoesNotExist:
-                self.validation_errors.append('Username or email ' + user_name + ' does not exist.')
+                self.validation_errors.append('User ' + user_name + ' does not exist.')
                 return None
                 # TODO - handle user key case

lms/djangoapps/teams/static/teams/js/views/manage.js

@@ -58,13 +58,15 @@
                 );
             },
 
-            handleCsvUploadSuccess: function() {
+            handleCsvUploadSuccess: function(data) {
+                TeamUtils.showInfoBanner(data.message, false);
+
                 // This handler is currently unimplemented (TODO MST-44)
                 this.teamEvents.trigger('teams:update', {});
             },
 
-            handleCsvUploadFailure: function() {
-                // This handler is currently unimplemented (TODO MST-44)
+            handleCsvUploadFailure: function(jqXHR) {
+                TeamUtils.showInfoBanner(jqXHR.responseJSON.errors, true);
             }
         });
         return ManageView;

lms/djangoapps/teams/static/teams/js/views/team_utils.js

@@ -72,6 +72,38 @@
                     return false;
                 }
                 return topicType.toLowerCase() !== 'open';
+            },
+
+            /** Shows info/error banner for team membership CSV upload
+             * @param: content - string or array for display
+             * @param: isError - true sets error styling, false/none uses info styling
+             */
+            showInfoBanner: function(content, isError) {
+                // clear message
+                var $message = $('#team-management-assign .page-banner .message-content');
+                $message.html('');
+
+                // set message
+                if (Array.isArray(content)) {
+                    content.forEach(function(item) {
+                        // xss-lint: disable=javascript-jquery-append
+                        $message.append($('<p>').text(item));
+                    });
+                } else {
+                    $('#team-management-assign .page-banner .message-content').text(content);
+                }
+
+                // set color sytling
+                $('#team-management-assign .page-banner .alert')
+                    .toggleClass('alert-success', !isError)
+                    .toggleClass('alert-danger', isError);
+
+                // set icon styling
+                $('#team-management-assign .page-banner .icon')
+                    .toggleClass('fa-check', !isError)
+                    .toggleClass('fa-warning', isError);
+
+                $('#team-management-assign .page-banner').show();
             }
         };
     });

lms/djangoapps/teams/static/teams/templates/manage.underscore

@@ -17,7 +17,7 @@
                 <%- gettext("Download Memberships") %>
             </button>
         </div>
-        <div class="team-management-section">
+        <div id="team-management-assign" class="team-management-section">
             <h3 class="team-detail-header">
                 <%- gettext("Assign Team Memberships") %>
             </h3>
@@ -37,6 +37,12 @@
                 <%- gettext("Upload Memberships") %>
             </div>
             <!-- We need to describe the format of the CSV here (TODO MST-49) -->
+            <div class="page-banner" hidden>
+                <div class="alert alert-danger" role="alert">
+                    <span class="icon icon-alert fa fa fa-warning" aria-hidden="true"></span>
+                    <div class="message-content"><%- gettext("Error assigning team memberships") %></div>
+                </div>
+            </div>
         </div>
     </div>
 </div>

lms/djangoapps/teams/views.py

@@ -11,12 +11,11 @@ from django.contrib.auth.models import User
 from django.core.exceptions import PermissionDenied
 from django.db.models.signals import post_save
 from django.dispatch import receiver
-from django.http import Http404, HttpResponse
+from django.http import Http404, HttpResponse, JsonResponse
 from django.shortcuts import get_object_or_404, render_to_response
 from django.utils.functional import cached_property
 from django.utils.translation import ugettext as _
 from django.utils.translation import ugettext_noop
-from django.views import View
 from django_countries import countries
 from edx_rest_framework_extensions.paginators import DefaultPagination, paginate_search_results
 from opaque_keys import InvalidKeyError
@@ -1387,13 +1386,16 @@ class MembershipBulkManagementView(GenericAPIView):
         Process uploaded CSV to modify team memberships for given course run.
         """
         self.check_access()
+
         inputfile_handle = request.FILES['csv']
         team_import_manager = TeamMembershipImportManager(self.course)
         team_import_manager.set_team_membership_from_csv(inputfile_handle)
+
         if team_import_manager.import_succeeded:
-            return Response(str(team_import_manager.number_of_records_added), status=status.HTTP_201_CREATED)
+            msg = "Successfully added {} students to teams".format(team_import_manager.number_of_records_added)
+            return JsonResponse({'message': msg}, status=status.HTTP_201_CREATED)
         else:
-            return Response({
+            return JsonResponse({
                 'errors': team_import_manager.validation_errors
             }, status=status.HTTP_400_BAD_REQUEST)
 

openedx/core/djangoapps/catalog/tests/test_models.py

@@ -3,7 +3,7 @@
 
 import ddt
 
-from django.test import TestCase, override_settings
+from django.test import override_settings
 
 from openedx.core.djangoapps.catalog.tests import mixins
 from openedx.core.djangoapps.site_configuration.tests.test_util import with_site_configuration

openedx/core/djangoapps/user_api/models.py

@@ -92,7 +92,7 @@ def pre_save_callback(sender, **kwargs):
     Event changes to user preferences.
     """
     user_preference = kwargs["instance"]
-    user_preference._old_value = get_changed_fields_dict(user_preference, sender).get("value", None)
+    user_preference._old_value = get_changed_fields_dict(user_preference, sender).get("value", None)  # pylint: disable=protected-access
 
 
 @receiver(post_save, sender=UserPreference)
@@ -107,9 +107,9 @@ def post_save_callback(sender, **kwargs):
             username=user_preference.user.username, new=user_preference.value))
     emit_setting_changed_event(
         user_preference.user, sender._meta.db_table, user_preference.key,
-        user_preference._old_value, user_preference.value
+        user_preference._old_value, user_preference.value  # pylint: disable=protected-access
     )
-    user_preference._old_value = None
+    user_preference._old_value = None  # pylint: disable=protected-access
 
 
 @receiver(post_delete, sender=UserPreference)
@@ -139,7 +139,7 @@ class UserCourseTag(models.Model):
         unique_together = ("user", "course_id", "key")
 
 
-class UserOrgTag(TimeStampedModel, DeletableByUserValue):  # pylint: disable=model-missing-unicode
+class UserOrgTag(TimeStampedModel, DeletableByUserValue):
     """
     Per-Organization user tags.
 
@@ -172,7 +172,6 @@ class RetirementState(models.Model):
     required = models.BooleanField(default=False)
 
     def __str__(self):
-        # pylint: disable=unicode-format-string
         return '{} (step {})'.format(self.state_name, self.state_execution_order)
 
     class Meta(object):
@@ -293,7 +292,9 @@ class UserRetirementStatus(TimeStampedModel):
             if new_state_index <= states.index(self.current_state.state_name):
                 raise ValueError()
         except ValueError:
-            err = u'{} does not exist or is an eariler state than current state {}'.format(new_state, self.current_state)
+            err = u'{} does not exist or is an eariler state than current state {}'.format(
+                new_state, self.current_state
+            )
             raise RetirementStateError(err)
 
     def _validate_update_data(self, data):
@@ -306,7 +307,9 @@ class UserRetirementStatus(TimeStampedModel):
 
         for required_key in required_keys:
             if required_key not in data:
-                raise RetirementStateError(u'RetirementStatus: Required key {} missing from update'.format(required_key))
+                raise RetirementStateError(u'RetirementStatus: Required key {} missing from update'.format(
+                    required_key
+                ))
 
         for key in data:
             if key not in known_keys: