From 89dda35a0c9cae38be403381a94a5cf18a0a333d Mon Sep 17 00:00:00 2001 From: "Dave St.Germain" Date: Tue, 12 Feb 2019 15:25:44 -0500 Subject: [PATCH 1/2] Moved the enrollment mode check to a user permission, which should be defined in edx-platform. --- edx_proctoring/__init__.py | 2 +- edx_proctoring/api.py | 46 +++++------------------ edx_proctoring/tests/__init__.py | 4 ++ edx_proctoring/tests/test_api.py | 35 ----------------- edx_proctoring/tests/test_student_view.py | 20 ++-------- package.json | 2 +- test_settings.py | 1 - 7 files changed, 19 insertions(+), 91 deletions(-) diff --git a/edx_proctoring/__init__.py b/edx_proctoring/__init__.py index abf81acba70..73607a9f0b9 100644 --- a/edx_proctoring/__init__.py +++ b/edx_proctoring/__init__.py @@ -5,6 +5,6 @@ from __future__ import absolute_import # Be sure to update the version number in edx_proctoring/package.json -__version__ = '1.5.14' +__version__ = '1.5.15' default_app_config = 'edx_proctoring.apps.EdxProctoringConfig' # pylint: disable=invalid-name diff --git a/edx_proctoring/api.py b/edx_proctoring/api.py index d12e7ee9393..995a0b171b6 100644 --- a/edx_proctoring/api.py +++ b/edx_proctoring/api.py @@ -1305,19 +1305,6 @@ def get_active_exams_for_user(user_id, course_id=None): return result -def _check_eligibility_of_enrollment_mode(credit_state): - """ - Inspects that the enrollment mode of the user - is valid for proctoring - """ - - # Allow only the verified students to take the exam as a proctored exam - # Also make an exception for the honor students to take the "practice exam" as a proctored exam. - # For the rest of the enrollment modes, None is returned which shows the exam content - # to the student rather than the proctoring prompt. - return credit_state and credit_state['enrollment_mode'] == 'verified' - - def _get_ordered_prerequisites(prerequisites_statuses, filter_out_namespaces=None): """ Apply filter and ordering of requirements status in our credit_state dictionary. This will @@ -1571,13 +1558,15 @@ def get_attempt_status_summary(user_id, course_id, content_id): # eligibility if credit_service and not exam['is_practice_exam']: credit_state = credit_service.get_credit_state(user_id, six.text_type(course_id), return_course_info=True) - if not _check_eligibility_of_enrollment_mode(credit_state): + user = User.objects.get(id=user_id) + if not user.has_perm('edx_proctoring.can_take_proctored_exam', exam): return None attempt = get_exam_attempt(exam['id'], user_id) if attempt: status = attempt['status'] - elif not exam['is_practice_exam'] and has_due_date_passed(credit_state.get('course_end_date', None)): + elif not exam['is_practice_exam'] \ + and credit_state and has_due_date_passed(credit_state.get('course_end_date', None)): status = ProctoredExamStudentAttemptStatus.expired else: status = ProctoredExamStudentAttemptStatus.eligible @@ -1863,20 +1852,10 @@ def _get_proctored_exam_view(exam, context, exam_id, user_id, course_id): """ student_view_template = None - credit_state = context.get('credit_state') - - # see if only 'verified' track students should see this *except* if it is a practice exam - check_mode = ( - settings.PROCTORING_SETTINGS.get('MUST_BE_VERIFIED_TRACK', True) and - credit_state - ) + user = User.objects.get(id=user_id) - if check_mode: - has_mode = _check_eligibility_of_enrollment_mode(credit_state) - if not has_mode: - # user does not have the required enrollment mode - # so do not override view this is a quick exit - return None + if not user.has_perm('edx_proctoring.can_take_proctored_exam', exam): + return None attempt = get_exam_attempt(exam_id, user_id) @@ -1899,10 +1878,7 @@ def _get_proctored_exam_view(exam, context, exam_id, user_id, course_id): # get information about prerequisites - credit_requirement_status = ( - credit_state.get('credit_requirement_status') - if credit_state else [] - ) + credit_requirement_status = context.get('credit_state', {}).get('credit_requirement_status', []) prerequisite_status = _are_prerequirements_satisfied( credit_requirement_status, @@ -2020,11 +1996,7 @@ def get_student_view(user_id, course_id, content_id, if user_role != 'student': return None - credit_service = get_runtime_service('credit') - - # call service to get course end date. - credit_state = credit_service.get_credit_state(user_id, course_id, return_course_info=True) - course_end_date = credit_state.get('course_end_date') if credit_state else None + course_end_date = context.get('credit_state', {}).get('course_end_date', None) exam_id = None try: diff --git a/edx_proctoring/tests/__init__.py b/edx_proctoring/tests/__init__.py index 0593fde652f..7ee460a3a73 100644 --- a/edx_proctoring/tests/__init__.py +++ b/edx_proctoring/tests/__init__.py @@ -32,6 +32,10 @@ def setup_test_perms(): rules.add_perm('accounts.can_retire_user', rules.is_staff) except KeyError: pass + try: + rules.add_perm('edx_proctoring.can_take_proctored_exam', rules.is_authenticated) + except KeyError: + pass setup_test_backends() diff --git a/edx_proctoring/tests/test_api.py b/edx_proctoring/tests/test_api.py index a98dd03822f..b942e617343 100644 --- a/edx_proctoring/tests/test_api.py +++ b/edx_proctoring/tests/test_api.py @@ -1618,26 +1618,6 @@ def test_practice_no_attempt(self): ) self.assertIn(summary, [expected]) - @ddt.data( - 'honor', 'staff' - ) - def test_status_summary_honor(self, enrollment_mode): - """ - Make sure status summary is None for a non-verified person - """ - - set_runtime_service('credit', MockCreditService(enrollment_mode=enrollment_mode)) - - exam_attempt = self._create_started_exam_attempt() - - summary = get_attempt_status_summary( - self.user.id, - exam_attempt.proctored_exam.course_id, - exam_attempt.proctored_exam.content_id - ) - - self.assertIsNone(summary) - def test_status_summary_bad(self): """ Make sure we get back a None when getting summary for content that does not @@ -1776,21 +1756,6 @@ def test_declined_prerequisites(self, content_id, self.assertEqual(len(results['pending_prerequisites']), expected_len_pending_prerequisites) self.assertEqual(len(results['declined_prerequisites']), expected_len_declined_prerequisites) - def test_summary_without_credit_state(self): - """ - Test that attempt status summary is None for users who are not enrolled. - """ - exam_id = self._create_exam_with_due_time() - set_runtime_service('credit', MockCreditServiceNone()) - - timed_exam = get_exam_by_id(exam_id) - summary = get_attempt_status_summary( - self.user.id, - timed_exam['course_id'], - timed_exam['content_id'] - ) - self.assertIsNone(summary) - def test_get_exam_violation_report(self): """ Test to get all the exam attempts. diff --git a/edx_proctoring/tests/test_student_view.py b/edx_proctoring/tests/test_student_view.py index 778d0a0f6bb..63f2033955b 100644 --- a/edx_proctoring/tests/test_student_view.py +++ b/edx_proctoring/tests/test_student_view.py @@ -168,18 +168,6 @@ def test_get_honor_view_with_practice_exam(self): }) self.assertIsNotNone(rendered_response) - def test_get_honor_view(self): - """ - Test for get_student_view prompting when the student is enrolled in non-verified - track, this should return None - """ - rendered_response = self.render_proctored_exam({ - 'credit_state': { - 'enrollment_mode': 'honor' - }, - }) - self.assertIsNone(rendered_response) - @ddt.data( (None, 'Make sure you are on a computer with a webcam, and that you have valid photo identification'), ('pending', 'Your verification is pending'), @@ -334,8 +322,7 @@ def test_proctored_exam_passed_end_date(self): Verify that we get a None back on a proctored exam if the course end date is passed """ - - set_runtime_service('credit', MockCreditServiceWithCourseEndDate()) + credit_state = MockCreditServiceWithCourseEndDate().get_credit_state(self.user_id, 'foo', True) rendered_response = get_student_view( user_id=self.user_id, @@ -348,6 +335,7 @@ def test_proctored_exam_passed_end_date(self): 'default_time_limit_mins': 90, 'due_date': None, 'hide_after_due': False, + 'credit_state': credit_state, }, user_role='student' ) @@ -358,8 +346,7 @@ def test_practice_exam_passed_end_date(self): Verify that we get a None back on a practice exam if the course end date is passed """ - - set_runtime_service('credit', MockCreditServiceWithCourseEndDate()) + credit_state = MockCreditServiceWithCourseEndDate().get_credit_state(self.user_id, 'foo', True) rendered_response = get_student_view( user_id=self.user_id, @@ -372,6 +359,7 @@ def test_practice_exam_passed_end_date(self): 'default_time_limit_mins': 90, 'due_date': None, 'hide_after_due': False, + 'credit_state': credit_state, }, user_role='student' ) diff --git a/package.json b/package.json index 55936cf4c56..a2b57bee5b3 100644 --- a/package.json +++ b/package.json @@ -2,7 +2,7 @@ "name": "@edx/edx-proctoring", "//": "Be sure to update the version number in edx_proctoring/__init__.py", "//": "Note that the version format is slightly different than that of the Python version when using prereleases.", - "version": "1.5.14", + "version": "1.5.15", "main": "edx_proctoring/static/index.js", "repository": { "type": "git", diff --git a/test_settings.py b/test_settings.py index fb408f839f0..239b783165b 100644 --- a/test_settings.py +++ b/test_settings.py @@ -103,7 +103,6 @@ } PROCTORING_SETTINGS = { - 'MUST_BE_VERIFIED_TRACK': True, 'MUST_COMPLETE_ICRV': True, 'LINK_URLS': { 'online_proctoring_rules': '', From d04c73779ffd92712c754761082d1eb93b2c3696 Mon Sep 17 00:00:00 2001 From: "Dave St.Germain" Date: Mon, 25 Feb 2019 10:50:52 -0500 Subject: [PATCH 2/2] Improved test coverage Mildly refactored User model access to support alternate User models --- edx_proctoring/api.py | 10 ++++---- edx_proctoring/models.py | 28 ++++++++++++----------- edx_proctoring/tests/test_api.py | 16 +++++++++++++ edx_proctoring/tests/test_student_view.py | 10 ++++++++ edx_proctoring/views.py | 7 +++--- 5 files changed, 51 insertions(+), 20 deletions(-) diff --git a/edx_proctoring/api.py b/edx_proctoring/api.py index 995a0b171b6..353fabb5a9e 100644 --- a/edx_proctoring/api.py +++ b/edx_proctoring/api.py @@ -14,7 +14,7 @@ from django.utils.translation import ugettext as _, ugettext_noop from django.conf import settings -from django.contrib.auth.models import User +from django.contrib.auth import get_user_model from django.template import loader from django.urls import reverse, NoReverseMatch from django.core.mail.message import EmailMessage @@ -65,6 +65,8 @@ REJECTED_GRADE_OVERRIDE_EARNED = 0.0 +USER_MODEL = get_user_model() + def create_exam(course_id, content_id, exam_name, time_limit_mins, due_date=None, is_proctored=True, is_practice_exam=False, external_id=None, is_active=True, hide_after_due=False, @@ -1078,7 +1080,7 @@ def create_proctoring_attempt_status_email(user_id, exam_attempt_obj, course_nam if not exam_attempt_obj.taking_as_proctored or exam_attempt_obj.is_sample_attempt: return None - user = User.objects.get(id=user_id) + user = USER_MODEL.objects.get(id=user_id) course_info_url = '' email_subject = ( _('Proctoring Results For {course_name} {exam_name}').format( @@ -1558,7 +1560,7 @@ def get_attempt_status_summary(user_id, course_id, content_id): # eligibility if credit_service and not exam['is_practice_exam']: credit_state = credit_service.get_credit_state(user_id, six.text_type(course_id), return_course_info=True) - user = User.objects.get(id=user_id) + user = USER_MODEL.objects.get(id=user_id) if not user.has_perm('edx_proctoring.can_take_proctored_exam', exam): return None @@ -1852,7 +1854,7 @@ def _get_proctored_exam_view(exam, context, exam_id, user_id, course_id): """ student_view_template = None - user = User.objects.get(id=user_id) + user = USER_MODEL.objects.get(id=user_id) if not user.has_perm('edx_proctoring.can_take_proctored_exam', exam): return None diff --git a/edx_proctoring/models.py b/edx_proctoring/models.py index c5e4679b2ac..b0892712018 100644 --- a/edx_proctoring/models.py +++ b/edx_proctoring/models.py @@ -8,7 +8,7 @@ from __future__ import absolute_import import six -from django.contrib.auth.models import User +from django.contrib.auth import get_user_model from django.db import models from django.db.models import Q from django.db.models.base import ObjectDoesNotExist @@ -24,6 +24,8 @@ ) from edx_proctoring.statuses import ProctoredExamStudentAttemptStatus, SoftwareSecureReviewStatus +USER_MODEL = get_user_model() + @six.python_2_unicode_compatible class ProctoredExam(TimeStampedModel): @@ -123,7 +125,7 @@ class ProctoredExamReviewPolicy(TimeStampedModel): """ # who set this ProctoredExamReviewPolicy - set_by_user = models.ForeignKey(User) + set_by_user = models.ForeignKey(USER_MODEL) # for which exam? proctored_exam = models.ForeignKey(ProctoredExam, db_index=True) @@ -166,7 +168,7 @@ class ProctoredExamReviewPolicyHistory(TimeStampedModel): original_id = models.IntegerField(db_index=True) # who set this ProctoredExamReviewPolicy - set_by_user = models.ForeignKey(User) + set_by_user = models.ForeignKey(USER_MODEL) # for which exam? proctored_exam = models.ForeignKey(ProctoredExam, db_index=True) @@ -287,7 +289,7 @@ class ProctoredExamStudentAttempt(TimeStampedModel): """ objects = ProctoredExamStudentAttemptManager() - user = models.ForeignKey(User, db_index=True) + user = models.ForeignKey(USER_MODEL, db_index=True) proctored_exam = models.ForeignKey(ProctoredExam, db_index=True) @@ -374,7 +376,7 @@ class ProctoredExamStudentAttemptHistory(TimeStampedModel): but will record (for audit history) all entries that have been updated. """ - user = models.ForeignKey(User, db_index=True) + user = models.ForeignKey(USER_MODEL, db_index=True) # this is the PK of the original table, note this is not a FK attempt_id = models.IntegerField(null=True) @@ -497,7 +499,7 @@ class ProctoredExamStudentAllowance(TimeStampedModel): objects = ProctoredExamStudentAllowanceManager() - user = models.ForeignKey(User) + user = models.ForeignKey(USER_MODEL) proctored_exam = models.ForeignKey(ProctoredExam) @@ -558,9 +560,9 @@ def add_allowance_for_user(cls, exam_id, user_info, key, value): user_id = user_info else: # we got a string, so try to resolve it - users = User.objects.filter(username=user_info) + users = USER_MODEL.objects.filter(username=user_info) if not users.exists(): - users = User.objects.filter(email=user_info) + users = USER_MODEL.objects.filter(email=user_info) if not users.exists(): err_msg = ( @@ -626,7 +628,7 @@ class ProctoredExamStudentAllowanceHistory(TimeStampedModel): # what was the original id of the allowance allowance_id = models.IntegerField() - user = models.ForeignKey(User) + user = models.ForeignKey(USER_MODEL) proctored_exam = models.ForeignKey(ProctoredExam) @@ -662,12 +664,12 @@ class ProctoredExamSoftwareSecureReview(TimeStampedModel): video_url = models.TextField() # user_id of person who did the review (can be None if submitted via server-to-server API) - reviewed_by = models.ForeignKey(User, null=True, related_name='+') + reviewed_by = models.ForeignKey(USER_MODEL, null=True, related_name='+') # student username for the exam # this is an optimization for the Django Admin pane (so we can search) # this is null because it is being added after initial production ship - student = models.ForeignKey(User, null=True, related_name='+') + student = models.ForeignKey(USER_MODEL, null=True, related_name='+') # exam_id for the review # this is an optimization for the Django Admin pane (so we can search) @@ -727,12 +729,12 @@ class ProctoredExamSoftwareSecureReviewHistory(TimeStampedModel): video_url = models.TextField() # user_id of person who did the review (can be None if submitted via server-to-server API) - reviewed_by = models.ForeignKey(User, null=True, related_name='+') + reviewed_by = models.ForeignKey(USER_MODEL, null=True, related_name='+') # student username for the exam # this is an optimization for the Django Admin pane (so we can search) # this is null because it is being added after initial production ship - student = models.ForeignKey(User, null=True, related_name='+') + student = models.ForeignKey(USER_MODEL, null=True, related_name='+') # exam_id for the review # this is an optimization for the Django Admin pane (so we can search) diff --git a/edx_proctoring/tests/test_api.py b/edx_proctoring/tests/test_api.py index b942e617343..f336512f3a8 100644 --- a/edx_proctoring/tests/test_api.py +++ b/edx_proctoring/tests/test_api.py @@ -1618,6 +1618,22 @@ def test_practice_no_attempt(self): ) self.assertIn(summary, [expected]) + def test_status_summary_no_perm(self): + """ + The summary should be None for users who don't have permission + (For the tests, that means non-authenticated users) + """ + set_runtime_service('credit', MockCreditService(enrollment_mode='verified')) + exam_attempt = self._create_started_exam_attempt() + with patch('django.contrib.auth.models.User.is_authenticated', False): + summary = get_attempt_status_summary( + self.user.id, + exam_attempt.proctored_exam.course_id, + exam_attempt.proctored_exam.content_id + ) + + self.assertIsNone(summary) + def test_status_summary_bad(self): """ Make sure we get back a None when getting summary for content that does not diff --git a/edx_proctoring/tests/test_student_view.py b/edx_proctoring/tests/test_student_view.py index 63f2033955b..f13cd3f0da1 100644 --- a/edx_proctoring/tests/test_student_view.py +++ b/edx_proctoring/tests/test_student_view.py @@ -737,6 +737,16 @@ def test_get_studentview_submitted_status_with_duedate(self): ) self.assertIsNotNone(rendered_response) + def test_get_no_perm_view(self): + """ + Test for get_student_view prompting when the student does not have permission + to view proctored exams, this should return None + (For edx-proctoring tests, only authenticated students have the permission) + """ + with patch('django.contrib.auth.models.User.is_authenticated', False): + rendered_response = self.render_proctored_exam() + self.assertIsNone(rendered_response) + def test_get_studentview_started_onboarding(self): """ Test fallthrough page case for onboarding exams diff --git a/edx_proctoring/views.py b/edx_proctoring/views.py index a5082112fba..1dce1cd52ad 100644 --- a/edx_proctoring/views.py +++ b/edx_proctoring/views.py @@ -11,7 +11,8 @@ from crum import get_current_request from django.conf import settings -from django.contrib.auth.models import User +from django.contrib.auth import get_user_model +from django.core.exceptions import ObjectDoesNotExist from django.core.paginator import Paginator, EmptyPage, PageNotAnInteger from django.urls import reverse, NoReverseMatch from django.shortcuts import redirect @@ -858,8 +859,8 @@ def make_review(self, attempt, data, backend=None): review.exam_id = attempt['proctored_exam']['id'] try: - review.reviewed_by = User.objects.get(email=data['reviewed_by']) - except (User.DoesNotExist, KeyError): + review.reviewed_by = get_user_model().objects.get(email=data['reviewed_by']) + except (ObjectDoesNotExist, KeyError): review.reviewed_by = None # If the reviewing user is a user in the system (user may be None for automated reviews) and does