fix: address final review findings for service clients plan
Fixes 9 numbered findings + 7 minor fixes from the whole-plan review of the MyWhoosh/Garmin service clients (Plan 3): Garmin uploader (app/garmin/uploader.py): - Detect Garmin-rejected imports (failures without successes) and raise new GarminImportRejected instead of reporting them as successful. - Reclassify 429/rate-limit/500 login failures as transient instead of falling through to permanent auth errors; unrecognized login failures are now treated as transient (retryable) rather than GarminAuthError. - Mirror the auth-token check from the login branch into the import branch so 401-at-import-time raises GarminAuthError instead of propagating raw. - Add common GarminError base class, hoist transient-token tuple to a shared module constant, check response.status_code==409 before the duplicate substring fallback, and create the tokenstore dir 0o700. MyWhoosh client (app/mywhoosh/client.py): - Add optional max_pages bound to list_activities pagination. - Add aclose()/__aenter__/__aexit__ so the client's own httpx.AsyncClient gets closed, while never closing an injected client. - Guard the two remaining unguarded JSON-decode paths (login body, download-fit metadata) so malformed bodies raise MyWhooshIntegrationError instead of raw ValueError/AttributeError. - Row-level malformation (missing id/activityFileId, unparseable startDatetime) is now skipped rather than aborting the whole page; envelope-shape failures still raise. id/activityFileId checks use explicit None/"" comparisons instead of Python falsiness. - Replace asserts in _authenticated_post with explicit exceptions; restrict the reauth retry to 401 only, treat 403 as immediately terminal; naive startDatetime values are now treated as already-UTC instead of host-local. MyWhoosh tokenstore (app/mywhoosh/tokenstore.py): - load() now treats any corrupt/malformed token file (bad JSON, missing keys, OS errors) as "absent" instead of raising, so a bad cache no longer permanently wedges a user. pyproject.toml: - Tighten garminconnect pin to >=0.3.10,<1 (import_activity requires 0.3.10+). Adds/updates tests across tests/mywhoosh/ and tests/garmin/ covering all of the above, including a fake client that wraps GarminUploadBlocked in a plain RuntimeError to mirror the real garminconnect library's MFA error wrapping. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
@@ -2,7 +2,13 @@ from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
from app.garmin.uploader import GarminUploadBlocked, GarminUploader
|
||||
from app.garmin.uploader import (
|
||||
GarminAuthError,
|
||||
GarminImportRejected,
|
||||
GarminTransientError,
|
||||
GarminUploadBlocked,
|
||||
GarminUploader,
|
||||
)
|
||||
|
||||
|
||||
class FakeGarmin:
|
||||
@@ -78,3 +84,93 @@ def test_mfa_code_is_returned_only_to_prompt(tmp_path: Path) -> None:
|
||||
)
|
||||
uploader.import_fit(tmp_path / "ride.fit", mfa_code="123456")
|
||||
assert seen == ["123456"]
|
||||
|
||||
|
||||
def test_import_rejected_by_garmin_raises_garmin_import_rejected(tmp_path: Path) -> None:
|
||||
rejected_response = {
|
||||
"detailedImportResult": {
|
||||
"successes": [],
|
||||
"failures": [{"internalId": 1, "messages": ["Invalid FIT file"]}],
|
||||
}
|
||||
}
|
||||
uploader = GarminUploader(
|
||||
email="g@example.com",
|
||||
password="pw",
|
||||
tokenstore=tmp_path / "garmin",
|
||||
client_factory=lambda *a, **kw: FakeGarmin(*a, **kw, import_result=rejected_response),
|
||||
)
|
||||
with pytest.raises(GarminImportRejected):
|
||||
uploader.import_fit(tmp_path / "ride.fit")
|
||||
|
||||
|
||||
def test_login_failure_with_429_is_transient(tmp_path: Path) -> None:
|
||||
uploader = GarminUploader(
|
||||
email="g@example.com",
|
||||
password="pw",
|
||||
tokenstore=tmp_path / "garmin",
|
||||
client_factory=lambda *a, **kw: FakeGarmin(*a, **kw, login_error=RuntimeError("429 too many requests")),
|
||||
)
|
||||
with pytest.raises(GarminTransientError):
|
||||
uploader.import_fit(tmp_path / "ride.fit")
|
||||
|
||||
|
||||
def test_login_failure_with_timeout_is_transient(tmp_path: Path) -> None:
|
||||
uploader = GarminUploader(
|
||||
email="g@example.com",
|
||||
password="pw",
|
||||
tokenstore=tmp_path / "garmin",
|
||||
client_factory=lambda *a, **kw: FakeGarmin(*a, **kw, login_error=RuntimeError("connection timeout")),
|
||||
)
|
||||
with pytest.raises(GarminTransientError):
|
||||
uploader.import_fit(tmp_path / "ride.fit")
|
||||
|
||||
|
||||
def test_login_failure_with_credential_message_is_auth_error(tmp_path: Path) -> None:
|
||||
uploader = GarminUploader(
|
||||
email="g@example.com",
|
||||
password="pw",
|
||||
tokenstore=tmp_path / "garmin",
|
||||
client_factory=lambda *a, **kw: FakeGarmin(*a, **kw, login_error=RuntimeError("invalid credential")),
|
||||
)
|
||||
with pytest.raises(GarminAuthError):
|
||||
uploader.import_fit(tmp_path / "ride.fit")
|
||||
|
||||
|
||||
def test_login_failure_unrecognized_is_transient(tmp_path: Path) -> None:
|
||||
uploader = GarminUploader(
|
||||
email="g@example.com",
|
||||
password="pw",
|
||||
tokenstore=tmp_path / "garmin",
|
||||
client_factory=lambda *a, **kw: FakeGarmin(*a, **kw, login_error=RuntimeError("something odd happened")),
|
||||
)
|
||||
with pytest.raises(GarminTransientError):
|
||||
uploader.import_fit(tmp_path / "ride.fit")
|
||||
|
||||
|
||||
def test_import_time_401_raises_auth_error(tmp_path: Path) -> None:
|
||||
uploader = GarminUploader(
|
||||
email="g@example.com",
|
||||
password="pw",
|
||||
tokenstore=tmp_path / "garmin",
|
||||
client_factory=lambda *a, **kw: FakeGarmin(*a, **kw, import_error=RuntimeError("401 unauthorized")),
|
||||
)
|
||||
with pytest.raises(GarminAuthError):
|
||||
uploader.import_fit(tmp_path / "ride.fit")
|
||||
|
||||
|
||||
def test_mfa_wrapped_in_generic_exception_still_blocks(tmp_path: Path) -> None:
|
||||
class WrappedMfaGarmin(FakeGarmin):
|
||||
def login(self, tokenstore=None):
|
||||
try:
|
||||
self.prompt_mfa()
|
||||
except GarminUploadBlocked as exc:
|
||||
raise RuntimeError(f"Login failed: Garmin requested MFA ({exc})") from exc
|
||||
|
||||
uploader = GarminUploader(
|
||||
email="g@example.com",
|
||||
password="pw",
|
||||
tokenstore=tmp_path / "garmin",
|
||||
client_factory=WrappedMfaGarmin,
|
||||
)
|
||||
with pytest.raises(GarminUploadBlocked):
|
||||
uploader.import_fit(tmp_path / "ride.fit")
|
||||
|
||||
Reference in New Issue
Block a user