Code cleanup due to review comments

* make `user_status` more intelligent
* remove some logic from the templates
* rename `parse_error_msg` to `parsed_error_msg`
* fix up and add more tests

LMS-1387

cms/envs/common.py

@@ -418,7 +418,6 @@ MKTG_URL_LINK_MAP = {
 
 COURSES_WITH_UNSAFE_CODE = []
 
-
 ############################## EVENT TRACKING #################################
 
 TRACK_MAX_EVENT = 10000

common/djangoapps/student/views.py

@@ -25,14 +25,13 @@ from django.core.validators import validate_email, validate_slug, ValidationErro
 from django.core.exceptions import ObjectDoesNotExist
 from django.db import IntegrityError, transaction
 from django.http import (HttpResponse, HttpResponseBadRequest, HttpResponseForbidden,
-                         HttpResponseNotAllowed, Http404)
+                         Http404)
 from django.shortcuts import redirect
 from django_future.csrf import ensure_csrf_cookie
-from django.utils.http import cookie_date, base36_to_int, urlencode
+from django.utils.http import cookie_date, base36_to_int
 from django.utils.translation import ugettext as _
 from django.views.decorators.http import require_POST, require_GET
 from django.contrib.admin.views.decorators import staff_member_required
-from django.utils.translation import ugettext as _u
 
 from ratelimitbackend.exceptions import RateLimitException
 
@@ -337,7 +336,7 @@ def dashboard(request):
         )
     )
     # Verification Attempts
-    verification_status = SoftwareSecurePhotoVerification.user_status(user)
+    verification_status, verification_msg = SoftwareSecurePhotoVerification.user_status(user)
     # get info w.r.t ExternalAuthMap
     external_auth_map = None
     try:
@@ -355,8 +354,8 @@ def dashboard(request):
                'all_course_modes': course_modes,
                'cert_statuses': cert_statuses,
                'show_email_settings_for': show_email_settings_for,
-               'verification_status': verification_status[0],
-               'verification_msg': verification_status[1],
+               'verification_status': verification_status,
+               'verification_msg': verification_msg,
                }
 
     return render_to_response('dashboard.html', context)
@@ -663,11 +662,11 @@ def manage_user_standing(request):
         row = [user.username, user.standing.all()[0].changed_by]
         rows.append(row)
 
-
     context = {'headers': headers, 'rows': rows}
 
     return render_to_response("manage_user_standing.html", context)
 
+
 @require_POST
 @login_required
 @ensure_csrf_cookie
@@ -681,34 +680,34 @@ def disable_account_ajax(request):
     username = request.POST.get('username')
     context = {}
     if username is None or username.strip() == '':
-        context['message'] = _u('Please enter a username')
+        context['message'] = _('Please enter a username')
         return JsonResponse(context, status=400)
 
     account_action = request.POST.get('account_action')
     if account_action is None:
-        context['message'] = _u('Please choose an option')
+        context['message'] = _('Please choose an option')
         return JsonResponse(context, status=400)
 
     username = username.strip()
     try:
         user = User.objects.get(username=username)
     except User.DoesNotExist:
-        context['message'] = _u("User with username {} does not exist").format(username)
+        context['message'] = _("User with username {} does not exist").format(username)
         return JsonResponse(context, status=400)
     else:
