diff --git a/app_versions.py b/app_versions.py index 932088e..b895f84 100644 --- a/app_versions.py +++ b/app_versions.py @@ -579,17 +579,10 @@ def _enrich_app_store_versions(rows: list[dict[str, Any]]) -> list[dict[str, Any and _should_lookup_app_store_version(row) ) ) - lookup_bundle_ids = bundle_ids[:APP_STORE_LOOKUP_LIMIT] - if len(bundle_ids) > APP_STORE_LOOKUP_LIMIT: - logging.warning( - "Skipping App Store lookup for %s iOS bundle IDs over the %s-bundle limit.", - len(bundle_ids) - APP_STORE_LOOKUP_LIMIT, - APP_STORE_LOOKUP_LIMIT, - ) app_store_versions: dict[str, dict[str, Any]] = {} - if lookup_bundle_ids: + if bundle_ids: try: - app_store_versions = _fetch_app_store_versions(lookup_bundle_ids) + app_store_versions = _fetch_app_store_versions(bundle_ids) except requests.RequestException as exc: logging.warning("App Store version lookup failed: %s", exc) except ValueError as exc: @@ -617,46 +610,51 @@ def _enrich_app_store_versions(rows: list[dict[str, Any]]) -> list[dict[str, Any def _fetch_app_store_versions(bundle_ids: list[str]) -> dict[str, dict[str, Any]]: + def lookup(ids: list[str]) -> list[dict[str, Any]]: + response = requests.get( + APP_STORE_LOOKUP_URL, + params={"bundleId": ",".join(ids), "country": "us"}, + timeout=APP_STORE_LOOKUP_TIMEOUT_SECONDS, + ) + response.raise_for_status() + payload = response.json() + if not isinstance(payload, dict) or not isinstance(payload.get("results"), list): + raise ValueError("Invalid App Store lookup response") + return payload["results"] + + batches = [ + bundle_ids[i : i + APP_STORE_LOOKUP_LIMIT] + for i in range(0, len(bundle_ids), APP_STORE_LOOKUP_LIMIT) + ] app_store_versions: dict[str, dict[str, Any]] = {} - max_workers = min(APP_STORE_LOOKUP_WORKERS, len(bundle_ids)) - with ThreadPoolExecutor(max_workers=max_workers) as executor: - future_by_bundle_id = { - executor.submit(_fetch_app_store_version, bundle_id): bundle_id - for bundle_id in bundle_ids - } - for future in as_completed(future_by_bundle_id): - bundle_id = future_by_bundle_id[future] + with ThreadPoolExecutor(max_workers=min(APP_STORE_LOOKUP_WORKERS, len(batches))) as executor: + futures = {executor.submit(lookup, batch): batch for batch in batches} + while futures: + future = next(as_completed(futures)) + batch = futures.pop(future) try: - result = future.result() + results = future.result() except (requests.RequestException, ValueError) as exc: - logging.warning("App Store version lookup failed for %s: %s", bundle_id, exc) + response = ( + getattr(exc, "response", None) if isinstance(exc, requests.HTTPError) else None + ) + if response is not None and response.status_code == 400 and len(batch) > 1: + mid = len(batch) // 2 + for smaller_batch in (batch[:mid], batch[mid:]): + futures[executor.submit(lookup, smaller_batch)] = smaller_batch + else: + logging.warning("App Store version lookup failed for %s: %s", batch, exc) continue - if result: - app_store_versions[result["bundleId"]] = result + for result in results: + if ( + isinstance(result, dict) + and result.get("bundleId") in batch + and _string_value(result.get("version")) + ): + app_store_versions[result["bundleId"]] = result return app_store_versions -def _fetch_app_store_version(bundle_id: str) -> dict[str, Any] | None: - response = requests.get( - APP_STORE_LOOKUP_URL, - params={ - "bundleId": bundle_id, - "country": "us", - }, - timeout=APP_STORE_LOOKUP_TIMEOUT_SECONDS, - ) - response.raise_for_status() - payload = response.json() - for result in payload.get("results", []): - if not isinstance(result, dict): - continue - fetched_bundle_id = _string_value(result.get("bundleId")) - version = _string_value(result.get("version")) - if fetched_bundle_id and version: - return {**result, "bundleId": fetched_bundle_id} - return None - - def _should_lookup_app_store_version(row: dict[str, Any]) -> bool: platform = (_string_value(row.get("apollos_platform")) or "").lower() bundle_id = _string_value(row.get("bundle_id")) or "" diff --git a/templates/app_versions.html b/templates/app_versions.html index 412ab0d..71bf487 100644 --- a/templates/app_versions.html +++ b/templates/app_versions.html @@ -174,7 +174,7 @@

App data is unavailable

{% for row in tab.rows %} - {{ row.church }}
+ {{ "Demo" if row.church == "apollos_demo" else row.church }}
{{ row.bundle_id }} @@ -182,7 +182,7 @@

App data is unavailable

Observed {{ row.app_version or "unknown" }} {% if row.latest_app_version_source == "app_store" and row.latest_app_version != row.app_version %}
- App Store {{ row.latest_app_version }} + App Store (live) {{ row.latest_app_version }} {% endif %} {{ row.freshness_display or row.apollos_version or "TBD" }} diff --git a/tests/test_app_versions.py b/tests/test_app_versions.py index cdd432f..652666e 100644 --- a/tests/test_app_versions.py +++ b/tests/test_app_versions.py @@ -293,7 +293,7 @@ def test_enriches_app_store_versions_by_bundle_id(self): self.assertEqual(android["latest_app_version"], "1.0.0") self.assertEqual(android["latest_app_version_source"], "observed") - def test_limits_app_store_lookup_count(self): + def test_looks_up_all_app_store_bundles(self): rows = [ { "church": f"church-{index}", @@ -314,13 +314,10 @@ def test_limits_app_store_lookup_count(self): app_versions._enrich_app_store_versions(rows) lookup_bundle_ids = fetch_app_store_versions.call_args.args[0] - self.assertEqual(len(lookup_bundle_ids), app_versions.APP_STORE_LOOKUP_LIMIT) - self.assertNotIn( - f"com.example.{app_versions.APP_STORE_LOOKUP_LIMIT}", - lookup_bundle_ids, - ) + self.assertEqual(len(lookup_bundle_ids), app_versions.APP_STORE_LOOKUP_LIMIT + 1) + self.assertIn(f"com.example.{app_versions.APP_STORE_LOOKUP_LIMIT}", lookup_bundle_ids) - def test_fetches_each_app_store_bundle_id_individually(self): + def test_batches_app_store_lookup_across_all_bundle_ids(self): class Response: def __init__(self, payload: dict[str, Any]): self.payload = payload @@ -332,48 +329,101 @@ def json(self) -> dict[str, Any]: return self.payload responses = [ - Response( - { - "results": [ - { - "bundleId": "com.example.one", - "version": "1.2.3", - }, - ], - } - ), - Response( - { - "results": [ - { - "bundleId": "com.example.two", - "version": "2.3.4", - }, - ], - } - ), + Response({"results": [{"bundleId": "com.example.one", "version": "1.40"}]}), + Response({"results": [{"bundleId": "com.example.last", "version": "2.3.4"}]}), ] + bundle_ids = ( + ["com.example.one"] + + [f"com.example.{i}" for i in range(app_versions.APP_STORE_LOOKUP_LIMIT - 1)] + + ["com.example.last"] + ) - with patch.object( - app_versions.requests, - "get", - side_effect=responses, - ) as get: - versions = app_versions._fetch_app_store_versions( - ["com.example.one", "com.example.two"] - ) - - self.assertEqual(versions["com.example.one"]["version"], "1.2.3") - self.assertEqual(versions["com.example.two"]["version"], "2.3.4") + def response_for_batch(url, *, params, timeout): + if "com.example.one" in params["bundleId"].split(","): + return responses[0] + return responses[1] + + with patch.object(app_versions.requests, "get", side_effect=response_for_batch) as get: + versions = app_versions._fetch_app_store_versions(bundle_ids) + + self.assertEqual(versions["com.example.one"]["version"], "1.40") + self.assertEqual(versions["com.example.last"]["version"], "2.3.4") self.assertEqual(get.call_count, 2) self.assertCountEqual( [call.kwargs["params"] for call in get.call_args_list], [ - {"bundleId": "com.example.one", "country": "us"}, - {"bundleId": "com.example.two", "country": "us"}, + { + "bundleId": ",".join(bundle_ids[: app_versions.APP_STORE_LOOKUP_LIMIT]), + "country": "us", + }, + {"bundleId": "com.example.last", "country": "us"}, ], ) + def test_failed_batch_does_not_hide_healthy_app_versions(self): + class Response: + def __init__(self, ids): + self.ids = ids + + def raise_for_status(self): + pass + + def json(self): + return { + "results": [ + {"bundleId": bundle_id, "version": "1.40"} for bundle_id in self.ids + ] + } + + def lookup(url, *, params, timeout): + ids = params["bundleId"].split(",") + if "com.example.bad" in ids: + error = app_versions.requests.HTTPError("bad bundle") + error.response = types.SimpleNamespace(status_code=400) + raise error + return Response(ids) + + bundle_ids = ["com.example.bad"] + [ + f"com.example.good{i}" for i in range(app_versions.APP_STORE_LOOKUP_LIMIT) + ] + with patch.object(app_versions.requests, "get", side_effect=lookup): + versions = app_versions._fetch_app_store_versions(bundle_ids) + + self.assertEqual(len(versions), app_versions.APP_STORE_LOOKUP_LIMIT) + self.assertEqual(versions["com.example.good0"]["version"], "1.40") + self.assertIn(f"com.example.good{app_versions.APP_STORE_LOOKUP_LIMIT - 1}", versions) + self.assertNotIn("com.example.bad", versions) + + def test_transient_or_malformed_batch_does_not_retry_every_bundle(self): + class Response: + def __init__(self, results): + self.results = results + + def raise_for_status(self): + pass + + def json(self): + return {"results": self.results} + + bundle_ids = [f"com.example.{i}" for i in range(app_versions.APP_STORE_LOOKUP_LIMIT)] + bundle_ids.append("com.example.last") + for failure in ("timeout", "malformed"): + with self.subTest(failure=failure): + + def lookup(url, *, params, timeout): + ids = params["bundleId"].split(",") + if len(ids) > 1: + if failure == "timeout": + raise app_versions.requests.RequestException("timeout") + return Response({"unexpected": "shape"}) + return Response([{"bundleId": ids[0], "version": "1.40"}]) + + with patch.object(app_versions.requests, "get", side_effect=lookup) as get: + versions = app_versions._fetch_app_store_versions(bundle_ids) + self.assertEqual(get.call_count, 2) + self.assertEqual(versions["com.example.last"]["version"], "1.40") + self.assertEqual(len(versions), 1) + def test_selects_highest_observed_version_instead_of_most_recent_event(self): rows = [ { @@ -709,13 +759,37 @@ def test_apps_route_renders_platform_tabs(self): self.assertIn("Expo Runtime", body) self.assertNotIn("Source", body) self.assertIn("1.0.1", body) - self.assertIn("App Store 1.0.1", body) - self.assertNotIn("App Store 1.0.0", body) + self.assertIn("App Store (live) 1.0.1", body) + self.assertNotIn("App Store (live) 1.0.0", body) self.assertIn("97", body) self.assertIn("Two Church", body) self.assertNotIn("Platform", body) self.assertNotIn("Latest Observed", body) + def test_preview_shows_demo_label_and_public_app_store_version(self): + row = { + "church": "apollos_demo", + "bundle_id": "com.differential.apollospreview", + "application_name": "Apollos Preview", + "app_version": "1.0.0", + "latest_app_version": "1.40", + "latest_app_version_source": "app_store", + "apollos_platform": "ios", + "freshness_display": "106", + } + context = { + "status": "ready", + "rows": [row], + "platform_tabs": app_versions.build_platform_tabs([row]), + "lookback_days": 30, + } + with patch.object(app_module, "get_app_versions_context", return_value=context): + body = self.client.get("/apps").get_data(as_text=True) + self.assertIn("Demo", body) + self.assertIn("Observed 1.0.0", body) + self.assertIn("App Store (live) 1.40", body) + self.assertNotIn("apollos_demo", body) + if __name__ == "__main__": unittest.main()