Skip to content

Commit

Permalink
feat: list_price is always populated in can_redeem
Browse files Browse the repository at this point in the history
ENT-9660
  • Loading branch information
pwnage101 committed Feb 26, 2025
1 parent 4836aef commit 5dc6183
Show file tree
Hide file tree
Showing 6 changed files with 189 additions and 51 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1784,6 +1784,13 @@ def setup_mocks(self):
lms_client_instance = lms_client.return_value
lms_client_instance.get_enterprise_user.return_value = TEST_USER_RECORD

catalog_get_and_cache_content_metadata_patcher = mock.patch(
'enterprise_access.apps.api.v1.views.subsidy_access_policy.get_and_cache_content_metadata',
)
self.mock_catalog_get_and_cache_content_metadata = catalog_get_and_cache_content_metadata_patcher.start()
self.mock_catalog_get_and_cache_content_metadata.return_value = {}

self.addCleanup(catalog_get_and_cache_content_metadata_patcher.stop)
self.addCleanup(lms_client_patcher.stop)
self.addCleanup(subsidy_client_patcher.stop)
self.addCleanup(contains_key_patcher.stop)
Expand Down Expand Up @@ -1956,6 +1963,12 @@ def test_can_redeem_policy_none_redeemable(
'unit': 'usd_cents',
'all_transactions': [],
}
self.mock_catalog_get_and_cache_content_metadata.return_value = {
'normalized_metadata': {
'content_price': 5000,
},
'normalized_metadata_by_run': {},
}
test_content_key_1 = "course-v1:edX+Privacy101+3T2020"
test_content_key_2 = "course-v1:edX+Privacy101+3T2020_2"
test_content_key_1_metadata_price = 29900
Expand Down Expand Up @@ -1996,9 +2009,9 @@ def mock_get_subsidy_content_data(*args, **kwargs):

# Check the response for the first content_key given.
assert response_list[0]["content_key"] == test_content_key_1
# We should not assume that a list price is fetchable if the
# content cant' be redeemed - the content may not be in any catalog for any policy.
assert response_list[0]["list_price"] is None
# When there's no redeemable policy, the price returned is the current
# fixed price fetched directly from catalog.
assert response_list[0]["list_price"] == {'usd': 50.0, 'usd_cents': 5000}
assert len(response_list[0]["redemptions"]) == 0
assert response_list[0]["has_successful_redemption"] is False
assert response_list[0]["redeemable_subsidy_access_policy"] is None
Expand Down Expand Up @@ -2031,7 +2044,7 @@ def mock_get_subsidy_content_data(*args, **kwargs):

# Check the response for the second content_key given.
assert response_list[1]["content_key"] == test_content_key_2
assert response_list[1]["list_price"] is None
assert response_list[1]["list_price"] == {'usd': 50.0, 'usd_cents': 5000}

assert len(response_list[1]["redemptions"]) == 0
assert response_list[1]["has_successful_redemption"] is False
Expand Down Expand Up @@ -2263,6 +2276,12 @@ def test_can_redeem_policy_beyond_enrollment_deadline(self, mock_lms_client, moc
"content_price": 1234,
"enroll_by_date": '2001-01-01T00:00:00Z',
}
self.mock_catalog_get_and_cache_content_metadata.return_value = {
'normalized_metadata': {
'content_price': 1234,
},
'normalized_metadata_by_run': {},
}

