Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: allow timed exams for providers that are no longer valid #1237

Merged
merged 4 commits into from
Oct 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,17 @@ Change Log
Unreleased
~~~~~~~~~~

[4.18.2] - 2024-10-03
~~~~~~~~~~~~~~~~~~~~~
* fix various bugs related to exams configured with removed proctoring backend

[4.17.0]
~~~~~~~~~~~~~~~~~~~~~
* Add support for Python 3.11 & 3.12

[4.16.1] - 2023-08-8
~~~~~~~~~~~~~~~~~~~~~
* Updated django-simple-history package to 3.3.0
* Updated django-simple-history package to 3.3.0
* Created no-op migrations needed for new django-simple-history package version

[4.16.0] - 2023-06-22
Expand Down
2 changes: 1 addition & 1 deletion edx_proctoring/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,4 @@
"""

# Be sure to update the version number in edx_proctoring/package.json
__version__ = '4.18.1'
__version__ = '4.18.2'
17 changes: 13 additions & 4 deletions edx_proctoring/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -1669,7 +1669,7 @@ def update_attempt_status(attempt_id, to_status,
if email:
try:
email.send()
except Exception as err: # pylint: disable=broad-except
except Exception as err:
log.exception(
('Exception occurred while trying to send proctoring attempt '
'status email for user_id=%(user_id)s in course_id=%(course_id)s -- %(err)s'),
Expand Down Expand Up @@ -2943,7 +2943,7 @@ def get_student_view(user_id, course_id, content_id,
is_proctored_exam = exam['is_proctored'] and not exam['is_practice_exam']
is_timed_exam = not exam['is_proctored'] and not exam['is_practice_exam']

exam_backend = get_backend_provider(name=exam['backend'])
exam_backend = get_backend_provider(exam=exam)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If an exam is passed in, as opposed to the provider name, a timed exam will always return "None".


sub_view_func = None
if is_timed_exam:
Expand Down Expand Up @@ -3019,8 +3019,17 @@ def is_backend_dashboard_available(course_id):
active_only=True
)
for exam in exams:
if get_backend_provider(name=exam.backend).has_dashboard:
return True
try:
if get_backend_provider(name=exam.backend).has_dashboard:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want this case to throw an error because we still want the instructor dashboard to load for courses that still have software secure chosen.

return True
except NotImplementedError:
log.exception(
'No proctoring backend configured for backend=%(backend)s',
{
'backend': exam.backend,
}
)

return False


Expand Down
2 changes: 1 addition & 1 deletion edx_proctoring/backends/rest.py
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ def on_exam_saved(self, exam):
try:
response = self.session.post(url, json=exam)
data = response.json()
except Exception as exc: # pylint: disable=broad-except
except Exception as exc:
if response:
if hasattr(exc, 'response') and exc.response is not None:
content = exc.response.content
Expand Down
2 changes: 1 addition & 1 deletion edx_proctoring/backends/software_secure.py
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,7 @@ def get_instructor_url(
# reformat video url as per MST-871 findings
reformatted_url = decrypted_video_url.replace('DirectLink-Generic', 'DirectLink-HTML5')
return reformatted_url
except Exception as err: # pylint: disable=broad-except
except Exception as err:
log.exception(
'Could not decrypt video url for attempt_id=%(attempt_id)s '
'due to the following error: %(error_string)s',
Expand Down
31 changes: 20 additions & 11 deletions edx_proctoring/handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,16 @@
exam = ProctoredExamJSONSafeSerializer(instance).data
# from the perspective of the backend, the exam is now inactive.
exam['is_active'] = False
backend = get_backend_provider(name=exam['backend'])
backend.on_exam_saved(exam)
try:
backend = get_backend_provider(name=exam['backend'])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

courses with software secure should be allowed to switch from a proctored to a timed exam. We don't want this code to throw an error in that case

backend.on_exam_saved(exam)
except NotImplementedError:
log.exception(

Check warning on line 34 in edx_proctoring/handlers.py

View check run for this annotation

Codecov / codecov/patch

edx_proctoring/handlers.py#L33-L34

Added lines #L33 - L34 were not covered by tests
'No proctoring backend configured for backend=%(backend)s',
{
'backend': exam['backend'],
}
)


@receiver(post_save, sender=models.ProctoredExamReviewPolicy)
Expand Down Expand Up @@ -127,15 +135,16 @@
else:
# remove the attempt on the backend
# timed exams have no backend
backend = get_backend_provider(name=instance.proctored_exam.backend)
if backend:
result = backend.remove_exam_attempt(instance.proctored_exam.external_id, instance.external_id)
if not result:
log.error(
'Failed to remove attempt_id=%s from backend=%s',
instance.id,
instance.proctored_exam.backend,
)
if instance.proctored_exam.is_proctored:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only remove from the backend if it is a proctored exam, so this does fix a bug where instructors wouldn't be able to delete a timed exam.

[Question] For the case of PSI, do we care if removing attempts blows up?

backend = get_backend_provider(name=instance.proctored_exam.backend)
if backend:
result = backend.remove_exam_attempt(instance.proctored_exam.external_id, instance.external_id)
if not result:
log.error(
'Failed to remove attempt_id=%s from backend=%s',
instance.id,
instance.proctored_exam.backend,
)


@receiver(post_delete, sender=models.ProctoredExamStudentAttempt)
Expand Down
2 changes: 1 addition & 1 deletion edx_proctoring/rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,5 @@ def is_in_reviewer_group(user, attempt):
return user.groups.filter(name=backend_group).exists()


# pylint: disable=unsupported-binary-operation
# pylint: disable-next=unsupported-binary-operation
rules.add_perm('edx_proctoring.can_review_attempt', is_in_reviewer_group | rules.is_staff)
5 changes: 5 additions & 0 deletions edx_proctoring/tests/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -2896,6 +2896,11 @@ def test_dashboard_availability(self):
# backend with a dashboard
self.assertTrue(is_backend_dashboard_available(self.course_id))

@patch('edx_proctoring.api.get_backend_provider')
def test_dashboard_availability_no_provider(self, mock_get_backend):
mock_get_backend.side_effect = NotImplementedError()
self.assertFalse(is_backend_dashboard_available(self.course_id))

def test_does_provider_support_onboarding(self):
self.assertTrue(does_backend_support_onboarding('test'))
self.assertFalse(does_backend_support_onboarding('mock'))
Expand Down
2 changes: 1 addition & 1 deletion edx_proctoring/tests/test_handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ def setUp(self):
self.proctored_exam = ProctoredExam.objects.create(
course_id='x/y/z', content_id=self.content_id, exam_name=self.exam_name,
time_limit_mins=self.default_time_limit, external_id=self.external_id,
backend=self.backend_name
backend=self.backend_name, is_proctored=True,
)
self.attempt = ProctoredExamStudentAttempt.objects.create(
proctored_exam=self.proctored_exam, user=self.user, attempt_code='12345',
Expand Down
22 changes: 22 additions & 0 deletions edx_proctoring/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -792,6 +792,18 @@ def test_no_onboarding_exam(self):
message = 'There is no onboarding exam related to this course id.'
self.assertEqual(response_data['detail'], message)

@patch('edx_proctoring.views.get_backend_provider')
def test_invalid_backend(self, mock_get_backend):
mock_get_backend.side_effect = NotImplementedError()
response = self.client.get(
reverse('edx_proctoring:user_onboarding.status')
+ f'?course_id={self.course_id}'
)
self.assertEqual(response.status_code, 404)
response_data = json.loads(response.content.decode('utf-8'))
message = 'There is no onboarding exam related to this course id.'
self.assertEqual(response_data['detail'], message)

@override_settings(LEARNING_MICROFRONTEND_URL='https://learningmfe')
def test_onboarding_mfe_link(self):
"""
Expand Down Expand Up @@ -1518,6 +1530,16 @@ def test_backend_does_not_support_onboarding(self):
self.assertEqual(response.status_code, 404)
test_backend.supports_onboarding = previous_value

