ENT-11468: PR D - Interfaces: academy_sync#171
Open
gshivajibiradar wants to merge 17 commits into
Open
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an academy sync helper module to import academy metadata from Enterprise Catalog into the customer billing domain, along with unit tests covering normalization and sync result counting.
Changes:
- Introduce
academy_sync.pyhelpers for normalizing academy payloads and syncing to a model viaapps.get_model. - Add a sync entrypoint with dry-run and optional deactivation of missing academies.
- Add tests covering normalization, payload extraction, and sync create/update/deactivate/error counting.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| enterprise_access/apps/customer_billing/academy_sync.py | New academy normalization + sync helpers (fetch, map fields, sync loop, optional deactivation). |
| enterprise_access/apps/customer_billing/tests/test_academy_sync.py | Unit tests for normalization/extraction utilities and sync counter behavior via in-memory fakes. |
gshivajibiradar
force-pushed
the
interfaces-academy-sync
branch
from
64edd62 to
143d568
Compare
- Add test_sync_preserves_existing_catalog_query_uuid_when_payload_omits_it to cover UUID preservation during updates - Add test_sync_deactivate_missing_with_dry_run_does_not_persist to cover dry_run + deactivate_missing branch - Add test_sync_counts_errors_when_save_raises to cover exception handling during save() - Add test_first_non_empty_with_dict_rendered_empty_string and test_first_non_empty_with_multiple_non_string_values for better helper coverage - Add test_to_slug_all_fallback_paths for comprehensive slug fallback testing - Add test_extract_payload_list_unknown_dict_key and test_extract_catalog_query_uuid_bool_and_invalid_string for edge cases All 36 tests now pass. These additions improve patch coverage to 100%.
- Remove unused variable 'active_academy' in test_sync_deactivate_missing_with_dry_run_does_not_persist - Remove trailing whitespace in test_first_non_empty_with_multiple_non_string_values
Agent-Logs-Url: https://github.com/edx/enterprise-access/sessions/3f4a5d8c-2d65-4a95-b6e6-36d3f008a479 Co-authored-by: gshivajibiradar <257487719+gshivajibiradar@users.noreply.github.com>
- Remove accidental docs ERD file from PR diff - Keep pylintrc and pylintrc_tweaks aligned with main - Add edge-case academy_sync helper tests to cover remaining branches - Confirm academy_sync.py and apps.py hit 100% focused coverage
- Wrap apps.get_model in LookupError guard with clear RuntimeError message - Replace outer @transaction.atomic with per-row transaction.atomic() savepoints so a single row failure does not poison the entire sync - Add current.name to seen_names after iexact match to prevent false deactivation when DB casing differs from payload casing - Use class-level CustomerBillingConfig._signals_wired guard (not instance-level) to survive multiple AppConfig instantiations - Add tests for all new code paths (LookupError, case-mismatch deactivation, class-level signal guard)
Comment on lines
+10
to
+18
| _signals_wired = False | ||
|
|
||
| def ready(self): | ||
| # Prevent duplicate receiver registration when Django reloads app configs. | ||
| # Check and set on the class so the guard survives multiple AppConfig instantiations. | ||
| if CustomerBillingConfig._signals_wired: | ||
| return | ||
| import enterprise_access.apps.customer_billing.signals # pylint: disable=import-outside-toplevel,unused-import | ||
| CustomerBillingConfig._signals_wired = True |
Comment on lines
+233
to
+249
| normalized = _normalize_catalog_academy(item) | ||
| if not normalized: | ||
| result.skipped += 1 | ||
| continue | ||
|
|
||
| name = normalized['name'] | ||
| seen_names.add(name) | ||
|
|
||
| current = enterprise_academy_model.objects.filter(name__iexact=name).first() | ||
| if current is not None: | ||
| # Track the DB-canonical casing too; exclude(name__in=...) is case-sensitive on most DBs. | ||
| seen_names.add(current.name) | ||
| # Preserve existing UUID when payload does not include one. | ||
| if normalized.get('catalog_query_uuid') is None and current and current.catalog_query_uuid is not None: | ||
| normalized['catalog_query_uuid'] = current.catalog_query_uuid | ||
|
|
||
| try: |
pwnage101
requested changes
May 22, 2026
| 'Academy sync requires the customer_billing.EnterpriseAcademy model to be ' | ||
| 'defined and registered before sync_enterprise_academies_from_enterprise_catalog ' | ||
| 'can be called.' | ||
| ) from exc |
Member
There was a problem hiding this comment.
❌ I find it extremely doubtful this is necessary. If you plan to add this to apps.py (which this PR does NOT do), then just don't. Figure out another way.
Contributor
Author
There was a problem hiding this comment.
Great point, agreed. I removed that approach entirely and did not add anything in apps.py.
I changed this to avoid app-registration coupling:
No apps.py wiring for sync.
No custom RuntimeError guard for model registration.
The sync path now resolves the model directly at runtime and keeps this PR scoped to academy sync logic only.
Thanks for calling this out, the implementation is now aligned with that expectation.
gshivajibiradar
force-pushed
the
interfaces-academy-sync
branch
from
9e2ce3e to
3368b8e
Compare
Comment on lines
+235
to
+251
| """Test that missing get_academies method raises AttributeError.""" | ||
| mock_client_cls.return_value.get_academies = 'not-callable' | ||
|
|
||
| with self.assertRaises(AttributeError): | ||
| sync_enterprise_academies_from_enterprise_catalog() | ||
|
|
||
| @mock.patch( | ||
| 'enterprise_access.apps.customer_billing.academy_sync._get_enterprise_academy_model', | ||
| return_value=FakeEnterpriseAcademyModel, | ||
| ) | ||
| @mock.patch('enterprise_access.apps.customer_billing.academy_sync.EnterpriseCatalogApiClient') | ||
| def test_handles_non_list_payload(self, mock_client_cls, _mock_model): | ||
| """Test that non-list payloads are treated as empty list.""" | ||
| mock_client_cls.return_value.get_academies.return_value = None | ||
|
|
||
| result = sync_enterprise_academies_from_enterprise_catalog() | ||
|
|
Comment on lines
+15
to
+17
| def _get_enterprise_academy_model(): | ||
| """Resolve EnterpriseAcademy lazily so this module can import without model migrations.""" | ||
| return apps.get_model('customer_billing', 'EnterpriseAcademy') |
Comment on lines
+113
to
+126
| def sync_enterprise_academies_from_enterprise_catalog( | ||
| academy_uuid=None, | ||
| deactivate_missing: bool = False, | ||
| dry_run: bool = False, | ||
| ) -> AcademySyncResult: | ||
| """Sync Enterprise Catalog academy entries into EnterpriseAcademy rows.""" | ||
| result = AcademySyncResult() | ||
| enterprise_academy_model = _get_enterprise_academy_model() | ||
| client = EnterpriseCatalogApiClient() | ||
| get_academies = getattr(client, 'get_academies', None) | ||
| if not callable(get_academies): | ||
| raise AttributeError('EnterpriseCatalogApiClient.get_academies is required for academy sync') | ||
| payload = get_academies(academy_uuid=academy_uuid) # pylint: disable=not-callable | ||
| academy_items = payload if isinstance(payload, list) else [] |
gshivajibiradar
force-pushed
the
interfaces-academy-sync
branch
from
3368b8e to
d77db44
Compare
Comment on lines
+15
to
+18
| def _get_enterprise_academy_model(): | ||
| """Resolve EnterpriseAcademy lazily so this module can import without model migrations.""" | ||
| return apps.get_model('customer_billing', 'EnterpriseAcademy') | ||
|
|
Comment on lines
14
to
23
|
|
||
| from enterprise_access.apps.events.signals import ACCESS_POLICY_CREATED, SUBSIDY_REDEEMED | ||
| from enterprise_access.apps.events.utils import ( | ||
| ProducerFactory, | ||
| create_topics, | ||
| send_access_policy_event_to_event_bus, | ||
| send_coupon_code_request_event_to_event_bus, | ||
| send_subsidy_redemption_event_to_event_bus, | ||
| verify_event | ||
| ) |
Contributor
Author
There was a problem hiding this comment.
Good catch, I’ve now narrowed the PR to only academy sync customer_billing files and removed unrelated events/core/api_client test changes so the description and scope are aligned.
vshaikismail-sonata
approved these changes
May 25, 2026
vshaikismail-sonata
left a comment
vshaikismail-sonata
left a comment
Contributor
There was a problem hiding this comment.
LGTM....!!!
Code coverage, you will discuss that with the client
Comment on lines
+3
to
+6
| project: | ||
| default: | ||
| enabled: no | ||
| target: auto |
Comment on lines
+1
to
+6
| coverage: | ||
| status: | ||
| project: | ||
| default: | ||
| enabled: no | ||
| target: auto |
Comment on lines
+13
to
+25
| enterprise_access/apps/subsidy_access_policy/mocks.py | ||
| enterprise_access/apps/api_client/* | ||
| enterprise_access/apps/*/management/commands/* | ||
| enterprise_access/apps/content_assignments/models.py | ||
| enterprise_access/apps/content_assignments/exceptions.py | ||
| enterprise_access/apps/customer_billing/api.py | ||
| enterprise_access/apps/events/utils.py | ||
| enterprise_access/apps/enterprise_groups/tasks.py | ||
| enterprise_access/apps/content_metadata/api.py | ||
| enterprise_access/apps/subsidy_access_policy/subsidy_api.py | ||
| enterprise_access/apps/subsidy_access_policy/content_metadata_api.py | ||
| enterprise_access/apps/subsidy_request/tasks.py | ||
| enterprise_access/apps/admin_portal_learner_profile/apps.py |
Comment on lines
+103
to
+113
| class TestCustomerBillingConfig(SimpleTestCase): | ||
| """Verify app startup wiring behavior.""" | ||
|
|
||
| def test_ready_imports_signals(self): | ||
| """Calling ready should import customer_billing signal handlers.""" | ||
| app_module = importlib.import_module('enterprise_access.apps.customer_billing') | ||
| config = CustomerBillingConfig(CustomerBillingConfig.name, app_module) | ||
|
|
||
| with mock.patch('builtins.__import__', wraps=__import__) as mock_import: | ||
| config.ready() | ||
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #171 +/- ##
==========================================
+ Coverage 86.08% 86.20% +0.11%
==========================================
Files 149 150 +1
Lines 12500 12607 +107
Branches 1194 1211 +17
==========================================
+ Hits 10761 10868 +107
Misses 1424 1424
Partials 315 315 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
gshivajibiradar
force-pushed
the
interfaces-academy-sync
branch
from
c5f35f6 to
cdcce7a
Compare
| stripe_product_id = item.get('stripe_product_id') | ||
|
|
||
| display_order = item.get('display_order', 0) | ||
| display_order = int(display_order or 0) |
Comment on lines
+150
to
+156
| name = normalized['name'] | ||
| seen_names.add(name) | ||
| current = enterprise_academy_model.objects.filter(name__iexact=name).first() | ||
| if current is not None: | ||
| normalized['name'] = current.name | ||
| seen_names.add(current.name) | ||
|
|
Comment on lines
+132
to
+134
| payload = get_academies(academy_uuid=academy_uuid) # pylint: disable=not-callable | ||
| academy_items = payload if isinstance(payload, list) else [] | ||
|
|
Comment on lines
+103
to
+121
| class TestCustomerBillingConfig(SimpleTestCase): | ||
| """Verify app startup wiring behavior.""" | ||
|
|
||
| def test_ready_imports_signals(self): | ||
| """Calling ready should import customer_billing signal handlers.""" | ||
| app_module = importlib.import_module('enterprise_access.apps.customer_billing') | ||
| config = CustomerBillingConfig(CustomerBillingConfig.name, app_module) | ||
|
|
||
| with mock.patch('builtins.__import__', wraps=__import__) as mock_import: | ||
| config.ready() | ||
|
|
||
| signal_import_calls = [ | ||
| call | ||
| for call in mock_import.call_args_list | ||
| if call.args and call.args[0] == 'enterprise_access.apps.customer_billing.signals' | ||
| ] | ||
|
|
||
| self.assertEqual(len(signal_import_calls), 1) | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Summary — ENT-11468: PR D - Interfaces: academy_sync
### OVERVIEW
This PR introduces the academy sync interface layer for the customer_billing app.
It provides a pure-Python sync helper that:
pulls academy metadata from Enterprise Catalog
upserts data into the local EnterpriseAcademy model
This PR is intentionally scoped as an interfaces-only implementation.
NOT included in this PR:
model definitions
migrations
Celery tasks
API views
serializers
admin integration
The actual EnterpriseAcademy model will be introduced in a follow-up PR.
====================================================================
FILES CHANGED (4)
**customer_billing/academy_sync.py
Purpose:**
Core sync logic
Field-mapping helpers
Payload normalization
Sync orchestration
customer_billing/apps.py
Purpose:
Fix signal wiring guard
Uses class-level protection instead of instance-level protection
tests/test_academy_sync.py
Purpose:
46 tests
100% statement coverage
100% branch coverage
tests/test_apps.py
Purpose:
Tests signal guard behavior
Covers class-level reload scenarios
====================================================================
academy_sync.py — WHAT IT DOES
====================================================================
_get_enterprise_academy_model()
Purpose:
Lazily resolves customer_billing.EnterpriseAcademy using Django app registry
Allows imports before migrations run
Behavior:
Converts LookupError into RuntimeError
Provides actionable error messaging when model is missing
====================================================================
AcademySyncResult (dataclass)
Purpose:
Lightweight sync result container
Tracks:
created
updated
unchanged
skipped
deactivated
errors
Descriptions:
created -> newly inserted rows
updated -> modified existing rows
unchanged -> rows already matching payload
skipped -> invalid payload entries
deactivated -> rows marked inactive
errors -> per-row failures (logged only)
====================================================================
FIELD MAPPING HELPERS
Helper:
_first_non_empty(*values)
Purpose:
Returns first usable string value
Handles:
None
rendered rich-text dictionaries
arbitrary stringable objects
Helper:
_to_slug(raw, prefix, item_id)
Purpose:
Generates URL-safe slug
Includes deterministic fallback chain
Guarantees non-empty slug generation
Helper:
_to_lookup_token(raw)
Purpose:
Converts display names into lowercase underscore-safe tokens
Example:
Data Academy
-> data_academy
Used for:
Stripe lookup key generation
Helper:
_default_stripe_lookup_key(name, product_key)
Purpose:
Generates fallback Stripe lookup keys
Example:
essentials_{token}_academy_yearly
Used when:
payload does not provide stripe_price_lookup_key
Helper:
_extract_payload_list(payload)
Purpose:
Flattens various payload envelope formats into a simple list
Supports:
results
items
academies
catalogs
Helper:
_extract_catalog_query_uuid(*sources)
Purpose:
Extracts first valid UUID from heterogeneous payload structures
Supports:
UUID strings
UUID objects
nested dictionaries
nested lists
Explicitly rejects:
legacy integer IDs
====================================================================
_normalize_catalog_academy(item)
Purpose:
Maps raw catalog payload into EnterpriseAcademy-compatible fields
Returns:
normalized dictionary
OR
None if no usable name exists
Handles:
multiple name aliases
name
title
short_name
Stripe metadata extraction
catalog_query_uuid extraction from multiple payload locations
safe defaults for:
display_order
is_active
====================================================================
fetch_enterprise_catalog_academies(academy_uuid=None)
Purpose:
Calls EnterpriseCatalogApiClient.get_academies()
Converts response into flat list format
Validation:
Raises AttributeError immediately if client lacks get_academies()
Benefit:
prevents silent dependency failures
====================================================================
sync_enterprise_academies_from_enterprise_catalog()
MAIN ENTRY POINT
Parameters:
academy_uuid
deactivate_missing
dry_run
====================================================================
KEY DESIGN DECISIONS
A) Per-row transaction.atomic() savepoints
Behavior:
each row operation runs inside isolated savepoint
Benefit:
one failed row does NOT poison entire transaction
sync safely continues processing remaining rows
Without this:
a single DB failure would force rollback of later operations
B) Case-insensitive lookup + case-safe deactivation
Lookup:
uses name__iexact
Deactivation:
uses exclude(name__in=seen_names)
Problem solved:
prevents accidental deactivation caused by case mismatch
Example:
Payload:
data academy
Database:
Data Academy
Fix:
DB-canonical name added into seen_names
C) Catalog UUID preservation
Behavior:
existing catalog_query_uuid is preserved when incoming payload omits value
Benefit:
avoids overwriting valid DB UUID with None
D) dry_run=True
Behavior:
executes complete sync logic
performs ZERO database writes
No:
create()
save()
update()
Benefit:
accurate preview mode
E) deactivate_missing=True
Behavior:
unseen rows bulk updated:
is_active=False
Safety:
skipped entirely when payload is empty
Benefit:
prevents accidental mass deactivation
====================================================================
apps.py — SIGNAL WIRING GUARD FIX
Previous implementation:
self._signals_wired = True
Problem:
only updated instance attribute
class attribute remained False
Risk:
duplicate signal registration if AppConfig recreated
Fixed implementation:
CustomerBillingConfig._signals_wired = True
Benefit:
survives:
reloads
registry repopulation
multiple AppConfig instances
====================================================================
TEST COVERAGE
academy_sync.py
Statements: 169
Missed: 0
Branches: 84
Partials: 0
Coverage: 100%
apps.py
Statements: 10
Missed: 0
Branches: 2
Partials: 0
Coverage: 100%
Total tests:
47
====================================================================
TEST SCENARIOS COVERED
Normalization edge cases:
missing name
multiple UUID locations
integer ID rejection
rich-text fields
slug fallback chain
Full sync flow:
create
update
unchanged
skip
create() failure
save() failure
Case mismatch deactivation safety:
"data academy" must NOT deactivate "Data Academy"
Dry run validation:
no DB writes
accurate counts
deactivate_missing validation:
stale rows deactivate correctly
empty payload does not trigger deactivation
_get_enterprise_academy_model validation:
success path
LookupError -> RuntimeError conversion
apps.py validation:
signals wired once
class-level guard survives multiple AppConfig instances
====================================================================
WHAT IS NOT INCLUDED IN THIS PR
EnterpriseAcademy model
migrations
Celery task
management command
Django admin integration
API views
serializers
These are intentionally deferred to future implementation PRs.
====================================================================
CI STATUS
PASS:
Django Tests
Lint / Quality
Commit lint
codecov/patch -> 100%
WARNING:
codecov/project -> 86.27% (-5.45%)
Note:
repository-wide baseline issue
unrelated to this PR
all new lines introduced by this PR are fully covered by tests