with mock.patch(
'enterprise_access.apps.subsidy_access_policy.content_metadata_api.get_and_cache_content_metadata',
Expand Down Expand Up @@ -2368,6 +2387,13 @@ def test_can_redeem_lms_user_id_override_for_staff(
'content_price': 19900,
}
self.mock_get_content_metadata.return_value = mock_subsidy_content_data
self.mock_catalog_get_and_cache_content_metadata.return_value = {
'normalized_metadata': {
'content_price': 19900,
},
'normalized_metadata_by_run': {},
}

query_params = {'content_key': test_content_key}
if lms_user_id_override:
query_params['lms_user_id'] = lms_user_id_override
Expand Down
50 changes: 29 additions & 21 deletions enterprise_access/apps/api/v1/views/subsidy_access_policy.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
from enterprise_access.apps.api.mixins import UserDetailsFromJwtMixin
from enterprise_access.apps.api_client.lms_client import LmsApiClient
from enterprise_access.apps.content_assignments.api import AllocationException
from enterprise_access.apps.content_metadata.api import get_and_cache_content_metadata
from enterprise_access.apps.core.constants import (
SUBSIDY_ACCESS_POLICY_ALLOCATION_PERMISSION,
SUBSIDY_ACCESS_POLICY_READ_PERMISSION,
Expand All @@ -53,6 +54,7 @@
MissingSubsidyAccessReasonUserMessages,
TransactionStateChoices
)
from enterprise_access.apps.subsidy_access_policy.content_metadata_api import list_price_dict_from_usd_cents
from enterprise_access.apps.subsidy_access_policy.exceptions import (
ContentPriceNullException,
MissingAssignment,
Expand Down Expand Up @@ -746,11 +748,16 @@ def can_redeem(self, request, enterprise_customer_uuid):
resolved_policy = None
list_price = None

redemptions_by_policy_uuid = redemptions_by_content_and_policy[content_key]
redemptions_by_policy = redemptions_by_content_and_policy[content_key]
policy_by_redemption_uuid = {
redemption['uuid']: policy
for policy, redemptions in redemptions_by_policy.items()
for redemption in redemptions
}
# Flatten dict of lists because the response doesn't need to be bucketed by policy_uuid.
redemptions = [
redemption
for redemptions in redemptions_by_policy_uuid.values()
for redemptions in redemptions_by_policy.values()
for redemption in redemptions
]

Expand Down Expand Up @@ -785,25 +792,26 @@ def can_redeem(self, request, enterprise_customer_uuid):
if redeemable_policies:
resolved_policy = SubsidyAccessPolicy.resolve_policy(redeemable_policies)

try:
if resolved_policy:
list_price = resolved_policy.get_list_price(lms_user_id, content_key)
elif successful_redemptions:
# Get the policy record used at time of successful redemption.
# [2023-12-05] TODO: consider cleaning this up.
# This is kind of silly, b/c we only need this policy to compute the
# list price, and it's really only *necessary* to fetch that price
# from within the context of a *policy record* for cases where that successful
# policy was assignment-based (because the list price for assignments might
# slightly different from the current list price in the canonical content metadata).
successfully_redeemed_policy = self.get_queryset().filter(
uuid=successful_redemptions[0]['subsidy_access_policy_uuid'],
).first()
list_price = successfully_redeemed_policy.get_list_price(lms_user_id, content_key)
except ContentPriceNullException as exc:
raise RedemptionRequestException(
detail=f'Could not determine list price for content_key: {content_key}',
) from exc
if resolved_policy:
list_price = resolved_policy.get_list_price(lms_user_id, content_key)
elif successful_redemptions:
# Get the policy used for redemption and use that to compute the price. If the redemption was the result
# of assignment, the historical assignment price might differ from the canonical price. We prefer to
# display the redeemed price to avoid confusion.
successfully_redeemed_policy = policy_by_redemption_uuid[successful_redemptions[0]['uuid']]
list_price = successfully_redeemed_policy.get_list_price(lms_user_id, content_key)
else:
# In the case where the learner cannot redeem and has never redeemed this content, bypass the subsidy
# metadata endpoint and go straight to the source (enterprise-catalog) to find price information. This
# is rare, but can happen in at least these cases:
# * Courses are searchable because enterprise has an assignment-based policy, but nothing was assigned.
# * Enterprise customers that leverage the API directly always expect a non-null price.
course_metadata = get_and_cache_content_metadata(content_key, coerce_to_parent_course=True)
usd_cents = (
course_metadata['normalized_metadata_by_run'].get(content_key, {}).get('content_price') or
course_metadata['normalized_metadata'].get('content_price')
)
list_price = list_price_dict_from_usd_cents(usd_cents)

element_response = {
"content_key": content_key,
Expand Down
48 changes: 45 additions & 3 deletions enterprise_access/apps/api_client/enterprise_catalog_client.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
"""
API client for enterprise-catalog service.
"""
from urllib.parse import urljoin

import backoff
from django.conf import settings

Expand All @@ -10,10 +12,14 @@

class EnterpriseCatalogApiClient(BaseOAuthClient):
"""
API client for calls to the enterprise catalog service.
V2 API client for calls to the enterprise catalog service.
"""
api_base_url = settings.ENTERPRISE_CATALOG_URL + '/api/v2/'
enterprise_catalog_endpoint = api_base_url + 'enterprise-catalogs/'
api_version = 'v2'

def __init__(self):
self.api_base_url = urljoin(settings.ENTERPRISE_CATALOG_URL, f'api/{self.api_version}/')
self.enterprise_catalog_endpoint = urljoin(self.api_base_url, 'enterprise-catalogs/')
super().__init__()

@backoff.on_exception(wait_gen=backoff.expo, exception=autoretry_for_exceptions)
def contains_content_items(self, catalog_uuid, content_ids):
Expand Down Expand Up @@ -84,3 +90,39 @@ def get_content_metadata_count(self, catalog_uuid):
response = self.client.get(endpoint)
response.raise_for_status()
return response.json()['count']

def content_metadata(self, content_id):
raise NotImplementedError('There is currently no v2 API implementation for this endpoint.')


class EnterpriseCatalogApiV1Client(EnterpriseCatalogApiClient):
"""
V1 API client for calls to the enterprise catalog service.
"""
api_version = 'v1'

def __init__(self):
super().__init__()
self.content_metadata_endpoint = urljoin(self.api_base_url, 'content-metadata/')

@backoff.on_exception(wait_gen=backoff.expo, exception=autoretry_for_exceptions)
def content_metadata(self, content_id, coerce_to_parent_course=False):
"""
Fetch catalog-/customer-agnostic content metadata.
Arguments:
content_id (str): ID of content to fetch.
Returns:
dict: serialized content metadata, or None if not found.
"""
query_params = {'content_identifiers': content_id}
if coerce_to_parent_course:
query_params |= {'coerce_to_parent_course': True}
endpoint = self.content_metadata_endpoint
response = self.client.get(endpoint, params=query_params)
response.raise_for_status()
response_json = response.json()
if results := response_json.get('results'):
return results[0]
return None
89 changes: 72 additions & 17 deletions enterprise_access/apps/content_metadata/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@

from django.conf import settings
from django.core.cache import cache
from requests.exceptions import HTTPError
from edx_django_utils.cache import TieredCache

from enterprise_access.cache_utils import versioned_cache_key

from ..api_client.enterprise_catalog_client import EnterpriseCatalogApiClient
from ..api_client.enterprise_catalog_client import EnterpriseCatalogApiClient, EnterpriseCatalogApiV1Client

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -64,7 +64,7 @@ def get_and_cache_catalog_content_metadata(
# Here's the list of results fetched from the catalog service
fetched_metadata = []
if keys_to_fetch:
fetched_metadata = _fetch_metadata_with_client(enterprise_catalog_uuid, keys_to_fetch)
fetched_metadata = _fetch_catalog_content_metadata_with_client(enterprise_catalog_uuid, keys_to_fetch)

# Do a bulk set into the cache of everything we just had to fetch from the catalog service
content_metadata_to_cache = {}
Expand All @@ -91,22 +91,77 @@ def get_and_cache_catalog_content_metadata(
return metadata_results_list


def _fetch_metadata_with_client(enterprise_catalog_uuid, content_keys):
def _fetch_catalog_content_metadata_with_client(enterprise_catalog_uuid, content_keys):
"""
Helper to isolate the task of fetching content metadata via our client.
"""
client = EnterpriseCatalogApiClient()
try:
response_payload = client.catalog_content_metadata(
enterprise_catalog_uuid,
list(content_keys),
)
results = response_payload['results']
logger.info(
'Fetched content metadata in catalog %s for the following content keys: %s',
enterprise_catalog_uuid,
[record.get('key') for record in results],
response_payload = client.catalog_content_metadata(
enterprise_catalog_uuid,
list(content_keys),
)
results = response_payload['results']
logger.info(
'Fetched content metadata in catalog %s for the following content keys: %s',
enterprise_catalog_uuid,
[record.get('key') for record in results],
)
return results


def get_and_cache_content_metadata(
content_identifier,
coerce_to_parent_course=False,
timeout=settings.CONTENT_METADATA_CACHE_TIMEOUT,
):
"""
Returns the metadata corresponding to the requested
``content_keys`` within the provided ``enterprise_catalog_uuid``,
as told by the enterprise-access service. Utilizes a cache per-content-record,
that is, each combination of (enterprise_catalog_uuid, key) for key in content_keys
is cached independently.
Returns: A list of dictionaries containing content metadata for the given keys.
Raises: An HTTPError if there's a problem getting the content metadata
via the enterprise-catalog service.
"""
cache_key = versioned_cache_key(
'get_and_cache_content_metadata',
content_identifier,
f'coerce_to_parent_course={coerce_to_parent_course}',
)
cached_response = TieredCache.get_cached_response(cache_key)
if cached_response.is_found:
return cached_response.value

content_metadata = EnterpriseCatalogApiV1Client().content_metadata(
content_identifier,
coerce_to_parent_course=coerce_to_parent_course,
)
if content_metadata:
TieredCache.set_all_tiers(
cache_key,
content_metadata,
django_cache_timeout=timeout,
)
return results
except HTTPError as exc:
raise exc
else:
logger.warning('Could not fetch metadata for content %s', content_identifier)
return content_metadata


def _fetch_content_metadata_with_client(enterprise_catalog_uuid, content_keys):
"""
Helper to isolate the task of fetching content metadata via our client.
"""
client = EnterpriseCatalogApiClient()
response_payload = client.catalog_content_metadata(
enterprise_catalog_uuid,
list(content_keys),
)
results = response_payload['results']
logger.info(
'Fetched content metadata in catalog %s for the following content keys: %s',
enterprise_catalog_uuid,
[record.get('key') for record in results],
)
return results
13 changes: 10 additions & 3 deletions enterprise_access/apps/subsidy_access_policy/subsidy_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ def get_redemptions_by_content_and_policy_for_learner(policies, lms_user_id):
"""
policies_by_subsidy_uuid = defaultdict(set)
for policy in policies:
policies_by_subsidy_uuid[policy.subsidy_uuid].add(str(policy.uuid))
policies_by_subsidy_uuid[policy.subsidy_uuid].add(policy)

result = defaultdict(lambda: defaultdict(list))

Expand All @@ -146,8 +146,15 @@ def get_redemptions_by_content_and_policy_for_learner(policies, lms_user_id):
content_key = redemption['content_key']
subsidy_access_policy_uuid = redemption['subsidy_access_policy_uuid']

if subsidy_access_policy_uuid in policies_with_subsidy:
result[content_key][subsidy_access_policy_uuid].append(redemption)
matching_policies = [
policy for policy in policies_with_subsidy
if str(policy.uuid) == subsidy_access_policy_uuid
]
# We can assume there's at most one matching policy because the ``policies`` arg passed into this function
# should not contain duplicates.
matching_policy = matching_policies[0] if matching_policies else None
if matching_policy:
result[content_key][matching_policy].append(redemption)
else:
logger.warning(
f"Transaction {transaction_uuid} has unmatched policy uuid for subsidy {subsidy_uuid}: "
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -145,9 +145,9 @@ def test_redemptions_by_content_and_policy(self, mock_transaction_cache):

self.assertEqual(
{
'content-1': {str(cherry_policy.uuid): [mock_pie_transactions[0]]},
'content-2': {str(apple_policy.uuid): [mock_pie_transactions[1]]},
'content-3': {str(german_chocolate_policy.uuid): [mock_cake_transactions[0]]},
'content-1': {cherry_policy: [mock_pie_transactions[0]]},
'content-2': {apple_policy: [mock_pie_transactions[1]]},
'content-3': {german_chocolate_policy: [mock_cake_transactions[0]]},
},
result,
)

0 comments on commit 5dc6183

Please sign in to comment.