From 289e682b8f5fd4770aff5fe9147c365a5074a43e Mon Sep 17 00:00:00 2001 From: Eugene Dyudyunov Date: Mon, 9 May 2022 19:48:26 +0300 Subject: [PATCH] FC-0001: Remove old EdxRestAPIClient usage across the platform (#30301) * refactor: remove EdxRestAPIClient * test: update tests according to EdxRestAPIClient removal * fix: remove unused import --- common/djangoapps/course_modes/helpers.py | 23 ++- common/djangoapps/student/models.py | 57 +++--- .../djangoapps/student/tests/test_refunds.py | 4 +- lms/djangoapps/commerce/api/v0/views.py | 25 ++- lms/djangoapps/commerce/api/v1/views.py | 17 +- ...rs_for_old_enterprise_course_enrollment.py | 23 +-- lms/djangoapps/commerce/tests/__init__.py | 25 ++- lms/djangoapps/commerce/utils.py | 37 +++- lms/djangoapps/verify_student/views.py | 36 ++-- lms/envs/common.py | 2 +- lms/envs/production.py | 2 +- openedx/core/djangoapps/api_admin/views.py | 127 ++++++++---- .../management/commands/cache_programs.py | 40 ++-- .../djangoapps/catalog/tests/test_utils.py | 14 +- openedx/core/djangoapps/catalog/utils.py | 129 +++++++----- openedx/core/djangoapps/commerce/utils.py | 21 +- .../djangoapps/credentials/tasks/v1/tasks.py | 76 ++++--- .../credentials/tests/test_tasks.py | 5 +- .../credentials/tests/test_utils.py | 2 +- openedx/core/djangoapps/credentials/utils.py | 39 +++- openedx/core/djangoapps/credit/email_utils.py | 10 +- .../core/djangoapps/credit/tests/test_api.py | 13 +- openedx/core/djangoapps/programs/tasks.py | 190 ++++++++++-------- .../djangoapps/programs/tests/test_tasks.py | 48 +++-- openedx/core/djangoapps/programs/utils.py | 30 +-- .../user_api/accounts/settings_views.py | 20 +- .../djangoapps/user_api/accounts/views.py | 18 +- .../commands/sync_hubspot_contacts.py | 17 +- openedx/core/djangoapps/video_pipeline/api.py | 23 ++- .../video_pipeline/tests/test_api.py | 18 +- openedx/core/lib/edx_api_utils.py | 107 +--------- openedx/core/lib/tests/test_edx_api_utils.py | 179 +++++++++++------ openedx/features/enterprise_support/api.py | 87 ++++---- .../features/enterprise_support/signals.py | 21 +- .../enterprise_support/tests/test_api.py | 62 +++--- .../enterprise_support/tests/test_signals.py | 20 +- 36 files changed, 869 insertions(+), 698 deletions(-) diff --git a/common/djangoapps/course_modes/helpers.py b/common/djangoapps/course_modes/helpers.py index c9a7e0926e..7724d402aa 100644 --- a/common/djangoapps/course_modes/helpers.py +++ b/common/djangoapps/course_modes/helpers.py @@ -2,15 +2,14 @@ import logging +from urllib.parse import urljoin + from django.conf import settings from django.utils.translation import gettext_lazy as _ -from urllib.parse import urljoin # lint-amnesty, pylint: disable=wrong-import-order - -from requests.exceptions import ConnectionError, Timeout # pylint: disable=redefined-builtin -from slumber.exceptions import SlumberBaseException +from requests.exceptions import RequestException from common.djangoapps.course_modes.models import CourseMode -from openedx.core.djangoapps.commerce.utils import ecommerce_api_client +from openedx.core.djangoapps.commerce.utils import get_ecommerce_api_base_url, get_ecommerce_api_client DISPLAY_VERIFIED = "verified" DISPLAY_HONOR = "honor" @@ -89,11 +88,17 @@ def get_course_final_price(user, sku, course_price): """ price_details = {} try: - price_details = ecommerce_api_client(user).baskets.calculate.get( - sku=[sku], - username=user.username, + api_url = urljoin(f"{get_ecommerce_api_base_url()}/", "baskets/calculate/") + response = get_ecommerce_api_client(user).get( + api_url, + params={ + "sku": [sku], + "username": user.username, + } ) - except (SlumberBaseException, ConnectionError, Timeout) as exc: + response.raise_for_status() + price_details = response.json() + except RequestException as exc: LOGGER.info( '[e-commerce calculate endpoint] Exception raise for sku [%s] - user [%s] and exception: %s', sku, diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 67870e48a3..5ef8ea06b4 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -11,8 +11,6 @@ file and check it in at the same time as your model changes. To do that, 3. Add the migration file created in edx-platform/common/djangoapps/student/migrations/ """ - -import crum import hashlib # lint-amnesty, pylint: disable=wrong-import-order import json # lint-amnesty, pylint: disable=wrong-import-order import logging # lint-amnesty, pylint: disable=wrong-import-order @@ -21,8 +19,9 @@ from collections import defaultdict, namedtuple # lint-amnesty, pylint: disable from datetime import date, datetime, timedelta # lint-amnesty, pylint: disable=wrong-import-order from functools import total_ordering # lint-amnesty, pylint: disable=wrong-import-order from importlib import import_module # lint-amnesty, pylint: disable=wrong-import-order -from urllib.parse import urlencode # lint-amnesty, pylint: disable=wrong-import-order +from urllib.parse import urlencode, urljoin +import crum from config_models.models import ConfigurationModel from django.apps import apps from django.conf import settings @@ -37,42 +36,35 @@ from django.db.models import Count, Index, Q from django.db.models.signals import post_save, pre_save from django.db.utils import ProgrammingError from django.dispatch import receiver - from django.utils.functional import cached_property from django.utils.translation import gettext_lazy as _ from django.utils.translation import gettext_noop from django_countries.fields import CountryField -from edx_django_utils.cache import RequestCache, TieredCache, get_cache_key from edx_django_utils import monitoring -from edx_rest_api_client.exceptions import SlumberBaseException +from edx_django_utils.cache import RequestCache, TieredCache, get_cache_key from eventtracking import tracker from model_utils.models import TimeStampedModel from opaque_keys.edx.django.models import CourseKeyField, LearningContextKeyField from opaque_keys.edx.keys import CourseKey -from pytz import UTC, timezone -from simple_history.models import HistoricalRecords -from slumber.exceptions import HttpClientError, HttpServerError -from user_util import user_util - -from openedx_events.learning.data import ( - CourseData, - CourseEnrollmentData, - UserData, - UserPersonalData, -) +from openedx_events.learning.data import CourseData, CourseEnrollmentData, UserData, UserPersonalData from openedx_events.learning.signals import ( COURSE_ENROLLMENT_CHANGED, COURSE_ENROLLMENT_CREATED, COURSE_UNENROLLMENT_COMPLETED, ) from openedx_filters.learning.filters import CourseEnrollmentStarted, CourseUnenrollmentStarted +from pytz import UTC, timezone +from requests.exceptions import HTTPError, RequestException +from simple_history.models import HistoricalRecords +from user_util import user_util + import openedx.core.djangoapps.django_comment_common.comment_client as cc from common.djangoapps.course_modes.models import CourseMode, get_cosmetic_verified_display_price -from common.djangoapps.student.emails import send_proctoring_requirements_email from common.djangoapps.student.email_helpers import ( generate_proctoring_requirements_email_context, - should_send_proctoring_requirements_email + should_send_proctoring_requirements_email, ) +from common.djangoapps.student.emails import send_proctoring_requirements_email from common.djangoapps.student.signals import ENROLL_STATUS_CHANGE, ENROLLMENT_TRACK_UPDATED, UNENROLL_DONE from common.djangoapps.track import contexts, segment from common.djangoapps.util.model_utils import emit_field_changed_events, get_changed_fields_dict @@ -1972,6 +1964,7 @@ class CourseEnrollment(models.Model): # Due to circular import issues this import was placed close to usage. To move this to the # top of the file would require a large scale refactor of the refund code. import lms.djangoapps.certificates.api + # If the student has already been given a certificate in a non refundable status they should not be refunded certificate = lms.djangoapps.certificates.api.get_certificate_for_user_id( self.user, @@ -2052,7 +2045,7 @@ class CourseEnrollment(models.Model): """ # NOTE: This is here to avoid circular references - from openedx.core.djangoapps.commerce.utils import ecommerce_api_client + from openedx.core.djangoapps.commerce.utils import get_ecommerce_api_base_url, get_ecommerce_api_client order_number = self.get_order_attribute_value('order_number') if not order_number: return None @@ -2065,23 +2058,21 @@ class CourseEnrollment(models.Model): else: try: # response is not cached, so make a call to ecommerce to fetch order details - order = ecommerce_api_client(self.user).orders(order_number).get() - except HttpClientError: + api_url = urljoin(f"{get_ecommerce_api_base_url()}/", f"orders/{order_number}/") + response = get_ecommerce_api_client(self.user).get(api_url) + response.raise_for_status() + order = response.json() + except HTTPError as err: log.warning( - "Encountered HttpClientError while getting order details from ecommerce. " - "Order={number} and user {user}".format(number=order_number, user=self.user.id)) + "Encountered HTTPError while getting order details from ecommerce. " + "Status code was %d, Order=%s and user %s", err.response.status_code, order_number, self.user.id + ) return None - - except HttpServerError: - log.warning( - "Encountered HttpServerError while getting order details from ecommerce. " - "Order={number} and user {user}".format(number=order_number, user=self.user.id)) - return None - - except SlumberBaseException: + except RequestException: log.warning( "Encountered an error while getting order details from ecommerce. " - "Order={number} and user {user}".format(number=order_number, user=self.user.id)) + "Order=%s and user %s", order_number, self.user.id + ) return None cache_time_out = getattr(settings, 'ECOMMERCE_ORDERS_API_CACHE_TIMEOUT', 3600) diff --git a/common/djangoapps/student/tests/test_refunds.py b/common/djangoapps/student/tests/test_refunds.py index 6493dea42a..54fef6ce9b 100644 --- a/common/djangoapps/student/tests/test_refunds.py +++ b/common/djangoapps/student/tests/test_refunds.py @@ -235,7 +235,7 @@ class RefundableTest(SharedModuleStoreTestCase): ) assert self.enrollment.is_order_voucher_refundable() is False - @patch('openedx.core.djangoapps.commerce.utils.ecommerce_api_client') + @patch('openedx.core.djangoapps.commerce.utils.get_ecommerce_api_client') def test_get_order_attribute_from_ecommerce(self, mock_ecommerce_api_client): """ Assert that the get_order_attribute_from_ecommerce method returns order details if it's already cached, @@ -254,7 +254,7 @@ class RefundableTest(SharedModuleStoreTestCase): assert self.enrollment.get_order_attribute_from_ecommerce("vouchers") == order_details["vouchers"] mock_ecommerce_api_client.assert_not_called() - @patch('openedx.core.djangoapps.commerce.utils.ecommerce_api_client') + @patch('openedx.core.djangoapps.commerce.utils.get_ecommerce_api_client') def test_refund_cutoff_date_with_date_placed_attr(self, mock_ecommerce_api_client): """ Assert that the refund_cutoff_date returns order placement date if order:date_placed diff --git a/lms/djangoapps/commerce/api/v0/views.py b/lms/djangoapps/commerce/api/v0/views.py index 26dafe0343..b0fa4fcc6e 100644 --- a/lms/djangoapps/commerce/api/v0/views.py +++ b/lms/djangoapps/commerce/api/v0/views.py @@ -2,12 +2,13 @@ import logging +from urllib.parse import urljoin from django.urls import reverse -from edx_rest_api_client import exceptions from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey +from requests.exceptions import HTTPError from rest_framework.authentication import SessionAuthentication from rest_framework.permissions import IsAuthenticated from rest_framework.status import HTTP_406_NOT_ACCEPTABLE, HTTP_409_CONFLICT @@ -18,7 +19,7 @@ from common.djangoapps.entitlements.models import CourseEntitlement from common.djangoapps.student.models import CourseEnrollment from common.djangoapps.util.json_request import JsonResponse from lms.djangoapps.courseware import courses -from openedx.core.djangoapps.commerce.utils import ecommerce_api_client +from openedx.core.djangoapps.commerce.utils import get_ecommerce_api_base_url, get_ecommerce_api_client from openedx.core.djangoapps.embargo import api as embargo_api from openedx.core.djangoapps.enrollments.api import add_enrollment from openedx.core.djangoapps.enrollments.views import EnrollmentCrossDomainSessionAuth @@ -159,15 +160,23 @@ class BasketsView(APIView): class BasketOrderView(APIView): - """ Retrieve the order associated with a basket. """ + """ + Retrieve the order associated with a basket. + """ authentication_classes = (SessionAuthentication,) permission_classes = (IsAuthenticated,) def get(self, request, *_args, **kwargs): - """ HTTP handler. """ + """ + HTTP handler. + """ try: - order = ecommerce_api_client(request.user).baskets(kwargs['basket_id']).order.get() - return JsonResponse(order) - except exceptions.HttpNotFoundError: - return JsonResponse(status=404) + api_url = urljoin(f"{get_ecommerce_api_base_url()}/", f"baskets/{kwargs['basket_id']}/order/") + response = get_ecommerce_api_client(request.user).get(api_url) + response.raise_for_status() + return JsonResponse(response.json()) + except HTTPError as err: + if err.response.status_code == 404: + return JsonResponse(status=404) + raise diff --git a/lms/djangoapps/commerce/api/v1/views.py b/lms/djangoapps/commerce/api/v1/views.py index c85719eccd..0e634ee362 100644 --- a/lms/djangoapps/commerce/api/v1/views.py +++ b/lms/djangoapps/commerce/api/v1/views.py @@ -4,11 +4,12 @@ Commerce views import logging +from urllib.parse import urljoin from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user from django.http import Http404 -from edx_rest_api_client import exceptions from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication +from requests.exceptions import HTTPError from rest_framework.authentication import SessionAuthentication from rest_framework.generics import ListAPIView, RetrieveUpdateAPIView from rest_framework.permissions import IsAuthenticated @@ -16,7 +17,7 @@ from rest_framework.views import APIView from common.djangoapps.course_modes.models import CourseMode from common.djangoapps.util.json_request import JsonResponse -from openedx.core.djangoapps.commerce.utils import ecommerce_api_client +from openedx.core.djangoapps.commerce.utils import get_ecommerce_api_base_url, get_ecommerce_api_client from openedx.core.lib.api.authentication import BearerAuthentication from openedx.core.lib.api.mixins import PutAsCreateMixin @@ -84,7 +85,11 @@ class OrderView(APIView): except User.DoesNotExist: return JsonResponse(status=403) try: - order = ecommerce_api_client(request.user).orders(number).get() - return JsonResponse(order) - except exceptions.HttpNotFoundError: - return JsonResponse(status=404) + api_url = urljoin(f"{get_ecommerce_api_base_url()}/", f"orders/{number}/") + response = get_ecommerce_api_client(request.user).get(api_url) + response.raise_for_status() + return JsonResponse(response.json()) + except HTTPError as err: + if err.response.status_code == 404: + return JsonResponse(status=404) + raise diff --git a/lms/djangoapps/commerce/management/commands/create_orders_for_old_enterprise_course_enrollment.py b/lms/djangoapps/commerce/management/commands/create_orders_for_old_enterprise_course_enrollment.py index beeb96cd80..e0a470ce35 100644 --- a/lms/djangoapps/commerce/management/commands/create_orders_for_old_enterprise_course_enrollment.py +++ b/lms/djangoapps/commerce/management/commands/create_orders_for_old_enterprise_course_enrollment.py @@ -8,18 +8,18 @@ Management command to import time import traceback from textwrap import dedent +from urllib.parse import urljoin from django.conf import settings from django.contrib.auth import get_user_model from django.core.management.base import BaseCommand, CommandError from enterprise.models import EnterpriseCourseEnrollment from opaque_keys.edx.keys import CourseKey -from requests import Timeout -from slumber.exceptions import HttpServerError, SlumberBaseException +from requests.exceptions import RequestException from common.djangoapps.student.models import CourseEnrollment from common.djangoapps.util.query import use_read_replica_if_available -from openedx.core.djangoapps.commerce.utils import ecommerce_api_client +from openedx.core.djangoapps.commerce.utils import get_ecommerce_api_base_url, get_ecommerce_api_client User = get_user_model() @@ -33,7 +33,7 @@ class Command(BaseCommand): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) service_user = User.objects.get(username=settings.ECOMMERCE_SERVICE_WORKER_USERNAME) - self.client = ecommerce_api_client(service_user) + self.client = get_ecommerce_api_client(service_user) def _get_enrollments_queryset(self, start_index, end_index): """ @@ -60,15 +60,14 @@ class Command(BaseCommand): Returns (success_count, fail_count) """ try: - order_response = self.client.manual_course_enrollment_order.post( - { - "enrollments": enrollments - } - ) - except (SlumberBaseException, ConnectionError, Timeout, HttpServerError) as exc: + api_url = urljoin(f"{get_ecommerce_api_base_url()}/", "manual_course_enrollment_order/") + response = self.client.post(api_url, data={"enrollments": enrollments}) + response.raise_for_status() + order_response = response.json() + except RequestException as exc: self.stderr.write( - "\t\t\tFailed to create order for manual enrollments for the following enrollments: {}. Reason: {}" - .format(enrollments, exc) + "\t\t\tFailed to create order for manual enrollments for the following " + f"enrollments: {enrollments}. Reason: {exc}" ) return 0, 0, len(enrollments), [] diff --git a/lms/djangoapps/commerce/tests/__init__.py b/lms/djangoapps/commerce/tests/__init__.py index d932f5acba..367cefae2c 100644 --- a/lms/djangoapps/commerce/tests/__init__.py +++ b/lms/djangoapps/commerce/tests/__init__.py @@ -2,6 +2,7 @@ from unittest import mock +from urllib.parse import urljoin import httpretty from django.conf import settings @@ -9,7 +10,7 @@ from django.test import TestCase from freezegun import freeze_time from common.djangoapps.student.tests.factories import UserFactory -from openedx.core.djangoapps.commerce.utils import ecommerce_api_client +from openedx.core.djangoapps.commerce.utils import get_ecommerce_api_base_url, get_ecommerce_api_client from openedx.core.djangoapps.oauth_dispatch.jwt import create_jwt_for_user JSON = 'application/json' @@ -25,8 +26,9 @@ TEST_PAYMENT_DATA = { class EdxRestApiClientTest(TestCase): - """ Tests to ensure the client is initialized properly. """ - + """ + Tests to ensure the client is initialized properly. + """ SCOPES = [ 'user_id', 'email', @@ -36,18 +38,18 @@ class EdxRestApiClientTest(TestCase): def setUp(self): super().setUp() self.user = UserFactory() + self.base_url = get_ecommerce_api_base_url() @httpretty.activate def test_tracking_context(self): """ - Ensure the tracking context is set up in the api client correctly and - automatically. + Ensure the tracking context is set up in the api client correctly and automatically. """ with freeze_time('2015-7-2'): # fake an E-Commerce API request. httpretty.register_uri( httpretty.POST, - '{}/baskets/1/'.format(settings.ECOMMERCE_API_URL.strip('/')), + f"{settings.ECOMMERCE_API_URL.strip('/')}/baskets/1/", status=200, body='{}', adding_headers={'Content-Type': JSON} ) @@ -55,7 +57,8 @@ class EdxRestApiClientTest(TestCase): mock_tracker = mock.Mock() mock_tracker.resolve_context = mock.Mock(return_value={'ip': '127.0.0.1'}) with mock.patch('openedx.core.djangoapps.commerce.utils.tracker.get_tracker', return_value=mock_tracker): - ecommerce_api_client(self.user).baskets(1).post() + api_url = urljoin(f"{self.base_url}/", "baskets/1/") + get_ecommerce_api_client(self.user).post(api_url) # Verify the JWT includes the tracking context for the user actual_header = httpretty.last_request().headers['Authorization'] @@ -73,17 +76,17 @@ class EdxRestApiClientTest(TestCase): @httpretty.activate def test_client_unicode(self): """ - The client should handle json responses properly when they contain - unicode character data. + The client should handle json responses properly when they contain unicode character data. Regression test for ECOM-1606. """ expected_content = '{"result": "Préparatoire"}' httpretty.register_uri( httpretty.GET, - '{}/baskets/1/order/'.format(settings.ECOMMERCE_API_URL.strip('/')), + f"{settings.ECOMMERCE_API_URL.strip('/')}/baskets/1/order/", status=200, body=expected_content, adding_headers={'Content-Type': JSON}, ) - actual_object = ecommerce_api_client(self.user).baskets(1).order.get() + api_url = urljoin(f"{self.base_url}/", "baskets/1/order/") + actual_object = get_ecommerce_api_client(self.user).get(api_url).json() assert actual_object == {'result': 'Préparatoire'} diff --git a/lms/djangoapps/commerce/utils.py b/lms/djangoapps/commerce/utils.py index 883a4ba5b2..c296ce753c 100644 --- a/lms/djangoapps/commerce/utils.py +++ b/lms/djangoapps/commerce/utils.py @@ -14,8 +14,11 @@ from django.utils.translation import gettext as _ from opaque_keys.edx.keys import CourseKey from common.djangoapps.course_modes.models import CourseMode -from common.djangoapps.student.models import CourseEnrollment # lint-amnesty, pylint: disable=unused-import -from openedx.core.djangoapps.commerce.utils import ecommerce_api_client, is_commerce_service_configured +from openedx.core.djangoapps.commerce.utils import ( + get_ecommerce_api_base_url, + get_ecommerce_api_client, + is_commerce_service_configured, +) from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from openedx.core.djangoapps.theming import helpers as theming_helpers @@ -164,7 +167,7 @@ def refund_entitlement(course_entitlement): return False service_user = user_model.objects.get(username=settings.ECOMMERCE_SERVICE_WORKER_USERNAME) - api_client = ecommerce_api_client(service_user) + api_client = get_ecommerce_api_client(service_user) log.info( 'Attempting to create a refund for user [%s], course entitlement [%s]...', @@ -173,13 +176,17 @@ def refund_entitlement(course_entitlement): ) try: - refund_ids = api_client.refunds.post( - { + refunds_url = urljoin(f"{get_ecommerce_api_base_url()}/", "refunds/") + refunds_response = api_client.post( + refunds_url, + data={ 'order_number': course_entitlement.order_number, 'username': enrollee.username, 'entitlement_uuid': entitlement_uuid, } ) + refunds_response.raise_for_status() + refund_ids = refunds_response.json() except Exception as exc: # pylint: disable=broad-except # Catch any possible exceptions from the Ecommerce service to ensure we fail gracefully log.exception( @@ -225,19 +232,26 @@ def refund_seat(course_enrollment, change_mode=False): (may be empty). Raises: - exceptions.SlumberBaseException: for any unhandled HTTP error during communication with the E-Commerce Service. - exceptions.Timeout: if the attempt to reach the commerce service timed out. + requests.exceptions.RequestException: for any unhandled HTTP error during communication with + the E-Commerce Service. + requests.exceptions.Timeout: if the attempt to reach the commerce service timed out. """ User = get_user_model() # pylint:disable=invalid-name course_key_str = str(course_enrollment.course_id) enrollee = course_enrollment.user service_user = User.objects.get(username=settings.ECOMMERCE_SERVICE_WORKER_USERNAME) - api_client = ecommerce_api_client(service_user) + api_client = get_ecommerce_api_client(service_user) log.info('Attempting to create a refund for user [%s], course [%s]...', enrollee.id, course_key_str) - refund_ids = api_client.refunds.post({'course_id': course_key_str, 'username': enrollee.username}) + refunds_url = urljoin(f"{get_ecommerce_api_base_url()}/", "refunds/") + refunds_response = api_client.post( + refunds_url, + data={'course_id': course_key_str, 'username': enrollee.username} + ) + refunds_response.raise_for_status() + refund_ids = refunds_response.json() if refunds_response.content else None if refund_ids: log.info('Refund successfully opened for user [%s], course [%s]: %r', enrollee.id, course_key_str, refund_ids) @@ -284,7 +298,10 @@ def _process_refund(refund_ids, api_client, mode, user, always_notify=False): # NOTE: The following assumes that the user has already been unenrolled. # We are then able to approve payment. Additionally, this ensures we don't tie up an # additional web worker when the E-Commerce Service tries to unenroll the learner. - api_client.refunds(refund_id).process.put({'action': 'approve_payment_only'}) + base_url = get_ecommerce_api_base_url() + api_url = urljoin(f"{base_url}/", f"refunds/{refund_id}/process/") + response = api_client.put(api_url, json={'action': 'approve_payment_only'}) + response.raise_for_status() log.info('Refund [%d] successfully approved.', refund_id) except: # pylint: disable=bare-except # Push the refund to Support to process diff --git a/lms/djangoapps/verify_student/views.py b/lms/djangoapps/verify_student/views.py index 5602816f39..99cf700499 100644 --- a/lms/djangoapps/verify_student/views.py +++ b/lms/djangoapps/verify_student/views.py @@ -6,6 +6,7 @@ import decimal import json import logging import urllib +from urllib.parse import urljoin from django.conf import settings from django.contrib.auth.decorators import login_required @@ -21,10 +22,11 @@ from django.utils.translation import gettext_lazy from django.views.decorators.csrf import csrf_exempt from django.views.decorators.http import require_POST from django.views.generic.base import View -from edx_rest_api_client.exceptions import SlumberBaseException from opaque_keys.edx.keys import CourseKey +from requests.exceptions import RequestException from rest_framework.response import Response from rest_framework.views import APIView +from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order from common.djangoapps.course_modes.models import CourseMode from common.djangoapps.edxmako.shortcuts import render_to_response @@ -39,11 +41,10 @@ from lms.djangoapps.verify_student.image import InvalidImageData, decode_image_d from lms.djangoapps.verify_student.models import SoftwareSecurePhotoVerification, VerificationDeadline from lms.djangoapps.verify_student.tasks import send_verification_status_email from lms.djangoapps.verify_student.utils import can_verify_now -from openedx.core.djangoapps.commerce.utils import ecommerce_api_client +from openedx.core.djangoapps.commerce.utils import get_ecommerce_api_base_url, get_ecommerce_api_client from openedx.core.djangoapps.embargo import api as embargo_api from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from openedx.core.lib.log_utils import audit_log -from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order from .services import IDVerificationService @@ -386,7 +387,10 @@ class PayAndVerifyView(View): verification_good_until = self._verification_valid_until(request.user) # get available payment processors - processors = ecommerce_api_client(request.user).payment.processors.get() + api_url = urljoin(f"{get_ecommerce_api_base_url()}/", "payment/processors/") + response = get_ecommerce_api_client(request.user).get(api_url) + response.raise_for_status() + processors = response.json() # Render the top-level page context = { @@ -706,20 +710,28 @@ class PayAndVerifyView(View): def checkout_with_ecommerce_service(user, course_key, course_mode, processor): - """ Create a new basket and trigger immediate checkout, using the E-Commerce API. """ + """ + Create a new basket and trigger immediate checkout, using the E-Commerce API. + """ course_id = str(course_key) try: - api = ecommerce_api_client(user) + api_client = get_ecommerce_api_client(user) + api_url = urljoin(f"{get_ecommerce_api_base_url()}/", "baskets/") # Make an API call to create the order and retrieve the results - result = api.baskets.post({ - 'products': [{'sku': course_mode.sku}], - 'checkout': True, - 'payment_processor_name': processor - }) + response = api_client.post( + api_url, + json={ + 'products': [{'sku': course_mode.sku}], + 'checkout': True, + 'payment_processor_name': processor + } + ) + response.raise_for_status() + result = response.json() # Pass the payment parameters directly from the API response. return result.get('payment_data') - except SlumberBaseException: + except RequestException: params = {'username': user.username, 'mode': course_mode.slug, 'course_id': course_id} log.exception('Failed to create order for %(username)s %(mode)s mode of %(course_id)s', params) raise diff --git a/lms/envs/common.py b/lms/envs/common.py index 2a32830671..d66cd4bf98 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -4507,7 +4507,7 @@ ENTERPRISE_INTEGRATIONS_EMAIL = "enterprise-integrations@edx.org" INTEGRATED_CHANNELS_API_CHUNK_TRANSMISSION_LIMIT = {} ############## ENTERPRISE SERVICE API CLIENT CONFIGURATION ###################### -# The LMS communicates with the Enterprise service via the EdxRestApiClient class +# The LMS communicates with the Enterprise service via the requests.Session() client # These default settings are utilized by the LMS when interacting with the service, # and are overridden by the configuration parameter accessors defined in production.py diff --git a/lms/envs/production.py b/lms/envs/production.py index 5e0213da9d..d989b1a48a 100644 --- a/lms/envs/production.py +++ b/lms/envs/production.py @@ -881,7 +881,7 @@ INTEGRATED_CHANNELS_API_CHUNK_TRANSMISSION_LIMIT = ENV_TOKENS.get( ) ############## ENTERPRISE SERVICE API CLIENT CONFIGURATION ###################### -# The LMS communicates with the Enterprise service via the EdxRestApiClient class +# The LMS communicates with the Enterprise service via the requests.Session() client # The below environmental settings are utilized by the LMS when interacting with # the service, and override the default parameters which are defined in common.py diff --git a/openedx/core/djangoapps/api_admin/views.py b/openedx/core/djangoapps/api_admin/views.py index 44638efa45..9b7d24a76f 100644 --- a/openedx/core/djangoapps/api_admin/views.py +++ b/openedx/core/djangoapps/api_admin/views.py @@ -1,7 +1,11 @@ -"""Views for API management.""" +""" +Views for API management. +""" import logging +from functools import cached_property +from urllib.parse import urljoin from django.conf import settings from django.contrib.sites.shortcuts import get_current_site @@ -14,13 +18,14 @@ from django.views.generic.edit import CreateView from oauth2_provider.generators import generate_client_id, generate_client_secret from oauth2_provider.models import get_application_model from oauth2_provider.views import ApplicationRegistration -from slumber.exceptions import HttpNotFoundError +from requests.exceptions import HTTPError from common.djangoapps.edxmako.shortcuts import render_to_response from openedx.core.djangoapps.api_admin.decorators import require_api_access from openedx.core.djangoapps.api_admin.forms import ApiAccessRequestForm, CatalogForm from openedx.core.djangoapps.api_admin.models import ApiAccessRequest, Catalog -from openedx.core.djangoapps.catalog.utils import create_catalog_api_client +from openedx.core.djangoapps.catalog.utils import get_catalog_api_base_url +from openedx.core.djangoapps.catalog.utils import get_catalog_api_client as create_catalog_api_client log = logging.getLogger(__name__) @@ -122,9 +127,29 @@ class ApiTosView(TemplateView): class CatalogApiMixin: + """ + Helpers for work with Catalog API. + """ def get_catalog_api_client(self, user): + """ + Returns catalog API client. + """ return create_catalog_api_client(user) + @cached_property + def catalogs_api_url(self): + """ + Returns the catalogs URL for the catalog API. + """ + return urljoin(f"{get_catalog_api_base_url()}/", "catalogs/") + + @cached_property + def courses_api_url(self): + """ + Returns the courses URL for the catalog API. + """ + return urljoin(f"{get_catalog_api_base_url()}/", "courses/") + class CatalogSearchView(View): """View to search for catalogs belonging to a user.""" @@ -143,89 +168,115 @@ class CatalogSearchView(View): class CatalogListView(CatalogApiMixin, View): - """View to list existing catalogs and create new ones.""" - + """ + View to list existing catalogs and create new ones. + """ template = 'api_admin/catalogs/list.html' def _get_catalogs(self, client, username): - """Retrieve catalogs for a user. Returns the empty list if none are found.""" + """ + Retrieve catalogs for a user. Returns the empty list if none are found. + """ try: - response = client.catalogs.get(username=username) - return [Catalog(attributes=catalog) for catalog in response['results']] - except HttpNotFoundError: - return [] + response = client.get(self.catalogs_api_url, params={"username": username}) + response.raise_for_status() + return [Catalog(attributes=catalog) for catalog in response.json()['results']] + except HTTPError as err: + if err.response.status_code == 404: + return [] + else: + raise def get_context_data(self, client, username, form): - """ Retrieve context data for the template. """ - + """ + Retrieve context data for the template. + """ return { 'username': username, 'catalogs': self._get_catalogs(client, username), 'form': form, 'preview_url': reverse('api_admin:catalog-preview'), - 'catalog_api_catalog_endpoint': client.catalogs.url().rstrip('/'), - 'catalog_api_url': client.courses.url(), + 'catalog_api_catalog_endpoint': self.catalogs_api_url, + 'catalog_api_url': self.courses_api_url, } def get(self, request, username): - """Display a list of a user's catalogs.""" + """ + Display a list of a user's catalogs. + """ client = self.get_catalog_api_client(request.user) form = CatalogForm(initial={'viewers': [username]}) return render_to_response(self.template, self.get_context_data(client, username, form)) def post(self, request, username): - """Create a new catalog for a user.""" + """ + Create a new catalog for a user. + """ form = CatalogForm(request.POST) client = self.get_catalog_api_client(request.user) if not form.is_valid(): return render_to_response(self.template, self.get_context_data(client, username, form), status=400) attrs = form.cleaned_data - catalog = client.catalogs.post(attrs) + response = client.post(self.catalogs_api_url, data=attrs) + response.raise_for_status() + catalog = response.json() return redirect(reverse('api_admin:catalog-edit', kwargs={'catalog_id': catalog['id']})) class CatalogEditView(CatalogApiMixin, View): - """View to edit an individual catalog.""" - + """ + View to edit an individual catalog. + """ template_name = 'api_admin/catalogs/edit.html' - def get_context_data(self, catalog, form, client): - """ Retrieve context data for the template. """ - + def get_context_data(self, catalog, form): + """ + Retrieve context data for the template. + """ return { 'catalog': catalog, 'form': form, 'preview_url': reverse('api_admin:catalog-preview'), - 'catalog_api_url': client.courses.url(), - 'catalog_api_catalog_endpoint': client.catalogs.url().rstrip('/'), + 'catalog_api_url': self.catalogs_api_url, + 'catalog_api_catalog_endpoint': self.courses_api_url, } def get(self, request, catalog_id): - """Display a form to edit this catalog.""" + """ + Display a form to edit this catalog. + """ client = self.get_catalog_api_client(request.user) - response = client.catalogs(catalog_id).get() - catalog = Catalog(attributes=response) + response = client.get(urljoin(f"{self.catalogs_api_url}/", f"{catalog_id}/")) + response.raise_for_status() + catalog = Catalog(attributes=response.json()) form = CatalogForm(instance=catalog) - return render_to_response(self.template_name, self.get_context_data(catalog, form, client)) + return render_to_response(self.template_name, self.get_context_data(catalog, form)) def post(self, request, catalog_id): - """Update or delete this catalog.""" + """ + Update or delete this catalog. + """ client = self.get_catalog_api_client(request.user) if request.POST.get('delete-catalog') == 'on': - client.catalogs(catalog_id).delete() + response = client.delete(urljoin(f"{self.catalogs_api_url}/", f"{catalog_id}/")) + response.raise_for_status() return redirect(reverse('api_admin:catalog-search')) form = CatalogForm(request.POST) if not form.is_valid(): - response = client.catalogs(catalog_id).get() - catalog = Catalog(attributes=response) - return render_to_response(self.template_name, self.get_context_data(catalog, form, client), status=400) - catalog = client.catalogs(catalog_id).patch(form.cleaned_data) - return redirect(reverse('api_admin:catalog-edit', kwargs={'catalog_id': catalog['id']})) + response = client.get(urljoin(f"{self.catalogs_api_url}/", f"{catalog_id}/")) + response.raise_for_status() + catalog = Catalog(attributes=response.json()) + return render_to_response(self.template_name, self.get_context_data(catalog, form), status=400) + catalog_response = client.patch(urljoin(f"{self.catalogs_api_url}/", f"{catalog_id}/"), data=form.cleaned_data) + catalog_response.raise_for_status() + return redirect(reverse('api_admin:catalog-edit', kwargs={'catalog_id': catalog_response.json()['id']})) class CatalogPreviewView(CatalogApiMixin, View): - """Endpoint to preview courses for a query.""" + """ + Endpoint to preview courses for a query. + """ def get(self, request): """ @@ -235,7 +286,9 @@ class CatalogPreviewView(CatalogApiMixin, View): client = self.get_catalog_api_client(request.user) # Just pass along the request params including limit/offset pagination if 'q' in request.GET: - results = client.courses.get(**request.GET) + response = client.get(self.courses_api_url, params=request.GET) + response.raise_for_status() + results = response.json() # Ensure that we don't just return all the courses if no query is given else: results = {'count': 0, 'results': [], 'next': None, 'prev': None} diff --git a/openedx/core/djangoapps/catalog/management/commands/cache_programs.py b/openedx/core/djangoapps/catalog/management/commands/cache_programs.py index 392b86cf13..b815f8b4e4 100644 --- a/openedx/core/djangoapps/catalog/management/commands/cache_programs.py +++ b/openedx/core/djangoapps/catalog/management/commands/cache_programs.py @@ -4,6 +4,7 @@ import logging import sys from collections import defaultdict +from urllib.parse import urljoin from django.contrib.auth import get_user_model from django.contrib.sites.models import Site @@ -11,8 +12,8 @@ from django.core.cache import cache from django.core.management import BaseCommand from openedx.core.djangoapps.catalog.cache import ( - COURSE_PROGRAMS_CACHE_KEY_TPL, CATALOG_COURSE_PROGRAMS_CACHE_KEY_TPL, + COURSE_PROGRAMS_CACHE_KEY_TPL, PATHWAY_CACHE_KEY_TPL, PROGRAM_CACHE_KEY_TPL, PROGRAMS_BY_ORGANIZATION_CACHE_KEY_TPL, @@ -25,7 +26,8 @@ from openedx.core.djangoapps.catalog.models import CatalogIntegration from openedx.core.djangoapps.catalog.utils import ( course_run_keys_for_program, course_uuids_for_program, - create_catalog_api_client, + get_catalog_api_base_url, + get_catalog_api_client, normalize_program_type ) @@ -74,10 +76,11 @@ class Command(BaseCommand): cache.set(SITE_PATHWAY_IDS_CACHE_KEY_TPL.format(domain=site.domain), [], None) continue - client = create_catalog_api_client(user, site=site) - uuids, program_uuids_failed = self.get_site_program_uuids(client, site) - new_programs, program_details_failed = self.fetch_program_details(client, uuids) - new_pathways, pathways_failed = self.get_pathways(client, site) + client = get_catalog_api_client(user) + api_base_url = get_catalog_api_base_url(site=site) + uuids, program_uuids_failed = self.get_site_program_uuids(client, site, api_base_url) + new_programs, program_details_failed = self.fetch_program_details(client, uuids, api_base_url) + new_pathways, pathways_failed = self.get_pathways(client, site, api_base_url) new_pathways, new_programs, pathway_processing_failed = self.process_pathways( site, new_pathways, new_programs ) @@ -134,7 +137,7 @@ class Command(BaseCommand): if failure: sys.exit(1) - def get_site_program_uuids(self, client, site): # lint-amnesty, pylint: disable=missing-function-docstring + def get_site_program_uuids(self, client, site, api_base_url): # lint-amnesty, pylint: disable=missing-function-docstring failure = False uuids = [] try: @@ -143,9 +146,11 @@ class Command(BaseCommand): 'status': ('active', 'retired'), 'uuids_only': 1, } - + api_url = urljoin(f"{api_base_url}/", "programs/") logger.info(f'Requesting program UUIDs for {site.domain}.') - uuids = client.programs.get(**querystring) + response = client.get(api_url, params=querystring) + response.raise_for_status() + uuids = response.json() except: # pylint: disable=bare-except logger.exception(f'Failed to retrieve program UUIDs for site: {site.domain}.') failure = True @@ -156,14 +161,17 @@ class Command(BaseCommand): )) return uuids, failure - def fetch_program_details(self, client, uuids): # lint-amnesty, pylint: disable=missing-function-docstring + def fetch_program_details(self, client, uuids, api_base_url): # lint-amnesty, pylint: disable=missing-function-docstring programs = {} failure = False for uuid in uuids: try: cache_key = PROGRAM_CACHE_KEY_TPL.format(uuid=uuid) logger.info(f'Requesting details for program {uuid}.') - program = client.programs(uuid).get(exclude_utm=1) + api_url = urljoin(f"{api_base_url}/", f"programs/{uuid}/") + response = client.get(api_url, params={"exclude_utm": 1}) + response.raise_for_status() + program = response.json() # pathways get added in process_pathways program['pathway_ids'] = [] programs[cache_key] = program @@ -173,20 +181,22 @@ class Command(BaseCommand): continue return programs, failure - def get_pathways(self, client, site): + def get_pathways(self, client, site, api_base_url): """ - Get all pathways for the current client + Get all pathways for the current client. """ pathways = [] failure = False logger.info(f'Requesting pathways for {site.domain}.') try: + api_url = urljoin(f"{api_base_url}/", "pathways/") next_page = 1 while next_page: - new_pathways = client.pathways.get(exclude_utm=1, page=next_page) + response = client.get(api_url, params=dict(exclude_utm=1, page=next_page)) + response.raise_for_status() + new_pathways = response.json() pathways.extend(new_pathways['results']) next_page = next_page + 1 if new_pathways['next'] else None - except: # pylint: disable=bare-except logger.exception( msg=f'Failed to retrieve pathways for site: {site.domain}.', diff --git a/openedx/core/djangoapps/catalog/tests/test_utils.py b/openedx/core/djangoapps/catalog/tests/test_utils.py index caddac0f4a..36feb63a67 100644 --- a/openedx/core/djangoapps/catalog/tests/test_utils.py +++ b/openedx/core/djangoapps/catalog/tests/test_utils.py @@ -383,7 +383,7 @@ class TestGetPathways(CacheIsolationTestCase): assert not mock_warning.called -@mock.patch(UTILS_MODULE + '.get_edx_api_data') +@mock.patch(UTILS_MODULE + '.get_api_data') class TestGetProgramTypes(CatalogIntegrationMixin, TestCase): """Tests covering retrieval of program types from the catalog service.""" @override_settings(COURSE_CATALOG_API_URL='https://api.example.com/v1/') @@ -406,7 +406,7 @@ class TestGetProgramTypes(CatalogIntegrationMixin, TestCase): assert data == program -@mock.patch(UTILS_MODULE + '.get_edx_api_data') +@mock.patch(UTILS_MODULE + '.get_api_data') class TestGetCurrency(CatalogIntegrationMixin, TestCase): """Tests covering retrieval of currency data from the catalog service.""" @override_settings(COURSE_CATALOG_API_URL='https://api.example.com/v1/') @@ -451,7 +451,7 @@ class TestGetLocalizedPriceText(TestCase): @skip_unless_lms -@mock.patch(UTILS_MODULE + '.get_edx_api_data') +@mock.patch(UTILS_MODULE + '.get_api_data') class TestGetCourseRuns(CatalogIntegrationMixin, CacheIsolationTestCase): """ Tests covering retrieval of course runs from the catalog service. @@ -471,7 +471,7 @@ class TestGetCourseRuns(CatalogIntegrationMixin, CacheIsolationTestCase): for arg in (self.catalog_integration, 'course_runs'): assert arg in args - assert kwargs['api']._store['base_url'] == self.catalog_integration.get_internal_api_url() # pylint: disable=protected-access, line-too-long + assert kwargs['base_api_url'] == self.catalog_integration.get_internal_api_url() # pylint: disable=protected-access, line-too-long querystring = { 'page_size': 20, @@ -534,7 +534,7 @@ class TestGetCourseRuns(CatalogIntegrationMixin, CacheIsolationTestCase): @skip_unless_lms -@mock.patch(UTILS_MODULE + '.get_edx_api_data') +@mock.patch(UTILS_MODULE + '.get_api_data') class TestGetCourseOwners(CatalogIntegrationMixin, TestCase): """ Tests covering retrieval of course runs from the catalog service. @@ -559,7 +559,7 @@ class TestGetCourseOwners(CatalogIntegrationMixin, TestCase): @skip_unless_lms -@mock.patch(UTILS_MODULE + '.get_edx_api_data') +@mock.patch(UTILS_MODULE + '.get_api_data') class TestSessionEntitlement(CatalogIntegrationMixin, TestCase): """ Test Covering data related Entitlements. @@ -667,7 +667,7 @@ class TestSessionEntitlement(CatalogIntegrationMixin, TestCase): @skip_unless_lms -@mock.patch(UTILS_MODULE + '.get_edx_api_data') +@mock.patch(UTILS_MODULE + '.get_api_data') class TestGetCourseRunDetails(CatalogIntegrationMixin, TestCase): """ Tests covering retrieval of information about a specific course run from the catalog service. diff --git a/openedx/core/djangoapps/catalog/utils.py b/openedx/core/djangoapps/catalog/utils.py index 6362ed202c..1f9040f3e9 100644 --- a/openedx/core/djangoapps/catalog/utils.py +++ b/openedx/core/djangoapps/catalog/utils.py @@ -7,19 +7,21 @@ import logging import uuid import pycountry +import requests from django.core.cache import cache from django.core.exceptions import ObjectDoesNotExist -from edx_rest_api_client.client import EdxRestApiClient +from edx_rest_api_client.auth import SuppliedJwtAuth from opaque_keys.edx.keys import CourseKey from pytz import UTC from common.djangoapps.entitlements.utils import is_course_run_entitlement_fulfillable +from common.djangoapps.student.models import CourseEnrollment from openedx.core.djangoapps.catalog.cache import ( - COURSE_PROGRAMS_CACHE_KEY_TPL, CATALOG_COURSE_PROGRAMS_CACHE_KEY_TPL, - PROGRAMS_BY_ORGANIZATION_CACHE_KEY_TPL, + COURSE_PROGRAMS_CACHE_KEY_TPL, PATHWAY_CACHE_KEY_TPL, PROGRAM_CACHE_KEY_TPL, + PROGRAMS_BY_ORGANIZATION_CACHE_KEY_TPL, PROGRAMS_BY_TYPE_CACHE_KEY_TPL, PROGRAMS_BY_TYPE_SLUG_CACHE_KEY_TPL, SITE_PATHWAY_IDS_CACHE_KEY_TPL, @@ -27,24 +29,32 @@ from openedx.core.djangoapps.catalog.cache import ( ) from openedx.core.djangoapps.catalog.models import CatalogIntegration from openedx.core.djangoapps.oauth_dispatch.jwt import create_jwt_for_user -from openedx.core.lib.edx_api_utils import get_edx_api_data -from common.djangoapps.student.models import CourseEnrollment +from openedx.core.lib.edx_api_utils import get_api_data logger = logging.getLogger(__name__) missing_details_msg_tpl = 'Failed to get details for program {uuid} from the cache.' -def create_catalog_api_client(user, site=None): - """Returns an API client which can be used to make Catalog API requests.""" - jwt = create_jwt_for_user(user) - +def get_catalog_api_base_url(site=None): + """ + Returns a base API url used to make Catalog API requests. + """ if site: - url = site.configuration.get_value('COURSE_CATALOG_API_URL') - else: - url = CatalogIntegration.current().get_internal_api_url() + return site.configuration.get_value('COURSE_CATALOG_API_URL') - return EdxRestApiClient(url, jwt=jwt) + return CatalogIntegration.current().get_internal_api_url() + + +def get_catalog_api_client(user): + """ + Returns an API client which can be used to make Catalog API requests. + """ + jwt = create_jwt_for_user(user) + client = requests.Session() + client.auth = SuppliedJwtAuth(jwt) + + return client def check_catalog_integration_and_get_user(error_message_field): @@ -231,11 +241,15 @@ def get_program_types(name=None): """ user, catalog_integration = check_catalog_integration_and_get_user(error_message_field='Program types') if user: - api = create_catalog_api_client(user) cache_key = f'{catalog_integration.CACHE_KEY}.program_types' - data = get_edx_api_data(catalog_integration, 'program_types', api=api, - cache_key=cache_key if catalog_integration.is_cache_enabled else None) + data = get_api_data( + catalog_integration, + "program_types", + api_client=get_catalog_api_client(user), + base_api_url=get_catalog_api_base_url(), + cache_key=cache_key if catalog_integration.is_cache_enabled else None + ) # Filter by name if a name was provided if name: @@ -311,11 +325,15 @@ def get_currency_data(): """ user, catalog_integration = check_catalog_integration_and_get_user(error_message_field='Currency data') if user: - api = create_catalog_api_client(user) cache_key = f'{catalog_integration.CACHE_KEY}.currency' - return get_edx_api_data(catalog_integration, 'currency', api=api, traverse_pagination=False, - cache_key=cache_key if catalog_integration.is_cache_enabled else None) + return get_api_data( + catalog_integration, + "currency", + api_client=get_catalog_api_client(user), + base_api_url=get_catalog_api_base_url(), + traverse_pagination=False, + cache_key=cache_key if catalog_integration.is_cache_enabled else None) else: return [] @@ -410,14 +428,18 @@ def get_course_runs(): course_runs = [] user, catalog_integration = check_catalog_integration_and_get_user(error_message_field='Course runs') if user: - api = create_catalog_api_client(user) - querystring = { 'page_size': catalog_integration.page_size, 'exclude_utm': 1, } - course_runs = get_edx_api_data(catalog_integration, 'course_runs', api=api, querystring=querystring) + course_runs = get_api_data( + catalog_integration, + 'course_runs', + api_client=get_catalog_api_client(user), + base_api_url=get_catalog_api_base_url(), + querystring=querystring + ) return course_runs @@ -425,16 +447,14 @@ def get_course_runs(): def get_course_runs_for_course(course_uuid): # lint-amnesty, pylint: disable=missing-function-docstring user, catalog_integration = check_catalog_integration_and_get_user(error_message_field='Course runs') if user: - api = create_catalog_api_client(user) - cache_key = '{base}.course.{uuid}.course_runs'.format( - base=catalog_integration.CACHE_KEY, - uuid=course_uuid - ) - data = get_edx_api_data( + cache_key = f"{catalog_integration.CACHE_KEY}.course.{course_uuid}.course_runs" + + data = get_api_data( catalog_integration, 'courses', resource_id=course_uuid, - api=api, + api_client=get_catalog_api_client(user), + base_api_url=get_catalog_api_base_url(), cache_key=cache_key if catalog_integration.is_cache_enabled else None, long_term_cache=True, many=False @@ -447,23 +467,21 @@ def get_course_runs_for_course(course_uuid): # lint-amnesty, pylint: disable=mi def get_owners_for_course(course_uuid): # lint-amnesty, pylint: disable=missing-function-docstring user, catalog_integration = check_catalog_integration_and_get_user(error_message_field='Owners') if user: - api = create_catalog_api_client(user) - cache_key = '{base}.course.{uuid}.course_runs'.format( - base=catalog_integration.CACHE_KEY, - uuid=course_uuid - ) - data = get_edx_api_data( + cache_key = f"{catalog_integration.CACHE_KEY}.course.{course_uuid}.course_runs" + + data = get_api_data( catalog_integration, 'courses', resource_id=course_uuid, - api=api, + api_client=get_catalog_api_client(user), + base_api_url=get_catalog_api_base_url(), cache_key=cache_key if catalog_integration.is_cache_enabled else None, long_term_cache=True, many=False ) return data.get('owners', []) - else: - return [] + + return [] def get_course_uuid_for_course(course_run_key): @@ -479,18 +497,17 @@ def get_course_uuid_for_course(course_run_key): """ user, catalog_integration = check_catalog_integration_and_get_user(error_message_field='Course UUID') if user: - api = create_catalog_api_client(user) + api_client = get_catalog_api_client(user) + base_api_url = get_catalog_api_base_url() - run_cache_key = '{base}.course_run.{course_run_key}'.format( - base=catalog_integration.CACHE_KEY, - course_run_key=course_run_key - ) + run_cache_key = f"{catalog_integration.CACHE_KEY}.course_run.{course_run_key}" - course_run_data = get_edx_api_data( + course_run_data = get_api_data( catalog_integration, 'course_runs', resource_id=str(course_run_key), - api=api, + api_client=api_client, + base_api_url=base_api_url, cache_key=run_cache_key if catalog_integration.is_cache_enabled else None, long_term_cache=True, many=False, @@ -499,16 +516,14 @@ def get_course_uuid_for_course(course_run_key): course_key_str = course_run_data.get('course', None) if course_key_str: - run_cache_key = '{base}.course.{course_key}'.format( - base=catalog_integration.CACHE_KEY, - course_key=course_key_str - ) + run_cache_key = f"{catalog_integration.CACHE_KEY}.course.{course_key_str}" - data = get_edx_api_data( + data = get_api_data( catalog_integration, 'courses', resource_id=course_key_str, - api=api, + api_client=api_client, + base_api_url=base_api_url, cache_key=run_cache_key if catalog_integration.is_cache_enabled else None, long_term_cache=True, many=False, @@ -598,12 +613,16 @@ def get_course_run_details(course_run_key, fields): error_message_field=f'Data for course_run {course_run_key}' ) if user: - api = create_catalog_api_client(user) - cache_key = f'{catalog_integration.CACHE_KEY}.course_runs' - course_run_details = get_edx_api_data(catalog_integration, 'course_runs', api, resource_id=course_run_key, - cache_key=cache_key, many=False, traverse_pagination=False, fields=fields) + course_run_details = get_api_data( + catalog_integration, + 'course_runs', + api_client=get_catalog_api_client(user), + base_api_url=get_catalog_api_base_url(), + resource_id=course_run_key, + cache_key=cache_key, many=False, traverse_pagination=False, fields=fields + ) return course_run_details diff --git a/openedx/core/djangoapps/commerce/utils.py b/openedx/core/djangoapps/commerce/utils.py index 0105b2c11d..c9bda98b04 100644 --- a/openedx/core/djangoapps/commerce/utils.py +++ b/openedx/core/djangoapps/commerce/utils.py @@ -4,7 +4,6 @@ import requests from django.conf import settings from edx_rest_api_client.auth import SuppliedJwtAuth -from edx_rest_api_client.client import EdxRestApiClient from eventtracking import tracker from openedx.core.djangoapps.oauth_dispatch.jwt import create_jwt_for_user @@ -32,25 +31,11 @@ def is_commerce_service_configured(): return bool(ecommerce_api_url) -def ecommerce_api_client(user, session=None): +def get_ecommerce_api_base_url(): """ - Returns an E-Commerce API client setup with authentication for the specified user. - - DEPRECATED: To be replaced with get_ecommerce_api_client as part of FC-0001. + Returns an E-Commerce API base URL. """ - claims = {'tracking_context': create_tracking_context(user)} - scopes = [ - 'user_id', - 'email', - 'profile' - ] - jwt = create_jwt_for_user(user, additional_claims=claims, scopes=scopes) - - return EdxRestApiClient( - configuration_helpers.get_value('ECOMMERCE_API_URL', settings.ECOMMERCE_API_URL), - jwt=jwt, - session=session - ) + return configuration_helpers.get_value('ECOMMERCE_API_URL', settings.ECOMMERCE_API_URL) def get_ecommerce_api_client(user): diff --git a/openedx/core/djangoapps/credentials/tasks/v1/tasks.py b/openedx/core/djangoapps/credentials/tasks/v1/tasks.py index c3b767c815..f2f84554a6 100644 --- a/openedx/core/djangoapps/credentials/tasks/v1/tasks.py +++ b/openedx/core/djangoapps/credentials/tasks/v1/tasks.py @@ -4,6 +4,7 @@ This file contains celery tasks for credentials-related functionality. import math import time +from urllib.parse import urljoin from celery import shared_task from celery.exceptions import MaxRetriesExceededError @@ -25,7 +26,7 @@ from openedx.core.djangoapps.catalog.utils import get_programs from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.credentials.helpers import is_learner_records_enabled_for_org from openedx.core.djangoapps.credentials.models import CredentialsApiConfig -from openedx.core.djangoapps.credentials.utils import get_credentials_api_client +from openedx.core.djangoapps.credentials.utils import get_credentials_api_base_url, get_credentials_api_client from openedx.core.djangoapps.programs.signals import handle_course_cert_awarded, handle_course_cert_changed from openedx.core.djangoapps.site_configuration.models import SiteConfiguration @@ -48,7 +49,9 @@ MAX_RETRIES = 11 @shared_task(bind=True, ignore_result=True) @set_code_owner_attribute def send_grade_to_credentials(self, username, course_run_key, verified, letter_grade, percent_grade): - """ Celery task to notify the Credentials IDA of a grade change via POST. """ + """ + Celery task to notify the Credentials IDA of a grade change via POST. + """ logger.info(f"Running task send_grade_to_credentials for username {username} and course {course_run_key}") countdown = 2 ** self.request.retries @@ -56,28 +59,31 @@ def send_grade_to_credentials(self, username, course_run_key, verified, letter_g try: credentials_client = get_credentials_api_client( - User.objects.get(username=settings.CREDENTIALS_SERVICE_USERNAME), - org=course_key.org, + User.objects.get(username=settings.CREDENTIALS_SERVICE_USERNAME) ) - - credentials_client.grades.post({ - 'username': username, - 'course_run': str(course_key), - 'letter_grade': letter_grade, - 'percent_grade': percent_grade, - 'verified': verified, - }) + api_url = urljoin(f"{get_credentials_api_base_url(org=course_key.org)}/", "grades/") + response = credentials_client.post( + api_url, + data={ + 'username': username, + 'course_run': str(course_key), + 'letter_grade': letter_grade, + 'percent_grade': percent_grade, + 'verified': verified, + } + ) + response.raise_for_status() logger.info(f"Sent grade for course {course_run_key} to user {username}") - except Exception as exc: # lint-amnesty, pylint: disable=unused-variable + except Exception: # lint-amnesty, pylint: disable=W0703 grade_str = f'(percent: {percent_grade} letter: {letter_grade})' error_msg = f'Failed to send grade{grade_str} for course {course_run_key} to user {username}.' logger.exception(error_msg) exception = MaxRetriesExceededError( f"Failed to send grade to credentials. Reason: {error_msg}" ) - raise self.retry(exc=exception, countdown=countdown, max_retries=MAX_RETRIES) + raise self.retry(exc=exception, countdown=countdown, max_retries=MAX_RETRIES) # pylint: disable=raise-missing-from @shared_task(base=LoggedTask, ignore_result=True) @@ -396,14 +402,22 @@ def backfill_date_for_all_course_runs(): credentials_client = get_credentials_api_client( User.objects.get(username=settings.CREDENTIALS_SERVICE_USERNAME), ) - credentials_client.course_certificates.post({ - "course_id": course_key, - "certificate_type": modes[0], - "certificate_available_date": course_run.certificate_available_date.strftime('%Y-%m-%dT%H:%M:%SZ'), - "is_active": True, - }) + api_url = urljoin(f"{get_credentials_api_base_url()}/", "course_certificates/") + response = credentials_client.post( + api_url, + json={ + "course_id": course_key, + "certificate_type": modes[0], + "certificate_available_date": course_run.certificate_available_date.strftime( + '%Y-%m-%dT%H:%M:%SZ' + ), + "is_active": True, + } + ) + response.raise_for_status() + logger.info(f"certificate_available_date updated for course {course_key}") - except Exception as exc: # lint-amnesty, pylint: disable=unused-variable,W0703 + except Exception: # lint-amnesty, pylint: disable=W0703 error_msg = f"Failed to send certificate_available_date for course {course_key}." logger.exception(error_msg) if index % 10 == 0: @@ -442,14 +456,20 @@ def clean_certificate_available_date(): credentials_client = get_credentials_api_client( User.objects.get(username=settings.CREDENTIALS_SERVICE_USERNAME), ) - credentials_client.course_certificates.post({ - "course_id": course_key, - "certificate_type": modes[0], - "certificate_available_date": None, - "is_active": True, - }) + credentials_api_base_url = get_credentials_api_base_url() + api_url = urljoin(f"{credentials_api_base_url}/", "course_certificates/") + response = credentials_client.post( + api_url, + json={ + "course_id": course_key, + "certificate_type": modes[0], + "certificate_available_date": None, + "is_active": True, + } + ) + response.raise_for_status() logger.info(f"certificate_available_date updated for course {course_key}") - except Exception as exc: # lint-amnesty, pylint: disable=unused-variable,W0703 + except Exception: # lint-amnesty, pylint: disable=W0703 error_msg = f"Failed to send certificate_available_date for course {course_key}." logger.exception(error_msg) if index % 10 == 0: diff --git a/openedx/core/djangoapps/credentials/tests/test_tasks.py b/openedx/core/djangoapps/credentials/tests/test_tasks.py index 18c9403b16..c48f4c5044 100644 --- a/openedx/core/djangoapps/credentials/tests/test_tasks.py +++ b/openedx/core/djangoapps/credentials/tests/test_tasks.py @@ -55,10 +55,9 @@ class TestSendGradeToCredentialTask(TestCase): assert mock_get_api_client.call_count == 1 assert mock_get_api_client.call_args[0] == (self.user,) - self.assertDictEqual(mock_get_api_client.call_args[1], {'org': 'org'}) - assert api_client.grades.post.call_count == 1 - self.assertDictEqual(api_client.grades.post.call_args[0][0], { + assert api_client.post.call_count == 1 + self.assertDictEqual(api_client.post.call_args[1]['data'], { 'username': 'user', 'course_run': 'course-v1:org+course+run', 'letter_grade': 'A', diff --git a/openedx/core/djangoapps/credentials/tests/test_utils.py b/openedx/core/djangoapps/credentials/tests/test_utils.py index 7401f013b9..ed8879386e 100644 --- a/openedx/core/djangoapps/credentials/tests/test_utils.py +++ b/openedx/core/djangoapps/credentials/tests/test_utils.py @@ -17,7 +17,7 @@ UTILS_MODULE = 'openedx.core.djangoapps.credentials.utils' @skip_unless_lms -@mock.patch(UTILS_MODULE + '.get_edx_api_data') +@mock.patch(UTILS_MODULE + '.get_api_data') class TestGetCredentials(CredentialsApiConfigMixin, CacheIsolationTestCase): """ Tests for credentials utility functions. """ diff --git a/openedx/core/djangoapps/credentials/utils.py b/openedx/core/djangoapps/credentials/utils.py index badebc152c..2b52b1f5e3 100644 --- a/openedx/core/djangoapps/credentials/utils.py +++ b/openedx/core/djangoapps/credentials/utils.py @@ -1,11 +1,10 @@ """Helper functions for working with Credentials.""" - - -from edx_rest_api_client.client import EdxRestApiClient +import requests +from edx_rest_api_client.auth import SuppliedJwtAuth from openedx.core.djangoapps.credentials.models import CredentialsApiConfig from openedx.core.djangoapps.oauth_dispatch.jwt import create_jwt_for_user -from openedx.core.lib.edx_api_utils import get_edx_api_data +from openedx.core.lib.edx_api_utils import get_api_data def get_credentials_records_url(program_uuid=None): @@ -25,23 +24,34 @@ def get_credentials_records_url(program_uuid=None): return base_url -def get_credentials_api_client(user, org=None): +def get_credentials_api_client(user): """ Returns an authenticated Credentials API client. Arguments: user (User): The user to authenticate as when requesting credentials. - org (str): Optional organization to look up the site config for, rather than the current request - """ scopes = ['email', 'profile', 'user_id'] jwt = create_jwt_for_user(user, scopes=scopes) + client = requests.Session() + client.auth = SuppliedJwtAuth(jwt) + return client + + +def get_credentials_api_base_url(org=None): + """ + Returns a credentials API base URL. + + Arguments: + org (str): Optional organization to look up the site config for, rather than the current request + """ if org is None: url = CredentialsApiConfig.current().internal_api_url # by current request else: url = CredentialsApiConfig.get_internal_api_url_for_org(org) # by org - return EdxRestApiClient(url, jwt=jwt) + + return url def get_credentials(user, program_uuid=None, credential_type=None): @@ -75,8 +85,15 @@ def get_credentials(user, program_uuid=None, credential_type=None): cache_key = f'{credential_configuration.CACHE_KEY}.{user.username}' if use_cache else None if cache_key and program_uuid: cache_key = f'{cache_key}.{program_uuid}' - api = get_credentials_api_client(user) - return get_edx_api_data( - credential_configuration, 'credentials', api=api, querystring=querystring, cache_key=cache_key + api_client = get_credentials_api_client(user) + base_api_url = get_credentials_api_base_url() + + return get_api_data( + credential_configuration, + 'credentials', + api_client=api_client, + base_api_url=base_api_url, + querystring=querystring, + cache_key=cache_key ) diff --git a/openedx/core/djangoapps/credit/email_utils.py b/openedx/core/djangoapps/credit/email_utils.py index f893068319..a162df7e76 100644 --- a/openedx/core/djangoapps/credit/email_utils.py +++ b/openedx/core/djangoapps/credit/email_utils.py @@ -10,6 +10,7 @@ import urllib import uuid from email.mime.image import MIMEImage from email.mime.multipart import MIMEMultipart +from urllib.parse import urljoin from django.conf import settings from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user @@ -19,14 +20,14 @@ from django.core.mail import EmailMessage, SafeMIMEText from django.urls import reverse from django.utils.translation import gettext as _ from eventtracking import tracker +from xmodule.modulestore.django import modulestore from common.djangoapps.edxmako.shortcuts import render_to_string from common.djangoapps.edxmako.template import Template -from openedx.core.djangoapps.commerce.utils import ecommerce_api_client +from openedx.core.djangoapps.commerce.utils import get_ecommerce_api_base_url, get_ecommerce_api_client from openedx.core.djangoapps.credit.models import CreditConfig, CreditProvider from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from openedx.core.djangolib.markup import HTML -from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order log = logging.getLogger(__name__) @@ -225,7 +226,10 @@ def get_credit_provider_attribute_values(course_key, attribute_name): try: user = User.objects.get(username=settings.ECOMMERCE_SERVICE_WORKER_USERNAME) - response = ecommerce_api_client(user).courses(course_id).get(include_products=1) + api_url = urljoin(f"{get_ecommerce_api_base_url()}/", f"courses/{course_id}/") + response = get_ecommerce_api_client(user).get(api_url, params={"include_products": 1}) + response.raise_for_status() + response = response.json() if response.content else None except Exception: # pylint: disable=broad-except log.exception("Failed to receive data from the ecommerce course API for Course ID '%s'.", course_id) return attribute_values diff --git a/openedx/core/djangoapps/credit/tests/test_api.py b/openedx/core/djangoapps/credit/tests/test_api.py index bca0ccbc2d..7dc644dc09 100644 --- a/openedx/core/djangoapps/credit/tests/test_api.py +++ b/openedx/core/djangoapps/credit/tests/test_api.py @@ -1222,13 +1222,14 @@ class CourseApiTests(CreditApiTestBase): response_providers = get_credit_provider_attribute_values(self.course_key, 'display_name') self.assertListEqual(self.PROVIDERS_LIST, response_providers) - @httpretty.activate - @mock.patch('edx_rest_api_client.client.EdxRestApiClient.__init__') - def test_get_credit_provider_display_names_method_with_exception(self, mock_init): - """Verify that in case of any exception it logs the error and return.""" - mock_init.side_effect = Exception + @mock.patch('openedx.core.djangoapps.credit.email_utils.get_ecommerce_api_client') + def test_get_credit_provider_display_names_method_with_exception(self, mock_get_client): + """ + Verify that in case of any exception it logs the error and return. + """ + mock_get_client.side_effect = Exception response = get_credit_provider_attribute_values(self.course_key, 'display_name') - assert mock_init.called + assert mock_get_client.called assert response is None @httpretty.activate diff --git a/openedx/core/djangoapps/programs/tasks.py b/openedx/core/djangoapps/programs/tasks.py index 7d9049dc5c..e6d1355ee7 100644 --- a/openedx/core/djangoapps/programs/tasks.py +++ b/openedx/core/djangoapps/programs/tasks.py @@ -2,6 +2,7 @@ This file contains celery tasks for programs-related functionality. """ from datetime import datetime +from urllib.parse import urljoin from celery import shared_task from celery.exceptions import MaxRetriesExceededError @@ -11,18 +12,22 @@ from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imp from django.contrib.sites.models import Site from django.core.exceptions import ObjectDoesNotExist from edx_django_utils.monitoring import set_code_owner_attribute -from edx_rest_api_client import exceptions from opaque_keys.edx.keys import CourseKey +from requests.exceptions import HTTPError +from xmodule.data import CertificatesDisplayBehaviors from common.djangoapps.course_modes.models import CourseMode from lms.djangoapps.certificates.api import available_date_for_certificate from lms.djangoapps.certificates.models import GeneratedCertificate from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.credentials.models import CredentialsApiConfig -from openedx.core.djangoapps.credentials.utils import get_credentials, get_credentials_api_client +from openedx.core.djangoapps.credentials.utils import ( + get_credentials, + get_credentials_api_base_url, + get_credentials_api_client, +) from openedx.core.djangoapps.programs.utils import ProgramProgressMeter from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers -from xmodule.data import CertificatesDisplayBehaviors # lint-amnesty, pylint: disable=wrong-import-order LOGGER = get_task_logger(__name__) # Maximum number of retries before giving up on awarding credentials. @@ -96,7 +101,7 @@ def award_program_certificate(client, user, program_uuid, visible_date): Args: client: - credentials API client (EdxRestApiClient) + credentials API client (requests.Session) user: The student's user data program_uuid: @@ -106,22 +111,27 @@ def award_program_certificate(client, user, program_uuid, visible_date): Returns: None - """ - client.credentials.post({ - 'username': user.username, - 'lms_user_id': user.id, - 'credential': { - 'type': PROGRAM_CERTIFICATE, - 'program_uuid': program_uuid - }, - 'attributes': [ - { - 'name': 'visible_date', - 'value': visible_date.strftime(DATE_FORMAT) - } - ] - }) + credentials_api_base_url = get_credentials_api_base_url() + api_url = urljoin(f"{credentials_api_base_url}/", "credentials/") + response = client.post( + api_url, + json={ + 'username': user.username, + 'lms_user_id': user.id, + 'credential': { + 'type': PROGRAM_CERTIFICATE, + 'program_uuid': program_uuid + }, + 'attributes': [ + { + 'name': 'visible_date', + 'value': visible_date.strftime(DATE_FORMAT) + } + ] + } + ) + response.raise_for_status() @shared_task(bind=True, ignore_result=True) @@ -233,17 +243,13 @@ def award_program_certificates(self, username): # lint-amnesty, pylint: disable LOGGER.info(f"Visible date for user {username} : program {program_uuid} is {visible_date}") award_program_certificate(credentials_client, student, program_uuid, visible_date) LOGGER.info(f"Awarded certificate for program {program_uuid} to user {username}") - except exceptions.HttpNotFoundError: - LOGGER.exception( - f"Certificate for program {program_uuid} could not be found. " + - f"Unable to award certificate to user {username}. The program might not be configured." - ) - except exceptions.HttpClientError as exc: - # Grab the status code from the client error, because our API - # client handles all 4XX errors the same way. In the future, - # we may want to fork slumber, add 429 handling, and use that - # in edx_rest_api_client. - if exc.response.status_code == 429: # lint-amnesty, pylint: disable=no-else-raise, no-member + except HTTPError as exc: + if exc.response.status_code == 404: + LOGGER.exception( + f"Certificate for program {program_uuid} could not be found. " + + f"Unable to award certificate to user {username}. The program might not be configured." + ) + elif exc.response.status_code == 429: rate_limit_countdown = 60 error_msg = ( f"Rate limited. " @@ -261,7 +267,7 @@ def award_program_certificates(self, username): # lint-amnesty, pylint: disable f"Unable to award certificate to user {username} for program {program_uuid}. " "The program might not be configured." ) - except Exception as exc: # pylint: disable=broad-except + except Exception: # pylint: disable=broad-except # keep trying to award other certs, but retry the whole task to fix any missing entries LOGGER.exception(f"Failed to award certificate for program {program_uuid} to user {username}.") failed_program_certificate_award_attempts.append(program_uuid) @@ -285,37 +291,51 @@ def award_program_certificates(self, username): # lint-amnesty, pylint: disable def post_course_certificate_configuration(client, cert_config, certificate_available_date=None): """ - POST a configuration for a course certificate and the date the certificate - will be available + POST to a course_certificates endpoint. + + POST a configuration for a course certificate and the date the certificate will be available. """ - client.course_certificates.post({ - "course_id": cert_config['course_id'], - "certificate_type": cert_config['mode'], - "certificate_available_date": certificate_available_date, - "is_active": True - }) + credentials_api_base_url = get_credentials_api_base_url() + api_url = urljoin(f"{credentials_api_base_url}/", "course_certificates/") + response = client.post( + api_url, + json={ + "course_id": cert_config['course_id'], + "certificate_type": cert_config['mode'], + "certificate_available_date": certificate_available_date, + "is_active": True + } + ) + response.raise_for_status() -def post_course_certificate(client, username, certificate, visible_date, date_override=None): +def post_course_certificate(client, username, certificate, visible_date, date_override=None, org=None): """ POST a certificate that has been updated to Credentials """ - client.credentials.post({ - 'username': username, - 'status': 'awarded' if certificate.is_valid() else 'revoked', # Only need the two options at this time - 'credential': { - 'course_run_key': str(certificate.course_id), - 'mode': certificate.mode, - 'type': COURSE_CERTIFICATE, - }, - 'date_override': {'date': date_override.strftime(DATE_FORMAT)} if date_override else None, - 'attributes': [ - { - 'name': 'visible_date', - 'value': visible_date.strftime(DATE_FORMAT) - } - ] - }) + credentials_api_base_url = get_credentials_api_base_url(org) + api_url = urljoin(f"{credentials_api_base_url}/", "credentials/") + + response = client.post( + api_url, + json={ + 'username': username, + 'status': 'awarded' if certificate.is_valid() else 'revoked', # Only need the two options at this time + 'credential': { + 'course_run_key': str(certificate.course_id), + 'mode': certificate.mode, + 'type': COURSE_CERTIFICATE, + }, + 'date_override': {'date': date_override.strftime(DATE_FORMAT)} if date_override else None, + 'attributes': [ + { + 'name': 'visible_date', + 'value': visible_date.strftime(DATE_FORMAT) + } + ] + } + ) + response.raise_for_status() # pylint: disable=W0613 @@ -440,9 +460,8 @@ def award_course_certificate(self, username, course_run_key, certificate_availab f"Task award_course_certificate was called without course overview data for course {course_key}" ) return - credentials_client = get_credentials_api_client(User.objects.get( - username=settings.CREDENTIALS_SERVICE_USERNAME), - org=course_key.org, + credentials_client = get_credentials_api_client( + User.objects.get(username=settings.CREDENTIALS_SERVICE_USERNAME), ) # Date is being passed via JSON and is encoded in the EMCA date time string format. The rest of the code @@ -474,7 +493,9 @@ def award_course_certificate(self, username, course_run_key, certificate_availab except ObjectDoesNotExist: date_override = None - post_course_certificate(credentials_client, username, certificate, visible_date, date_override) + post_course_certificate( + credentials_client, username, certificate, visible_date, date_override, org=course_key.org + ) LOGGER.info(f"Awarded certificate for course {course_key} to user {username}") except Exception as exc: @@ -517,22 +538,27 @@ def revoke_program_certificate(client, username, program_uuid): Revoke a certificate of the given student for the given program. Args: - client: credentials API client (EdxRestApiClient) + client: credentials API client (requests.Session) username: The username of the student program_uuid: uuid of the program Returns: None - """ - client.credentials.post({ - 'username': username, - 'status': 'revoked', - 'credential': { - 'type': PROGRAM_CERTIFICATE, - 'program_uuid': program_uuid + credentials_api_base_url = get_credentials_api_base_url() + api_url = urljoin(f"{credentials_api_base_url}/", "credentials/") + response = client.post( + api_url, + json={ + 'username': username, + 'status': 'revoked', + 'credential': { + 'type': PROGRAM_CERTIFICATE, + 'program_uuid': program_uuid + } } - }) + ) + response.raise_for_status() @shared_task(bind=True, ignore_result=True) @@ -634,21 +660,17 @@ def revoke_program_certificates(self, username, course_key): # lint-amnesty, py try: revoke_program_certificate(credentials_client, username, program_uuid) LOGGER.info(f"Revoked certificate for program {program_uuid} for user {username}") - except exceptions.HttpNotFoundError: - LOGGER.exception( - f"Certificate for program {program_uuid} could not be found. " - f"Unable to revoke certificate for user {username}" - ) - except exceptions.HttpClientError as exc: - # Grab the status code from the client error, because our API - # client handles all 4XX errors the same way. In the future, - # we may want to fork slumber, add 429 handling, and use that - # in edx_rest_api_client. - if exc.response.status_code == 429: # pylint: disable=no-member, no-else-raise + except HTTPError as exc: + if exc.response.status_code == 404: + LOGGER.exception( + f"Certificate for program {program_uuid} could not be found. " + f"Unable to revoke certificate for user {username}" + ) + elif exc.response.status_code == 429: rate_limit_countdown = 60 error_msg = ( - "Rate limited. " - f"Retrying task to revoke certificates for user {username} in {rate_limit_countdown} seconds" + "Rate limited. Retrying task to revoke certificates " + f"for user {username} in {rate_limit_countdown} seconds" ) LOGGER.info(error_msg) # Retry after 60 seconds, when we should be in a new throttling window @@ -659,7 +681,9 @@ def revoke_program_certificates(self, username, course_key): # lint-amnesty, py countdown=rate_limit_countdown ) from exc else: - LOGGER.exception(f"Unable to revoke certificate for user {username} for program {program_uuid}.") + LOGGER.exception( + f"Unable to revoke certificate for user {username} for program {program_uuid}." + ) except Exception: # pylint: disable=broad-except # keep trying to revoke other certs, but retry the whole task to fix any missing entries LOGGER.warning(f"Failed to revoke certificate for program {program_uuid} of user {username}.") diff --git a/openedx/core/djangoapps/programs/tests/test_tasks.py b/openedx/core/djangoapps/programs/tests/test_tasks.py index d86e6f166b..e81ca7d25a 100644 --- a/openedx/core/djangoapps/programs/tests/test_tasks.py +++ b/openedx/core/djangoapps/programs/tests/test_tasks.py @@ -12,11 +12,13 @@ import ddt import httpretty import pytest import pytz +import requests from celery.exceptions import MaxRetriesExceededError from django.conf import settings from django.test import TestCase, override_settings -from edx_rest_api_client import exceptions -from edx_rest_api_client.client import EdxRestApiClient +from edx_rest_api_client.auth import SuppliedJwtAuth +from requests.exceptions import HTTPError +from xmodule.data import CertificatesDisplayBehaviors # lint-amnesty, pylint: disable=wrong-import-order from common.djangoapps.course_modes.tests.factories import CourseModeFactory from common.djangoapps.student.tests.factories import UserFactory @@ -28,7 +30,6 @@ from openedx.core.djangoapps.oauth_dispatch.tests.factories import ApplicationFa from openedx.core.djangoapps.programs import tasks from openedx.core.djangoapps.site_configuration.tests.factories import SiteConfigurationFactory, SiteFactory from openedx.core.djangolib.testing.utils import skip_unless_lms -from xmodule.data import CertificatesDisplayBehaviors # lint-amnesty, pylint: disable=wrong-import-order log = logging.getLogger(__name__) @@ -81,15 +82,17 @@ class AwardProgramCertificateTestCase(TestCase): """ Test the award_program_certificate function """ - @httpretty.activate - def test_award_program_certificate(self): + @mock.patch('openedx.core.djangoapps.programs.tasks.get_credentials_api_base_url') + def test_award_program_certificate(self, mock_get_api_base_url): """ Ensure the correct API call gets made """ + mock_get_api_base_url.return_value = 'http://test-server/' student = UserFactory(username='test-username', email='test-email@email.com') - test_client = EdxRestApiClient('http://test-server', jwt='test-token') + test_client = requests.Session() + test_client.auth = SuppliedJwtAuth('test-token') httpretty.register_uri( httpretty.POST, @@ -401,7 +404,7 @@ class AwardProgramCertificatesTestCase(CatalogIntegrationMixin, CredentialsApiCo """ Verify that a 429 error causes the task to fail and then retry. """ - exception = exceptions.HttpClientError() + exception = HTTPError() exception.response = mock.Mock(status_code=429) mock_get_completed_programs.return_value = {1: 1, 2: 2} mock_award_program_certificate.side_effect = self._make_side_effect( @@ -421,7 +424,7 @@ class AwardProgramCertificatesTestCase(CatalogIntegrationMixin, CredentialsApiCo """ Verify that a 404 error causes the task to fail but there is no retry. """ - exception = exceptions.HttpNotFoundError() + exception = HTTPError() exception.response = mock.Mock(status_code=404) mock_get_completed_programs.return_value = {1: 1, 2: 2} mock_award_program_certificate.side_effect = self._make_side_effect( @@ -441,7 +444,7 @@ class AwardProgramCertificatesTestCase(CatalogIntegrationMixin, CredentialsApiCo """ Verify that other 4XX errors cause task to fail but there is no retry. """ - exception = exceptions.HttpClientError() + exception = HTTPError() exception.response = mock.Mock(status_code=418) mock_get_completed_programs.return_value = {1: 1, 2: 2} mock_award_program_certificate.side_effect = self._make_side_effect( @@ -472,11 +475,14 @@ class PostCourseCertificateTestCase(TestCase): ) @httpretty.activate - def test_post_course_certificate(self): + @mock.patch('openedx.core.djangoapps.programs.tasks.get_credentials_api_base_url') + def test_post_course_certificate(self, mock_get_api_base_url): """ Ensure the correct API call gets made """ - test_client = EdxRestApiClient('http://test-server', jwt='test-token') + mock_get_api_base_url.return_value = 'http://test-server/' + test_client = requests.Session() + test_client.auth = SuppliedJwtAuth('test-token') httpretty.register_uri( httpretty.POST, @@ -651,12 +657,15 @@ class RevokeProgramCertificateTestCase(TestCase): """ @httpretty.activate - def test_revoke_program_certificate(self): + @mock.patch('openedx.core.djangoapps.programs.tasks.get_credentials_api_base_url') + def test_revoke_program_certificate(self, mock_get_api_base_url): """ Ensure the correct API call gets made """ + mock_get_api_base_url.return_value = 'http://test-server/' test_username = 'test-username' - test_client = EdxRestApiClient('http://test-server', jwt='test-token') + test_client = requests.Session() + test_client.auth = SuppliedJwtAuth('test-token') httpretty.register_uri( httpretty.POST, @@ -863,7 +872,7 @@ class RevokeProgramCertificatesTestCase(CatalogIntegrationMixin, CredentialsApiC """ Verify that a 429 error causes the task to fail and then retry. """ - exception = exceptions.HttpClientError() + exception = HTTPError() exception.response = mock.Mock(status_code=429) mock_get_inverted_programs.return_value = self.inverted_programs mock_get_certified_programs.return_value = [1, 2] @@ -884,7 +893,7 @@ class RevokeProgramCertificatesTestCase(CatalogIntegrationMixin, CredentialsApiC """ Verify that a 404 error causes the task to fail but there is no retry. """ - exception = exceptions.HttpNotFoundError() + exception = HTTPError() exception.response = mock.Mock(status_code=404) mock_get_inverted_programs.return_value = self.inverted_programs mock_get_certified_programs.return_value = [1, 2] @@ -905,7 +914,7 @@ class RevokeProgramCertificatesTestCase(CatalogIntegrationMixin, CredentialsApiC """ Verify that other 4XX errors cause task to fail but there is no retry. """ - exception = exceptions.HttpClientError() + exception = HTTPError() exception.response = mock.Mock(status_code=418) mock_get_inverted_programs.return_value = self.inverted_programs mock_get_certified_programs.return_value = [1, 2] @@ -994,11 +1003,14 @@ class PostCourseCertificateConfigurationTestCase(TestCase): } @httpretty.activate - def test_post_course_certificate_configuration(self): + @mock.patch('openedx.core.djangoapps.programs.tasks.get_credentials_api_base_url') + def test_post_course_certificate_configuration(self, mock_get_api_base_url): """ Ensure the correct API call gets made """ - test_client = EdxRestApiClient('http://test-server', jwt='test-token') + mock_get_api_base_url.return_value = 'http://test-server/' + test_client = requests.Session() + test_client.auth = SuppliedJwtAuth('test-token') httpretty.register_uri( httpretty.POST, diff --git a/openedx/core/djangoapps/programs/utils.py b/openedx/core/djangoapps/programs/utils.py index 87a408b413..fde07e242a 100644 --- a/openedx/core/djangoapps/programs/utils.py +++ b/openedx/core/djangoapps/programs/utils.py @@ -15,15 +15,17 @@ from django.contrib.sites.models import Site from django.core.cache import cache from django.urls import reverse from django.utils.functional import cached_property -from edx_rest_api_client.exceptions import SlumberBaseException from opaque_keys.edx.keys import CourseKey from pytz import utc -from requests.exceptions import ConnectionError, Timeout # lint-amnesty, pylint: disable=redefined-builtin +from requests.exceptions import RequestException +from xmodule.modulestore.django import modulestore from common.djangoapps.course_modes.api import get_paid_modes_for_course from common.djangoapps.course_modes.models import CourseMode from common.djangoapps.entitlements.api import get_active_entitlement_list_for_user from common.djangoapps.entitlements.models import CourseEntitlement +from common.djangoapps.student.models import CourseEnrollment +from common.djangoapps.util.date_utils import strftime_localized from lms.djangoapps.certificates import api as certificate_api from lms.djangoapps.certificates.data import CertificateStatuses from lms.djangoapps.certificates.models import GeneratedCertificate @@ -32,19 +34,16 @@ from openedx.core.djangoapps.catalog.api import get_programs_by_type from openedx.core.djangoapps.catalog.constants import PathwayType from openedx.core.djangoapps.catalog.utils import ( get_fulfillable_course_runs_for_entitlement, + get_pathways, get_programs, - get_pathways ) -from openedx.core.djangoapps.commerce.utils import ecommerce_api_client +from openedx.core.djangoapps.commerce.utils import get_ecommerce_api_base_url, get_ecommerce_api_client from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.credentials.utils import get_credentials, get_credentials_records_url from openedx.core.djangoapps.enrollments.api import get_enrollments from openedx.core.djangoapps.enrollments.permissions import ENROLL_IN_COURSE from openedx.core.djangoapps.programs import ALWAYS_CALCULATE_PROGRAM_PRICE_AS_ANONYMOUS_USER from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers -from common.djangoapps.student.models import CourseEnrollment -from common.djangoapps.util.date_utils import strftime_localized -from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order # The datetime module's strftime() methods require a year >= 1900. DEFAULT_ENROLLMENT_START_DATE = datetime.datetime(1900, 1, 1, tzinfo=utc) @@ -756,24 +755,27 @@ class ProgramDataExtender: api_user = service_user is_anonymous = True - api = ecommerce_api_client(api_user) + api_client = get_ecommerce_api_client(api_user) + api_url = urljoin(f"{get_ecommerce_api_base_url()}/", "baskets/calculate/") # The user specific program price is slow to calculate, so use switch to force the # anonymous price for all users. See LEARNER-5555 for more details. if is_anonymous or ALWAYS_CALCULATE_PROGRAM_PRICE_AS_ANONYMOUS_USER.is_enabled(): # The bundle uuid is necessary to see the program's discounted price if bundle_uuid: - discount_data = api.baskets.calculate.get(sku=skus, is_anonymous=True, bundle=bundle_uuid) + params = dict(sku=skus, is_anonymous=True, bundle=bundle_uuid) else: - discount_data = api.baskets.calculate.get(sku=skus, is_anonymous=True) + params = dict(sku=skus, is_anonymous=True) else: if bundle_uuid: - discount_data = api.baskets.calculate.get( + params = dict( sku=skus, username=self.user.username, bundle=bundle_uuid ) else: - discount_data = api.baskets.calculate.get(sku=skus, username=self.user.username) - + params = dict(sku=skus, username=self.user.username) + response = api_client.get(api_url, params=params) + response.raise_for_status() + discount_data = response.json() program_discounted_price = discount_data['total_incl_tax'] program_full_price = discount_data['total_incl_tax_excl_discounts'] discount_data['is_discounted'] = program_discounted_price < program_full_price @@ -784,7 +786,7 @@ class ProgramDataExtender: 'full_program_price': discount_data['total_incl_tax'], 'variant': bundle_variant }) - except (ConnectionError, SlumberBaseException, Timeout): + except RequestException: log.exception('Failed to get discount price for following product SKUs: %s ', ', '.join(skus)) self.data.update({ 'discount_data': {'is_discounted': False} diff --git a/openedx/core/djangoapps/user_api/accounts/settings_views.py b/openedx/core/djangoapps/user_api/accounts/settings_views.py index 4f835fbd38..ae7bca3014 100644 --- a/openedx/core/djangoapps/user_api/accounts/settings_views.py +++ b/openedx/core/djangoapps/user_api/accounts/settings_views.py @@ -16,9 +16,12 @@ from django_countries import countries from common.djangoapps import third_party_auth from common.djangoapps.edxmako.shortcuts import render_to_response +from common.djangoapps.student.models import UserProfile +from common.djangoapps.third_party_auth import pipeline +from common.djangoapps.util.date_utils import strftime_localized from lms.djangoapps.commerce.models import CommerceConfiguration from lms.djangoapps.commerce.utils import EcommerceService -from openedx.core.djangoapps.commerce.utils import ecommerce_api_client +from openedx.core.djangoapps.commerce.utils import get_ecommerce_api_base_url, get_ecommerce_api_client from openedx.core.djangoapps.dark_lang.models import DarkLangConfig from openedx.core.djangoapps.lang_pref.api import all_languages, released_languages from openedx.core.djangoapps.programs.models import ProgramsApiConfig @@ -28,13 +31,10 @@ from openedx.core.djangoapps.user_api.accounts.toggles import ( should_redirect_to_order_history_microfrontend ) from openedx.core.djangoapps.user_api.preferences.api import get_user_preferences -from openedx.core.lib.edx_api_utils import get_edx_api_data +from openedx.core.lib.edx_api_utils import get_api_data from openedx.core.lib.time_zone_utils import TIME_ZONE_CHOICES from openedx.features.enterprise_support.api import enterprise_customer_for_request from openedx.features.enterprise_support.utils import update_account_settings_context_for_enterprise -from common.djangoapps.student.models import UserProfile -from common.djangoapps.third_party_auth import pipeline -from common.djangoapps.util.date_utils import strftime_localized log = logging.getLogger(__name__) @@ -195,9 +195,13 @@ def get_user_orders(user): use_cache = commerce_configuration.is_cache_enabled cache_key = commerce_configuration.CACHE_KEY + '.' + str(user.id) if use_cache else None - api = ecommerce_api_client(user) - commerce_user_orders = get_edx_api_data( - commerce_configuration, 'orders', api=api, querystring=user_query, cache_key=cache_key + commerce_user_orders = get_api_data( + commerce_configuration, + 'orders', + api_client=get_ecommerce_api_client(user), + base_api_url=get_ecommerce_api_base_url(), + querystring=user_query, + cache_key=cache_key ) for order in commerce_user_orders: diff --git a/openedx/core/djangoapps/user_api/accounts/views.py b/openedx/core/djangoapps/user_api/accounts/views.py index d25c6e1c0b..23fdac04e4 100644 --- a/openedx/core/djangoapps/user_api/accounts/views.py +++ b/openedx/core/djangoapps/user_api/accounts/views.py @@ -51,12 +51,7 @@ from common.djangoapps.student.models import ( # lint-amnesty, pylint: disable= is_username_retired, is_email_retired ) -from common.djangoapps.student.models_api import ( - confirm_name_change, - do_name_change_request, - get_pending_name_change -) -from openedx.core.djangoapps.user_api.accounts import RETIRED_EMAIL_MSG +from common.djangoapps.student.models_api import confirm_name_change, do_name_change_request, get_pending_name_change from openedx.core.djangoapps.ace_common.template_context import get_base_template_context from openedx.core.djangoapps.api_admin.models import ApiAccessRequest from openedx.core.djangoapps.course_groups.models import UnregisteredLearnerCohortAssignments @@ -64,6 +59,7 @@ from openedx.core.djangoapps.credit.models import CreditRequest, CreditRequireme from openedx.core.djangoapps.external_user_ids.models import ExternalId, ExternalIdType from openedx.core.djangoapps.lang_pref import LANGUAGE_KEY from openedx.core.djangoapps.profile_images.images import remove_profile_images +from openedx.core.djangoapps.user_api.accounts import RETIRED_EMAIL_MSG from openedx.core.djangoapps.user_api.accounts.image_helpers import get_profile_image_names, set_has_profile_image from openedx.core.djangoapps.user_authn.exceptions import AuthFailedError from openedx.core.lib.api.authentication import BearerAuthenticationAllowInactiveUser @@ -76,7 +72,7 @@ from ..models import ( RetirementStateError, UserOrgTag, UserRetirementPartnerReportingStatus, - UserRetirementStatus + UserRetirementStatus, ) from .api import get_account_settings, update_account_settings from .permissions import CanDeactivateUser, CanGetAccountInfo, CanReplaceUsername, CanRetireUser @@ -84,7 +80,7 @@ from .serializers import ( PendingNameChangeSerializer, UserRetirementPartnerReportSerializer, UserRetirementStatusSerializer, - UserSearchEmailSerializer + UserSearchEmailSerializer, ) from .signals import USER_RETIRE_LMS_CRITICAL, USER_RETIRE_LMS_MISC, USER_RETIRE_MAILINGS from .utils import create_retirement_request_and_deactivate_account, username_suffix_generator @@ -966,10 +962,8 @@ class AccountRetirementStatusView(ViewSet): Updates the RetirementStatus row for the given user to the new status, and append any messages to the message log. - Note that this implementation DOES NOT use the "merge patch" - implementation seen in AccountViewSet. Slumber, the project - we use to power edx-rest-api-client, does not currently support - it. The content type for this request is 'application/json'. + Note that this implementation DOES NOT use the "merge patch" implementation seen in AccountViewSet. + The content type for this request is 'application/json'. """ try: username = request.data['username'] diff --git a/openedx/core/djangoapps/user_api/management/commands/sync_hubspot_contacts.py b/openedx/core/djangoapps/user_api/management/commands/sync_hubspot_contacts.py index 4f29c4d449..ef1f386317 100644 --- a/openedx/core/djangoapps/user_api/management/commands/sync_hubspot_contacts.py +++ b/openedx/core/djangoapps/user_api/management/commands/sync_hubspot_contacts.py @@ -8,18 +8,18 @@ Management command to sync platform users with hubspot import json import time import traceback +import urllib.parse # pylint: disable=import-error from datetime import datetime, timedelta from textwrap import dedent -import urllib.parse # pylint: disable=import-error +import requests from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user from django.core.management.base import BaseCommand, CommandError -from edx_rest_api_client.client import EdxRestApiClient -from slumber.exceptions import HttpClientError, HttpServerError +from requests.exceptions import HTTPError -from openedx.core.djangoapps.site_configuration.models import SiteConfiguration from common.djangoapps.student.models import UserAttribute from common.djangoapps.util.query import use_read_replica_if_available +from openedx.core.djangoapps.site_configuration.models import SiteConfiguration HUBSPOT_API_BASE_URL = 'https://api.hubapi.com' @@ -139,13 +139,14 @@ class Command(BaseCommand): contacts.append(contact) api_key = site_conf.get_value('HUBSPOT_API_KEY') - client = EdxRestApiClient(urllib.parse.urljoin(HUBSPOT_API_BASE_URL, 'contacts/v1/contact')) + api_url = urllib.parse.urljoin(f"{HUBSPOT_API_BASE_URL}/", 'contacts/v1/contact/batch/') try: - client.batch.post(contacts, hapikey=api_key) + response = requests.post(api_url, json=contacts, params={"hapikey": api_key}) + response.raise_for_status() return len(contacts) - except (HttpClientError, HttpServerError) as ex: + except HTTPError as ex: message = 'An error occurred while syncing batch of contacts for site {domain}, {message}'.format( - domain=site_conf.site.domain, message=ex.message # lint-amnesty, pylint: disable=no-member + domain=site_conf.site.domain, message=str(ex) ) self.stderr.write(message) return 0 diff --git a/openedx/core/djangoapps/video_pipeline/api.py b/openedx/core/djangoapps/video_pipeline/api.py index 8daf8e6e02..77069e3565 100644 --- a/openedx/core/djangoapps/video_pipeline/api.py +++ b/openedx/core/djangoapps/video_pipeline/api.py @@ -7,7 +7,7 @@ import logging from django.core.exceptions import ObjectDoesNotExist from oauth2_provider.models import Application -from slumber.exceptions import HttpClientError +from requests.exceptions import HTTPError from openedx.core.djangoapps.video_pipeline.models import VEMPipelineIntegration from openedx.core.djangoapps.video_pipeline.utils import create_video_pipeline_api_client @@ -29,27 +29,30 @@ def send_transcript_credentials(pipeline_integration, credentials_payload): oauth_client.client_id, oauth_client.client_secret ) - error_message = "Unable to update transcript credentials -- org={}, provider={}, response={}" + error_message = "Unable to update transcript credentials -- org=%s, provider=%s, response=%s" try: - response = client.request("POST", pipeline_integration.api_url, json=credentials_payload) + response = client.post(pipeline_integration.api_url, json=credentials_payload) + response.raise_for_status() if response.ok: is_updated = True else: is_updated = False error_response = json.loads(response.text) - log.error(error_message.format( + log.error( + error_message, credentials_payload.get('org'), credentials_payload.get('provider'), response.text - )) - except HttpClientError as ex: + ) + except HTTPError as ex: is_updated = False - log.exception(error_message.format( + log.exception( + error_message, credentials_payload.get('org'), credentials_payload.get('provider'), - ex.content - )) - error_response = json.loads(ex.content) + ex.response.content + ) + error_response = json.loads(ex.response.content) return error_response, is_updated diff --git a/openedx/core/djangoapps/video_pipeline/tests/test_api.py b/openedx/core/djangoapps/video_pipeline/tests/test_api.py index b4d3dd58fb..35cc9eaf29 100644 --- a/openedx/core/djangoapps/video_pipeline/tests/test_api.py +++ b/openedx/core/djangoapps/video_pipeline/tests/test_api.py @@ -7,9 +7,9 @@ from unittest.mock import Mock, patch import ddt from django.test.testcases import TestCase -from slumber.exceptions import HttpClientError -from common.djangoapps.student.tests.factories import UserFactory +from requests.exceptions import HTTPError +from common.djangoapps.student.tests.factories import UserFactory from openedx.core.djangoapps.video_pipeline.api import update_3rd_party_transcription_service_credentials from openedx.core.djangoapps.video_pipeline.tests.mixins import VideoPipelineMixin @@ -82,7 +82,10 @@ class TestAPIUtils(VideoPipelineMixin, TestCase): during communication with edx-video-pipeline. """ error_content = '{"error_type": "1"}' - mock_client.return_value.request = Mock(side_effect=HttpClientError(content=error_content)) + resp = Mock() + resp.content = error_content + resp.raise_for_status.side_effect = HTTPError(response=resp) + mock_client.return_value.post.return_value = resp # try updating the transcription service credentials credentials_payload = { @@ -96,9 +99,8 @@ class TestAPIUtils(VideoPipelineMixin, TestCase): assert not is_updated self.assertDictEqual(error_response, json.loads(error_content)) mock_logger.exception.assert_called_with( - 'Unable to update transcript credentials -- org={}, provider={}, response={}'.format( - credentials_payload['org'], - credentials_payload['provider'], - error_content - ) + 'Unable to update transcript credentials -- org=%s, provider=%s, response=%s', + credentials_payload['org'], + credentials_payload['provider'], + error_content ) diff --git a/openedx/core/lib/edx_api_utils.py b/openedx/core/lib/edx_api_utils.py index 3828c06743..b34780f78b 100644 --- a/openedx/core/lib/edx_api_utils.py +++ b/openedx/core/lib/edx_api_utils.py @@ -19,107 +19,6 @@ def get_fields(fields, response): return results -def get_edx_api_data(api_config, resource, api, resource_id=None, querystring=None, cache_key=None, many=True, - traverse_pagination=True, fields=None, long_term_cache=False): - """ - GET data from an edX REST API_client. - - DEPRECATED: To be replaced with get_api_data as part of FC-0001. - - DRY utility for handling caching and pagination. - - Arguments: - api_config (ConfigurationModel): The configuration model governing interaction with the API. - resource (str): Name of the API resource being requested. - - Keyword Arguments: - api (APIClient): API client to use for requesting data. - resource_id (int or str): Identifies a specific resource to be retrieved. - querystring (dict): Optional query string parameters. - cache_key (str): Where to cache retrieved data. The cache will be ignored if this is omitted - (neither inspected nor updated). - many (bool): Whether the resource requested is a collection of objects, or a single object. - If false, an empty dict will be returned in cases of failure rather than the default empty list. - traverse_pagination (bool): Whether to traverse pagination or return paginated response.. - long_term_cache (bool): Whether to use the long term cache ttl or the standard cache ttl - - Returns: - Data returned by the API. When hitting a list endpoint, extracts "results" (list of dict) - returned by DRF-powered APIs. - """ - no_data = [] if many else {} - - if not api_config.enabled: - log.warning('%s configuration is disabled.', api_config.API_NAME) - return no_data - - if cache_key: - cache_key = f'{cache_key}.{resource_id}' if resource_id is not None else cache_key - cache_key += '.zpickled' - - cached = cache.get(cache_key) - if cached: - try: - cached_response = zunpickle(cached) - except Exception: # pylint: disable=broad-except - # Data is corrupt in some way. - log.warning("Data for cache is corrupt for cache key %s", cache_key) - cache.delete(cache_key) - else: - if fields: - cached_response = get_fields(fields, cached_response) - - return cached_response - - try: - endpoint = getattr(api, resource) - querystring = querystring if querystring else {} - response = endpoint(resource_id).get(**querystring) - - if resource_id is None and traverse_pagination: - results = _traverse_pagination(response, endpoint, querystring, no_data) - else: - results = response - - except: # pylint: disable=bare-except - log.exception('Failed to retrieve data from the %s API.', api_config.API_NAME) - return no_data - - if cache_key: - zdata = zpickle(results) - cache_ttl = api_config.cache_ttl - if long_term_cache: - cache_ttl = api_config.long_term_cache_ttl - cache.set(cache_key, zdata, cache_ttl) - - if fields: - results = get_fields(fields, results) - - return results - - -def _traverse_pagination(response, endpoint, querystring, no_data): - """ - Traverse a paginated API response. - - DEPRECATED: To be replaced with get_results_with_traverse_pagination as part of FC-0001. - - Extracts and concatenates "results" (list of dict) returned by DRF-powered APIs. - """ - results = response.get('results', no_data) - - page = 1 - next_page = response.get('next') - while next_page: - page += 1 - querystring['page'] = page - response = endpoint.get(**querystring) - results += response.get('results', no_data) - next_page = response.get('next') - - return results - - def get_api_data(api_config, resource, api_client, base_api_url, resource_id=None, querystring=None, cache_key=None, many=True, traverse_pagination=True, fields=None, long_term_cache=False): @@ -178,14 +77,14 @@ def get_api_data(api_config, resource, api_client, base_api_url, resource_id=Non querystring = querystring if querystring else {} api_url = urljoin( f"{base_api_url}/", - f"{resource}/{resource_id if resource_id else ''}" + f"{resource}/{str(resource_id) + '/' if resource_id is not None else ''}" ) response = api_client.get(api_url, params=querystring) response.raise_for_status() response = response.json() if resource_id is None and traverse_pagination: - results = get_results_with_traverse_pagination(response, api_client, api_url, querystring, no_data) + results = _traverse_pagination(response, api_client, api_url, querystring, no_data) else: results = response @@ -206,7 +105,7 @@ def get_api_data(api_config, resource, api_client, base_api_url, resource_id=Non return results -def get_results_with_traverse_pagination(response, api_client, api_url, querystring, no_data): +def _traverse_pagination(response, api_client, api_url, querystring, no_data): """ Traverse a paginated API response. diff --git a/openedx/core/lib/tests/test_edx_api_utils.py b/openedx/core/lib/tests/test_edx_api_utils.py index e07af3591a..5a71eeb1e7 100644 --- a/openedx/core/lib/tests/test_edx_api_utils.py +++ b/openedx/core/lib/tests/test_edx_api_utils.py @@ -3,17 +3,18 @@ import json from unittest import mock +from urllib.parse import urljoin import httpretty from django.core.cache import cache +from common.djangoapps.student.tests.factories import UserFactory from openedx.core.djangoapps.catalog.models import CatalogIntegration from openedx.core.djangoapps.catalog.tests.mixins import CatalogIntegrationMixin -from openedx.core.djangoapps.catalog.utils import create_catalog_api_client +from openedx.core.djangoapps.catalog.utils import get_catalog_api_client from openedx.core.djangoapps.credentials.tests.mixins import CredentialsApiConfigMixin from openedx.core.djangolib.testing.utils import CacheIsolationTestCase, skip_unless_lms -from openedx.core.lib.edx_api_utils import get_edx_api_data -from common.djangoapps.student.tests.factories import UserFactory +from openedx.core.lib.edx_api_utils import get_api_data UTILITY_MODULE = 'openedx.core.lib.edx_api_utils' TEST_API_URL = 'http://www-internal.example.com/api' @@ -22,13 +23,16 @@ TEST_API_URL = 'http://www-internal.example.com/api' @skip_unless_lms @httpretty.activate class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, CacheIsolationTestCase): - """Tests for edX API data retrieval utility.""" + """ + Tests for edX API data retrieval utility. + """ ENABLED_CACHES = ['default'] def setUp(self): super().setUp() self.user = UserFactory() + self.base_api_url = CatalogIntegration.current().get_internal_api_url().strip('/') httpretty.httpretty.reset() cache.clear() @@ -36,18 +40,22 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach def _mock_catalog_api(self, responses, url=None): assert httpretty.is_enabled(), 'httpretty must be enabled to mock Catalog API calls.' - url = url if url else CatalogIntegration.current().get_internal_api_url().strip('/') + '/programs/' + url = url if url else urljoin(f"{self.base_api_url}/", "programs/") httpretty.register_uri(httpretty.GET, url, responses=responses) def _assert_num_requests(self, count): - """DRY helper for verifying request counts.""" + """ + DRY helper for verifying request counts. + """ assert len(httpretty.httpretty.latest_requests) == count def test_get_unpaginated_data(self): - """Verify that unpaginated data can be retrieved.""" + """ + Verify that unpaginated data can be retrieved. + """ catalog_integration = self.create_catalog_integration() - api = create_catalog_api_client(self.user) + api = get_catalog_api_client(self.user) expected_collection = ['some', 'test', 'data'] data = { @@ -59,8 +67,10 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach [httpretty.Response(body=json.dumps(data), content_type='application/json')] ) - with mock.patch('edx_rest_api_client.client.EdxRestApiClient.__init__') as mock_init: - actual_collection = get_edx_api_data(catalog_integration, 'programs', api=api) + with mock.patch('requests.Session') as mock_init: + actual_collection = get_api_data( + catalog_integration, 'programs', api_client=api, base_api_url=self.base_api_url + ) # Verify that the helper function didn't initialize its own client. assert not mock_init.called @@ -70,12 +80,14 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach self._assert_num_requests(1) def test_get_paginated_data(self): - """Verify that paginated data can be retrieved.""" + """ + Verify that paginated data can be retrieved. + """ catalog_integration = self.create_catalog_integration() - api = create_catalog_api_client(self.user) + api = get_catalog_api_client(self.user) expected_collection = ['some', 'test', 'data'] - url = CatalogIntegration.current().get_internal_api_url().strip('/') + '/programs/?page={}' + url = urljoin(f"{self.base_api_url}/", "/programs/?page={}") responses = [] for page, record in enumerate(expected_collection, start=1): @@ -91,7 +103,9 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach self._mock_catalog_api(responses) - actual_collection = get_edx_api_data(catalog_integration, 'programs', api=api) + actual_collection = get_api_data( + catalog_integration, 'programs', api_client=api, base_api_url=self.base_api_url + ) assert actual_collection == expected_collection self._assert_num_requests(len(expected_collection)) @@ -101,9 +115,9 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach Verify that pagination is not traversed if traverse_pagination=False is passed as argument. """ catalog_integration = self.create_catalog_integration() - api = create_catalog_api_client(self.user) + api = get_catalog_api_client(self.user) - url = CatalogIntegration.current().get_internal_api_url().strip('/') + '/programs/?page={}' + url = urljoin(f"{self.base_api_url}/", "/programs/?page={}") responses = [ { 'next': url.format(2), @@ -120,20 +134,21 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach [httpretty.Response(body=json.dumps(body), content_type='application/json') for body in responses] ) - actual_collection = get_edx_api_data(catalog_integration, 'programs', api=api, traverse_pagination=False) + actual_collection = get_api_data( + catalog_integration, 'programs', api_client=api, base_api_url=self.base_api_url, traverse_pagination=False + ) assert actual_collection == expected_response self._assert_num_requests(1) def test_get_specific_resource(self): - """Verify that a specific resource can be retrieved.""" + """ + Verify that a specific resource can be retrieved. + """ catalog_integration = self.create_catalog_integration() - api = create_catalog_api_client(self.user) + api = get_catalog_api_client(self.user) resource_id = 1 - url = '{api_root}/programs/{resource_id}/'.format( - api_root=CatalogIntegration.current().get_internal_api_url().strip('/'), - resource_id=resource_id, - ) + url = urljoin(f"{self.base_api_url}/", f"programs/{resource_id}/") expected_resource = {'key': 'value'} @@ -142,13 +157,17 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach url=url ) - actual_resource = get_edx_api_data(catalog_integration, 'programs', api=api, resource_id=resource_id) + actual_resource = get_api_data( + catalog_integration, 'programs', api_client=api, base_api_url=self.base_api_url, resource_id=resource_id + ) assert actual_resource == expected_resource self._assert_num_requests(1) def test_get_specific_resource_with_falsey_id(self): """ + Check the call with falsey resource_id. + Verify that a specific resource can be retrieved, and pagination parsing is not attempted, if the ID passed is falsey (e.g., 0). The expected resource contains a "results" key, as a paginatable item would have, so if the function looks for falsey @@ -156,13 +175,10 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach return the value of that "results" key. """ catalog_integration = self.create_catalog_integration() - api = create_catalog_api_client(self.user) + api = get_catalog_api_client(self.user) resource_id = 0 - url = '{api_root}/programs/{resource_id}/'.format( - api_root=CatalogIntegration.current().get_internal_api_url().strip('/'), - resource_id=resource_id, - ) + url = urljoin(f"{self.base_api_url}/", f"programs/{resource_id}/") expected_resource = {'key': 'value', 'results': []} @@ -171,23 +187,24 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach url=url ) - actual_resource = get_edx_api_data(catalog_integration, 'programs', api=api, resource_id=resource_id) + actual_resource = get_api_data( + catalog_integration, 'programs', api_client=api, base_api_url=self.base_api_url, resource_id=resource_id + ) assert actual_resource == expected_resource self._assert_num_requests(1) def test_get_specific_fields_from_cache_response(self): - """Verify that resource response is cached and get required fields from cached response""" + """ + Verify that resource response is cached and get required fields from cached response. + """ catalog_integration = self.create_catalog_integration(cache_ttl=5) - api = create_catalog_api_client(self.user) + api = get_catalog_api_client(self.user) response = {'lang': 'en', 'weeks_to_complete': '5'} resource_id = 'course-v1:testX+testABC+1T2019' - url = '{api_root}/course_runs/{resource_id}/'.format( - api_root=CatalogIntegration.current().get_internal_api_url().strip('/'), - resource_id=resource_id, - ) + url = urljoin(f"{self.base_api_url}/", f"course_runs/{resource_id}/") expected_resource_for_lang = {'lang': 'en'} expected_resource_for_weeks_to_complete = {'weeks_to_complete': '5'} @@ -200,14 +217,25 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach cache_key = CatalogIntegration.current().CACHE_KEY # get response and set the cache. - actual_resource_for_lang = get_edx_api_data( - catalog_integration, 'course_runs', resource_id=resource_id, api=api, cache_key=cache_key, fields=['lang'] + actual_resource_for_lang = get_api_data( + catalog_integration, + 'course_runs', + resource_id=resource_id, + api_client=api, + base_api_url=self.base_api_url, + cache_key=cache_key, + fields=['lang'] ) assert actual_resource_for_lang == expected_resource_for_lang # Hit the cache - actual_resource = get_edx_api_data( - catalog_integration, 'course_runs', api=api, resource_id=resource_id, cache_key=cache_key, + actual_resource = get_api_data( + catalog_integration, + 'course_runs', + api_client=api, + base_api_url=self.base_api_url, + resource_id=resource_id, + cache_key=cache_key, fields=['weeks_to_complete'] ) @@ -217,9 +245,11 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach self._assert_num_requests(1) def test_cache_utilization(self): - """Verify that when enabled, the cache is used.""" + """ + Verify that when enabled, the cache is used. + """ catalog_integration = self.create_catalog_integration(cache_ttl=5) - api = create_catalog_api_client(self.user) + api = get_catalog_api_client(self.user) expected_collection = ['some', 'test', 'data'] data = { @@ -232,10 +262,7 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach ) resource_id = 1 - url = '{api_root}/programs/{resource_id}/'.format( - api_root=CatalogIntegration.current().get_internal_api_url().strip('/'), - resource_id=resource_id, - ) + url = urljoin(f"{self.base_api_url}/", f"programs/{resource_id}/") expected_resource = {'key': 'value'} @@ -247,15 +274,31 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach cache_key = CatalogIntegration.current().CACHE_KEY # Warm up the cache. - get_edx_api_data(catalog_integration, 'programs', api=api, cache_key=cache_key) - get_edx_api_data(catalog_integration, 'programs', api=api, resource_id=resource_id, cache_key=cache_key) + get_api_data( + catalog_integration, 'programs', api_client=api, base_api_url=self.base_api_url, cache_key=cache_key + ) + get_api_data( + catalog_integration, + 'programs', + api_client=api, + base_api_url=self.base_api_url, + resource_id=resource_id, + cache_key=cache_key + ) # Hit the cache. - actual_collection = get_edx_api_data(catalog_integration, 'programs', api=api, cache_key=cache_key) + actual_collection = get_api_data( + catalog_integration, 'programs', api_client=api, base_api_url=self.base_api_url, cache_key=cache_key + ) assert actual_collection == expected_collection - actual_resource = get_edx_api_data( - catalog_integration, 'programs', api=api, resource_id=resource_id, cache_key=cache_key + actual_resource = get_api_data( + catalog_integration, + 'programs', + api_client=api, + base_api_url=self.base_api_url, + resource_id=resource_id, + cache_key=cache_key ) assert actual_resource == expected_resource @@ -264,38 +307,45 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach @mock.patch(UTILITY_MODULE + '.log.warning') def test_api_config_disabled(self, mock_warning): - """Verify that no data is retrieved if the provided config model is disabled.""" + """ + Verify that no data is retrieved if the provided config model is disabled. + """ catalog_integration = self.create_catalog_integration(enabled=False) - actual = get_edx_api_data(catalog_integration, 'programs', api=None) + actual = get_api_data(catalog_integration, 'programs', api_client=None, base_api_url=self.base_api_url) assert mock_warning.called assert actual == [] @mock.patch(UTILITY_MODULE + '.log.exception') def test_data_retrieval_failure(self, mock_exception): - """Verify that an exception is logged when data can't be retrieved.""" + """ + Verify that an exception is logged when data can't be retrieved. + """ catalog_integration = self.create_catalog_integration() - api = create_catalog_api_client(self.user) + api = get_catalog_api_client(self.user) self._mock_catalog_api( [httpretty.Response(body='clunk', content_type='application/json', status_code=500)] ) - actual = get_edx_api_data(catalog_integration, 'programs', api=api) + actual = get_api_data(catalog_integration, 'programs', api_client=api, base_api_url=self.base_api_url) assert mock_exception.called assert actual == [] @mock.patch(UTILITY_MODULE + '.log.warning') def test_api_config_disabled_with_id_and_not_collection(self, mock_warning): - """Verify that no data is retrieved if the provided config model is disabled.""" + """ + Verify that no data is retrieved if the provided config model is disabled. + """ catalog_integration = self.create_catalog_integration(enabled=False) - actual = get_edx_api_data( + actual = get_api_data( catalog_integration, 'programs', - api=None, + api_client=None, + base_api_url=self.base_api_url, resource_id=100, many=False ) @@ -305,18 +355,21 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach @mock.patch(UTILITY_MODULE + '.log.exception') def test_data_retrieval_failure_with_id(self, mock_exception): - """Verify that an exception is logged when data can't be retrieved.""" + """ + Verify that an exception is logged when data can't be retrieved. + """ catalog_integration = self.create_catalog_integration() - api = create_catalog_api_client(self.user) + api = get_catalog_api_client(self.user) self._mock_catalog_api( [httpretty.Response(body='clunk', content_type='application/json', status_code=500)] ) - actual = get_edx_api_data( + actual = get_api_data( catalog_integration, 'programs', - api=api, + api_client=api, + base_api_url=self.base_api_url, resource_id=100, many=False ) diff --git a/openedx/features/enterprise_support/api.py b/openedx/features/enterprise_support/api.py index 1ec3445b3a..a09600dfa3 100644 --- a/openedx/features/enterprise_support/api.py +++ b/openedx/features/enterprise_support/api.py @@ -2,11 +2,12 @@ APIs providing support for enterprise functionality. """ - import logging import traceback from functools import wraps +from urllib.parse import urljoin +import requests from crum import get_current_request from django.conf import settings from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user @@ -18,8 +19,8 @@ from django.urls import reverse from django.utils.http import urlencode from django.utils.translation import gettext as _ from edx_django_utils.cache import TieredCache, get_cache_key -from edx_rest_api_client.client import EdxRestApiClient -from slumber.exceptions import HttpClientError, HttpNotFoundError, HttpServerError +from edx_rest_api_client.auth import SuppliedJwtAuth +from requests.exceptions import HTTPError from common.djangoapps.third_party_auth.pipeline import get as get_partial_pipeline from common.djangoapps.third_party_auth.provider import Registry @@ -70,13 +71,12 @@ class ConsentApiClient: provided user. """ jwt = create_jwt_for_user(user) - url = configuration_helpers.get_value('ENTERPRISE_CONSENT_API_URL', settings.ENTERPRISE_CONSENT_API_URL) - self.client = EdxRestApiClient( - url, - jwt=jwt, - append_slash=False, + base_api_url = configuration_helpers.get_value( + 'ENTERPRISE_CONSENT_API_URL', settings.ENTERPRISE_CONSENT_API_URL ) - self.consent_endpoint = self.client.data_sharing_consent + self.client = requests.Session() + self.client.auth = SuppliedJwtAuth(jwt) + self.consent_endpoint = urljoin(f"{base_api_url}/", "data_sharing_consent") def revoke_consent(self, **kwargs): """ @@ -85,7 +85,9 @@ class ConsentApiClient: This endpoint takes any given kwargs, which are understood as filtering the conceptual scope of the consent involved in the request. """ - return self.consent_endpoint.delete(**kwargs) + response = self.client.delete(self.consent_endpoint, json=kwargs) + response.raise_for_status() + return response.json() def provide_consent(self, **kwargs): """ @@ -94,7 +96,9 @@ class ConsentApiClient: This endpoint takes any given kwargs, which are understood as filtering the conceptual scope of the consent involved in the request. """ - return self.consent_endpoint.post(kwargs) + response = self.client.post(self.consent_endpoint, json=kwargs) + response.raise_for_status() + return response.json() def consent_required(self, enrollment_exists=False, **kwargs): """ @@ -105,7 +109,9 @@ class ConsentApiClient: """ # Call the endpoint with the given kwargs, and check the value that it provides. - response = self.consent_endpoint.get(**kwargs) + response = self.client.get(self.consent_endpoint, params=kwargs) + response.raise_for_status() + response = response.json() LOGGER.info( '[ENTERPRISE DSC] Consent Requirement Info. APIParams: [%s], APIResponse: [%s], EnrollmentExists: [%s]', @@ -149,19 +155,21 @@ class EnterpriseApiClient: def __init__(self, user): """ - Initialize an authenticated Enterprise service API client by using the - provided user. + Initialize an authenticated Enterprise service API client. + + Authentificate by jwt token using the provided user. """ self.user = user jwt = create_jwt_for_user(user) - self.client = EdxRestApiClient( - configuration_helpers.get_value('ENTERPRISE_API_URL', settings.ENTERPRISE_API_URL), - jwt=jwt - ) + self.base_api_url = configuration_helpers.get_value('ENTERPRISE_API_URL', settings.ENTERPRISE_API_URL) + self.client = requests.Session() + self.client.auth = SuppliedJwtAuth(jwt) def get_enterprise_customer(self, uuid): - endpoint = getattr(self.client, 'enterprise-customer') - return endpoint(uuid).get() + api_url = urljoin(f"{self.base_api_url}/", f"enterprise-customer/{uuid}/") + response = self.client.get(api_url) + response.raise_for_status() + return response.json() def post_enterprise_course_enrollment(self, username, course_id): """ @@ -171,10 +179,11 @@ class EnterpriseApiClient: 'username': username, 'course_id': course_id, } - endpoint = getattr(self.client, 'enterprise-course-enrollment') + api_url = urljoin(f"{self.base_api_url}/", "enterprise-course-enrollment/") try: - endpoint.post(data=data) - except (HttpClientError, HttpServerError): + response = self.client.post(api_url, data=data) + response.raise_for_status() + except HTTPError: message = ( "An error occured while posting EnterpriseCourseEnrollment for user {username} and " "course run {course_id}." @@ -251,26 +260,17 @@ class EnterpriseApiClient: } ], } - - Raises: - ConnectionError: requests exception "ConnectionError", raised if if ecommerce is unable to connect - to enterprise api server. - SlumberBaseException: base slumber exception "SlumberBaseException", raised if API response contains - http error status like 4xx, 5xx etc. - Timeout: requests exception "Timeout", raised if enterprise API is taking too long for returning - a response. This exception is raised for both connection timeout and read timeout. - """ if not user.is_authenticated: return None - api_resource_name = 'enterprise-learner' + api_url = urljoin(f"{self.base_api_url}/", "enterprise-learner/") try: - endpoint = getattr(self.client, api_resource_name) querystring = {'username': user.username} - response = endpoint().get(**querystring) - except (HttpClientError, HttpServerError): + response = self.client.get(api_url, params=querystring) + response.raise_for_status() + except HTTPError: LOGGER.exception( 'Failed to get enterprise-learner for user [%s] with client user [%s]. Caller: %s, Request PATH: %s', user.username, @@ -280,7 +280,7 @@ class EnterpriseApiClient: ) return None - return response + return response.json() class EnterpriseApiServiceClient(EnterpriseServiceClientMixin, EnterpriseApiClient): @@ -295,8 +295,10 @@ class EnterpriseApiServiceClient(EnterpriseServiceClientMixin, EnterpriseApiClie """ enterprise_customer = enterprise_customer_from_cache(uuid=uuid) if enterprise_customer is _CACHE_MISS: - endpoint = getattr(self.client, 'enterprise-customer') - enterprise_customer = endpoint(uuid).get() + api_url = urljoin(f"{self.base_api_url}/", f"enterprise-customer/{uuid}/") + response = self.client.get(api_url) + response.raise_for_status() + enterprise_customer = response.json() if response.content else None if enterprise_customer: cache_enterprise(enterprise_customer) @@ -476,8 +478,11 @@ def enterprise_customer_from_api(request): try: enterprise_customer = enterprise_api_client.get_enterprise_customer(enterprise_customer_uuid) - except HttpNotFoundError: - enterprise_customer = None + except HTTPError as err: + if err.response.status_code == 404: + enterprise_customer = None + else: + raise return enterprise_customer diff --git a/openedx/features/enterprise_support/signals.py b/openedx/features/enterprise_support/signals.py index 975058b867..9c05e988a0 100644 --- a/openedx/features/enterprise_support/signals.py +++ b/openedx/features/enterprise_support/signals.py @@ -4,23 +4,24 @@ This module contains signals related to enterprise. import logging +from urllib.parse import urljoin from django.conf import settings from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user from django.db.models.signals import post_save, pre_save from django.dispatch import receiver -from enterprise.models import EnterpriseCourseEnrollment, EnterpriseCustomer, EnterpriseCustomerUser # lint-amnesty, pylint: disable=unused-import +from enterprise.models import EnterpriseCourseEnrollment, EnterpriseCustomer from integrated_channels.integrated_channel.tasks import ( transmit_single_learner_data, transmit_single_subsection_learner_data ) -from slumber.exceptions import HttpClientError +from requests.exceptions import HTTPError -from openedx.core.djangoapps.commerce.utils import ecommerce_api_client -from openedx.core.djangoapps.signals.signals import COURSE_GRADE_NOW_PASSED, COURSE_ASSESSMENT_GRADE_CHANGED +from common.djangoapps.student.signals import UNENROLL_DONE +from openedx.core.djangoapps.commerce.utils import get_ecommerce_api_base_url, get_ecommerce_api_client +from openedx.core.djangoapps.signals.signals import COURSE_ASSESSMENT_GRADE_CHANGED, COURSE_GRADE_NOW_PASSED from openedx.features.enterprise_support.tasks import clear_enterprise_customer_data_consent_share_cache from openedx.features.enterprise_support.utils import clear_data_consent_share_cache, is_enterprise_learner -from common.djangoapps.student.signals import UNENROLL_DONE log = logging.getLogger(__name__) @@ -105,13 +106,17 @@ def refund_order_voucher(sender, course_enrollment, skip_refund=False, **kwargs) return service_user = User.objects.get(username=settings.ECOMMERCE_SERVICE_WORKER_USERNAME) - client = ecommerce_api_client(service_user) + client = get_ecommerce_api_client(service_user) + api_url = urljoin( + f"{get_ecommerce_api_base_url()}/", "coupons/create_refunded_voucher/" + ) order_number = course_enrollment.get_order_attribute_value('order_number') if order_number: error_message = "Encountered {} from ecommerce while creating refund voucher. Order={}, enrollment={}, user={}" try: - client.enterprise.coupons.create_refunded_voucher.post({"order": order_number}) - except HttpClientError as ex: + response = client.post(api_url, data={"order": order_number}) + response.raise_for_status() + except HTTPError as ex: log.info( error_message.format(type(ex).__name__, order_number, course_enrollment, course_enrollment.user) ) diff --git a/openedx/features/enterprise_support/tests/test_api.py b/openedx/features/enterprise_support/tests/test_api.py index 5ff059fcd1..08e108a67e 100644 --- a/openedx/features/enterprise_support/tests/test_api.py +++ b/openedx/features/enterprise_support/tests/test_api.py @@ -15,8 +15,8 @@ from django.test.utils import override_settings from django.urls import reverse from edx_django_utils.cache import get_cache_key from enterprise.models import EnterpriseCustomerUser # lint-amnesty, pylint: disable=wrong-import-order +from requests.exceptions import HTTPError from six.moves.urllib.parse import parse_qs -from slumber.exceptions import HttpClientError from common.djangoapps.student.tests.factories import UserFactory from openedx.core.djangoapps.site_configuration.tests.factories import SiteFactory @@ -48,13 +48,13 @@ from openedx.features.enterprise_support.api import ( get_enterprise_learner_data_from_db, get_enterprise_learner_portal_enabled_message, insert_enterprise_pipeline_elements, - unlink_enterprise_user_from_idp + unlink_enterprise_user_from_idp, ) from openedx.features.enterprise_support.tests import FEATURES_WITH_ENTERPRISE_ENABLED from openedx.features.enterprise_support.tests.factories import ( EnterpriseCourseEnrollmentFactory, EnterpriseCustomerIdentityProviderFactory, - EnterpriseCustomerUserFactory + EnterpriseCustomerUserFactory, ) from openedx.features.enterprise_support.tests.mixins.enterprise import EnterpriseServiceMockMixin from openedx.features.enterprise_support.utils import clear_data_consent_share_cache @@ -98,7 +98,7 @@ class TestEnterpriseApi(EnterpriseServiceMockMixin, CacheIsolationTestCase): mocked_jwt_builder.assert_called_once_with(enterprise_service_user) # pylint: disable=protected-access - assert enterprise_api_service_client.client._store['session'].auth.token == 'test-token' + assert enterprise_api_service_client.client.auth.token == 'test-token' def _assert_api_client_with_user(self, api_client, mocked_jwt_builder): """ @@ -115,7 +115,7 @@ class TestEnterpriseApi(EnterpriseServiceMockMixin, CacheIsolationTestCase): mocked_jwt_builder.assert_called_once_with(dummy_enterprise_user) # pylint: disable=protected-access - assert enterprise_api_service_client.client._store['session'].auth.token == 'test-token' + assert enterprise_api_service_client.client.auth.token == 'test-token' return enterprise_api_service_client def _assert_get_enterprise_customer(self, api_client, enterprise_api_data_for_mock): @@ -181,10 +181,10 @@ class TestEnterpriseApi(EnterpriseServiceMockMixin, CacheIsolationTestCase): authenticate and access enterprise API. """ api_client = self._assert_api_client_with_user(EnterpriseApiClient, mock_jwt_builder) - setattr(api_client.client, 'enterprise-course-enrollment', mock.Mock()) - mock_endpoint = getattr(api_client.client, 'enterprise-course-enrollment') + mock_client = mock.Mock() + api_client.client = mock_client if should_raise_http_error: - mock_endpoint.post.side_effect = HttpClientError + mock_client.post.side_effect = HTTPError username = 'spongebob' course_id = 'burger-flipping-101' @@ -195,10 +195,13 @@ class TestEnterpriseApi(EnterpriseServiceMockMixin, CacheIsolationTestCase): else: api_client.post_enterprise_course_enrollment(username, course_id) - mock_endpoint.post.assert_called_once_with(data={ - 'username': username, - 'course_id': course_id, - }) + mock_client.post.assert_called_once_with( + f"{api_client.base_api_url}enterprise-course-enrollment/", + data={ + 'username': username, + 'course_id': course_id, + } + ) @mock.patch('openedx.features.enterprise_support.api.enterprise_customer_uuid_for_request') @mock.patch('openedx.features.enterprise_support.api.EnterpriseApiClient') @@ -227,7 +230,8 @@ class TestEnterpriseApi(EnterpriseServiceMockMixin, CacheIsolationTestCase): user to authenticate and access enterprise API. """ consent_client = self._assert_api_client_with_user(ConsentApiClient, mock_jwt_builder) - consent_client.consent_endpoint = mock.Mock() + mock_client = mock.Mock() + consent_client.client = mock_client kwargs = { 'foo': 'a', @@ -236,8 +240,8 @@ class TestEnterpriseApi(EnterpriseServiceMockMixin, CacheIsolationTestCase): consent_client.provide_consent(**kwargs) consent_client.revoke_consent(**kwargs) - consent_client.consent_endpoint.post.assert_called_once_with(kwargs) - consent_client.consent_endpoint.delete.assert_called_once_with(**kwargs) + mock_client.post.assert_called_once_with(consent_client.consent_endpoint, json=kwargs) + mock_client.delete.assert_called_once_with(consent_client.consent_endpoint, json=kwargs) @httpretty.activate @mock.patch('openedx.features.enterprise_support.api.get_enterprise_learner_data_from_db') @@ -343,23 +347,33 @@ class TestEnterpriseApi(EnterpriseServiceMockMixin, CacheIsolationTestCase): @mock.patch('openedx.features.enterprise_support.api.create_jwt_for_user') def test_fetch_enterprise_learner_data(self, mock_jwt_builder): + """ + Test EnterpriseApiClient's fetch_enterprise_learner_data method. + """ api_client = self._assert_api_client_with_user(EnterpriseApiClient, mock_jwt_builder) - setattr(api_client.client, 'enterprise-learner', mock.Mock()) - mock_endpoint = getattr(api_client.client, 'enterprise-learner') + mock_client = mock.Mock() + api_client.client = mock_client user = mock.Mock(is_authenticated=True, username='spongebob') response = api_client.fetch_enterprise_learner_data(user) - assert mock_endpoint.return_value.get.return_value == response - mock_endpoint.return_value.get.assert_called_once_with(username=user.username) + assert mock_client.get.return_value.json.return_value == response + mock_client.get.assert_called_once_with( + f"{api_client.base_api_url}enterprise-learner/", + params={'username': user.username}, + ) @mock.patch('openedx.features.enterprise_support.api.get_current_request') @mock.patch('openedx.features.enterprise_support.api.create_jwt_for_user') def test_fetch_enterprise_learner_data_http_error(self, mock_jwt_builder, mock_get_current_request): + """ + Test error handling for the EnterpriseApiClient's fetch_enterprise_learner_data method. + """ api_client = self._assert_api_client_with_user(EnterpriseApiClient, mock_jwt_builder) - setattr(api_client.client, 'enterprise-learner', mock.Mock()) - mock_endpoint = getattr(api_client.client, 'enterprise-learner') - mock_endpoint.return_value.get.side_effect = HttpClientError + + mock_client = mock.Mock() + mock_client.get.side_effect = HTTPError + api_client.client = mock_client mock_get_current_request.return_value.META = { 'PATH_INFO': 'whatever', } @@ -367,8 +381,8 @@ class TestEnterpriseApi(EnterpriseServiceMockMixin, CacheIsolationTestCase): user = mock.Mock(is_authenticated=True, username='spongebob') assert api_client.fetch_enterprise_learner_data(user) is None - - mock_endpoint.return_value.get.assert_called_once_with(username=user.username) + url = f"{api_client.base_api_url}enterprise-learner/" + mock_client.get.assert_called_once_with(url, params={'username': user.username}) @mock.patch('openedx.features.enterprise_support.api.EnterpriseApiClient') def test_get_enterprise_learner_data_from_api(self, mock_api_client_class): diff --git a/openedx/features/enterprise_support/tests/test_signals.py b/openedx/features/enterprise_support/tests/test_signals.py index 35818ef767..5ebb1123e1 100644 --- a/openedx/features/enterprise_support/tests/test_signals.py +++ b/openedx/features/enterprise_support/tests/test_signals.py @@ -10,7 +10,8 @@ from django.test.utils import override_settings from django.utils.timezone import now from edx_django_utils.cache import TieredCache from opaque_keys.edx.keys import CourseKey -from slumber.exceptions import HttpClientError, HttpServerError +# from slumber.exceptions import HttpClientError, HttpServerError +from requests.exceptions import HTTPError from testfixtures import LogCapture from common.djangoapps.course_modes.tests.factories import CourseModeFactory @@ -152,27 +153,30 @@ class EnterpriseSupportSignals(SharedModuleStoreTestCase): api_called, mock_is_order_voucher_refundable ): - """Test refund_order_voucher signal""" + """ + Test refund_order_voucher signal + """ mock_is_order_voucher_refundable.return_value = order_voucher_refundable enrollment = self._create_enrollment_to_refund(no_of_days_placed, enterprise_enrollment_exists) - with patch('openedx.features.enterprise_support.signals.ecommerce_api_client') as mock_ecommerce_api_client: + with patch('openedx.features.enterprise_support.signals.get_ecommerce_api_client') as mock_ecommerce_api_client: enrollment.update_enrollment(is_active=False, skip_refund=skip_refund) assert mock_ecommerce_api_client.called == api_called @patch('common.djangoapps.student.models.CourseEnrollment.is_order_voucher_refundable') @ddt.data( - (HttpClientError, 'INFO'), - (HttpServerError, 'ERROR'), + (HTTPError, 'INFO'), (Exception, 'ERROR'), ) @ddt.unpack def test_refund_order_voucher_with_client_errors(self, mock_error, log_level, mock_is_order_voucher_refundable): - """Test refund_order_voucher signal client_error""" + """ + Test refund_order_voucher signal client_error. + """ mock_is_order_voucher_refundable.return_value = True enrollment = self._create_enrollment_to_refund() - with patch('openedx.features.enterprise_support.signals.ecommerce_api_client') as mock_ecommerce_api_client: + with patch('openedx.features.enterprise_support.signals.get_ecommerce_api_client') as mock_ecommerce_api_client: client_instance = mock_ecommerce_api_client.return_value - client_instance.enterprise.coupons.create_refunded_voucher.post.side_effect = mock_error() + client_instance.post.side_effect = mock_error() with LogCapture(LOGGER_NAME) as logger: enrollment.update_enrollment(is_active=False) assert mock_ecommerce_api_client.called is True