@patch('edx_proctoring.views.get_backend_provider')
def test_invalid_backend(self, mock_get_backend):
mock_get_backend.side_effect = NotImplementedError()
response = self.client.get(reverse(
'edx_proctoring:user_onboarding.status.course',
kwargs={'course_id': 'a/b/c'}
)
)
self.assertEqual(response.status_code, 404)

def test_multiple_onboarding_exams(self):
onboarding_exam_2_id = create_exam(
course_id=self.course_id,
Expand Down
26 changes: 23 additions & 3 deletions edx_proctoring/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -640,7 +640,7 @@ class StudentOnboardingStatusView(ProctoredAPIView):
* 'onboarding_past_due': Whether the onboarding exam is past due. All onboarding exams in the course must
be past due in order for onboarding_past_due to be true.
"""
def get(self, request):
def get(self, request): # pylint: disable=too-many-statements
"""
HTTP GET handler. Returns the learner's onboarding status.
"""
Expand Down Expand Up @@ -677,7 +677,17 @@ def get(self, request):

# If there are multiple onboarding exams, use the last created exam accessible to the user
onboarding_exams = list(ProctoredExam.get_practice_proctored_exams_for_course(course_id).order_by('-created'))
if not onboarding_exams or not get_backend_provider(name=onboarding_exams[0].backend).supports_onboarding:
provider = None
try:
provider = get_backend_provider(name=onboarding_exams[0].backend) if onboarding_exams else None
except NotImplementedError:
logging.exception(
'No proctoring backend configured for backend=%(backend)s',
{
'backend': onboarding_exams[0].backend,
}
)
if not onboarding_exams or not (provider and provider.supports_onboarding):
return Response(
status=404,
data={'detail': _('There is no onboarding exam related to this course id.')}
Expand Down Expand Up @@ -848,7 +858,17 @@ def get(self, request, course_id):
# get last created onboarding exam if there are multiple
onboarding_exam = (ProctoredExam.get_practice_proctored_exams_for_course(course_id)
.order_by('-created').first())
if not onboarding_exam or not get_backend_provider(name=onboarding_exam.backend).supports_onboarding:
provider = None
try:
provider = get_backend_provider(name=onboarding_exam.backend) if onboarding_exam else None
except NotImplementedError:
logging.exception(
'No proctoring backend configured for backend=%(backend)s',
{
'backend': onboarding_exam.backend,
}
)
if not onboarding_exam or not (provider and provider.supports_onboarding):
return Response(
status=404,
data={'detail': _('There is no onboarding exam related to this course id.')}
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"name": "@edx/edx-proctoring",
"//": "Note that the version format is slightly different than that of the Python version when using prereleases.",
"version": "4.18.1",
"version": "4.18.2",
"main": "edx_proctoring/static/index.js",
"scripts": {
"test": "gulp test"
Expand Down
Loading