Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 35 additions & 1 deletion credentials/apps/badges/credly/api_client.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import base64
import logging
import time
from functools import lru_cache
from urllib.parse import urljoin

Expand Down Expand Up @@ -86,7 +87,40 @@ def fetch_badge_templates(self):
"""
Fetches the badge templates from the Credly API.
"""
return self.perform_request("get", f"badge_templates/?filter=state::{CredlyBadgeTemplate.STATES.active}")
results = []
url = f"badge_templates/?filter=state::{CredlyBadgeTemplate.STATES.active}"
response = self.perform_request("get", url)
results.extend(response.get("data", []))

metadata = response.get("metadata", {})
total_pages = metadata.get("total_pages", 1)
next_page_url = metadata.get("next_page_url")

# Loop through all remaining pages based on the total_pages value.
# For each iteration, fetch data using the 'next_page_url' provided by the API,
# append the results to the main list, and update the URL for the next request.
# The loop stops when there are no more pages to retrieve.
for _ in range(2, total_pages + 1):
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few robustness questions:

  1. What's a reasonable maximum number of pages?
  2. What happens if you hit a rate limit or timeout?
  3. should you throttle the calls to not be flagged as a bot or overwhelm the backend?
  4. Is this ever called from a request to the credentials frontend?

Copy link
Author

@luisfelipec95 luisfelipec95 Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback!

  1. I think 3 or 4 pages is a reasonable maximum.
  2. With the current change, any error raised during perform_request will propagate as an exception, which allows us to immediately detect issues such as rate limiting or timeouts.
  3. Yes, I added a minimal throttle between page requests to avoid overwhelming the Credly API and to reduce the risk of hitting rate limits.
  4. As far as I’ve seen, it’s only used in the Credentials admin

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to do them all at once like this? If there's enough to paginate, do you not want to paginate our results? This could be an iterator with yield and the consumer could handle that way?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback!

I see your point, but in this case I don’t think we need to introduce a generator. The number of active Credly badge templates is usually small, so pagination is unlikely to produce a large dataset. Since the consumer only needs the full list, handling everything at once keeps the implementation simpler without adding unnecessary complexity.

if not next_page_url:
break

time.sleep(0.2)

for attempt in range(3):
try:
response = self.perform_request("get", next_page_url)
break
except (requests.Timeout, requests.ConnectionError) as exc:
sleep_time = 0.5 * (2**attempt)
time.sleep(sleep_time)

if attempt == 2:
raise CredlyError(f"Failed to fetch page due to network error: {exc}")

results.extend(response.get("data", []))
next_page_url = response.get("metadata", {}).get("next_page_url")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no try block, so you want to raise exception if any one call fails (eg. for rate limiting reasons)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback!

I added the retry loop, so transient failures are handled, and if all attempts fail, the exception is still raised.


return {"data": results}

def fetch_event_information(self, event_id):
"""
Expand Down
7 changes: 5 additions & 2 deletions credentials/apps/badges/tests/test_api_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,13 @@ def test_fetch_organization(self):

def test_fetch_badge_templates(self):
with mock.patch.object(CredlyAPIClient, "perform_request") as mock_perform_request:
mock_perform_request.return_value = {"badge_templates": ["template1", "template2"]}
mock_perform_request.return_value = {
"data": ["template1", "template2"],
"metadata": {"total_pages": 1},
}
result = self.api_client.fetch_badge_templates()
mock_perform_request.assert_called_once_with("get", "badge_templates/?filter=state::active")
self.assertEqual(result, {"badge_templates": ["template1", "template2"]})
self.assertEqual(result, {"data": ["template1", "template2"]})

def test_fetch_event_information(self):
event_id = "event123"
Expand Down