Merge pull request #5556 from edx/jeskew/fix_split_old_analytics_call

Fix old analytics api call for Split modulestore.

common/lib/xmodule/xmodule/tests/test_progress.py

@@ -23,12 +23,12 @@ class ProgressTest(unittest.TestCase):
 
     def test_create_object(self):
         # These should work:
-        p = Progress(0, 2)
-        p = Progress(1, 2)
-        p = Progress(2, 2)
+        prg1 = Progress(0, 2)  # pylint: disable=W0612
+        prg2 = Progress(1, 2)  # pylint: disable=W0612
+        prg3 = Progress(2, 2)  # pylint: disable=W0612
 
-        p = Progress(2.5, 5.0)
-        p = Progress(3.7, 12.3333)
+        prg4 = Progress(2.5, 5.0)  # pylint: disable=W0612
+        prg5 = Progress(3.7, 12.3333)  # pylint: disable=W0612
 
         # These shouldn't
         self.assertRaises(ValueError, Progress, 0, 0)
@@ -44,10 +44,10 @@ class ProgressTest(unittest.TestCase):
         self.assertEqual((0, 2), Progress(-2, 2).frac())
 
     def test_frac(self):
-        p = Progress(1, 2)
-        (a, b) = p.frac()
-        self.assertEqual(a, 1)
-        self.assertEqual(b, 2)
+        prg = Progress(1, 2)
+        (a_mem, b_mem) = prg.frac()
+        self.assertEqual(a_mem, 1)
+        self.assertEqual(b_mem, 2)
 
     def test_percent(self):
         self.assertEqual(self.not_started.percent(), 0)
@@ -98,38 +98,38 @@ class ProgressTest(unittest.TestCase):
     def test_to_js_detail_str(self):
         '''Test the Progress.to_js_detail_str() method'''
         f = Progress.to_js_detail_str
-        for p in (self.not_started, self.half_done, self.done):
-            self.assertEqual(f(p), str(p))
+        for prg in (self.not_started, self.half_done, self.done):
+            self.assertEqual(f(prg), str(prg))
         # But None should be encoded as 0
         self.assertEqual(f(None), "0")
 
     def test_add(self):
         '''Test the Progress.add_counts() method'''
-        p = Progress(0, 2)
-        p2 = Progress(1, 3)
-        p3 = Progress(2, 5)
-        pNone = None
+        prg1 = Progress(0, 2)
+        prg2 = Progress(1, 3)
+        prg3 = Progress(2, 5)
+        prg_none = None
         add = lambda a, b: Progress.add_counts(a, b).frac()
 
-        self.assertEqual(add(p, p), (0, 4))
-        self.assertEqual(add(p, p2), (1, 5))
-        self.assertEqual(add(p2, p3), (3, 8))
+        self.assertEqual(add(prg1, prg1), (0, 4))
+        self.assertEqual(add(prg1, prg2), (1, 5))
+        self.assertEqual(add(prg2, prg3), (3, 8))
 
-        self.assertEqual(add(p2, pNone), p2.frac())
-        self.assertEqual(add(pNone, p2), p2.frac())
+        self.assertEqual(add(prg2, prg_none), prg2.frac())
+        self.assertEqual(add(prg_none, prg2), prg2.frac())
 
     def test_equality(self):
         '''Test that comparing Progress objects for equality
         works correctly.'''
-        p = Progress(1, 2)
-        p2 = Progress(2, 4)
-        p3 = Progress(1, 2)
-        self.assertTrue(p == p3)
-        self.assertFalse(p == p2)
+        prg1 = Progress(1, 2)
+        prg2 = Progress(2, 4)
+        prg3 = Progress(1, 2)
+        self.assertTrue(prg1 == prg3)
+        self.assertFalse(prg1 == prg2)
 
         # Check != while we're at it
-        self.assertTrue(p != p2)
-        self.assertFalse(p != p3)
+        self.assertTrue(prg1 != prg2)
+        self.assertFalse(prg1 != prg3)
 
 
 class ModuleProgressTest(unittest.TestCase):
@@ -137,6 +137,6 @@ class ModuleProgressTest(unittest.TestCase):
     '''
     def test_xmodule_default(self):
         '''Make sure default get_progress exists, returns None'''
-        xm = x_module.XModule(Mock(), get_test_system(), DictFieldData({'location': 'a://b/c/d/e'}), Mock())
-        p = xm.get_progress()
-        self.assertEqual(p, None)
+        xmod = x_module.XModule(Mock(), get_test_system(), DictFieldData({'location': 'a://b/c/d/e'}), Mock())
+        prg = xmod.get_progress()
+        self.assertEqual(prg, None)

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

@@ -25,6 +25,7 @@ from django.http import HttpResponse, HttpResponseBadRequest, HttpResponseForbid
 from django.utils.html import strip_tags
 import string  # pylint: disable=W0402
 import random
+import urllib
 from util.json_request import JsonResponse
 from instructor.views.instructor_task_helpers import extract_email_features, extract_task_features
 
@@ -1696,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
     )
@@ -1791,13 +1792,15 @@ def proxy_legacy_analytics(request, course_id):
     analytics_name = request.GET.get('aname')
 
     # abort if misconfigured
-    if not (hasattr(settings, 'ANALYTICS_SERVER_URL') and hasattr(settings, 'ANALYTICS_API_KEY')):
+    if not (hasattr(settings, 'ANALYTICS_SERVER_URL') and
+            hasattr(settings, 'ANALYTICS_API_KEY') and
+            settings.ANALYTICS_SERVER_URL and settings.ANALYTICS_API_KEY):
         return HttpResponse("Analytics service not configured.", status=501)
 
     url = "{}get?aname={}&course_id={}&apikey={}".format(
         settings.ANALYTICS_SERVER_URL,
         analytics_name,
-        course_id.to_deprecated_string(),
+        urllib.quote(unicode(course_id)),
         settings.ANALYTICS_API_KEY,
     )
 

lms/djangoapps/instructor/views/legacy.py

@@ -11,6 +11,7 @@ import logging
 import os
 import re
 import requests
+import urllib
 
 from collections import defaultdict, OrderedDict
 from markupsafe import escape
@@ -910,7 +911,7 @@ def instructor_dashboard(request, course_id):
         """
         url = settings.ANALYTICS_SERVER_URL + \
             u"get?aname={}&course_id={}&apikey={}".format(
-                analytics_name, course_key.to_deprecated_string(), settings.ANALYTICS_API_KEY
+                analytics_name, urllib.quote(unicode(course_key)), settings.ANALYTICS_API_KEY
             )
         try:
             res = requests.get(url)