diff --git a/apps/reports/services/data_sources.py b/apps/reports/services/data_sources.py index 3d07a30..2600cac 100644 --- a/apps/reports/services/data_sources.py +++ b/apps/reports/services/data_sources.py @@ -1,6 +1,6 @@ import logging -from utils.accounts_client import get_user_detailed_info +from utils.accounts_client import get_user_detailed_info, list_users from utils.advertising_client import get_crm_application_tickets logger = logging.getLogger(__name__) @@ -32,24 +32,33 @@ class ReportDataSources: return tickets def fetch_users(self, filters): - tickets = self.fetch_tickets(filters) users = [] - seen = set() + limit = 100 + offset = 0 - for ticket in tickets: - user_id = str(ticket.user) - if user_id in seen: - continue - seen.add(user_id) + while True: + page = list_users(limit=limit, offset=offset) + results = page.get("results") or [] + if not results: + break - try: - user = get_user_detailed_info(user_id) - except Exception: - logger.exception("Failed to fetch user %s", user_id) - continue + for entry in results: + user_id = entry.get("uuid") + if not user_id: + continue - if user is not None: - users.append(user) + try: + user = get_user_detailed_info(user_id) + except Exception: + logger.exception("Failed to fetch user %s", user_id) + continue + + if user is not None: + users.append(user) + + if not page.get("next"): + break + offset += limit return users diff --git a/apps/reports/tests.py b/apps/reports/tests.py index 32a7045..b89be09 100644 --- a/apps/reports/tests.py +++ b/apps/reports/tests.py @@ -3,13 +3,54 @@ from io import BytesIO from unittest.mock import MagicMock, patch from uuid import uuid4 +from django.test import TestCase from openpyxl import load_workbook from rest_framework import status from rest_framework.test import APITestCase +from apps.reports.services.data_sources import ReportDataSources from utils.clients.gooyal_advertising_client.models.ticket_state_enum import TicketStateEnum +class ReportDataSourcesFetchUsersTests(TestCase): + @patch("apps.reports.services.data_sources.get_user_detailed_info") + @patch("apps.reports.services.data_sources.list_users") + def test_fetch_users_paginates_full_user_list(self, mock_list_users, mock_get_user): + first_uuid, second_uuid = str(uuid4()), str(uuid4()) + mock_list_users.side_effect = [ + {"results": [{"uuid": first_uuid}], "next": "http://accounts/users/api/users/?limit=1&offset=1"}, + {"results": [{"uuid": second_uuid}], "next": None}, + ] + mock_get_user.side_effect = lambda uuid: MagicMock(uuid=uuid) + + users = ReportDataSources().fetch_users({}) + + self.assertEqual([user.uuid for user in users], [first_uuid, second_uuid]) + mock_list_users.assert_any_call(limit=100, offset=0) + mock_list_users.assert_any_call(limit=100, offset=100) + mock_get_user.assert_any_call(first_uuid) + mock_get_user.assert_any_call(second_uuid) + + @patch("apps.reports.services.data_sources.get_user_detailed_info") + @patch("apps.reports.services.data_sources.list_users") + def test_fetch_users_skips_user_whose_details_call_fails(self, mock_list_users, mock_get_user): + ok_uuid, broken_uuid = str(uuid4()), str(uuid4()) + mock_list_users.return_value = { + "results": [{"uuid": broken_uuid}, {"uuid": ok_uuid}], + "next": None, + } + def get_user(uuid): + if uuid == broken_uuid: + raise Exception("boom") + return MagicMock(uuid=uuid) + + mock_get_user.side_effect = get_user + + users = ReportDataSources().fetch_users({}) + + self.assertEqual([user.uuid for user in users], [ok_uuid]) + + class GenerateReportTests(APITestCase): def _build_ticket(self, user_uuid=None): ticket = MagicMock() diff --git a/utils/accounts_client.py b/utils/accounts_client.py index 8d79e73..d2d293b 100644 --- a/utils/accounts_client.py +++ b/utils/accounts_client.py @@ -74,3 +74,26 @@ def get_user_detailed_info(uuid): return response +def list_users(limit=100, offset=0): + """Fetch one page of the accounts user list. + + The generated OpenAPI client (users_api_users_list) only supports + `page`, and that param is silently ignored by the upstream endpoint -- + every page returns the same first results. Its real pagination is + limit/offset, so this bypasses the generated wrapper with a raw request. + """ + access_token = login_as_client_credentials()['access_token'] + base_url = settings.ACCOUNTS_BASE_PUBLIC_URL + url = f"{base_url.rstrip('/')}/users/api/users/" + + response = request_with_ssl_fallback( + 'GET', + url, + params={"limit": limit, "offset": offset}, + headers={"Authorization": f"Bearer {access_token}"}, + verify=resolve_verify_ssl(base_url), + ) + response.raise_for_status() + return response.json() + +