From c5e90464e996eb2c156d501616d380b4ff5ba3cf Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 26 Jul 2018 11:11:17 +0930 Subject: [PATCH] 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)