feat: ENT-11468-PR : B- Updated Interfaces: external clients by tsunkara-sonata · Pull Request #172 · edx/enterprise-access

tsunkara-sonata · 2026-05-21T10:40:04Z

Description:
Add a description of your changes here.

Part of #168

Interfaces: external clients

[enterprise_access/apps/api_client/enterprise_catalog_client.py]—get_academies(),get_catalogs().
[enterprise_access/apps/api_client/tests/test_enterprise_catalog_client.py]

Copilot

Pull request overview

Adds new EnterpriseCatalogApiClient v2 methods to fetch academies and enterprise catalogs from enterprise-catalog, along with unit tests covering the new endpoints. This supports the broader Essential Academy integration work referenced in PR #168.

Changes:

Add get_academies(academy_uuid=None) and get_catalogs() to the enterprise-catalog v2 API client.
Add tests for the new client methods and their request URL/params behavior.

No critical issues found.

Non-critical issues to address:

New tests include unused request_response variables (dead code).
“compatibility wrapper” tests don’t actually validate any wrapper/alias behavior (they only check hasattr).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
enterprise_access/apps/api_client/enterprise_catalog_client.py	Adds v2 client methods for academies and enterprise catalog listing.
enterprise_access/apps/api_client/tests/test_enterprise_catalog_client.py	Adds unit tests for the new client methods (URL/params assertions).

Comments suppressed due to low confidence (3)

enterprise_access/apps/api_client/tests/test_enterprise_catalog_client.py:147

request_response is instantiated and modified but never used; the test currently relies on the mock's return value instead. Either remove request_response or set mock_oauth_client.return_value.get.return_value = request_response to avoid dead code and make the test intent clearer.

        mock_response_json = {'results': []}
        request_response = Response()
        request_response.status_code = 200
        mock_oauth_client.return_value.get.return_value.json.return_value = mock_response_json

enterprise_access/apps/api_client/tests/test_enterprise_catalog_client.py:164

request_response is unused in this test (it’s not set as the .get() return value). Removing it (or wiring it up as the return value) will prevent dead code and make the test assertions more meaningful.

        mock_response_json = {'results': [{'uuid': str(uuid4())}]}
        request_response = Response()
        request_response.status_code = 200
        mock_oauth_client.return_value.get.return_value.json.return_value = mock_response_json

enterprise_access/apps/api_client/tests/test_enterprise_catalog_client.py:186

Similar to the academies compat test: this only checks hasattr(client, 'get_catalogs'), which doesn’t actually verify a legacy compatibility wrapper. Consider renaming the test or asserting behavior of the intended legacy entrypoint (if one exists/should exist).

    def test_compat_get_enterprise_customer_catalogs(self, _mock_oauth_client):
        """Test that the compatibility wrapper method works."""
        client = EnterpriseCatalogApiClient()
        # This tests the legacy compatibility wrapper
        self.assertTrue(hasattr(client, 'get_catalogs'))

+        mock_response_json = {'results': [{'title': 'Artificial Intelligence'}]}
+        request_response = Response()
+        request_response.status_code = 200
+        mock_oauth_client.return_value.get.return_value.json.return_value = mock_response_json


codecov · 2026-05-21T10:46:57Z

Codecov Report

❌ Patch coverage is 93.93939% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.10%. Comparing base (4a4f726) to head (a8f375d).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
...ccess/apps/api_client/enterprise_catalog_client.py	93.93%	1 Missing and 1 partial ⚠️

