feat: retrieve list of academy info#194
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a public, read-only REST API for listing and retrieving “academy-backed” SSP products (to support frontend checkout and internal consumers), enriching the SSP product model with additional academy metadata fields and updating the enterprise-catalog client’s academies endpoint behavior.
Changes:
- Added
/api/v1/ssp-products/public endpoints (list/retrieve) with optional Stripe-backed pricing and scoped throttling. - Extended
SspProductacademy metadata properties (long name, description fallbacks, thumbnail field preference) and added/expanded unit tests. - Updated enterprise-catalog API client/tests to fetch Academies from the v1 endpoint.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| enterprise_access/settings/base.py | Adds a new DRF throttle scope/rate for the public SSP products endpoint. |
| enterprise_access/apps/customer_billing/tests/test_models.py | Expands SspProduct academy-property tests and adds fallback-logic coverage. |
| enterprise_access/apps/customer_billing/models.py | Enhances academy metadata properties (long name/description/thumbnail selection). |
| enterprise_access/apps/api/v1/views/customer_billing.py | Implements SspProductViewSet public list/retrieve with Stripe pricing lookup + thumbnail URL building. |
| enterprise_access/apps/api/v1/views/init.py | Exposes the new SspProductViewSet via the views package. |
| enterprise_access/apps/api/v1/urls.py | Registers the ssp-products router endpoint under the customer billing API flag. |
| enterprise_access/apps/api/v1/tests/test_customer_billing_ssp_products.py | Adds comprehensive endpoint tests (pricing behavior, fallbacks, anonymous access). |
| enterprise_access/apps/api/serializers/customer_billing.py | Adds response serializer(s) for SSP essentials product payloads. |
| enterprise_access/apps/api/serializers/init.py | Re-exports the new SSP serializers. |
| enterprise_access/apps/api_client/tests/test_enterprise_catalog_client.py | Updates academy fetch test to expect the v1 academies endpoint. |
| enterprise_access/apps/api_client/enterprise_catalog_client.py | Points get_academy calls to the enterprise-catalog v1 academies endpoint. |
| return { | ||
| 'unit_amount_decimal': Decimal(price.unit_amount or 0) / 100, | ||
| 'stripe_name': stripe_product_name, | ||
| 'stripe_description': stripe_product_description, |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #194 +/- ##
==========================================
+ Coverage 86.25% 86.39% +0.13%
==========================================
Files 153 153
Lines 12660 12772 +112
Branches 1211 1224 +13
==========================================
+ Hits 10920 11034 +114
+ Misses 1425 1424 -1
+ Partials 315 314 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| # Last-resort fallback: scan active prices with auto-pagination to capture keys missed by filtered lookups. | ||
| if still_missing_lookup_keys: | ||
| try: | ||
| all_active_prices = stripe.Price.list( | ||
| active=True, | ||
| expand=['data.product'], | ||
| limit=100, | ||
| ) | ||
| requested_slugs = set((slug_by_lookup_key or {}).values()) | ||
| missing_lookup_keys = set(still_missing_lookup_keys) | ||
| resolved_lookup_keys = set() | ||
| resolved_slugs = set() | ||
|
|
||
| for stripe_price in all_active_prices.auto_paging_iter(): | ||
| lookup_key = getattr(stripe_price, 'lookup_key', None) | ||
| stripe_metadata = getattr(stripe_price, 'metadata', None) or {} | ||
| stripe_product_slug = self._metadata_value(stripe_metadata, 'ssp_product_slug') | ||
| lookup_key_matches = lookup_key in missing_lookup_keys | ||
| slug_matches = stripe_product_slug in requested_slugs | ||
| if lookup_key_matches or slug_matches: | ||
| prices.append(stripe_price) | ||
| if lookup_key_matches: | ||
| resolved_lookup_keys.add(lookup_key) | ||
| if slug_matches: | ||
| resolved_slugs.add(stripe_product_slug) | ||
|
|
||
| if resolved_lookup_keys >= missing_lookup_keys and resolved_slugs >= requested_slugs: | ||
| break |
| # Last-resort fallback: scan active prices with auto-pagination to capture keys missed by filtered lookups. | ||
| if still_missing_lookup_keys: | ||
| try: | ||
| all_active_prices = stripe.Price.list( | ||
| active=True, | ||
| expand=['data.product'], | ||
| limit=100, | ||
| ) |
| detail_url = reverse('api:v1:ssp-products-detail', kwargs={'slug': 'public-mapped-slug'}) | ||
| response = self.client.get(detail_url) |
| def test_chunk_values_and_metadata_value_and_payment_helpers(): | ||
| # Test chunking | ||
| vals = list(range(7)) | ||
| chunks = list(SspProductViewSet._chunk_values(vals, 3)) | ||
| assert chunks == [vals[0:3], vals[3:6], vals[6:7]] | ||
|
|
||
| # metadata_value: non-dict object that raises KeyError/TypeError | ||
| class BadMeta: | ||
| """A metadata-like object that raises on lookup to exercise error handling.""" | ||
| def __getitem__(self, key): | ||
| raise KeyError() | ||
|
|
||
| assert SspProductViewSet._metadata_value({'a': 1}, 'a') == 1 | ||
| assert SspProductViewSet._metadata_value(BadMeta(), 'a') is None | ||
|
|
||
| # Payment method status mapping | ||
|
|
||
| assert BillingManagementViewSet._get_payment_method_status({'type': 'card'}) == 'verified' | ||
| assert BillingManagementViewSet._get_payment_method_status({ | ||
| 'type': 'us_bank_account', | ||
| 'us_bank_account': {'status_details': {'status': 'verified'}} | ||
| }) == 'verified' |
| self.api_base_url = urljoin(settings.ENTERPRISE_CATALOG_URL, f'api/{self.api_version}/') | ||
| self.academies_endpoint = urljoin(self.api_base_url, 'academies/') | ||
| # Academies are exposed on v1 of the enterprise-catalog API, not v2. | ||
| self.academies_endpoint = urljoin(settings.ENTERPRISE_CATALOG_URL, 'api/v1/academies/') |
5a6b59e to
6ac522e
Compare
|
Looks good to me. Please squash to single commit. |
| def test_normalize_invoice_status_and_yearly_amount_and_license_count(monkeypatch): | ||
|
|
||
| # normalize invoice status | ||
| assert BillingManagementViewSet._normalize_invoice_status('paid') == 'paid' | ||
| assert BillingManagementViewSet._normalize_invoice_status('open') == 'open' | ||
| assert BillingManagementViewSet._normalize_invoice_status('void') == 'void' | ||
| assert BillingManagementViewSet._normalize_invoice_status('uncollectible') == 'uncollectible' | ||
| assert BillingManagementViewSet._normalize_invoice_status('weird') == 'open' |
| cache_key = versioned_cache_key('all_stripe_prices') | ||
| cached_response = TieredCache.get_cached_response(cache_key) | ||
| if cached_response.is_found: | ||
| all_prices = get_all_stripe_prices() | ||
| used_cache = True |
| # Last-resort fallback: scan active prices with auto-pagination to capture keys missed by filtered lookups. | ||
| if still_missing_lookup_keys: | ||
| try: | ||
| all_active_prices = stripe.Price.list( | ||
| active=True, | ||
| expand=['data.product'], | ||
| limit=100, | ||
| ) |
fbf6626 to
ff40adf
Compare
| except (KeyError, TypeError): # pragma: no cover - metadata type varies by Stripe SDK object wrappers | ||
| return None | ||
|
|
||
| def _get_pricing_by_lookup_key(self, lookup_keys, slug_by_lookup_key=None): # pylint: disable=too-many-statements |
There was a problem hiding this comment.
# pylint: disable=too-many-statements we generally don't allow disabling this pylint warning. This function is indeed too long/complex. Also, this logic is very duplicative with code that already exists: https://github.com/edx/enterprise-access/blob/main/enterprise_access/apps/customer_billing/pricing_api.py#L253 Try to make use of that code, doing light refactoring as needed.
| def test_normalize_invoice_status_and_yearly_amount_and_license_count(monkeypatch): | ||
|
|
||
| # normalize invoice status | ||
| assert BillingManagementViewSet._normalize_invoice_status('paid') == 'paid' | ||
| assert BillingManagementViewSet._normalize_invoice_status('open') == 'open' |
| self.api_base_url = urljoin(settings.ENTERPRISE_CATALOG_URL, f'api/{self.api_version}/') | ||
| self.academies_endpoint = urljoin(self.api_base_url, 'academies/') | ||
| # Academies are exposed on v1 of the enterprise-catalog API, not v2. | ||
| self.academies_endpoint = urljoin(settings.ENTERPRISE_CATALOG_URL, 'api/v1/academies/') |
| long_name = self._academy_data.get('long_name') | ||
| if long_name: | ||
| return long_name | ||
|
|
||
| return self._academy_data.get('long_name') or self.academy_title |
| cond_is_str = isinstance(requested_slug, str) | ||
| cond_dash_only = ('-' in requested_slug and '_' not in requested_slug) | ||
| is_public_slug = cond_is_str and cond_dash_only # pragma: no cover - heuristic |
| # First, check whether the requested slug actually matches a | ||
| # database `stripe_price_lookup_key`. This allows direct lookup_key | ||
| # URLs (where slug==lookup_key) to resolve without calling Stripe. | ||
| db_qs = self.get_queryset().filter(stripe_price_lookup_key=requested_slug) # pragma: no cover - db | ||
| product = db_qs.first() |
| unit_amount_decimal = Decimal( | ||
| price.unit_amount or 0, | ||
| ) / 100 # pragma: no cover - stripe price payload extraction |
013403f to
f1b5d97
Compare
Two structural concerns worth addressing before mergePricing lookup: reinventing what
|
ba1bb19 to
2168b54
Compare
| except (exceptions.NotFound, Http404) as exc: | ||
| requested_slug = kwargs.get('slug') | ||
| # Preserve prior behavior for obvious public slugs (e.g., 'teams-yearly') | ||
| # which should return 404 without attempting Stripe resolution. Heuristic: | ||
| # treat values containing '-' and not '_' as public slugs and re-raise. | ||
| cond_is_str = isinstance(requested_slug, str) | ||
| cond_dash_only = ('-' in requested_slug and '_' not in requested_slug) | ||
| is_public_slug = cond_is_str and cond_dash_only | ||
| if is_public_slug: | ||
| raise Http404() from exc | ||
|
|
||
| product = None | ||
| lookup_key_candidate = None | ||
| used_cache = False | ||
|
|
||
| # First, check whether the requested slug actually matches a DB stored lookup_key | ||
| db_qs = self.get_queryset().filter(stripe_price_lookup_key=requested_slug) | ||
| product = db_qs.first() | ||
| logger.debug('DB lookup for stripe_price_lookup_key=%s returned %s', requested_slug, bool(product)) | ||
|
|
||
| # If we found a DB product by lookup_key, skip Stripe resolution. | ||
| if not product: | ||
| # Try direct lookup_key match in Stripe (active prices first). | ||
| product_from_price, lookup_key_from_price = self._stripe_single_lookup(requested_slug, active=True) | ||
| if product_from_price: | ||
| product = product_from_price | ||
| if lookup_key_from_price: | ||
| lookup_key_candidate = lookup_key_from_price | ||
|
|
||
| # If the active lookup did not return a result, try a non-active single lookup. | ||
| if not (lookup_key_candidate or product): | ||
| product_from_price, lookup_key_from_price = self._stripe_single_lookup(requested_slug, active=False) | ||
| if product_from_price: | ||
| product = product_from_price | ||
| if lookup_key_from_price: | ||
| lookup_key_candidate = lookup_key_from_price | ||
|
|
||
| # If still nothing found, consult the cached all-prices mapping | ||
| # only if it exists to avoid triggering a live full Stripe scan. | ||
| if not (lookup_key_candidate or product): | ||
| cached_lookup, cache_used = self._consult_cached_mapping(requested_slug) | ||
| if cache_used: | ||
| used_cache = True | ||
| lookup_key_candidate = cached_lookup | ||
|
|
||
| # If direct lookup_key match didn't yield a product, do a | ||
| # full active-price scan and search for Stripe metadata that | ||
| # references the requested slug (metadata.ssp_product_slug). | ||
| if not product and not lookup_key_candidate and not used_cache: | ||
| lookup_key_candidate = self._scan_active_prices_for_slug(requested_slug) | ||
|
|
||
| if not product and lookup_key_candidate: | ||
| product = self.get_queryset().filter( | ||
| stripe_price_lookup_key=lookup_key_candidate, |
There was a problem hiding this comment.
this is too much code that's far too nested just to help a caller using the wrong value. just use default DRF retrieval behavior and skip all of this.
| def _lookup_key_from_cached_prices(requested_slug, all_prices): | ||
| """Resolve lookup_key from cached Stripe price map using metadata then fuzzy match.""" | ||
| if requested_slug in all_prices: | ||
| return requested_slug | ||
|
|
||
| for lookup_key, price_data in all_prices.items(): | ||
| meta = (price_data.get('product') or {}).get('metadata') or {} | ||
| if meta.get('ssp_product_slug') == requested_slug: | ||
| return lookup_key | ||
|
|
||
| for lookup_key in all_prices.keys(): | ||
| if requested_slug in lookup_key or lookup_key in requested_slug: | ||
| return lookup_key | ||
|
|
||
| return None | ||
|
|
||
| def _stripe_single_lookup(self, requested_slug, active=True): | ||
| """Attempt a single-lookup Key query against Stripe and resolve product/lookup_key. | ||
|
|
||
| Returns a tuple `(product_or_none, lookup_key_or_none)`. | ||
| """ | ||
| try: | ||
| if active: | ||
| price_resp = stripe.Price.list( | ||
| lookup_keys=[requested_slug], | ||
| active=True, | ||
| expand=['data.product'], | ||
| limit=1, | ||
| ) | ||
| else: | ||
| price_resp = stripe.Price.list( | ||
| lookup_keys=[requested_slug], | ||
| expand=['data.product'], | ||
| limit=1, | ||
| ) | ||
| except stripe.error.StripeError: | ||
| return None, None | ||
|
|
||
| return self._resolve_product_from_price_response(price_resp) | ||
|
|
||
| def _consult_cached_mapping(self, requested_slug): | ||
| """If the cached all-prices map exists, consult it for a lookup_key. | ||
|
|
||
| Returns a tuple `(lookup_key_or_none, used_cache_bool)`. | ||
| """ | ||
| try: | ||
| cache_key = versioned_cache_key('all_stripe_prices') | ||
| cached_response = TieredCache.get_cached_response(cache_key) | ||
| if cached_response.is_found: | ||
| all_prices = get_all_stripe_prices() | ||
| lookup_key = self._lookup_key_from_cached_prices(requested_slug, all_prices) | ||
| return lookup_key, True | ||
| except StripePricingError: | ||
| return None, False | ||
| return None, False | ||
|
|
||
| def _scan_active_prices_for_slug(self, requested_slug): | ||
| """Run a full active-price scan against Stripe and return a matching lookup_key if found.""" | ||
| try: | ||
| all_active = stripe.Price.list(active=True, expand=['data.product'], limit=100) | ||
| for price in all_active.auto_paging_iter(): | ||
| meta = getattr(price, 'metadata', None) or {} | ||
| meta_slug = self._metadata_value(meta, 'ssp_product_slug') | ||
| lookup_key_val = getattr(price, 'lookup_key', None) | ||
|
|
||
| if meta_slug == requested_slug: | ||
| return lookup_key_val | ||
|
|
||
| if lookup_key_val and (requested_slug in lookup_key_val or lookup_key_val in requested_slug): | ||
| return lookup_key_val | ||
| except stripe.error.StripeError: | ||
| return None | ||
| return None |
There was a problem hiding this comment.
none of this seems necessary, the SspProduct model maps the stripe lookup key to a product slug
Dismissing review to unblock since I will be out of the office starting tomorrow.
929de55 to
4f0717a
Compare
| assert BillingManagementViewSet._get_license_count(sub) == 5 | ||
|
|
||
|
|
||
| def test_normalize_invoice_status_and_yearly_amount_and_license_count(monkeypatch): |
There was a problem hiding this comment.
I don't really understand why we need this - is it used anywhere?
| self.essentials_product = SspProduct.objects.create( | ||
| slug='ai-academy-yearly', | ||
| stripe_price_lookup_key='ai_academy_yearly_price', | ||
| academy_uuid=uuid.uuid4(), | ||
| catalog_query_uuid=uuid.uuid4(), | ||
| license_manager_product_id_trial=2, | ||
| license_manager_product_id_paid=1, | ||
| is_active=True, | ||
| ) | ||
| self.teams_product, _ = SspProduct.objects.get_or_create( | ||
| slug='teams-yearly', | ||
| defaults={ | ||
| 'stripe_price_lookup_key': 'teams_subscription_license_yearly', | ||
| 'academy_uuid': None, | ||
| 'catalog_query_uuid': uuid.uuid4(), | ||
| 'license_manager_product_id_trial': 2, | ||
| 'license_manager_product_id_paid': 1, | ||
| 'is_active': True, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
if the tests don't ever modify these objects, its more efficient to define them inside of setUpClass(). that way, they'll only be created once, instead of once per test function.
| @override_settings(SSP_ESSENTIALS_THUMBNAIL_S3_BASE_URL='https://s3.amazonaws.com/essentials-bucket') | ||
| @mock.patch('enterprise_access.apps.api.v1.views.customer_billing.get_all_stripe_prices') | ||
| @mock.patch('enterprise_access.apps.customer_billing.models.get_cached_academy_data') | ||
| def test_list_ssp_products_success(self, mock_get_cached_academy_data, mock_get_all_stripe_prices): |
There was a problem hiding this comment.
we could probably compact these tests quite a bit by using https://ddt.readthedocs.io/en/latest/
look for usage examples in this repo, e.g.
https://2u-internal.atlassian.net/browse/ENT-11468
Description:
To ensure the centralized Academy information stored in edX Enterprise is accessible to the frontend checkout flow and other internal services, we need to implement a RESTful API.