-        user_account, _ = UserStanding.objects.get_or_create(
+        user_account, _succss = UserStanding.objects.get_or_create(
             user=user, defaults={'changed_by': request.user},
         )
         if account_action == 'disable':
             user_account.account_status = UserStanding.ACCOUNT_DISABLED
-            context['message'] = _u("Successfully disabled {}'s account").format(username)
+            context['message'] = _("Successfully disabled {}'s account").format(username)
             log.info("{} disabled {}'s account".format(request.user, username))
         elif account_action == 'reenable':
             user_account.account_status = UserStanding.ACCOUNT_ENABLED
-            context['message'] = _u("Successfully reenabled {}'s account").format(username)
+            context['message'] = _("Successfully reenabled {}'s account").format(username)
             log.info("{} reenabled {}'s account".format(request.user, username))
         else:
-            context['message'] = _u("Unexpected account status")
+            context['message'] = _("Unexpected account status")
             return JsonResponse(context, status=400)
         user_account.changed_by = request.user
         user_account.standing_last_changed_at = datetime.datetime.now(UTC)

lms/djangoapps/verify_student/models.py

@@ -115,7 +115,6 @@ class PhotoVerification(StatusModel):
         attempt.status == "created"
         pending_requests = PhotoVerification.submitted.all()
     """
-
     ######################## Fields Set During Creation ########################
     # See class docstring for description of status states
     STATUS = Choices('created', 'ready', 'submitted', 'must_retry', 'approved', 'denied')
@@ -233,27 +232,44 @@ class PhotoVerification(StatusModel):
     @classmethod
     def user_status(cls, user):
         """
-        Returns the status of the user based on their latest verification attempt
+        Returns the status of the user based on their past verification attempts
 
         If no such verification exists, returns 'none'
         If verification has expired, returns 'expired'
-        """
-        try:
-            attempts = cls.objects.filter(user=user).order_by('-updated_at')
-            attempt = attempts[0]
-        except IndexError:
-            return ('none', '')
-
-        if attempt.created_at < cls._earliest_allowed_date():
-            return ('expired', '')
-
-        error_msg = attempt.error_msg
-        if attempt.error_msg:
-            error_msg = attempt.parse_error_msg()
-
-        return (attempt.status, error_msg)
-
-    def parse_error_msg(self):
+        If the verification has been approved, returns 'approved'
+        If the verification process is still ongoing, returns 'pending'
+        If the verification has been denied and the user must resubmit photos, returns 'must_reverify'
+        """
+        status = 'none'
+        error_msg = ''
+
+        if cls.user_is_verified(user):
+            status = 'approved'
+        elif cls.user_has_valid_or_pending(user):
+            # user_has_valid_or_pending does include 'approved', but if we are
+            # here, we know that the attempt is still pending
+            status = 'pending'
+        else:
+            # we need to check the most recent attempt to see if we need to ask them to do
+            # a retry
+            try:
+                attempts = cls.objects.filter(user=user).order_by('-updated_at')
+                attempt = attempts[0]
+            except IndexError:
+                return ('none', error_msg)
+            if attempt.created_at < cls._earliest_allowed_date():
+                return ('expired', error_msg)
+
+            # right now, this is the only state at which they must reverify. It
+            # may change later
+            if attempt.status == 'denied':
+                status = 'must_reverify'
+            if attempt.error_msg:
+                error_msg = attempt.parsed_error_msg()
+
+        return (status, error_msg)
+
+    def parsed_error_msg(self):
         """
         Sometimes, the error message we've received needs to be parsed into
         something more human readable
@@ -523,7 +539,7 @@ class SoftwareSecurePhotoVerification(PhotoVerification):
             self.status = "must_retry"
             self.save()
 
-    def parse_error_msg(self):
+    def parsed_error_msg(self):
         """
         Parse the error messages we receive from SoftwareSecure
 

lms/djangoapps/verify_student/tests/test_models.py

@@ -321,7 +321,7 @@ class TestPhotoVerification(TestCase):
         attempt.save()
 
         status = SoftwareSecurePhotoVerification.user_status(user)
-        self.assertEquals(status, (attempt.status, ''))
+        self.assertEquals(status, ('approved', ''))
 
         # create another one for the same user, make sure the right one is
         # returned
@@ -331,14 +331,20 @@ class TestPhotoVerification(TestCase):
         attempt2.save()
 
         status = SoftwareSecurePhotoVerification.user_status(user)
-        self.assertEquals(status, (attempt2.status, "No photo ID was provided."))
+        self.assertEquals(status, ('approved', ''))
+
+        # now delete the first one and verify that the denial is being handled
+        # properly
+        attempt.delete()
+        status = SoftwareSecurePhotoVerification.user_status(user)
+        self.assertEquals(status, ('must_reverify', "No photo ID was provided."))
 
     def test_parse_error_msg_success(self):
         user = UserFactory.create()
         attempt = SoftwareSecurePhotoVerification(user=user)
         attempt.status = 'denied'
         attempt.error_msg = '[{"photoIdReasons": ["Not provided"]}]'
-        parsed_error_msg = attempt.parse_error_msg()
+        parsed_error_msg = attempt.parsed_error_msg()
         self.assertEquals("No photo ID was provided.", parsed_error_msg)
 
     def test_parse_error_msg_failure(self):
@@ -350,8 +356,9 @@ class TestPhotoVerification(TestCase):
             'Not Provided',
             '[{"IdReasons": ["Not provided"]}]',
             '{"IdReasons": ["Not provided"]}',
+            u'[{"ïḋṚëäṡöṅṡ": ["Ⓝⓞⓣ ⓟⓡⓞⓥⓘⓓⓔⓓ "]}]',
         }
         for msg in bad_messages:
             attempt.error_msg = msg
-            parsed_error_msg = attempt.parse_error_msg()
+            parsed_error_msg = attempt.parsed_error_msg()
             self.assertEquals(parsed_error_msg, "There was an error verifying your ID photos.")

lms/templates/dashboard/_dashboard_status_verification.html

@@ -21,10 +21,7 @@
 </li>
 %endif
 
-<%doc>
-This is for 'pending' statuses, where we are waiting for a response
-</%doc>
-%if verification_status in ('must_retry', 'submitted'):
+%if verification_status == 'pending':
 <li class="status status-verification is-pending">
   <span class="title status-title">${_("ID-Verification Status")}</span>
 
@@ -41,7 +38,7 @@ This is for 'pending' statuses, where we are waiting for a response
 
 %endif
 
-%if verification_status == 'denied':
+%if verification_status == 'must_reverify':
 <li class="status status-verification is-denied">
   <span class="title status-title">${_("ID-Verification Status")}</span>