Add tests for empty analytics server info and url-encoded params.

lms/djangoapps/instructor/tests/test_api.py

@@ -29,6 +29,8 @@ from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from courseware.tests.helpers import LoginEnrollmentTestCase
 from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
+from xmodule.modulestore import ModuleStoreEnum
+from xmodule.modulestore.django import modulestore
 from student.tests.factories import UserFactory
 from courseware.tests.factories import StaffFactory, InstructorFactory, BetaTesterFactory
 from student.roles import CourseBetaTesterRole
@@ -2563,6 +2565,7 @@ class TestInstructorEmailContentList(ModuleStoreTestCase, LoginEnrollmentTestCas
         self.assertDictEqual(expected_info, returned_info)
 
 
+@ddt.ddt
 @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
 @override_settings(ANALYTICS_SERVER_URL="http://robotanalyticsserver.netbot:900/")
 @override_settings(ANALYTICS_API_KEY="robot_api_key")
@@ -2590,25 +2593,84 @@ class TestInstructorAPIAnalyticsProxy(ModuleStoreTestCase, LoginEnrollmentTestCa
         self.instructor = InstructorFactory(course_key=self.course.id)
         self.client.login(username=self.instructor.username, password='test')
 
+    @ddt.data((ModuleStoreEnum.Type.mongo, False), (ModuleStoreEnum.Type.split, True))
+    @ddt.unpack
     @patch.object(instructor.views.api.requests, 'get')
-    def test_analytics_proxy_url(self, act):
+    def test_analytics_proxy_url(self, store_type, assert_wo_encoding, act):
         """ Test legacy analytics proxy url generation. """
+        with modulestore().default_store(store_type):
+            course = CourseFactory.create()
+            instructor_local = InstructorFactory(course_key=course.id)
+            self.client.login(username=instructor_local.username, password='test')
+
+            act.return_value = self.FakeProxyResponse()
+
+            url = reverse('proxy_legacy_analytics', kwargs={'course_id': course.id.to_deprecated_string()})
+            response = self.client.get(url, {
+                'aname': 'ProblemGradeDistribution'
+            })
+            self.assertEqual(response.status_code, 200)
+
+            # Make request URL pattern - everything but course id.
+            url_pattern = "{url}get?aname={aname}&course_id={course_id}&apikey={api_key}".format(
+                url="http://robotanalyticsserver.netbot:900/",
+                aname="ProblemGradeDistribution",
+                course_id="{course_id!s}",
+                api_key="robot_api_key",
+            )
+
+            if assert_wo_encoding:
+                # Format url with no URL-encoding of parameters.
+                assert_url = url_pattern.format(course_id=course.id.to_deprecated_string())
+                with self.assertRaises(AssertionError):
+                    act.assert_called_once_with(assert_url)
+
+            # Format url *with* URL-encoding of parameters.
+            expected_url = url_pattern.format(course_id=quote(course.id.to_deprecated_string()))
+            act.assert_called_once_with(expected_url)
+
+    @override_settings(ANALYTICS_SERVER_URL="")
+    @patch.object(instructor.views.api.requests, 'get')
+    def test_analytics_proxy_server_url(self, act):
+        """
+        Test legacy analytics when empty server url.
+        """
         act.return_value = self.FakeProxyResponse()
 
         url = reverse('proxy_legacy_analytics', kwargs={'course_id': self.course.id.to_deprecated_string()})
         response = self.client.get(url, {
             'aname': 'ProblemGradeDistribution'
         })
-        self.assertEqual(response.status_code, 200)
+        self.assertEqual(response.status_code, 501)
 
-        # check request url
-        expected_url = "{url}get?aname={aname}&course_id={course_id!s}&apikey={api_key}".format(
-            url="http://robotanalyticsserver.netbot:900/",
-            aname="ProblemGradeDistribution",
-            course_id=self.course.id.to_deprecated_string(),
-            api_key="robot_api_key",
-        )
-        act.assert_called_once_with(expected_url)
+    @override_settings(ANALYTICS_API_KEY="")
+    @patch.object(instructor.views.api.requests, 'get')
+    def test_analytics_proxy_api_key(self, act):
+        """
+        Test legacy analytics when empty server API key.
+        """
+        act.return_value = self.FakeProxyResponse()
+
+        url = reverse('proxy_legacy_analytics', kwargs={'course_id': self.course.id.to_deprecated_string()})
+        response = self.client.get(url, {
+            'aname': 'ProblemGradeDistribution'
+        })
+        self.assertEqual(response.status_code, 501)
+
+    @override_settings(ANALYTICS_SERVER_URL="")
+    @override_settings(ANALYTICS_API_KEY="")
+    @patch.object(instructor.views.api.requests, 'get')
+    def test_analytics_proxy_empty_url_and_api_key(self, act):
+        """
+        Test legacy analytics when empty server url & API key.
+        """
+        act.return_value = self.FakeProxyResponse()
+
+        url = reverse('proxy_legacy_analytics', kwargs={'course_id': self.course.id.to_deprecated_string()})
+        response = self.client.get(url, {
+            'aname': 'ProblemGradeDistribution'
+        })
+        self.assertEqual(response.status_code, 501)
 
     @patch.object(instructor.views.api.requests, 'get')
     def test_analytics_proxy(self, act):

lms/djangoapps/instructor/views/api.py

@@ -1697,7 +1697,7 @@ def send_email(request, course_id):
         course_id,
         request.user,
         send_to,
-        subject,message,
+        subject, message,
         template_name=template_name,
         from_addr=from_addr
     )
@@ -1800,7 +1800,7 @@ def proxy_legacy_analytics(request, course_id):
     url = "{}get?aname={}&course_id={}&apikey={}".format(
         settings.ANALYTICS_SERVER_URL,
         analytics_name,
-        urllib.quote(course_id.to_deprecated_string()),
+        urllib.quote(unicode(course_id)),
         settings.ANALYTICS_API_KEY,
     )