diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index 757eb83aa2..e5d7ec3aec 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -61,8 +61,7 @@ import instructor_task.api import instructor.views.api from instructor.views.api import require_finance_admin from instructor.tests.utils import FakeContentTask, FakeEmail, FakeEmailInfo -from instructor.views.api import generate_unique_password -from instructor.views.api import _split_input_list, common_exceptions_400 +from instructor.views.api import _split_input_list, common_exceptions_400, generate_unique_password from instructor_task.api_helper import AlreadyRunningError from openedx.core.djangoapps.course_groups.cohorts import set_course_cohort_settings @@ -208,14 +207,12 @@ class TestInstructorAPIDenyLevels(ModuleStoreTestCase, LoginEnrollmentTestCase): {'identifiers': 'foo@example.org', 'action': 'enroll'}), ('get_grading_config', {}), ('get_students_features', {}), - ('get_distribution', {}), ('get_student_progress_url', {'unique_student_identifier': self.user.username}), ('reset_student_attempts', {'problem_to_reset': self.problem_urlname, 'unique_student_identifier': self.user.email}), ('update_forum_role_membership', {'unique_student_identifier': self.user.email, 'rolename': 'Moderator', 'action': 'allow'}), ('list_forum_members', {'rolename': FORUM_ROLE_COMMUNITY_TA}), - ('proxy_legacy_analytics', {'aname': 'ProblemGradeDistribution'}), ('send_email', {'send_to': 'staff', 'subject': 'test', 'message': 'asdf'}), ('list_instructor_tasks', {}), ('list_background_email_tasks', {}), @@ -291,7 +288,7 @@ class TestInstructorAPIDenyLevels(ModuleStoreTestCase, LoginEnrollmentTestCase): for endpoint, args in self.staff_level_endpoints: # TODO: make these work - if endpoint in ['update_forum_role_membership', 'proxy_legacy_analytics', 'list_forum_members']: + if endpoint in ['update_forum_role_membership', 'list_forum_members']: continue self._access_endpoint( endpoint, @@ -320,7 +317,7 @@ class TestInstructorAPIDenyLevels(ModuleStoreTestCase, LoginEnrollmentTestCase): for endpoint, args in self.staff_level_endpoints: # TODO: make these work - if endpoint in ['update_forum_role_membership', 'proxy_legacy_analytics']: + if endpoint in ['update_forum_role_membership']: continue self._access_endpoint( endpoint, @@ -1978,7 +1975,7 @@ class TestInstructorAPILevelsDataDump(ModuleStoreTestCase, LoginEnrollmentTestCa percentage_discount=10, created_by=self.instructor, is_active=True) self.coupon.save() - #create testing invoice 1 + # Create testing invoice 1 self.sale_invoice_1 = Invoice.objects.create( total_amount=1234.32, company_name='Test1', company_contact_name='TestName', company_contact_email='Test@company.com', recipient_name='Testw', recipient_email='test1@test.com', customer_reference_number='2Fwe23S', @@ -2205,7 +2202,7 @@ class TestInstructorAPILevelsDataDump(ModuleStoreTestCase, LoginEnrollmentTestCa ) course_registration_code.save() - #create test invoice 2 + # Create test invoice 2 sale_invoice_2 = Invoice.objects.create( total_amount=1234.32, company_name='Test1', company_contact_name='TestName', company_contact_email='Test@company.com', recipient_name='Testw_2', recipient_email='test2@test.com', customer_reference_number='2Fwe23S', @@ -2602,46 +2599,6 @@ class TestInstructorAPILevelsDataDump(ModuleStoreTestCase, LoginEnrollmentTestCa " report when it is complete.".format(report_type=report_type) self.assertIn(already_running_status, response.content) - def test_get_distribution_no_feature(self): - """ - Test that get_distribution lists available features - when supplied no feature parameter. - """ - url = reverse('get_distribution', kwargs={'course_id': self.course.id.to_deprecated_string()}) - response = self.client.get(url) - self.assertEqual(response.status_code, 200) - res_json = json.loads(response.content) - self.assertEqual(type(res_json['available_features']), list) - - url = reverse('get_distribution', kwargs={'course_id': self.course.id.to_deprecated_string()}) - response = self.client.get(url + u'?feature=') - self.assertEqual(response.status_code, 200) - res_json = json.loads(response.content) - self.assertEqual(type(res_json['available_features']), list) - - def test_get_distribution_unavailable_feature(self): - """ - Test that get_distribution fails gracefully with - an unavailable feature. - """ - url = reverse('get_distribution', kwargs={'course_id': self.course.id.to_deprecated_string()}) - response = self.client.get(url, {'feature': 'robot-not-a-real-feature'}) - self.assertEqual(response.status_code, 400) - - def test_get_distribution_gender(self): - """ - Test that get_distribution fails gracefully with - an unavailable feature. - """ - url = reverse('get_distribution', kwargs={'course_id': self.course.id.to_deprecated_string()}) - response = self.client.get(url, {'feature': 'gender'}) - self.assertEqual(response.status_code, 200) - res_json = json.loads(response.content) - self.assertEqual(res_json['feature_results']['data']['m'], 6) - self.assertEqual(res_json['feature_results']['choices_display_names']['m'], 'Male') - self.assertEqual(res_json['feature_results']['data']['no_data'], 0) - self.assertEqual(res_json['feature_results']['choices_display_names']['no_data'], 'No Data') - def test_get_student_progress_url(self): """ Test that progress_url is in the successful response. """ url = reverse('get_student_progress_url', kwargs={'course_id': self.course.id.to_deprecated_string()}) @@ -3459,155 +3416,6 @@ class TestInstructorEmailContentList(ModuleStoreTestCase, LoginEnrollmentTestCas self.assertDictEqual(expected_info, returned_info) -@attr('shard_1') -@ddt.ddt -@override_settings(ANALYTICS_SERVER_URL="http://robotanalyticsserver.netbot:900/") -@override_settings(ANALYTICS_API_KEY="robot_api_key") -class TestInstructorAPIAnalyticsProxy(ModuleStoreTestCase, LoginEnrollmentTestCase): - """ - Test instructor analytics proxy endpoint. - """ - - class FakeProxyResponse(object): - """ Fake successful requests response object. """ - - def __init__(self): - self.status_code = requests.status_codes.codes.OK - self.content = '{"test_content": "robot test content"}' - - class FakeBadProxyResponse(object): - """ Fake strange-failed requests response object. """ - - def __init__(self): - self.status_code = 'notok.' - self.content = '{"test_content": "robot test content"}' - - def setUp(self): - super(TestInstructorAPIAnalyticsProxy, self).setUp() - - self.course = CourseFactory.create() - 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, 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, 501) - - @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): - """ - Test legacy analytics content proxyin, actg. - """ - 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) - - # check response - self.assertTrue(act.called) - expected_res = {'test_content': "robot test content"} - self.assertEqual(json.loads(response.content), expected_res) - - @patch.object(instructor.views.api.requests, 'get') - def test_analytics_proxy_reqfailed(self, act): - """ Test proxy when server reponds with failure. """ - act.return_value = self.FakeBadProxyResponse() - - 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, 500) - - @patch.object(instructor.views.api.requests, 'get') - def test_analytics_proxy_missing_param(self, act): - """ Test proxy when missing the aname query parameter. """ - act.return_value = self.FakeProxyResponse() - - url = reverse('proxy_legacy_analytics', kwargs={'course_id': self.course.id.to_deprecated_string()}) - response = self.client.get(url, {}) - self.assertEqual(response.status_code, 400) - self.assertFalse(act.called) - - @attr('shard_1') class TestInstructorAPIHelpers(TestCase): """ Test helpers for instructor.api """ diff --git a/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py b/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py index 2fad82fdd6..3694308b45 100644 --- a/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py +++ b/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py @@ -54,11 +54,11 @@ class TestInstructorDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase): return 'Enrollment data is now available in Example.'.format(unicode(self.course.id)) - def get_dashboard_demographic_message(self): + def get_dashboard_analytics_message(self): """ Returns expected dashboard demographic message with link to Insights. """ - return 'Demographic data is now available in Example.'.format(unicode(self.course.id)) def test_instructor_tab(self): @@ -157,38 +157,28 @@ class TestInstructorDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase): expected_message = self.get_dashboard_enrollment_message() self.assertTrue(expected_message in response.content) - @patch.dict(settings.FEATURES, {'DISPLAY_ANALYTICS_DEMOGRAPHICS': True}) @override_settings(ANALYTICS_DASHBOARD_URL='') @override_settings(ANALYTICS_DASHBOARD_NAME='') - def test_show_dashboard_demographic_data(self): + def test_dashboard_analytics_tab_not_shown(self): """ - Test enrollment demographic data is shown. + Test dashboard analytics tab isn't shown if insights isn't configured. """ response = self.client.get(self.url) - # demographic information displayed - self.assertTrue('data-feature="year_of_birth"' in response.content) - self.assertTrue('data-feature="gender"' in response.content) - self.assertTrue('data-feature="level_of_education"' in response.content) + analytics_section = '
  • Analytics
    • ' + self.assertFalse(analytics_section in response.content) - # dashboard link hidden - self.assertFalse(self.get_dashboard_demographic_message() in response.content) - - @patch.dict(settings.FEATURES, {'DISPLAY_ANALYTICS_DEMOGRAPHICS': False}) @override_settings(ANALYTICS_DASHBOARD_URL='http://example.com') @override_settings(ANALYTICS_DASHBOARD_NAME='Example') - def test_show_dashboard_demographic_message(self): + def test_dashboard_analytics_points_at_insights(self): """ - Test enrollment demographic dashboard message is shown and data is hidden. + Test analytics dashboard message is shown """ response = self.client.get(self.url) - - # demographics are hidden - self.assertFalse('data-feature="year_of_birth"' in response.content) - self.assertFalse('data-feature="gender"' in response.content) - self.assertFalse('data-feature="level_of_education"' in response.content) + analytics_section = '
  • Analytics
    • ' + self.assertTrue(analytics_section in response.content) # link to dashboard shown - expected_message = self.get_dashboard_demographic_message() + expected_message = self.get_dashboard_analytics_message() self.assertTrue(expected_message in response.content) def add_course_to_user_cart(self, cart, course_key): diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 432f399655..6027525c94 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -1645,56 +1645,6 @@ def get_anon_ids(request, course_id): # pylint: disable=unused-argument return csv_response(course_id.to_deprecated_string().replace('/', '-') + '-anon-ids.csv', header, rows) -@ensure_csrf_cookie -@cache_control(no_cache=True, no_store=True, must_revalidate=True) -@require_level('staff') -def get_distribution(request, course_id): - """ - Respond with json of the distribution of students over selected features which have choices. - - Ask for a feature through the `feature` query parameter. - If no `feature` is supplied, will return response with an - empty response['feature_results'] object. - A list of available will be available in the response['available_features'] - """ - course_id = SlashSeparatedCourseKey.from_deprecated_string(course_id) - feature = request.GET.get('feature') - # alternate notations of None - if feature in (None, 'null', ''): - feature = None - else: - feature = str(feature) - - available_features = instructor_analytics.distributions.AVAILABLE_PROFILE_FEATURES - # allow None so that requests for no feature can list available features - if feature not in available_features + (None,): - return HttpResponseBadRequest(strip_tags( - "feature '{}' not available.".format(feature) - )) - - response_payload = { - 'course_id': course_id.to_deprecated_string(), - 'queried_feature': feature, - 'available_features': available_features, - 'feature_display_names': instructor_analytics.distributions.DISPLAY_NAMES, - } - - p_dist = None - if feature is not None: - p_dist = instructor_analytics.distributions.profile_distribution(course_id, feature) - response_payload['feature_results'] = { - 'feature': p_dist.feature, - 'feature_display_name': p_dist.feature_display_name, - 'data': p_dist.data, - 'type': p_dist.type, - } - - if p_dist.type == 'EASY_CHOICE': - response_payload['feature_results']['choices_display_names'] = p_dist.choices_display_names - - return JsonResponse(response_payload) - - @ensure_csrf_cookie @cache_control(no_cache=True, no_store=True, must_revalidate=True) @common_exceptions_400 @@ -2361,62 +2311,6 @@ def update_forum_role_membership(request, course_id): return JsonResponse(response_payload) -@ensure_csrf_cookie -@cache_control(no_cache=True, no_store=True, must_revalidate=True) -@require_level('staff') -@require_query_params( - aname="name of analytic to query", -) -@common_exceptions_400 -def proxy_legacy_analytics(request, course_id): - """ - Proxies to the analytics cron job server. - - `aname` is a query parameter specifying which analytic to query. - """ - course_id = SlashSeparatedCourseKey.from_deprecated_string(course_id) - analytics_name = request.GET.get('aname') - - # abort if misconfigured - 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, - urllib.quote(unicode(course_id)), - settings.ANALYTICS_API_KEY, - ) - - try: - res = requests.get(url) - except Exception: # pylint: disable=broad-except - log.exception(u"Error requesting from analytics server at %s", url) - return HttpResponse("Error requesting from analytics server.", status=500) - - if res.status_code is 200: - payload = json.loads(res.content) - add_block_ids(payload) - content = json.dumps(payload) - # return the successful request content - return HttpResponse(content, content_type="application/json") - elif res.status_code is 404: - # forward the 404 and content - return HttpResponse(res.content, content_type="application/json", status=404) - else: - # 500 on all other unexpected status codes. - log.error( - u"Error fetching %s, code: %s, msg: %s", - url, res.status_code, res.content - ) - return HttpResponse( - "Error from analytics server ({}).".format(res.status_code), - status=500 - ) - - @require_POST def get_user_invoice_preference(request, course_id): # pylint: disable=unused-argument """ diff --git a/lms/djangoapps/instructor/views/api_urls.py b/lms/djangoapps/instructor/views/api_urls.py index 10ac79a528..f55d6df9f6 100644 --- a/lms/djangoapps/instructor/views/api_urls.py +++ b/lms/djangoapps/instructor/views/api_urls.py @@ -33,8 +33,6 @@ urlpatterns = patterns( 'instructor.views.api.sale_validation', name="sale_validation"), url(r'^get_anon_ids$', 'instructor.views.api.get_anon_ids', name="get_anon_ids"), - url(r'^get_distribution$', - 'instructor.views.api.get_distribution', name="get_distribution"), url(r'^get_student_progress_url$', 'instructor.views.api.get_student_progress_url', name="get_student_progress_url"), url(r'^reset_student_attempts$', @@ -71,8 +69,6 @@ urlpatterns = patterns( 'instructor.views.api.list_forum_members', name="list_forum_members"), url(r'^update_forum_role_membership$', 'instructor.views.api.update_forum_role_membership', name="update_forum_role_membership"), - url(r'^proxy_legacy_analytics$', - 'instructor.views.api.proxy_legacy_analytics', name="proxy_legacy_analytics"), url(r'^send_email$', 'instructor.views.api.send_email', name="send_email"), url(r'^change_due_date$', 'instructor.views.api.change_due_date', diff --git a/lms/envs/common.py b/lms/envs/common.py index 4d1e76ce68..833c8c6d61 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -334,9 +334,6 @@ FEATURES = { # and register for course. 'ALLOW_AUTOMATED_SIGNUPS': False, - # Display demographic data on the analytics tab in the instructor dashboard. - 'DISPLAY_ANALYTICS_DEMOGRAPHICS': True, - # Enable display of enrollment counts in instructor dash, analytics section 'DISPLAY_ANALYTICS_ENROLLMENTS': True, diff --git a/lms/static/coffee/src/instructor_dashboard/instructor_analytics.coffee b/lms/static/coffee/src/instructor_dashboard/instructor_analytics.coffee deleted file mode 100644 index 8f6ef7654d..0000000000 --- a/lms/static/coffee/src/instructor_dashboard/instructor_analytics.coffee +++ /dev/null @@ -1,241 +0,0 @@ -### -Analytics Section - -imports from other modules. -wrap in (-> ... apply) to defer evaluation -such that the value can be defined later than this assignment (file load order). -### - -plantTimeout = -> window.InstructorDashboard.util.plantTimeout.apply this, arguments -std_ajax_err = -> window.InstructorDashboard.util.std_ajax_err.apply this, arguments - - -class ProfileDistributionWidget - constructor: ({@$container, @feature, @title, @endpoint}) -> - # render template - template_params = - title: @title - feature: @feature - endpoint: @endpoint - template_html = $("#profile-distribution-widget-template").text() - @$container.html Mustache.render template_html, template_params - - reset_display: -> - @$container.find('.display-errors').empty() - @$container.find('.display-text').empty() - @$container.find('.display-graph').empty() - @$container.find('.display-table').empty() - - show_error: (msg) -> - @$container.find('.display-errors').text msg - - # display data - load: -> - @reset_display() - - @get_profile_distributions @feature, - error: std_ajax_err => - `// Translators: "Distribution" refers to a grade distribution. This error message appears when there is an error getting the data on grade distribution.` - @show_error gettext("Error fetching distribution.") - success: (data) => - feature_res = data.feature_results - if feature_res.type is 'EASY_CHOICE' - # display on SlickGrid - options = - enableCellNavigation: true - enableColumnReorder: false - forceFitColumns: true - - columns = [ - id: @feature - field: @feature - name: data.feature_display_names[@feature] - , - id: 'count' - field: 'count' - name: 'Count' - ] - - grid_data = _.map feature_res.data, (value, key) => - datapoint = {} - datapoint[@feature] = feature_res.choices_display_names[key] - datapoint['count'] = value - datapoint - - table_placeholder = $ '
    ', class: 'slickgrid' - @$container.find('.display-table').append table_placeholder - grid = new Slick.Grid(table_placeholder, grid_data, columns, options) - else if feature_res.feature is 'year_of_birth' - graph_placeholder = $ '
    ', class: 'graph-placeholder' - @$container.find('.display-graph').append graph_placeholder - - graph_data = _.map feature_res.data, (value, key) -> [parseInt(key), value] - - $.plot graph_placeholder, [ - data: graph_data - ] - else - console.warn("unable to show distribution #{feature_res.type}") - @show_error gettext('Unavailable metric display.') - - # fetch distribution data from server. - # `handler` can be either a callback for success - # or a mapping e.g. {success: ->, error: ->, complete: ->} - get_profile_distributions: (feature, handler) -> - settings = - dataType: 'json' - url: @endpoint - data: feature: feature - - if typeof handler is 'function' - _.extend settings, success: handler - else - _.extend settings, handler - - $.ajax settings - - -class GradeDistributionDisplay - constructor: ({@$container, @endpoint}) -> - template_params = {} - template_html = $('#grade-distributions-widget-template').text() - @$container.html Mustache.render template_html, template_params - @$problem_selector = @$container.find '.problem-selector' - - reset_display: -> - @$container.find('.display-errors').empty() - @$container.find('.display-text').empty() - @$container.find('.display-graph').empty() - - show_error: (msg) -> - @$container.find('.display-errors').text msg - - load: -> - @get_grade_distributions - error: std_ajax_err => @show_error gettext("Error fetching grade distributions.") - success: (data) => - time_updated = gettext("Last Updated: <%= timestamp %>") - full_time_updated = _.template(time_updated, {timestamp: data.time}) - @$container.find('.last-updated').text full_time_updated - - # populate selector - @$problem_selector.empty() - for {module_id, block_id, grade_info} in data.data - label = block_id - label ?= module_id - - @$problem_selector.append $ '