From 68b0bf15d4f1752b7502dc5f337613d2d803cdae Mon Sep 17 00:00:00 2001 From: Sagirov Eugeniy Date: Tue, 5 Apr 2022 20:57:19 +0300 Subject: [PATCH] refactor: Remove EdxRestApiClient usage in edx-analytics-dashboard --- .../management/commands/generate_data.py | 14 +++---- analyticsdataserver/clients.py | 34 ++++++++++++----- analyticsdataserver/settings/local.py | 1 - analyticsdataserver/tests/test_clients.py | 37 ++++++++++++------- 4 files changed, 54 insertions(+), 32 deletions(-) diff --git a/analytics_data_api/management/commands/generate_data.py b/analytics_data_api/management/commands/generate_data.py index 13bb2248..9d1ce43c 100644 --- a/analytics_data_api/management/commands/generate_data.py +++ b/analytics_data_api/management/commands/generate_data.py @@ -298,14 +298,12 @@ def generate_tags_distribution_data(course_id, database): def fetch_videos_from_course_blocks(course_id): logger.info("Fetching video ids from Course Blocks API...") - try: - api_base_url = settings.LMS_BASE_URL + 'api/courses/v1/' - except AttributeError: - logger.warning("LMS_BASE_URL is not configured! Cannot get video ids.") - return None - logger.info("Assuming the Course Blocks API is hosted at: %s", api_base_url) - - blocks_api = CourseBlocksApiClient(api_base_url, settings.COURSE_BLOCK_API_AUTH_TOKEN, timeout=5) + + blocks_api = CourseBlocksApiClient( + settings.BACKEND_SERVICE_EDX_OAUTH2_PROVIDER_URL, + settings.BACKEND_SERVICE_EDX_OAUTH2_KEY, + settings.BACKEND_SERVICE_EDX_OAUTH2_SECRET, + ) return blocks_api.all_videos(course_id) diff --git a/analyticsdataserver/clients.py b/analyticsdataserver/clients.py index 3f0a5964..a81ffd1a 100644 --- a/analyticsdataserver/clients.py +++ b/analyticsdataserver/clients.py @@ -1,17 +1,18 @@ import logging +from urllib.parse import urljoin -from edx_rest_api_client.client import EdxRestApiClient -from edx_rest_api_client.exceptions import HttpClientError +from django.conf import settings +from edx_rest_api_client.client import OAuthAPIClient from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import UsageKey -from requests.exceptions import RequestException +from requests.exceptions import HTTPError, RequestException from analyticsdataserver.utils import temp_log_level logger = logging.getLogger(__name__) -class CourseBlocksApiClient(EdxRestApiClient): +class CourseBlocksApiClient(OAuthAPIClient): """ This class is a sub-class of the edX Rest API Client (https://github.com/edx/edx-rest-api-client). @@ -22,15 +23,28 @@ class CourseBlocksApiClient(EdxRestApiClient): Currently, this client is only used for a local-only developer script (generate_fake_course_data). """ - def __init__(self, url, access_token, timeout): - super().__init__(url, oauth_access_token=access_token, timeout=timeout) - def all_videos(self, course_id): try: logger.debug('Retrieving course video blocks for course_id: %s', course_id) - response = self.blocks.get(course_id=course_id, all_blocks=True, depth='all', block_types_filter='video') + + try: + api_base_url = urljoin(settings.LMS_BASE_URL + '/', 'api/courses/v1/') + except AttributeError: + logger.warning("LMS_BASE_URL is not configured! Cannot get video ids.") + return None + logger.info("Assuming the Course Blocks API is hosted at: %s", api_base_url) + + blocks_kwargs = { + 'course_id': course_id, + 'all_blocks': True, + 'depth': 'all', + 'block_types_filter': 'video' + } + response = self.get(urljoin(api_base_url, 'blocks/'), params=blocks_kwargs) + response.raise_for_status() + data = response.json() logger.info("Successfully authenticated with the Course Blocks API.") - except HttpClientError as e: + except HTTPError as e: if e.response.status_code == 401: logger.warning("Course Blocks API failed to return video ids (%s). " "See README for instructions on how to authenticate the API with your local LMS.", @@ -50,7 +64,7 @@ def all_videos(self, course_id): # (The UsageKey utility still works despite the import errors, so I think the errors are not important). with temp_log_level('stevedore', log_level=logging.CRITICAL): videos = [] - for video in response['blocks'].values(): + for video in data['blocks'].values(): try: encoded_id = UsageKey.from_string(video['id']).html_id() except InvalidKeyError: diff --git a/analyticsdataserver/settings/local.py b/analyticsdataserver/settings/local.py index 23aca865..ce2ec376 100644 --- a/analyticsdataserver/settings/local.py +++ b/analyticsdataserver/settings/local.py @@ -85,7 +85,6 @@ # These two settings are used in generate_fake_course_data.py. # Replace with correct values to generate local fake video data. LMS_BASE_URL = 'http://localhost:18000/' # the base URL for your running local LMS instance -COURSE_BLOCK_API_AUTH_TOKEN = 'paste auth token here' # see README for instructions on how to configure this value # In Insights, we run this API as a separate service called "analyticsapi" to run acceptance/integration tests. Docker # saves the service name as a host in the Insights container so it can reach the API by requesting http://analyticsapi/. diff --git a/analyticsdataserver/tests/test_clients.py b/analyticsdataserver/tests/test_clients.py index d6a47175..ccb9442a 100644 --- a/analyticsdataserver/tests/test_clients.py +++ b/analyticsdataserver/tests/test_clients.py @@ -1,7 +1,8 @@ -import json import unittest.mock as mock +from urllib.parse import urljoin import responses +from django.conf import settings from django.test import TestCase from requests.exceptions import ConnectionError as RequestsConnectionError @@ -9,20 +10,30 @@ class ClientTests(TestCase): - @mock.patch('analyticsdataserver.clients.EdxRestApiClient') - def setUp(self, restApiClientMock): - self.client = CourseBlocksApiClient('http://example.com/', 'token', 5) + + def setUp(self): + self.client = CourseBlocksApiClient(settings.LMS_BASE_URL, 'client_id', 'client_secret') + responses.add( + responses.POST, + urljoin(settings.LMS_BASE_URL, 'oauth2/access_token'), + status=200, + json={ + 'access_token': 'test_access_token', + 'expires_in': 10, + }, + content_type='application/json' + ) @responses.activate def test_all_videos(self): - responses.add(responses.GET, 'http://example.com/blocks/', body=json.dumps({'blocks': { + responses.add(responses.GET, urljoin(settings.LMS_BASE_URL, 'api/courses/v1/blocks/'), json={'blocks': { 'block-v1:edX+DemoX+Demo_Course+type@video+block@5c90cffecd9b48b188cbfea176bf7fe9': { 'id': 'block-v1:edX+DemoX+Demo_Course+type@video+block@5c90cffecd9b48b188cbfea176bf7fe9' }, 'block-v1:edX+DemoX+Demo_Course+type@video+block@7e9b434e6de3435ab99bd3fb25bde807': { 'id': 'block-v1:edX+DemoX+Demo_Course+type@video+block@7e9b434e6de3435ab99bd3fb25bde807' } - }}), status=200, content_type='application/json') + }}, status=200, content_type='application/json') videos = self.client.all_videos('course_id') self.assertListEqual(videos, [ { @@ -38,7 +49,7 @@ def test_all_videos(self): @responses.activate @mock.patch('analyticsdataserver.clients.logger') def test_all_videos_401(self, logger): - responses.add(responses.GET, 'http://example.com/blocks/', status=401, content_type='application/json') + responses.add(responses.GET, urljoin(settings.LMS_BASE_URL, 'api/courses/v1/blocks/'), status=401, content_type='application/json') videos = self.client.all_videos('course_id') logger.warning.assert_called_with( 'Course Blocks API failed to return video ids (%s). ' @@ -48,7 +59,7 @@ def test_all_videos_401(self, logger): @responses.activate @mock.patch('analyticsdataserver.clients.logger') def test_all_videos_404(self, logger): - responses.add(responses.GET, 'http://example.com/blocks/', status=404, content_type='application/json') + responses.add(responses.GET, urljoin(settings.LMS_BASE_URL, 'api/courses/v1/blocks/'), status=404, content_type='application/json') videos = self.client.all_videos('course_id') logger.warning.assert_called_with('Course Blocks API failed to return video ids (%s). ' 'Does the course exist in the LMS?', 404) @@ -57,7 +68,7 @@ def test_all_videos_404(self, logger): @responses.activate @mock.patch('analyticsdataserver.clients.logger') def test_all_videos_500(self, logger): - responses.add(responses.GET, 'http://example.com/blocks/', status=418, content_type='application/json') + responses.add(responses.GET, urljoin(settings.LMS_BASE_URL, 'api/courses/v1/blocks/'), status=418, content_type='application/json') videos = self.client.all_videos('course_id') logger.warning.assert_called_with('Course Blocks API failed to return video ids (%s).', 418) self.assertEqual(videos, None) @@ -66,22 +77,22 @@ def test_all_videos_500(self, logger): @mock.patch('analyticsdataserver.clients.logger') def test_all_videos_connection_error(self, logger): exception = RequestsConnectionError('LMS is dead') - responses.add(responses.GET, 'http://example.com/blocks/', body=exception) + responses.add(responses.GET, urljoin(settings.LMS_BASE_URL, 'api/courses/v1/blocks/'), body=exception) videos = self.client.all_videos('course_id') logger.warning.assert_called_with('Course Blocks API request failed. Is the LMS running?: %s', str(exception)) self.assertEqual(videos, None) @responses.activate def test_all_videos_pass_through_bad_id(self): - responses.add(responses.GET, 'http://example.com/blocks/', body=json.dumps({'blocks': { + responses.add(responses.GET, urljoin(settings.LMS_BASE_URL, 'api/courses/v1/blocks/'), json={'blocks': { 'block-v1:edX+DemoX+Demo_Course+type@video+block@5c90cffecd9b48b188cbfea176bf7fe9': { 'id': 'bad_key' }, 'block-v1:edX+DemoX+Demo_Course+type@video+block@7e9b434e6de3435ab99bd3fb25bde807': { 'id': 'bad_key' } - }}), status=200, content_type='application/json') - responses.add(responses.GET, 'http://example.com/blocks/', status=200, content_type='application/json') + }}, status=200, content_type='application/json') + responses.add(responses.GET, urljoin(settings.LMS_BASE_URL, 'api/courses/v1/blocks/'), status=200, content_type='application/json') videos = self.client.all_videos('course_id') self.assertListEqual(videos, [ {