From e0b0c375bed6d783b2df7e2dfd53ecfc7dc936d0 Mon Sep 17 00:00:00 2001 From: Daniel Clemente Laboreo Date: Mon, 15 Oct 2018 16:57:01 +0300 Subject: [PATCH] Fix error when saving CourseEnrollment in admin --- common/djangoapps/course_modes/admin.py | 2 +- common/djangoapps/student/admin.py | 18 +++++++- .../student/tests/test_admin_views.py | 46 ++++++++++++++++++- 3 files changed, 62 insertions(+), 4 deletions(-) diff --git a/common/djangoapps/course_modes/admin.py b/common/djangoapps/course_modes/admin.py index 0181c0c50b..eb5adcf864 100644 --- a/common/djangoapps/course_modes/admin.py +++ b/common/djangoapps/course_modes/admin.py @@ -56,7 +56,7 @@ class CourseModeForm(forms.ModelForm): def __init__(self, *args, **kwargs): # If args is a QueryDict, then the ModelForm addition request came in as a POST with a course ID string. # Change the course ID string to a CourseLocator object by copying the QueryDict to make it mutable. - if len(args) > 0 and 'course' in args[0] and isinstance(args[0], QueryDict): + if args and 'course' in args[0] and isinstance(args[0], QueryDict): args_copy = args[0].copy() args_copy['course'] = CourseKey.from_string(args_copy['course']) args = [args_copy] diff --git a/common/djangoapps/student/admin.py b/common/djangoapps/student/admin.py index 2ddc58497f..5acddf43b6 100644 --- a/common/djangoapps/student/admin.py +++ b/common/djangoapps/student/admin.py @@ -7,6 +7,7 @@ from django.contrib.auth import get_user_model from django.contrib.auth.admin import UserAdmin as BaseUserAdmin from django.contrib.auth.forms import ReadOnlyPasswordHashField, UserChangeForm as BaseUserChangeForm from django.db import models +from django.http.request import QueryDict from django.utils.translation import ugettext_lazy as _ from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey @@ -149,13 +150,26 @@ class LinkedInAddToProfileConfigurationAdmin(admin.ModelAdmin): class CourseEnrollmentForm(forms.ModelForm): def __init__(self, *args, **kwargs): + # If args is a QueryDict, then the ModelForm addition request came in as a POST with a course ID string. + # Change the course ID string to a CourseLocator object by copying the QueryDict to make it mutable. + if args and 'course' in args[0] and isinstance(args[0], QueryDict): + args_copy = args[0].copy() + try: + args_copy['course'] = CourseKey.from_string(args_copy['course']) + except InvalidKeyError: + raise forms.ValidationError("Cannot make a valid CourseKey from id {}!".format(args_copy['course'])) + args = [args_copy] + super(CourseEnrollmentForm, self).__init__(*args, **kwargs) if self.data.get('course'): try: self.data['course'] = CourseKey.from_string(self.data['course']) - except InvalidKeyError: - raise forms.ValidationError("Cannot make a valid CourseKey from id {}!".format(self.data['course'])) + except AttributeError: + # Change the course ID string to a CourseLocator. + # On a POST request, self.data is a QueryDict and is immutable - so this code will fail. + # However, the args copy above before the super() call handles this case. + pass def clean_course_id(self): course_id = self.cleaned_data['course'] diff --git a/common/djangoapps/student/tests/test_admin_views.py b/common/djangoapps/student/tests/test_admin_views.py index ef5d2cb882..e60170f8e2 100644 --- a/common/djangoapps/student/tests/test_admin_views.py +++ b/common/djangoapps/student/tests/test_admin_views.py @@ -4,6 +4,7 @@ Tests student admin.py import ddt from django.contrib.admin.sites import AdminSite from django.contrib.auth.models import User +from django.forms import ValidationError from django.urls import reverse from django.test import TestCase from mock import Mock @@ -209,7 +210,7 @@ class CourseEnrollmentAdminTest(SharedModuleStoreTestCase): super(CourseEnrollmentAdminTest, self).setUp() self.user = UserFactory.create(is_staff=True, is_superuser=True) self.course = CourseFactory() - CourseEnrollmentFactory( + self.course_enrollment = CourseEnrollmentFactory( user=self.user, course_id=self.course.id, # pylint: disable=no-member ) @@ -254,3 +255,46 @@ class CourseEnrollmentAdminTest(SharedModuleStoreTestCase): # Locate the column containing the username user_field = next(col for col in response.context['results'][idx] if "field-user" in col) self.assertIn(username, user_field) + + def test_save_toggle_active(self): + """ + Edit a CourseEnrollment to toggle its is_active checkbox, save it and verify that it was toggled. + When the form is saved, Django uses a QueryDict object which is immutable and needs special treatment. + This test implicitly verifies that the POST parameters are handled correctly. + """ + # is_active will change from True to False + self.assertTrue(self.course_enrollment.is_active) + data = { + 'user': unicode(self.course_enrollment.user.id), + 'course': unicode(self.course_enrollment.course.id), + 'is_active': 'false', + 'mode': self.course_enrollment.mode, + } + + with COURSE_ENROLLMENT_ADMIN_SWITCH.override(active=True): + response = self.client.post( + reverse('admin:student_courseenrollment_change', args=(self.course_enrollment.id, )), + data=data, + ) + self.assertEqual(response.status_code, 302) + + self.course_enrollment.refresh_from_db() + self.assertFalse(self.course_enrollment.is_active) + + def test_save_invalid_course_id(self): + """ + Send an invalid course ID instead of "org.0/course_0/Run_0" when saving, and verify that it fails. + """ + data = { + 'user': unicode(self.course_enrollment.user.id), + 'course': 'invalid-course-id', + 'is_active': 'true', + 'mode': self.course_enrollment.mode, + } + + with COURSE_ENROLLMENT_ADMIN_SWITCH.override(active=True): + with self.assertRaises(ValidationError): + self.client.post( + reverse('admin:student_courseenrollment_change', args=(self.course_enrollment.id, )), + data=data, + )