❌ Your patch check has failed because the patch coverage (93.93%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #172      +/-   ##
==========================================
+ Coverage   86.08%   86.10%   +0.02%     
==========================================
  Files         149      149              
  Lines       12500    12533      +33     
  Branches     1194     1199       +5     
==========================================
+ Hits        10761    10792      +31     
- Misses       1424     1425       +1     
- Partials      315      316       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

gshivajibiradar · 2026-05-21T11:32:21Z

approved

vshaikismail-sonata

LGTM...!!!

pwnage101 · 2026-05-21T23:57:30Z

+        path_template = getattr(
+            settings,
+            'ACADEMY_ENTERPRISE_CATALOG_ACADEMIES_PATH',
+            'academies/',
+        )
+        endpoint = urljoin(self.api_base_url, path_template)


❌ Never use fallback values when fetching a setting. Fallback values should only be configured in enterprise_access/settings/base.py. That said, in this case no fallback is needed, you can simply hard-code the path.

❌ Furthermore, please follow the established pattern in this class of constructing the endpoints within the init() function.

Addressing the above two concerns would result in init() with one extra line:

self.academies_endpoint = urljoin(self.api_base_url, 'academies/')

and this code would change to:

Suggested change

path_template = getattr(

settings,

'ACADEMY_ENTERPRISE_CATALOG_ACADEMIES_PATH',

'academies/',

)

endpoint = urljoin(self.api_base_url, path_template)

endpoint = urljoin(self.api_base_url, self.academies_endpoint)

Finally, ❌ this endpoint will likely be paginated, and this method as written has no accommodations for paging. Without knowing how you intend to use this method, I'll just leave it to you to decide how to support pagination.

pwnage101 · 2026-05-22T00:01:55Z

        raise NotImplementedError('There is currently no v2 API implementation for this endpoint.')

+    @backoff.on_exception(wait_gen=backoff.expo, exception=autoretry_for_exceptions)
+    def get_academies(self, academy_uuid=None):


Please type hint both function signatures.

pwnage101 · 2026-05-22T00:02:15Z


+    @backoff.on_exception(wait_gen=backoff.expo, exception=autoretry_for_exceptions)
+    def get_academies(self, academy_uuid=None):
+        """Fetch academies from enterprise-catalog."""


Describe the shape of the return value in this docstring.

pwnage101 · 2026-05-22T00:05:26Z

+        path_template = getattr(
+            settings,
+            'ACADEMY_ENTERPRISE_CATALOG_CATALOGS_PATH',
+            'enterprise-catalogs/',
+        )
+        endpoint = urljoin(self.api_base_url, path_template)
+        response = self.client.get(endpoint)


❌ This endpoint string is redundant with self.enterprise_catalog_endpoint which is already available on the class.

Suggested change

path_template = getattr(

settings,

'ACADEMY_ENTERPRISE_CATALOG_CATALOGS_PATH',

'enterprise-catalogs/',

)

endpoint = urljoin(self.api_base_url, path_template)

response = self.client.get(endpoint)

response = self.client.get(self.enterprise_catalog_endpoint)

pwnage101 · 2026-05-22T00:16:22Z

+        path_template = getattr(
+            settings,
+            'ACADEMY_ENTERPRISE_CATALOG_CATALOGS_PATH',
+            'enterprise-catalogs/',
+        )
+        endpoint = urljoin(self.api_base_url, path_template)
+        response = self.client.get(endpoint)


❌ Without filter params, this API call will fetch ALL catalogs, which is almost guaranteed to be paginated. Yet, this function does not handle a paginated output, so it will only return the first page. If you planned to paginate in the caller, that still won't work because there's no way to pass the page number to this method.

It would be helpful if you also described the use case of this method in the docstring.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

+    @backoff.on_exception(wait_gen=backoff.expo, exception=autoretry_for_exceptions)
+    def get_academies(self, academy_uuid: str | None = None) -> dict:
+        """
+        Fetch academies for Essentials flows from enterprise-catalog.
+
+        Returns:
+            dict: Paginated response shape with keys including count/next/previous/results.
+            If the endpoint paginates, all pages are merged into a single response payload.
+        """
+        params = {'academy_uuid': str(academy_uuid)} if academy_uuid else None
+        return self._fetch_all_pages(self.academies_endpoint, params=params)
+
+    @backoff.on_exception(wait_gen=backoff.expo, exception=autoretry_for_exceptions)
+    def get_catalogs(self, enterprise_customer_uuid: str | None = None) -> dict:
+        """
+        Fetch enterprise catalogs, optionally scoped to an enterprise customer UUID.
+
+        Returns:
+            dict: Paginated response shape with keys including count/next/previous/results.
+            If the endpoint paginates, all pages are merged into a single response payload.
+        """
+        params = {'enterprise_customer': str(enterprise_customer_uuid)} if enterprise_customer_uuid else None
+        return self._fetch_all_pages(self.enterprise_catalog_endpoint, params=params)
+


Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

+
+        base_payload['results'] = merged_results
+        base_payload['count'] = len(merged_results)
+        base_payload['next'] = None


pwnage101

Looking much better! Still a few more minor issues.

pwnage101 · 2026-05-26T16:21:05Z

+        merged_results = []
+        next_url = endpoint
+        request_params = params
+        base_payload = None


❌ Please refer to already established patterns in this same repo:

enterprise-access/enterprise_access/apps/api_client/lms_client.py

Line 113 in 30c3d85

results = []

There's no need for the return value of _fetch_all_pages to be a dict. The metadata in the response payload surrorunding the results list is only needed for pagination, and the purpose of this function is to abstract away the pagination.

pwnage101 · 2026-05-26T16:23:40Z

        return response.json()['count']

+    @backoff.on_exception(wait_gen=backoff.expo, exception=autoretry_for_exceptions)
+    def get_academies(self, academy_uuid: str | None = None) -> dict:


Nit: Is there really a use case for passing academy_uuid? Doesn't this function serve the broader use case of a scheduled sync job? It's possible we could simplify by removing the argument:

Suggested change

def get_academies(self, academy_uuid: str | None = None) -> dict:

def get_academies(self) -> list:

pwnage101 · 2026-05-26T16:24:57Z

        self.enterprise_catalog_endpoint = urljoin(self.api_base_url, 'enterprise-catalogs/')
        super().__init__()

+    def _fetch_all_pages(self, endpoint, params=None) -> dict:


❌ should return list

Suggested change

def _fetch_all_pages(self, endpoint, params=None) -> dict:

def _fetch_all_pages(self, endpoint, params=None) -> list:

pwnage101 · 2026-05-26T16:26:37Z

+            dict: Paginated response shape with keys including count/next/previous/results.
+            If the endpoint paginates, all pages are merged into a single response payload.
+        """
+        params = {'enterprise_customer': str(enterprise_customer_uuid)} if enterprise_customer_uuid else None


❌ There's no point in converting to str if the type hint already makes str | None explicit.

Suggested change

params = {'enterprise_customer': str(enterprise_customer_uuid)} if enterprise_customer_uuid else None

params = {'enterprise_customer': enterprise_customer_uuid} if enterprise_customer_uuid else None

tsunkara-sonata requested review from a team as code owners May 21, 2026 10:40

Copilot AI review requested due to automatic review settings May 21, 2026 10:40

tsunkara-sonata requested a review from a team as a code owner May 21, 2026 10:40

Copilot started reviewing on behalf of tsunkara-sonata May 21, 2026 10:40 View session

Copilot AI reviewed May 21, 2026

View reviewed changes

tsunkara-sonata changed the title ~~feat: Updated Interfaces: external clients~~ feat: ENT-11468-PR : D- Updated Interfaces: external clients May 21, 2026

tsunkara-sonata changed the title ~~feat: ENT-11468-PR : D- Updated Interfaces: external clients~~ feat: ENT-11468-PR : B- Updated Interfaces: external clients May 21, 2026

tsunkara-sonata requested a review from vshaikismail-sonata May 21, 2026 16:45

vshaikismail-sonata approved these changes May 21, 2026

View reviewed changes

pwnage101 requested changes May 21, 2026

View reviewed changes

pwnage101 requested changes May 22, 2026

View reviewed changes

pwnage101 mentioned this pull request May 22, 2026

ENT-11468: PR D - Interfaces: academy_sync #171

Open

Copilot AI review requested due to automatic review settings May 26, 2026 11:27

vshaikismail-sonata force-pushed the tsunkara/11468-PR-B branch from a7a1284 to 4e78bb2 Compare May 26, 2026 11:57

Copilot AI reviewed May 26, 2026

Copilot started reviewing on behalf of vshaikismail-sonata May 26, 2026 12:18 View session

feat: addressed review comments

c280fb6

vshaikismail-sonata force-pushed the tsunkara/11468-PR-B branch from 7ae69d1 to c280fb6 Compare May 26, 2026 12:37

feat: Added tests for coverage

cd04738

Copilot AI review requested due to automatic review settings May 26, 2026 13:13

Copilot started reviewing on behalf of vshaikismail-sonata May 26, 2026 13:13 View session

Copilot AI reviewed May 26, 2026

View reviewed changes

feat: more tests for coverage

a8f375d

Copilot AI review requested due to automatic review settings May 26, 2026 13:31

Copilot started reviewing on behalf of vshaikismail-sonata May 26, 2026 13:32 View session

Copilot AI reviewed May 26, 2026

View reviewed changes

Comment thread enterprise_access/apps/api_client/enterprise_catalog_client.py

base_payload['results'] = merged_results

base_payload['count'] = len(merged_results)

base_payload['next'] = None

pwnage101 requested changes May 26, 2026

View reviewed changes

	def get_academies(self, academy_uuid: str \| None = None) -> dict:
	def get_academies(self) -> list:

	def _fetch_all_pages(self, endpoint, params=None) -> dict:
	def _fetch_all_pages(self, endpoint, params=None) -> list:

	params = {'enterprise_customer': str(enterprise_customer_uuid)} if enterprise_customer_uuid else None
	params = {'enterprise_customer': enterprise_customer_uuid} if enterprise_customer_uuid else None

Conversation

tsunkara-sonata commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

codecov Bot commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

gshivajibiradar commented May 21, 2026

Uh oh!

vshaikismail-sonata left a comment

Choose a reason for hiding this comment

Uh oh!

pwnage101 May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

pwnage101 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tsunkara-sonata commented May 21, 2026 •

edited

Loading

codecov Bot commented May 21, 2026 •

edited

Loading

pwnage101 May 21, 2026 •

edited

Loading