Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: enrollment intention saves should not be blocked on catalog inclusion #2314

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/decisions/0015-default-enrollments.rst
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ will always primarily be associated with the course run associated with the ``De
* If the content_key is a specific course run, we'll always try to enroll in the explicitly
configured course run key (not the advertised course run).

This content_key will be passed to the ``EnterpriseCatalogApiClient().get_content_metadata_content_identifier``
This content_key will be passed to the ``EnterpriseCatalogApiClient().get_customer_content_metadata_content_identifier``
method. When passing a course run key to this endpoint, it'll actually return the top-level *course* metadata
associated with the course run, not just the specified course run's metadata
(this is primarily due to catalogs containing courses, not course runs, and we generally say that
Expand Down
2 changes: 1 addition & 1 deletion enterprise/api_client/discovery.py
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ def get_course_run(self, course_run_id):
course_run_id(string): Course run ID (aka Course Key) in string format.
Returns:
dict: Course run data provided by Course Catalog API.
dict: Course run data provided by Course Catalog API, or empty dict if not found.
"""
return self._load_data(
Expand Down
50 changes: 44 additions & 6 deletions enterprise/api_client/enterprise_catalog.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,12 @@
from logging import getLogger
from urllib.parse import urljoin

from requests.exceptions import ConnectionError, RequestException, Timeout # pylint: disable=redefined-builtin
from rest_framework.exceptions import NotFound
from requests.exceptions import ( # pylint: disable=redefined-builtin
ConnectionError,
HTTPError,
RequestException,
Timeout,
)

from django.conf import settings

Expand All @@ -29,8 +33,9 @@ class EnterpriseCatalogApiClient(UserAPIClient):
REFRESH_CATALOG_ENDPOINT = ENTERPRISE_CATALOG_ENDPOINT + '/{}/refresh_metadata'
CATALOG_DIFF_ENDPOINT = ENTERPRISE_CATALOG_ENDPOINT + '/{}/generate_diff'
ENTERPRISE_CUSTOMER_ENDPOINT = 'enterprise-customer'
CONTENT_METADATA_IDENTIFIER_ENDPOINT = ENTERPRISE_CUSTOMER_ENDPOINT + \
CUSTOMER_CONTENT_METADATA_IDENTIFIER_ENDPOINT = ENTERPRISE_CUSTOMER_ENDPOINT + \
"/{}/content-metadata/" + "{}"
CONTENT_METADATA_IDENTIFIER_ENDPOINT = "content-metadata/"
CATALOG_QUERIES_ENDPOINT = 'catalog-queries'
GET_CONTENT_FILTER_HASH_ENDPOINT = CATALOG_QUERIES_ENDPOINT + '/get_content_filter_hash'
GET_QUERY_BY_HASH_ENDPOINT = CATALOG_QUERIES_ENDPOINT + '/get_query_by_hash?hash={}'
Expand Down Expand Up @@ -386,26 +391,59 @@ def enterprise_contains_content_items(self, enterprise_uuid, content_ids):
return response.json()

@UserAPIClient.refresh_token
def get_content_metadata_content_identifier(self, enterprise_uuid, content_id):
def get_customer_content_metadata_content_identifier(self, enterprise_uuid, content_id):
"""
Return all content metadata contained in the catalogs associated with the
given EnterpriseCustomer and content_id.
"""
try:
api_url = self.get_api_url(
f"{self.CONTENT_METADATA_IDENTIFIER_ENDPOINT.format(enterprise_uuid, content_id)}"
f"{self.CUSTOMER_CONTENT_METADATA_IDENTIFIER_ENDPOINT.format(enterprise_uuid, content_id)}"
)
response = self.client.get(api_url)
response.raise_for_status()
return response.json()
except NotFound as exc:
except HTTPError as exc:
LOGGER.exception(
"No matching content found in catalog for customer: [%s] or content_id: [%s], Error: %s",
enterprise_uuid,
content_id,
str(exc),
)
return {}
except (RequestException, ConnectionError, Timeout) as exc:
LOGGER.exception(
(
"Exception raised in "
"EnterpriseCatalogApiClient::get_customer_content_metadata_content_identifier: [%s]"
),
str(exc),
)
return {}

@UserAPIClient.refresh_token
def get_content_metadata_content_identifier(self, content_id, coerce_to_parent_course=False):
"""
Return the content metadata for the content_id.
"""
try:
api_url = self.get_api_url(self.CONTENT_METADATA_IDENTIFIER_ENDPOINT)
query_params = {}
if coerce_to_parent_course:
query_params["coerce_to_parent_course"] = True
response = self.client.get(
api_url + content_id,
params=query_params,
)
response.raise_for_status()
return response.json()
except HTTPError as exc:
LOGGER.exception(
"No matching content found for content_id: [%s], Error: %s",
content_id,
str(exc),
)
return {}
except (RequestException, ConnectionError, Timeout) as exc:
LOGGER.exception(
"Exception raised in EnterpriseCatalogApiClient::get_content_metadata_content_identifier: [%s]",
Expand Down
48 changes: 46 additions & 2 deletions enterprise/content_metadata/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,46 @@
DEFAULT_CACHE_TIMEOUT = getattr(settings, 'CONTENT_METADATA_CACHE_TIMEOUT', 60 * 5)


def get_and_cache_content_metadata(content_key, timeout=None, coerce_to_parent_course=False):
"""
Returns the metadata corresponding to the requested ``content_key``
REGARDLESS of catalog/customer associations.

The response is cached in a ``TieredCache`` (meaning in both the RequestCache,
_and_ the django cache for the configured expiration period).

Returns: A dict with content metadata for the given key.
Raises: An HTTPError if there's a problem getting the content metadata
via the enterprise-catalog service.
"""
cache_key = versioned_cache_key('get_content_metadata_content_identifier', content_key, coerce_to_parent_course)
cached_response = TieredCache.get_cached_response(cache_key)
if cached_response.is_found:
logger.info(f'cache hit for content_key {content_key} and coerce_to_parent_course={coerce_to_parent_course}')
return cached_response.value

try:
result = EnterpriseCatalogApiClient().get_content_metadata_content_identifier(
content_id=content_key,
coerce_to_parent_course=coerce_to_parent_course,
)
except HTTPError as exc:
raise exc

Check warning on line 43 in enterprise/content_metadata/api.py

View check run for this annotation

Codecov / codecov/patch

enterprise/content_metadata/api.py#L42-L43

Added lines #L42 - L43 were not covered by tests

if not result:
logger.warning('No content found for content_key %s', content_key)
return {}

Check warning on line 47 in enterprise/content_metadata/api.py

View check run for this annotation

Codecov / codecov/patch

enterprise/content_metadata/api.py#L46-L47

Added lines #L46 - L47 were not covered by tests

logger.info(
'Fetched catalog for content_key %s and coerce_to_parent_course=%s. Result = %s',
content_key,
coerce_to_parent_course,
result,
)
TieredCache.set_all_tiers(cache_key, result, timeout or DEFAULT_CACHE_TIMEOUT)
return result


def get_and_cache_customer_content_metadata(enterprise_customer_uuid, content_key, timeout=None):
"""
Returns the metadata corresponding to the requested
Expand All @@ -28,14 +68,18 @@
Raises: An HTTPError if there's a problem getting the content metadata
via the enterprise-catalog service.
"""
cache_key = versioned_cache_key('get_content_metadata_content_identifier', enterprise_customer_uuid, content_key)
cache_key = versioned_cache_key(

Check warning on line 71 in enterprise/content_metadata/api.py

View check run for this annotation

Codecov / codecov/patch

enterprise/content_metadata/api.py#L71

Added line #L71 was not covered by tests
'get_customer_content_metadata_content_identifier',
enterprise_customer_uuid,
content_key,
)
cached_response = TieredCache.get_cached_response(cache_key)
if cached_response.is_found:
logger.info(f'cache hit for enterprise customer {enterprise_customer_uuid} and content {content_key}')
return cached_response.value

try:
result = EnterpriseCatalogApiClient().get_content_metadata_content_identifier(
result = EnterpriseCatalogApiClient().get_customer_content_metadata_content_identifier(

Check warning on line 82 in enterprise/content_metadata/api.py

View check run for this annotation

Codecov / codecov/patch

enterprise/content_metadata/api.py#L82

Added line #L82 was not covered by tests
enterprise_uuid=enterprise_customer_uuid,
content_id=content_key,
)
Expand Down
21 changes: 17 additions & 4 deletions enterprise/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@
json_serialized_course_modes,
)
from enterprise.content_metadata.api import (
get_and_cache_customer_content_metadata,
get_and_cache_content_metadata,
get_and_cache_enterprise_contains_content_items,
)
from enterprise.errors import LinkUserToEnterpriseError
Expand Down Expand Up @@ -2543,12 +2543,18 @@ class Meta:
@property
def content_metadata_for_content_key(self):
"""
Retrieves the content metadata for the instance's enterprise customer and content key.
Retrieves the content metadata for the instance's content key.

NOTE (ENT-9840): Prior versions of this method used `get_and_cache_customer_content_metadata()` instead of
`get_and_cache_content_metadata()`. The goal was to ensure that model saves only succeed when the requested
content is actually contained in the customer's catalogs. However, as part of ENT-9840 we are relaxing this
requirement because delays in discovery->catalog replication can easily result in default intentions being
un-saveable when simultaneously modifying catalog definitions to include the content.
"""
try:
return get_and_cache_customer_content_metadata(
enterprise_customer_uuid=self.enterprise_customer.uuid,
return get_and_cache_content_metadata(
content_key=self.content_key,
coerce_to_parent_course=True,
)
except HTTPError as e:
LOGGER.error(
Expand Down Expand Up @@ -2640,6 +2646,8 @@ def applicable_enterprise_catalog_uuids(self):
enterprise_customer_uuid=self.enterprise_customer.uuid,
content_keys=[self.course_run_key],
)
if not contains_content_items_response.get('contains_content_items'):
return []
return contains_content_items_response.get('catalog_list', [])

def determine_content_type(self):
Expand Down Expand Up @@ -2695,6 +2703,11 @@ def clean(self):
if not self.course_run:
# NOTE: This validation check also acts as an inferred check on the derived content_type
# from the content metadata.
# NOTE 2: This check does NOT assert that the content is actually contained in the
# customer's catalogs. ADR 0015, as written, would _like_ for that check to exist in
# this clean() method, but that has proven infeasible due to the nature of how data
# replication delays from discovery->catalog cause contains_content_items calls to be
# temporarily unreliable. Instead this only checks that the content key exists at all.
raise ValidationError({
'content_key': _('The content key did not resolve to a valid course run.')
})
Expand Down
2 changes: 1 addition & 1 deletion integrated_channels/degreed2/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -349,7 +349,7 @@ def _fetch_and_assign_skills_to_course(self, external_id):
# We need to do 2 steps here:
# 1. Fetch skills from enterprise-catalog

metadata = self.enterprise_catalog_api_client.get_content_metadata_content_identifier(
metadata = self.enterprise_catalog_api_client.get_customer_content_metadata_content_identifier(
enterprise_uuid=self.enterprise_configuration.enterprise_customer.uuid,
content_id=external_id)
LOGGER.info(
Expand Down
2 changes: 0 additions & 2 deletions tests/test_enterprise/api/test_default_enrollment_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,6 @@ def test_default_enterprise_enrollment_intentions_not_in_catalog(self, mock_cata
enrollment_intention = create_mock_default_enterprise_enrollment_intention(
enterprise_customer=self.enterprise_customer,
mock_catalog_api_client=mock_catalog_api_client,
contains_content_items=False,
catalog_list=[],
)
query_params = f'enterprise_customer_uuid={str(self.enterprise_customer.uuid)}'
Expand Down Expand Up @@ -370,7 +369,6 @@ def test_default_enrollment_intentions_learner_status_content_not_in_catalog(
enrollment_intention = create_mock_default_enterprise_enrollment_intention(
enterprise_customer=self.enterprise_customer,
mock_catalog_api_client=mock_catalog_api_client,
contains_content_items=False,
catalog_list=[],
)
mock_get_best_mode_from_course_key.return_value = VERIFIED_COURSE_MODE
Expand Down
11 changes: 9 additions & 2 deletions tests/test_enterprise/api/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import ddt
import pytz
import responses
from edx_django_utils.cache import TieredCache
from edx_toggles.toggles.testutils import override_waffle_flag
from faker import Faker
from oauth2_provider.models import get_application_model
Expand Down Expand Up @@ -177,19 +178,18 @@ def create_mock_default_enterprise_enrollment_intention(
enterprise_customer,
mock_catalog_api_client,
content_metadata=None,
contains_content_items=False,
catalog_list=None,
):
"""
Create a mock default enterprise enrollment intention.
"""
mock_content_metadata = content_metadata or fake_catalog_api.FAKE_COURSE
mock_contains_content_items = contains_content_items
mock_catalog_list = (
catalog_list
if catalog_list is not None
else [fake_catalog_api.FAKE_CATALOG_RESULT.get('uuid')]
)
mock_contains_content_items = bool(mock_catalog_list)

mock_catalog_api_client.return_value = mock.Mock(
get_content_metadata_content_identifier=mock.Mock(
Expand Down Expand Up @@ -239,6 +239,13 @@ def setUp(self):
super().setUp()
self.set_jwt_cookie(ENTERPRISE_OPERATOR_ROLE, ALL_ACCESS_CONTEXT)

def tearDown(self):
"""
Clears TieredCache so that mock catalog API calls do not bleed across tests.
"""
super().tearDown()
TieredCache.dangerous_clear_all_tiers()

@staticmethod
def create_user(username=TEST_USERNAME, password=TEST_PASSWORD, **kwargs):
# AED 2024-06-12: For simplicity, I've tried to refactor the test setup in the base APITest
Expand Down
Loading
Loading