From 7f0bbedba1c0d5b9a504bcccf553fd5de25cb7d5 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 25 Jul 2018 15:04:40 +0930 Subject: [PATCH 1/2] Adds a waffle switch which gates access to the CourseEnrollmentAdmin views --- common/djangoapps/student/admin.py | 46 ++++++++++++++--- .../student/tests/test_admin_views.py | 50 ++++++++++++++++++- 2 files changed, 88 insertions(+), 8 deletions(-) diff --git a/common/djangoapps/student/admin.py b/common/djangoapps/student/admin.py index 060295b819..ae2d63c0ae 100644 --- a/common/djangoapps/student/admin.py +++ b/common/djangoapps/student/admin.py @@ -1,5 +1,6 @@ """ Django admin pages for student app """ from config_models.admin import ConfigurationModelAdmin +import waffle from django import forms from django.contrib import admin from django.contrib.admin.sites import NotRegistered @@ -165,13 +166,7 @@ class CourseEnrollmentForm(forms.ModelForm): fields = '__all__' -# Page disabled because it makes DB quries that impact performance enough to -# cause a site outage. It may be re-enabled when it is updated to make more -# efficent DB queries -# https://openedx.atlassian.net/browse/OPS-2943 -# Learner ticket to add functionality to /support -# https://openedx.atlassian.net/browse/LEARNER-4744 -#@admin.register(CourseEnrollment) +@admin.register(CourseEnrollment) class CourseEnrollmentAdmin(admin.ModelAdmin): """ Admin interface for the CourseEnrollment model. """ list_display = ('id', 'course_id', 'mode', 'user', 'is_active',) @@ -183,6 +178,43 @@ class CourseEnrollmentAdmin(admin.ModelAdmin): def queryset(self, request): return super(CourseEnrollmentAdmin, self).queryset(request).select_related('user') + def has_permission(self, request, method): + """ + Returns True if the given method is allowed. + + Access to these admin views is restricted because it makes DB quries that impact performance enough to cause a + site outage, cf https://openedx.atlassian.net/browse/OPS-2943 + + Enable these views using the waffle switch: `student.coursenrollment.admin` + """ + if waffle.switch_is_active('student.coursenrollment.admin'): + return getattr(super(CourseEnrollmentAdmin, self), method)(request) + return False + + def has_add_permission(self, request): + """ + Returns True if CourseEnrollment objects can be added via the admin view. + """ + return self.has_permission(request, 'has_add_permission') + + def has_change_permission(self, request, obj=None): + """ + Returns True if CourseEnrollment objects can be modified via the admin view. + """ + return self.has_permission(request, 'has_change_permission') + + def has_delete_permission(self, request, obj=None): + """ + Returns True if CourseEnrollment objects can be deleted via the admin view. + """ + return self.has_permission(request, 'has_delete_permission') + + def has_module_permission(self, request): + """ + Returns True if links to the CourseEnrollment admin view can be displayed. + """ + return self.has_permission(request, 'has_module_permission') + class UserProfileInline(admin.StackedInline): """ Inline admin interface for UserProfile model. """ diff --git a/common/djangoapps/student/tests/test_admin_views.py b/common/djangoapps/student/tests/test_admin_views.py index bd52275dcd..7189f6f71e 100644 --- a/common/djangoapps/student/tests/test_admin_views.py +++ b/common/djangoapps/student/tests/test_admin_views.py @@ -1,6 +1,8 @@ """ Tests student admin.py """ +import ddt +from waffle.testutils import override_switch from django.contrib.admin.sites import AdminSite from django.contrib.auth.models import User from django.urls import reverse @@ -8,7 +10,7 @@ from django.test import TestCase from mock import Mock from student.admin import UserAdmin -from student.tests.factories import UserFactory +from student.tests.factories import CourseEnrollmentFactory, UserFactory from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory @@ -186,3 +188,49 @@ class AdminUserPageTest(TestCase): request = Mock() user = Mock() self.assertIn('username', self.admin.get_readonly_fields(request, user)) + + +@ddt.ddt +class CourseEnrollmentAdminTest(SharedModuleStoreTestCase): + """ + Unit tests for the CourseEnrollmentAdmin view. + """ + ADMIN_URLS = ( + ('get', reverse('admin:student_courseenrollment_add')), + ('get', reverse('admin:student_courseenrollment_changelist')), + ('get', reverse('admin:student_courseenrollment_change', args=(1,))), + ('get', reverse('admin:student_courseenrollment_delete', args=(1,))), + ('post', reverse('admin:student_courseenrollment_add')), + ('post', reverse('admin:student_courseenrollment_changelist')), + ('post', reverse('admin:student_courseenrollment_change', args=(1,))), + ('post', reverse('admin:student_courseenrollment_delete', args=(1,))), + ) + + def setUp(self): + super(CourseEnrollmentAdminTest, self).setUp() + self.user = UserFactory.create(is_staff=True, is_superuser=True) + self.client.login(username=self.user.username, password='test') + CourseEnrollmentFactory( + user=self.user, + course_id=CourseFactory().id, # pylint: disable=no-member + ) + + @ddt.data(*ADMIN_URLS) + @ddt.unpack + def test_view_disabled(self, method, url): + """ + All CourseEnrollmentAdmin views are disabled by default. + """ + response = getattr(self.client, method)(url) + self.assertEqual(response.status_code, 403) + + @override_switch('student.coursenrollment.admin', active=True) + @ddt.data(*ADMIN_URLS) + @ddt.unpack + def test_view_enabled(self, method, url): + """ + Ensure CourseEnrollmentAdmin views can be enabled with the waffle flag. + """ + self.client.login(username=self.user.username, password='test') + response = getattr(self.client, method)(url) + self.assertEqual(response.status_code, 200) From c5e90464e996eb2c156d501616d380b4ff5ba3cf Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 26 Jul 2018 11:11:17 +0930 Subject: [PATCH 2/2] Address PR review: * uses openedx waffle_utils instead of straight waffle * adds student.STUDENT_WAFFLE_NAMESPACE * improves comment Also changed the waffle switch to use _ instead of . to respect current conventions. --- common/djangoapps/student/__init__.py | 8 ++++++++ common/djangoapps/student/admin.py | 17 +++++++++-------- .../student/tests/test_admin_views.py | 10 ++++------ 3 files changed, 21 insertions(+), 14 deletions(-) diff --git a/common/djangoapps/student/__init__.py b/common/djangoapps/student/__init__.py index e69de29bb2..a77f14c551 100644 --- a/common/djangoapps/student/__init__.py +++ b/common/djangoapps/student/__init__.py @@ -0,0 +1,8 @@ +""" +Student app helpers and settings +""" +from openedx.core.djangoapps.waffle_utils import WaffleSwitchNamespace + + +# Namespace for student app waffle switches +STUDENT_WAFFLE_NAMESPACE = WaffleSwitchNamespace(name='student') diff --git a/common/djangoapps/student/admin.py b/common/djangoapps/student/admin.py index ae2d63c0ae..9446a02249 100644 --- a/common/djangoapps/student/admin.py +++ b/common/djangoapps/student/admin.py @@ -1,6 +1,5 @@ """ Django admin pages for student app """ from config_models.admin import ConfigurationModelAdmin -import waffle from django import forms from django.contrib import admin from django.contrib.admin.sites import NotRegistered @@ -10,7 +9,9 @@ from django.utils.translation import ugettext_lazy as _ from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey +from openedx.core.djangoapps.waffle_utils import WaffleSwitch from openedx.core.lib.courses import clean_course_id +from student import STUDENT_WAFFLE_NAMESPACE from student.models import ( CourseAccessRole, CourseEnrollment, @@ -29,6 +30,11 @@ from xmodule.modulestore.django import modulestore User = get_user_model() # pylint:disable=invalid-name +# This switch exists because the CourseEnrollment admin views make DB queries that impact performance. +# In a large enough deployment of Open edX, this is enough to cause a site outage. +# See https://openedx.atlassian.net/browse/OPS-2943 +COURSE_ENROLLMENT_ADMIN_SWITCH = WaffleSwitch(STUDENT_WAFFLE_NAMESPACE, 'courseenrollment_admin') + class CourseAccessRoleForm(forms.ModelForm): """Form for adding new Course Access Roles view the Django Admin Panel.""" @@ -180,14 +186,9 @@ class CourseEnrollmentAdmin(admin.ModelAdmin): def has_permission(self, request, method): """ - Returns True if the given method is allowed. - - Access to these admin views is restricted because it makes DB quries that impact performance enough to cause a - site outage, cf https://openedx.atlassian.net/browse/OPS-2943 - - Enable these views using the waffle switch: `student.coursenrollment.admin` + Returns True if the given admin method is allowed. """ - if waffle.switch_is_active('student.coursenrollment.admin'): + if COURSE_ENROLLMENT_ADMIN_SWITCH.is_enabled(): return getattr(super(CourseEnrollmentAdmin, self), method)(request) return False diff --git a/common/djangoapps/student/tests/test_admin_views.py b/common/djangoapps/student/tests/test_admin_views.py index 7189f6f71e..641d934397 100644 --- a/common/djangoapps/student/tests/test_admin_views.py +++ b/common/djangoapps/student/tests/test_admin_views.py @@ -2,14 +2,13 @@ Tests student admin.py """ import ddt -from waffle.testutils import override_switch from django.contrib.admin.sites import AdminSite from django.contrib.auth.models import User from django.urls import reverse from django.test import TestCase from mock import Mock -from student.admin import UserAdmin +from student.admin import COURSE_ENROLLMENT_ADMIN_SWITCH, UserAdmin from student.tests.factories import CourseEnrollmentFactory, UserFactory from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory @@ -224,13 +223,12 @@ class CourseEnrollmentAdminTest(SharedModuleStoreTestCase): response = getattr(self.client, method)(url) self.assertEqual(response.status_code, 403) - @override_switch('student.coursenrollment.admin', active=True) @ddt.data(*ADMIN_URLS) @ddt.unpack def test_view_enabled(self, method, url): """ - Ensure CourseEnrollmentAdmin views can be enabled with the waffle flag. + Ensure CourseEnrollmentAdmin views can be enabled with the waffle switch. """ - self.client.login(username=self.user.username, password='test') - response = getattr(self.client, method)(url) + with COURSE_ENROLLMENT_ADMIN_SWITCH.override(active=True): + response = getattr(self.client, method)(url) self.assertEqual(response.status_code, 200)