Update schema for bok-choy database

Finalize tests for embargo middleware app

cms/envs/common.py

@@ -78,6 +78,9 @@ FEATURES = {
 
     # Allow editing of short description in course settings in cms
     'EDITABLE_SHORT_DESCRIPTION': True,
+
+    # Toggles embargo functionality
+    'EMBARGO': False,
 }
 ENABLE_JASMINE = False
 

cms/envs/test.py

@@ -190,3 +190,6 @@ FEATURES['ENABLE_SERVICE_STATUS'] = True
 
 # This is to disable a test under the common directory that will not pass when run under CMS
 FEATURES['DISABLE_RESET_EMAIL_TEST'] = True
+
+# Toggles embargo on for testing
+FEATURES['EMBARGO'] = True

common/djangoapps/embargo/admin.py

@@ -15,14 +15,15 @@ class EmbargoedCourseAdmin(admin.ModelAdmin):
     fieldsets = (
         (None, {
             'fields': ('course_id', 'embargoed'),
-            'description': textwrap.dedent("""Enter a course id in the following box.
-            Do not enter leading or trailing slashes. There is no need to surround the
-            course ID with quotes.
-            Validation will be performed on the course name, and if it is invalid, an
-            error message will display.
-
-            To enable embargos against this course (restrict course access from embargoed
-            states), check the "Embargoed" box, then click "Save".
+            'description': textwrap.dedent("""\
+                Enter a course id in the following box.
+                Do not enter leading or trailing slashes. There is no need to surround the
+                course ID with quotes.
+                Validation will be performed on the course name, and if it is invalid, an
+                error message will display.
+
+                To enable embargos against this course (restrict course access from embargoed
+                states), check the "Embargoed" box, then click "Save".
             """)
         }),
     )

common/djangoapps/embargo/forms.py

@@ -9,9 +9,7 @@ from embargo.fixtures.country_codes import COUNTRY_CODES
 
 import socket
 
-from xmodule.course_module import CourseDescriptor
 from xmodule.modulestore.django import modulestore
-from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError
 
 
 class EmbargoedCourseForm(forms.ModelForm):  # pylint: disable=incomplete-protocol
@@ -23,18 +21,17 @@ class EmbargoedCourseForm(forms.ModelForm):  # pylint: disable=incomplete-protoc
     def clean_course_id(self):
         """Validate the course id"""
         course_id = self.cleaned_data["course_id"]
+
+        # Try to get the course.  If this returns None, it's not a real course
         try:
-            # Try to get the course descriptor, if we can do that,
-            # it's a real course.
-            course_loc = CourseDescriptor.id_to_location(course_id)
-            modulestore().get_instance(course_id, course_loc, depth=1)
-        except (KeyError, ItemNotFoundError):
+            course = modulestore().get_course(course_id)
+        except ValueError:
             msg = 'COURSE NOT FOUND'
             msg += u' --- Entered course id was: "{0}". '.format(course_id)
             msg += 'Please recheck that you have supplied a valid course id.'
             raise forms.ValidationError(msg)
-        except (ValueError, InvalidLocationError):
-            msg = 'INVALID LOCATION'
+        if not course:
+            msg = 'COURSE NOT FOUND'
             msg += u' --- Entered course id was: "{0}". '.format(course_id)
             msg += 'Please recheck that you have supplied a valid course id.'
             raise forms.ValidationError(msg)
@@ -50,14 +47,12 @@ class EmbargoedStateForm(forms.ModelForm):  # pylint: disable=incomplete-protoco
 
     def _is_valid_code(self, code):
         """Whether or not code is a valid country code"""
-        if code in COUNTRY_CODES:
-            return True
-        return False
+        return code in COUNTRY_CODES
 
     def clean_embargoed_countries(self):
         """Validate the country list"""
         embargoed_countries = self.cleaned_data["embargoed_countries"]
-        if embargoed_countries == '':
+        if not embargoed_countries:
             return ''
 
         error_countries = []

common/djangoapps/embargo/middleware.py

@@ -9,8 +9,8 @@ HTTP_X_FORWARDED_FOR).
 """
 
 import pygeoip
-import django.core.exceptions
 
+from django.core.exceptions import MiddlewareNotUsed
 from django.conf import settings
 from django.shortcuts import redirect
 from ipware.ip import get_ip
@@ -29,7 +29,7 @@ class EmbargoMiddleware(object):
     def __init__(self):
         # If embargoing is turned off, make this middleware do nothing
         if not settings.FEATURES.get('EMBARGO', False):
-            raise django.core.exceptions.MiddlewareNotUsed()
+            raise MiddlewareNotUsed()
 
     def process_request(self, request):
         """

common/djangoapps/embargo/tests/test_forms.py

@@ -25,14 +25,6 @@ class EmbargoCourseFormTest(ModuleStoreTestCase):
         self.true_form_data = {'course_id': self.course.id, 'embargoed': True}
         self.false_form_data = {'course_id': self.course.id, 'embargoed': False}
 
-    def tearDown(self):
-        # Delete any EmbargoedCourse record we may have created
-        try:
-            record = EmbargoedCourse.objects.get(course_id=self.course.id)
-            record.delete()
-        except EmbargoedCourse.DoesNotExist:
-            return
-
     def test_embargo_course(self):
         self.assertFalse(EmbargoedCourse.is_embargoed(self.course.id))
         # Test adding embargo to this course
@@ -94,7 +86,7 @@ class EmbargoCourseFormTest(ModuleStoreTestCase):
         # Validation shouldn't work
         self.assertFalse(form.is_valid())
 
-        msg = 'INVALID LOCATION'
+        msg = 'COURSE NOT FOUND'
         msg += u' --- Entered course id was: "{0}". '.format(bad_id)
         msg += 'Please recheck that you have supplied a valid course id.'
         self.assertEquals(msg, form._errors['course_id'][0])  # pylint: disable=protected-access
@@ -106,6 +98,10 @@ class EmbargoCourseFormTest(ModuleStoreTestCase):
 class EmbargoedStateFormTest(TestCase):
     """Test form for adding new states"""
 
+    def setUp(self):
+        # Explicitly clear the cache, since ConfigurationModel relies on the cache
+        cache.clear()
+
     def tearDown(self):
         # Explicitly clear ConfigurationModel's cache so tests have a clear cache
         # and don't interfere with each other
@@ -128,7 +124,6 @@ class EmbargoedStateFormTest(TestCase):
         form.save()
         self.assertTrue(len(EmbargoedState.current().embargoed_countries_list) == 0)
 
-
     def test_add_invalid_states(self):
         # test adding invalid codes
         # xx is not valid

common/djangoapps/embargo/tests/test_middleware.py

@@ -45,10 +45,14 @@ class EmbargoMiddlewareTests(TestCase):
         # Text from lms/templates/static_templates/embargo.html
         self.embargo_text = "Unfortunately, at this time edX must comply with export controls, and we cannot allow you to access this particular course."
 
+        self.patcher = mock.patch.object(pygeoip.GeoIP, 'country_code_by_addr', self.mock_country_code_by_addr)
+        self.patcher.start()
+
     def tearDown(self):
         # Explicitly clear ConfigurationModel's cache so tests have a clear cache
         # and don't interfere with each other
         cache.clear()
+        self.patcher.stop()
 
     def mock_country_code_by_addr(self, ip_addr):
         """
@@ -65,32 +69,29 @@ class EmbargoMiddlewareTests(TestCase):
 
     @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms')
     def test_countries(self):
-        with mock.patch.object(pygeoip.GeoIP, 'country_code_by_addr') as mocked_method:
-            mocked_method.side_effect = self.mock_country_code_by_addr
-
-            # Accessing an embargoed page from a blocked IP should cause a redirect
-            response = self.client.get(self.embargoed_page, HTTP_X_FORWARDED_FOR='1.0.0.0', REMOTE_ADDR='1.0.0.0')
-            self.assertEqual(response.status_code, 302)
-            # Following the redirect should give us the embargo page
-            response = self.client.get(
-                self.embargoed_page,
-                HTTP_X_FORWARDED_FOR='1.0.0.0',
-                REMOTE_ADDR='1.0.0.0',
-                follow=True
-            )
-            self.assertIn(self.embargo_text, response.content)
-
-            # Accessing a regular page from a blocked IP should succeed
-            response = self.client.get(self.regular_page, HTTP_X_FORWARDED_FOR='1.0.0.0', REMOTE_ADDR='1.0.0.0')
-            self.assertEqual(response.status_code, 200)
-
-            # Accessing an embargoed page from a non-embargoed IP should succeed
-            response = self.client.get(self.embargoed_page, HTTP_X_FORWARDED_FOR='5.0.0.0', REMOTE_ADDR='5.0.0.0')
-            self.assertEqual(response.status_code, 200)
-
-            # Accessing a regular page from a non-embargoed IP should succeed
-            response = self.client.get(self.regular_page, HTTP_X_FORWARDED_FOR='5.0.0.0', REMOTE_ADDR='5.0.0.0')
-            self.assertEqual(response.status_code, 200)
+        # Accessing an embargoed page from a blocked IP should cause a redirect
+        response = self.client.get(self.embargoed_page, HTTP_X_FORWARDED_FOR='1.0.0.0', REMOTE_ADDR='1.0.0.0')
+        self.assertEqual(response.status_code, 302)
+        # Following the redirect should give us the embargo page
+        response = self.client.get(
+            self.embargoed_page,
+            HTTP_X_FORWARDED_FOR='1.0.0.0',
+            REMOTE_ADDR='1.0.0.0',
+            follow=True
+        )
+        self.assertIn(self.embargo_text, response.content)
+
+        # Accessing a regular page from a blocked IP should succeed
+        response = self.client.get(self.regular_page, HTTP_X_FORWARDED_FOR='1.0.0.0', REMOTE_ADDR='1.0.0.0')
+        self.assertEqual(response.status_code, 200)
+
+        # Accessing an embargoed page from a non-embargoed IP should succeed
+        response = self.client.get(self.embargoed_page, HTTP_X_FORWARDED_FOR='5.0.0.0', REMOTE_ADDR='5.0.0.0')
+        self.assertEqual(response.status_code, 200)
+
+        # Accessing a regular page from a non-embargoed IP should succeed
+        response = self.client.get(self.regular_page, HTTP_X_FORWARDED_FOR='5.0.0.0', REMOTE_ADDR='5.0.0.0')
+        self.assertEqual(response.status_code, 200)
 
     @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms')
     def test_ip_exceptions(self):
@@ -102,31 +103,57 @@ class EmbargoMiddlewareTests(TestCase):
             enabled=True
         ).save()
 
-        with mock.patch.object(pygeoip.GeoIP, 'country_code_by_addr') as mocked_method:
-            mocked_method.side_effect = self.mock_country_code_by_addr
-
-            # Accessing an embargoed page from a blocked IP that's been whitelisted
-            #  should succeed
-            response = self.client.get(self.embargoed_page, HTTP_X_FORWARDED_FOR='1.0.0.0', REMOTE_ADDR='1.0.0.0')
-            self.assertEqual(response.status_code, 200)
-
-            # Accessing a regular course from a blocked IP that's been whitelisted should succeed
-            response = self.client.get(self.regular_page, HTTP_X_FORWARDED_FOR='1.0.0.0', REMOTE_ADDR='1.0.0.0')
-            self.assertEqual(response.status_code, 200)
-
-            # Accessing an embargoed course from non-embargoed IP that's been blacklisted
-            #  should cause a redirect
-            response = self.client.get(self.embargoed_page, HTTP_X_FORWARDED_FOR='5.0.0.0', REMOTE_ADDR='5.0.0.0')
-            self.assertEqual(response.status_code, 302)
-            # Following the redirect should give us the embargo page
-            response = self.client.get(
-                self.embargoed_page,
-                HTTP_X_FORWARDED_FOR='5.0.0.0',
-                REMOTE_ADDR='1.0.0.0',
-                follow=True
-            )
-            self.assertIn(self.embargo_text, response.content)
-
-            # Accessing a regular course from a non-embargoed IP that's been blacklisted should succeed
-            response = self.client.get(self.regular_page, HTTP_X_FORWARDED_FOR='5.0.0.0', REMOTE_ADDR='5.0.0.0')
-            self.assertEqual(response.status_code, 200)
+        # Accessing an embargoed page from a blocked IP that's been whitelisted
+        #  should succeed
+        response = self.client.get(self.embargoed_page, HTTP_X_FORWARDED_FOR='1.0.0.0', REMOTE_ADDR='1.0.0.0')
+        self.assertEqual(response.status_code, 200)
+
+        # Accessing a regular course from a blocked IP that's been whitelisted should succeed
+        response = self.client.get(self.regular_page, HTTP_X_FORWARDED_FOR='1.0.0.0', REMOTE_ADDR='1.0.0.0')
+        self.assertEqual(response.status_code, 200)
+
+        # Accessing an embargoed course from non-embargoed IP that's been blacklisted
+        #  should cause a redirect
+        response = self.client.get(self.embargoed_page, HTTP_X_FORWARDED_FOR='5.0.0.0', REMOTE_ADDR='5.0.0.0')
+        self.assertEqual(response.status_code, 302)
+        # Following the redirect should give us the embargo page
+        response = self.client.get(
+            self.embargoed_page,
+            HTTP_X_FORWARDED_FOR='5.0.0.0',
+            REMOTE_ADDR='1.0.0.0',
+            follow=True
+        )
+        self.assertIn(self.embargo_text, response.content)
+
+        # Accessing a regular course from a non-embargoed IP that's been blacklisted should succeed
+        response = self.client.get(self.regular_page, HTTP_X_FORWARDED_FOR='5.0.0.0', REMOTE_ADDR='5.0.0.0')
+        self.assertEqual(response.status_code, 200)
+
+    @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms')
+    @mock.patch.dict(settings.FEATURES, {'EMBARGO': False})
+    def test_countries_embargo_off(self):
+        # When the middleware is turned off, all requests should go through
+        # Accessing an embargoed page from a blocked IP OK
+        response = self.client.get(self.embargoed_page, HTTP_X_FORWARDED_FOR='1.0.0.0', REMOTE_ADDR='1.0.0.0')
+        self.assertEqual(response.status_code, 200)
+
+        # Accessing a regular page from a blocked IP should succeed
+        response = self.client.get(self.regular_page, HTTP_X_FORWARDED_FOR='1.0.0.0', REMOTE_ADDR='1.0.0.0')
+        self.assertEqual(response.status_code, 200)
+
+        # Explicitly whitelist/blacklist some IPs
+        IPFilter(
+            whitelist='1.0.0.0',
+            blacklist='5.0.0.0',
+            changed_by=self.user,
+            enabled=True
+        ).save()
+
+        # Accessing an embargoed course from non-embargoed IP that's been blacklisted
+        #  should be OK
+        response = self.client.get(self.embargoed_page, HTTP_X_FORWARDED_FOR='5.0.0.0', REMOTE_ADDR='5.0.0.0')
+        self.assertEqual(response.status_code, 200)
+
+        # Accessing a regular course from a non-embargoed IP that's been blacklisted should succeed
+        response = self.client.get(self.regular_page, HTTP_X_FORWARDED_FOR='5.0.0.0', REMOTE_ADDR='5.0.0.0')
+        self.assertEqual(response.status_code, 200)

lms/envs/common.py

@@ -218,6 +218,9 @@ FEATURES = {
 
     # Turn off account locking if failed login attempts exceeds a limit
     'ENABLE_MAX_FAILED_LOGIN_ATTEMPTS': False,
+
+    # Toggle embargo functionality
+    'EMBARGO': False,
 }
 
 # Used for A/B testing

lms/envs/test.py

@@ -40,6 +40,9 @@ FEATURES['ENABLE_SHOPPING_CART'] = True
 FEATURES['ENABLE_S3_GRADE_DOWNLOADS'] = True
 FEATURES['ALLOW_COURSE_STAFF_GRADE_DOWNLOADS'] = True
 
+# Toggles embargo on for testing
+FEATURES['EMBARGO'] = True
+
 # Need wiki for courseware views to work. TODO (vshnayder): shouldn't need it.
 WIKI_ENABLED = True