From 17610edf77b7903b613b25098e582ee53fdc7d18 Mon Sep 17 00:00:00 2001 From: Diana Huang Date: Thu, 31 Oct 2013 14:20:57 -0400 Subject: [PATCH] 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 | 1 - common/djangoapps/student/views.py | 27 ++++++----- lms/djangoapps/verify_student/models.py | 46 +++++++++++++------ .../verify_student/tests/test_models.py | 15 ++++-- .../_dashboard_status_verification.html | 7 +-- 5 files changed, 57 insertions(+), 39 deletions(-) diff --git a/cms/envs/common.py b/cms/envs/common.py index f225e22061..8d28ddcd8d 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -418,7 +418,6 @@ MKTG_URL_LINK_MAP = { COURSES_WITH_UNSAFE_CODE = [] - ############################## EVENT TRACKING ################################# TRACK_MAX_EVENT = 10000 diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index e5582e2f27..ca8ccf91b0 100644 --- a/common/djangoapps/student/views.py +++ b/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) diff --git a/lms/djangoapps/verify_student/models.py b/lms/djangoapps/verify_student/models.py index 6acfe0e889..523e8393a4 100644 --- a/lms/djangoapps/verify_student/models.py +++ b/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' + 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' """ - try: - attempts = cls.objects.filter(user=user).order_by('-updated_at') - attempt = attempts[0] - except IndexError: - return ('none', '') + status = 'none' + error_msg = '' - if attempt.created_at < cls._earliest_allowed_date(): - return ('expired', '') + 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) - error_msg = attempt.error_msg - if attempt.error_msg: - error_msg = attempt.parse_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 (attempt.status, error_msg) + return (status, error_msg) - def parse_error_msg(self): + 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 diff --git a/lms/djangoapps/verify_student/tests/test_models.py b/lms/djangoapps/verify_student/tests/test_models.py index e497c2d46c..fd2b767859 100644 --- a/lms/djangoapps/verify_student/tests/test_models.py +++ b/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.") diff --git a/lms/templates/dashboard/_dashboard_status_verification.html b/lms/templates/dashboard/_dashboard_status_verification.html index 0f542cce21..6bdd76970c 100644 --- a/lms/templates/dashboard/_dashboard_status_verification.html +++ b/lms/templates/dashboard/_dashboard_status_verification.html @@ -21,10 +21,7 @@ %endif -<%doc> -This is for 'pending' statuses, where we are waiting for a response - -%if verification_status in ('must_retry', 'submitted'): +%if verification_status == 'pending':
  • ${_("ID-Verification Status")} @@ -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':
  • ${_("ID-Verification Status")}