Conversation
9d05e2d to
3623aa3
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds multi-license subscription support to the Enterprise BFF, including catalog-to-license indexing in the response and Algolia key scoping across all licensed catalogs behind a feature flag.
Changes:
- Introduces
license_schema_versionandlicenses_by_catalogin BFF subscription responses, gated byenterprise_access.enable_multi_license_entitlements_bff. - Updates license selection logic to use “first activated” tie-breaking (ENT-11672) and adds multi-license course-to-license mapping.
- Adds support for scoping secured Algolia API keys by multiple catalog UUIDs, including caching and client updates, plus expanded test coverage.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
enterprise_access/toggles.py |
Defines the new Waffle flag for multi-license BFF behavior. |
enterprise_access/tests/test_toggles.py |
Adds unit tests for the toggle helper. |
enterprise_access/apps/bffs/handlers.py |
Implements multi-license processing, catalog indexing, selection rules, and Algolia scoping logic. |
enterprise_access/apps/bffs/serializers.py |
Adds licenses_by_catalog and license_schema_version to the subscriptions serializer. |
enterprise_access/apps/bffs/context.py |
Adds support for refreshing Algolia key metadata with an optional catalog scope. |
enterprise_access/apps/bffs/api.py |
Extends Algolia key caching and downstream calls to include catalog_uuids scope. |
enterprise_access/apps/api_client/enterprise_catalog_client.py |
Adds catalog_uuids query params support for the secured Algolia key endpoint. |
enterprise_access/apps/api_client/tests/test_enterprise_catalog_client.py |
Updates/extends tests for enterprise-catalog client URL construction and scoped Algolia calls. |
enterprise_access/apps/bffs/tests/test_multi_license.py |
Adds a dedicated test suite for multi-license selection, indexing, mapping, and flag behavior. |
enterprise_access/apps/bffs/tests/test_handlers.py |
Adds/updates handler unit tests for selection rules and Algolia scoping. |
enterprise_access/apps/bffs/tests/test_serializers.py |
Adds initial serializer validation tests for BFF serializers. |
enterprise_access/apps/customer_billing/tests/test_tasks.py |
Removes reinstatement-email task tests and refactors some defaults dict literals. |
enterprise_access/apps/customer_billing/tests/test_stripe_event_handlers.py |
Removes reinstatement-email behavior tests for subscription updated events. |
enterprise_access/apps/customer_billing/tests/test_migrations.py |
Refactors ddt scenario inputs to dict(...) style. |
enterprise_access/settings/base.py |
Adds rest_framework_swagger to installed apps; removes a Braze campaign setting. |
enterprise_access/settings/test.py |
Removes a Braze campaign test setting. |
enterprise_access/apps/api/tasks.py |
Adds an import (currently unused). |
Comments suppressed due to low confidence (1)
enterprise_access/apps/api/tasks.py:10
requestsis imported but not used anywhere in this module. Please remove the unused import to avoid lint/test failures.
import logging
import requests
from celery import shared_task
from django.conf import settings
from enterprise_access.apps.api.serializers import CouponCodeRequestSerializer, LicenseRequestSerializer
3623aa3 to
6255028
Compare
6255028 to
c288c50
Compare
c288c50 to
be897b5
Compare
be897b5 to
22bd8b7
Compare
081715c to
4801f8d
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #131 +/- ##
==========================================
+ Coverage 84.28% 84.60% +0.31%
==========================================
Files 144 145 +1
Lines 12215 12347 +132
Branches 1163 1188 +25
==========================================
+ Hits 10296 10446 +150
+ Misses 1598 1586 -12
+ Partials 321 315 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4801f8d to
be0105a
Compare
be0105a to
c629f52
Compare
c629f52 to
f34118c
Compare
8a91237 to
18de80f
Compare
18de80f to
61e1992
Compare
61e1992 to
37bd02b
Compare
37bd02b to
a084dc7
Compare
a084dc7 to
f4174c7
Compare
f4174c7 to
6959455
Compare
9264667 to
0960776
Compare
0960776 to
5851026
Compare
vshaikismail-sonata
left a comment
There was a problem hiding this comment.
Looks good, Overall validations and logging are well handled.
5851026 to
5328776
Compare
| else: | ||
| digest = hashlib.sha256( | ||
| ','.join(sorted(str(u) for u in catalog_uuids)).encode() | ||
| ).hexdigest()[:16] |
There was a problem hiding this comment.
secured_algolia_api_key_cache_key truncates the SHA-256 digest to 16 hex chars ([:16]), which introduces a (small but real) risk of cache-key collisions between different catalog UUID sets. Since versioned_cache_key() already hashes the final key to a fixed-length digest (enterprise_access/cache_utils.py:15-26), there’s no cache-backend key-length constraint here; consider using the full hexdigest (or avoid the extra hashing entirely if input size isn’t a concern) to eliminate collision risk while still keeping the cache key bounded.
| ).hexdigest()[:16] | |
| ).hexdigest() |
There was a problem hiding this comment.
this is a good suggestion, versioned_cache_key() just hashes the entire set of strings you throw into it.
| context, error_response, error_status = self._create_context( | ||
| request, context_class, context_kwargs=context_kwargs, | ||
| ) | ||
| if context is None: |
There was a problem hiding this comment.
After creating the context, consider short-circuiting when it already contains errors / an error status_code (e.g., invalid enterprise_customer_uuid causes HandlerContext to return early with a 400). Currently the handler will still run and may trigger unnecessary downstream calls or add secondary errors due to missing context data.
| if context is None: | |
| if context is None or error_status is not None: |
There was a problem hiding this comment.
this is also a good suggestion
5328776 to
52ae1b9
Compare
iloveagent57
left a comment
There was a problem hiding this comment.
Approach looks good in general, here's some feedback mostly around the algolia key thing.
| else: | ||
| digest = hashlib.sha256( | ||
| ','.join(sorted(str(u) for u in catalog_uuids)).encode() | ||
| ).hexdigest()[:16] |
There was a problem hiding this comment.
this is a good suggestion, versioned_cache_key() just hashes the entire set of strings you throw into it.
| context, error_response, error_status = self._create_context( | ||
| request, context_class, context_kwargs=context_kwargs, | ||
| ) | ||
| if context is None: |
There was a problem hiding this comment.
this is also a good suggestion
| 'developer_message' (str): Message of corresponding error indicating an actionable developer message | ||
| """ | ||
| response = self.get(self.secured_algolia_api_key_endpoint(enterprise_customer_uuid)) | ||
| query_params = {'catalog_uuids': catalog_uuids} if catalog_uuids is not None else None |
There was a problem hiding this comment.
ref: https://github.com/edx/enterprise-catalog/blob/3f600ae3e8879a9a30ce118eb730a5c5d433b8f6/enterprise_catalog/apps/api/v1/views/enterprise_customer.py#L248
I think we can actually get rid of this query params, because the endpoint already scopes to all available catalog queries by default (with no method for overriding).
| # Preserve main's flat list behavior and expose all licenses. | ||
| response_subscription_licenses = subscription_licenses |
There was a problem hiding this comment.
I don't understand what these two lines are meant to do.
| # Note: secured Algolia key initialization happens during context setup via | ||
| # _initialize_secured_algolia_api_keys() (unless deferred). Handlers may still call | ||
| # refresh_secured_algolia_api_keys() later, once relevant catalog UUIDs are known, | ||
| # to perform scoped/catalog-specific refreshes. Do not assume that later refresh | ||
| # has already happened here. |
There was a problem hiding this comment.
nit: this can be added to the docstring for your helper method below
| """ | ||
| self.refresh_secured_algolia_api_keys() | ||
|
|
||
| def refresh_secured_algolia_api_keys(self, catalog_uuids=None): |
There was a problem hiding this comment.
We may not need this at all if the enterprise-catalog algolia key endpoint already scopes the key to all available catalogs for the customer (and provides no means of override with query params).
| class SubscriptionLicenseProcessor: | ||
| """ | ||
| Handles subscription license data transformation. | ||
| Preserves collection semantics while maintaining backward compatibility. | ||
|
|
||
| This processor supports multi-license scenarios where a learner may have | ||
| access to multiple subscription licenses across different catalogs. | ||
| """ |
There was a problem hiding this comment.
👍 I like how you modeled this class.
feat: Multi-License Subscription Support for Enterprise BFF
Tickets
Summary
This PR adds multi-license support for learner BFF flows behind
enterprise_access.enable_multi_license_entitlements_bff, while preserving legacy behavior when the flag is OFF.Problems We Resolved (Summary)
API Contract Changes
licenses_by_catalogwhenenterprise_access.enable_multi_license_entitlements_bffis ON.subscription_licenseselection now follows first-activation deterministic behavior (ENT-11672).subscription_licensescontinues to return the flat list for backward compatibility.Example (flag ON)
{ "subscription_license": { "...": "..." }, "subscription_licenses": [{ "...": "..." }], "licenses_by_catalog": { "cat-uuid": [{ "...": "..." }] } }Example (flag OFF)
{ "subscription_license": { "...": "..." }, "subscription_licenses": [{ "...": "..." }] }Code Changes
Major files updated:
enterprise_access/apps/bffs/handlers.pyenterprise_access/apps/bffs/serializers.pyenterprise_access/apps/bffs/context.pyenterprise_access/toggles.pyTests updated/added:
enterprise_access/apps/bffs/tests/test_multi_license.pyenterprise_access/apps/bffs/tests/test_handlers.pyenterprise_access/apps/bffs/tests/test_serializers.pyenterprise_access/apps/api/v1/tests/test_bff_views.pyenterprise_access/tests/test_toggles.pyTest Coverage
Code Changes
Modified Files
enterprise_access/apps/bffs/handlers.pytransform_subscriptions_result()to buildlicenses_by_catalogand apply first-activated selection rule when flag is ON; updated_scope_secured_algolia_by_flag()to scope across all catalog UUIDsenterprise_access/apps/bffs/serializers.pylicenses_by_catalog(DictField) toLearnerDashboardBFFResponseSerializerenterprise_access/apps/bffs/context.pyenterprise_access/toggles.pyENABLE_MULTI_LICENSE_ENTITLEMENTS_BFFWaffleFlag definitionNew Test Files
enterprise_access/apps/bffs/tests/test_multi_license.pyenterprise_access/tests/test_toggles.pyChanged Test Files
enterprise_access/apps/bffs/tests/test_handlers.py_subscriptions_resulthelper; updatedTestScopeSecuredAlgoliaByFlagto cover multi-catalog scopingenterprise_access/apps/api_client/tests/test_enterprise_catalog_client.pyTest Cases Covered
test_multi_license.py—TestTransformSubscriptionsResulttest_flag_off_returns_v1_responselicenses_by_catalogtest_flag_on_single_licenselicenses_by_cataloghas exactly one entrytest_flag_on_alice_three_licensestest_flag_on_bob_four_licenseslicenses_by_catalogtest_flag_on_carol_five_licenseslicenses_by_catalogtest_flag_on_dave_activation_flowlicenses_by_catalogreflects final statetest_flag_on_eve_tiebreakertest_no_licensestest_multi_license.py—TestScopeSecuredAlgoliaByFlagtest_flag_off_uses_single_catalogtest_flag_on_scopes_all_catalogstest_no_licenses_no_algolia_scopetest_toggles.pytest_flag_is_off_by_defaultenable_multi_license_entitlements_bffis disabled by defaulttest_flag_can_be_enabledtest_handlers.py(updated)test_scope_secured_algolia_single_licensetest_scope_secured_algolia_multi_licenseTest Learner Profiles (from
test-data/)test-multi-alice@example.com)test-multi-bob@example.com)test-multi-carol@example.com)test-multi-dave@example.com)test-multi-eve@example.com)Test results:




In case of waffle flag off
Response:
In case of waffle flag on