From a5e89224c8159fbb99cce3564c01f933b108de0d Mon Sep 17 00:00:00 2001 From: Sameen Fatima Date: Fri, 2 Feb 2024 18:41:14 +0500 Subject: [PATCH 1/2] feat: fetch LMS_USER_ID from LMS --- license_manager/apps/api/mixins.py | 11 +++++-- license_manager/apps/api/v1/views.py | 14 +++++++++ license_manager/apps/api_client/lms.py | 41 ++++++++++++++++++++++++++ 3 files changed, 64 insertions(+), 2 deletions(-) create mode 100644 license_manager/apps/api_client/lms.py diff --git a/license_manager/apps/api/mixins.py b/license_manager/apps/api/mixins.py index ad622d24..43d78bc7 100644 --- a/license_manager/apps/api/mixins.py +++ b/license_manager/apps/api/mixins.py @@ -1,4 +1,6 @@ from functools import cached_property +from rest_framework.exceptions import ParseError, status +from license_manager.apps.api_client.lms import LMSApiClient from license_manager.apps.api import utils @@ -18,9 +20,14 @@ def decoded_jwt(self): return utils.get_decoded_jwt(self.request) - @property + @cached_property def lms_user_id(self): - return utils.get_key_from_jwt(self.decoded_jwt, 'user_id') + try: + return utils.get_key_from_jwt(self.decoded_jwt, 'user_id') + except ParseError: + lms_client = LMSApiClient() + user_id = lms_client.fetch_lms_user_id(self.request.user.email) + return user_id @property def user_email(self): diff --git a/license_manager/apps/api/v1/views.py b/license_manager/apps/api/v1/views.py index 8c0b8940..064fa4e5 100644 --- a/license_manager/apps/api/v1/views.py +++ b/license_manager/apps/api/v1/views.py @@ -1,6 +1,7 @@ import logging from collections import OrderedDict from contextlib import suppress +from functools import cached_property from uuid import uuid4 from celery import chain @@ -45,6 +46,7 @@ track_license_changes_task, update_user_email_for_licenses_task, ) +from license_manager.apps.api_client.lms import LMSApiClient from license_manager.apps.subscriptions import constants, event_utils from license_manager.apps.subscriptions.api import revoke_license from license_manager.apps.subscriptions.exceptions import ( @@ -1186,6 +1188,18 @@ def _check_param_has_type(self, param_name, required_type): self.validation_errors.append(param_name) return param_value + """" + Another approach would be to override this property at child class level. + @cached_property + def lms_user_id(self): + try: + return utils.get_key_from_jwt(self.decoded_jwt, 'user_id') + except ParseError: + lms_client = LMSApiClient() + user_id = lms_client.fetch_lms_user_id(self.request.user.email) + return user_id + """ + @property def requested_notify_learners(self): return self._check_param_has_type('notify', bool) diff --git a/license_manager/apps/api_client/lms.py b/license_manager/apps/api_client/lms.py new file mode 100644 index 00000000..34cf27d7 --- /dev/null +++ b/license_manager/apps/api_client/lms.py @@ -0,0 +1,41 @@ +import logging + +from django.conf import settings +import requests + +from license_manager.apps.api_client.base_oauth import BaseOAuthClient +logger = logging.getLogger(__name__) + + +class LMSApiClient(BaseOAuthClient): + """ + API client for calls to the LMS. + """ + api_base_url = settings.LMS_URL + user_details_endpoint = api_base_url + '/api/user/v1/accounts' + + def fetch_lms_user_id(self, email): + """ + Fetch user details for the specified user email. + + Arguments: + email (str): Email of the user for which we want to fetch details for. + + Returns: + str: lms_user_id of the user. + """ + # {base_api_url}/api/user/v1/accounts?email=edx@example.com + try: + query_params = {'email': email} + response = self.client.get(self.user_details_endpoint, params=query_params) + response.raise_for_status() + response_json = response.json() + return response_json[0].get('id') + except requests.exceptions.HTTPError as exc: + logger.error( + 'Failed to fetch user details for user {email} because {reason}'.format( + email=email, + reason=str(exc), + ) + ) + raise exc From cc9e558c6d2e45e620a93c4bc75a65e512f44e7c Mon Sep 17 00:00:00 2001 From: zamanafzal Date: Mon, 12 Feb 2024 21:29:42 +0500 Subject: [PATCH 2/2] feat: added test cases and make fixes --- license_manager/apps/api/mixins.py | 8 ++++++-- license_manager/apps/api/v1/tests/test_views.py | 14 ++++++++++---- license_manager/apps/api/v1/views.py | 14 -------------- license_manager/apps/api_client/lms.py | 4 +++- 4 files changed, 19 insertions(+), 21 deletions(-) diff --git a/license_manager/apps/api/mixins.py b/license_manager/apps/api/mixins.py index 43d78bc7..1aee9356 100644 --- a/license_manager/apps/api/mixins.py +++ b/license_manager/apps/api/mixins.py @@ -1,8 +1,9 @@ from functools import cached_property -from rest_framework.exceptions import ParseError, status -from license_manager.apps.api_client.lms import LMSApiClient + +from rest_framework.exceptions import ParseError from license_manager.apps.api import utils +from license_manager.apps.api_client.lms import LMSApiClient class UserDetailsFromJwtMixin: @@ -22,6 +23,9 @@ def decoded_jwt(self): @cached_property def lms_user_id(self): + """ + Retrieve the LMS user ID. + """ try: return utils.get_key_from_jwt(self.decoded_jwt, 'user_id') except ParseError: diff --git a/license_manager/apps/api/v1/tests/test_views.py b/license_manager/apps/api/v1/tests/test_views.py index d1150d0d..df1409bb 100644 --- a/license_manager/apps/api/v1/tests/test_views.py +++ b/license_manager/apps/api/v1/tests/test_views.py @@ -3198,17 +3198,23 @@ def test_get_subsidy_missing_course_key(self): assert response.status_code == status.HTTP_400_BAD_REQUEST @mock.patch('license_manager.apps.api.v1.views.utils.get_decoded_jwt') - def test_get_subsidy_no_jwt(self, mock_get_decoded_jwt): + @mock.patch('license_manager.apps.api.mixins.LMSApiClient') + def test_get_subsidy_no_jwt(self, MockLMSApiClient, mock_get_decoded_jwt): """ - Verify the view returns a 400 if the user_id could not be found in the JWT. + Verify the view makes an API call to fetch lmsUserId if user_id could not be found in the JWT. """ self._assign_learner_roles() mock_get_decoded_jwt.return_value = {} url = self._get_url_with_params() + + # Mock the behavior of LMSApiClient to return a sample user ID + mock_lms_client = MockLMSApiClient.return_value + mock_lms_client.fetch_lms_user_id.return_value = 443 response = self.api_client.get(url) + assert status.HTTP_404_NOT_FOUND == response.status_code - assert status.HTTP_400_BAD_REQUEST == response.status_code - assert '`user_id` is required and could not be found in your jwt' in str(response.content) + # Assert that the LMSApiClient.fetch_lms_user_id method was called once with the correct argument + mock_lms_client.fetch_lms_user_id.assert_called_once_with(self.user.email) def test_get_subsidy_no_subscription_for_enterprise_customer(self): """ diff --git a/license_manager/apps/api/v1/views.py b/license_manager/apps/api/v1/views.py index 064fa4e5..8c0b8940 100644 --- a/license_manager/apps/api/v1/views.py +++ b/license_manager/apps/api/v1/views.py @@ -1,7 +1,6 @@ import logging from collections import OrderedDict from contextlib import suppress -from functools import cached_property from uuid import uuid4 from celery import chain @@ -46,7 +45,6 @@ track_license_changes_task, update_user_email_for_licenses_task, ) -from license_manager.apps.api_client.lms import LMSApiClient from license_manager.apps.subscriptions import constants, event_utils from license_manager.apps.subscriptions.api import revoke_license from license_manager.apps.subscriptions.exceptions import ( @@ -1188,18 +1186,6 @@ def _check_param_has_type(self, param_name, required_type): self.validation_errors.append(param_name) return param_value - """" - Another approach would be to override this property at child class level. - @cached_property - def lms_user_id(self): - try: - return utils.get_key_from_jwt(self.decoded_jwt, 'user_id') - except ParseError: - lms_client = LMSApiClient() - user_id = lms_client.fetch_lms_user_id(self.request.user.email) - return user_id - """ - @property def requested_notify_learners(self): return self._check_param_has_type('notify', bool) diff --git a/license_manager/apps/api_client/lms.py b/license_manager/apps/api_client/lms.py index 34cf27d7..21b8eed3 100644 --- a/license_manager/apps/api_client/lms.py +++ b/license_manager/apps/api_client/lms.py @@ -1,9 +1,11 @@ import logging -from django.conf import settings import requests +from django.conf import settings from license_manager.apps.api_client.base_oauth import BaseOAuthClient + + logger = logging.getLogger(__name__)