Skip to content

feat(journal): page-loop pageIndex=2..N until totleSize / short page#205

Merged
DasBasti merged 2 commits into
DasBasti:mainfrom
jusii:feat/journal-page-loop
Jun 13, 2026
Merged

feat(journal): page-loop pageIndex=2..N until totleSize / short page#205
DasBasti merged 2 commits into
DasBasti:mainfrom
jusii:feat/journal-page-loop

Conversation

@jusii

@jusii jusii commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Builds on the trip-journal foundation merged in #194 (same author). That PR added the page-1 fetch; this PR teaches the same method to walk subsequent pages when the cloud reports more entries than fit on page 1.

Summary

account.get_trip_journal previously fetched only pageIndex=1 (server caps at 20 entries) and returned whatever the cloud sent, even when pagination.totleSize reported more trips available. Accounts with deeper history saw silent truncation. This change loops pageIndex=2..N until len(accumulated) >= totleSize, the previous page is short, or the cloud signals end-of-data via code 8153 mid-loop, then returns the page-1 dict with data.list replaced by the merged accumulation and data.pagination.totleSize refreshed. Single-page accounts (the common case) are byte-identical to the prior behaviour.

What's new

  • pysmarthashtag/account.pyget_trip_journal gains two keyword-defaulted kwargs:
    async def get_trip_journal(self, vin, page_size=20, window_days=14,
                                raise_on_truncation=False, page_gap_seconds=0.0) -> dict
    
    Positional shape unchanged. Loops pages, merges data.list preserving the cloud's desc-by-time ordering across pages.
  • New private helper _fetch_journal_page factored from the old single-page body. Same 3-retry semantics for SmartTokenRefreshNecessary / SmartHumanCarConnectionError as before.
  • Module-level helpers _is_benign_empty, _unwrap_journal_page and constant _BENIGN_EMPTY_CODE = "8153".
  • pysmarthashtag/models.py — new JournalTruncationError(SmartAPIError). Subclasses SmartAPIError so existing broad-except handlers still catch it; the distinct type lets backfill callers distinguish "transient cloud hiccup" from "page-loop terminated short" without string-matching messages.

Behaviour

Loop termination conditions, evaluated before each pageIndex=N fetch:

  1. len(accumulated) >= totleSize from the page-1 response — stop, return merged.
  2. Previous page was short (last_page_count < page_size) — stop. Cloud has no more entries even if totleSize disagrees.
  3. Mid-loop code == 8153 — treat as end-of-data signal, stop without raising.
  4. Mid-loop non-8153 error — propagate httpx.HTTPStatusError.
  5. Mid-loop empty list (page returns [] but totleSize not yet reached) — stop.

Page-1 8153 still raises httpx.HTTPStatusError. This asymmetry is deliberate: get_vehicles has its own benign-empty handling around the page-1 call, so flipping page-1 semantics would silently shift error-handling responsibility. Mid-loop 8153 is the cloud's documented "no more data" signal, hence treated as a terminator.

raise_on_truncation decision tree when len(accumulated) != totleSize after the loop exits:

  • raise_on_truncation=False (default): WARNING logged with VIN sanitized, partial merged response returned, totleSize preserved from the cloud's reported value.
  • raise_on_truncation=True: JournalTruncationError raised with the accumulated/expected counts in the message.

Page-1 server failure still returns {} — matches the sibling get_vehicle_ota_info pattern and the prior single-page contract. The one subtle delta: page-1 success with empty/None data.list now returns the raw first-page dict instead of entering the loop. TripJournal.from_response in pysmarthashtag/vehicle/journal.py handles missing-list gracefully, so this is functionally equivalent.

Backward compatibility

No caller breakage. Existing call sites passing (vin), (vin, page_size=...), or (vin, page_size=..., window_days=...) work unchanged — the new kwargs are defaulted. Return shape is still dict with code/message/success/data, and data.list + data.pagination.totleSize present. TripJournal.from_response only reads data.list[0] and pagination.totleSize, so the merged shape is invariant. total_trips now reflects the final cloud-reported total rather than the page-1 total — almost always identical; only differs if the cloud rotated total mid-loop, in which case the new value is strictly more correct. get_vehicles' internal call and its existing benign-empty handling (covered by tests/test_journal.py) are untouched. JournalTruncationError is opt-in via raise_on_truncation=True; default callers never see it.

Why these defaults

raise_on_truncation=False: routine polling (consumer coordinators running every 60-300s) cannot afford a hard failure on a partial page — the next iteration picks up the missing trips. A WARNING surfaces the anomaly without breaking the user's setup. Backfill and archive callers — where silent loss IS worse than aborting — opt in with raise_on_truncation=True. This mirrors the existing pattern in get_vehicles, which catches journal errors with _LOGGER.debug(exc_info=True) rather than crashing the refresh for one bad vehicle.

page_gap_seconds=0.0: the SDK is a transport library, not a rate-limiter. Polite-API pacing is policy that belongs to the consumer — coordinators, batch backfill scripts, and interactive CLIs all want different gaps. Defaulting to 0 keeps single-page accounts (the common case) identical to pre-loop behaviour: zero added latency, zero behaviour change. Consumers who want pacing pass an explicit value, matching the pattern used elsewhere in the codebase (no built-in retry backoff in the existing request-retry loops either). The guard if page_gap_seconds > 0 before await asyncio.sleep(...) is technically redundant — asyncio.sleep(0) is a documented event-loop yield — but it avoids the unnecessary scheduling round-trip on the default path.

Test coverage

New file pysmarthashtag/tests/test_journal_pagination.py (260 lines, 7 respx-driven scenarios, every while-loop branch exercised):

  • test_loops_until_totleSize_reached — 2-page loop, asserts merged order preserves cloud desc-by-time ordering across page boundaries and that totleSize matches.
  • test_short_page_stops_loop — short page (1 item with page_size=2) terminates the loop even when totleSize claims more; verifies no pageIndex=2 request is issued.
  • test_mid_loop_8153_breaks_looppageIndex=2 returns code=8153; no exception, returns page-1 accumulation.
  • test_mid_loop_other_error_propagatespageIndex=2 returns code=5000; raises httpx.HTTPStatusError.
  • test_truncation_warn_when_count_disagrees — default behaviour: WARNING logged, partial returned, totleSize=10 preserved in the merged response.
  • test_truncation_raises_when_caller_opts_inraise_on_truncation=True raises JournalTruncationError.
  • test_no_loop_when_first_page_short — page-1 returns 2 of 2 (total reached + short): no pageIndex=2 issued.

Existing tests/test_journal.py continues to cover end-to-end get_vehicles → journal population and the page-1 8153 survival path, both still relevant after the refactor.

Known coverage gaps, called out for transparency: page_gap_seconds is not directly asserted (calls asyncio.sleep straight through); the SmartTokenRefreshNecessary / SmartHumanCarConnectionError retry path inside _fetch_journal_page is unchanged but uncovered (was uncovered before too); grant rotation mid-loop is not explicitly tested but relies on the existing inner retry. Happy to add any of these if reviewers want.

Verification

Summary by CodeRabbit

  • New Features

    • Trip journal fetching now automatically retrieves multi-page results for complete journey histories
    • Added configurable options to control truncation handling and page-request timing
  • Bug Fixes

    • Improved handling of cloud "data unavailable" responses and reconstruction of merged journal results
    • Consistent behavior when mid-stream pages are short or missing
  • Tests

    • Added unit tests covering pagination, truncation, error scenarios, and response normalization

@coderabbitai

coderabbitai Bot commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 56a2721c-158e-4ba8-ab62-95a83054c6c6

📥 Commits

Reviewing files that changed from the base of the PR and between c438bc7 and 1d4dba4.

📒 Files selected for processing (3)
  • pysmarthashtag/account.py
  • pysmarthashtag/models.py
  • pysmarthashtag/tests/test_journal_pagination.py
🚧 Files skipped from review as they are similar to previous changes (3)
  • pysmarthashtag/models.py
  • pysmarthashtag/tests/test_journal_pagination.py
  • pysmarthashtag/account.py

📝 Walkthrough

Walkthrough

Extends SmartAccount.get_trip_journal() to page through journalLogV4 responses, normalizing pages (including misspelled totleSize), treating cloud 8153 as benign end-of-data mid-loop, optionally sleeping between pages, and validating accumulated counts (log WARNING or raise JournalTruncationError).

Changes

Journal Pagination with Truncation Handling

Layer / File(s) Summary
Imports, benign-empty constant, and page unwrapping
pysmarthashtag/account.py
Adds asyncio, extends exception imports, defines _BENIGN_EMPTY_CODE, and implements _unwrap_journal_page() to normalize data.list and parse data.pagination.totleSize.
Updated journal fetch API signature
pysmarthashtag/account.py
Extends SmartAccount.get_trip_journal() signature with raise_on_truncation: bool = False and page_gap_seconds: float = 0.0 and updates the method docstring.
Truncation error exception type
pysmarthashtag/models.py
Adds JournalTruncationError (subclassing SmartAPIError) for accumulated-count vs totleSize mismatch when raise_on_truncation=True.
Pagination loop implementation
pysmarthashtag/account.py
Replaces single-page fetch with a pageIndex loop that accumulates data.list until totleSize is reached or a short page appears, treats mid-loop 8153 as benign end-of-data, optionally sleeps between pages, validates counts (WARNING or raise), and reconstructs merged response envelope.
_fetch_journal_page request refactor
pysmarthashtag/account.py
Renames request result variable to response, logs response.status_code, and parses response.json() for returned data.
Pagination test infrastructure and cases
pysmarthashtag/tests/test_journal_pagination.py
Adds tests and respx dispatcher covering multi-page accumulation, short-page termination, benign 8153 handling, non-8153 error propagation, truncation-warning vs exception, single-request optimization, and totleSize coercion cases.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • DasBasti/pySmartHashtag#194: Prior PR adding get_trip_journal and authorization for journalLogV4; this PR extends that method with multi-page looping and truncation handling.

Poem

🐰 I hop through pages, one by one,
Counting trips beneath the sun,
Eight-one-five-three whispers "end"—it's fine,
I merge the lists and mark the line,
If counts don't match, I voice a chime.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat(journal): page-loop pageIndex=2..N until totleSize / short page' directly describes the main change: implementing multi-page pagination for trip journals using pageIndex parameter, stopping at totleSize or when encountering short pages.
Docstring Coverage ✅ Passed Docstring coverage is 80.95% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 6

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
pysmarthashtag/tests/test_journal_pagination.py (1)

231-261: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add type hints to the test and nested helper.

Both the test function and the nested _dispatch helper are missing type annotations. As per coding guidelines, Python typing is required for function signatures.

✨ Proposed fix
 `@pytest.mark.asyncio`
-async def test_no_loop_when_first_page_short(smart_fixture: respx.Router):
+async def test_no_loop_when_first_page_short(smart_fixture: respx.Router) -> None:
     """If page 1 is already short and totleSize matches, no second page is fetched."""
     page1 = _page_body([_trip(1), _trip(2)], total=2, page_size=20)
 
     captured: list[httpx.Request] = []
 
-    def _dispatch(request: httpx.Request) -> httpx.Response:
+    def _dispatch(request: httpx.Request) -> httpx.Response:
         captured.append(request)

Note: The nested _dispatch already has a return type hint, so only the outer function needs updating.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pysmarthashtag/tests/test_journal_pagination.py` around lines 231 - 261, Add
missing type hints on the test coroutine: change the signature of
test_no_loop_when_first_page_short to include the fixture type and a return
type, e.g. async def test_no_loop_when_first_page_short(smart_fixture:
respx.Router) -> None:. Keep the nested helper _dispatch as-is (it already has
def _dispatch(request: httpx.Request) -> httpx.Response) and do not modify its
return annotation.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pysmarthashtag/account.py`:
- Around line 64-69: In _unwrap_journal_page, handle string-encoded pagination
totals: locate symbols items_raw, pagination, total_raw and total in the
_unwrap_journal_page function and change the total extraction so it accepts str
as well as int/float (e.g., treat total_raw if isinstance(total_raw, (int,
float, str))), then convert to int with a safe parse (wrap int(total_raw) in
try/except catching ValueError/TypeError and fall back to None) so values like
"100" become 100 while preserving None on invalid input.

In `@pysmarthashtag/tests/test_journal_pagination.py`:
- Around line 218-229: Add a return type annotation to the test function
signature: update the async def
test_truncation_raises_when_caller_opts_in(smart_fixture: respx.Router) to
include -> None so the signature becomes async def
test_truncation_raises_when_caller_opts_in(smart_fixture: respx.Router) -> None,
keeping the rest of the body unchanged; this satisfies the project typing
guideline for test functions.
- Around line 170-190: The test function test_mid_loop_other_error_propagates is
missing a return type annotation; add the Python typing return of None to its
signature (i.e., change "async def
test_mid_loop_other_error_propagates(smart_fixture: respx.Router):" to include
"-> None") so the function signature follows project typing guidelines and tools
like mypy; ensure only the signature is changed and no runtime behavior is
altered.
- Around line 113-139: The test function test_short_page_stops_loop is missing a
type hint for its smart_fixture parameter; update the function signature to
annotate smart_fixture with the respx.Router type (i.e., smart_fixture:
respx.Router) so it matches typing guidelines and static checks, and ensure
respx is imported where tests are defined if not already.
- Around line 141-168: The test function test_mid_loop_8153_breaks_loop
currently lacks a return type annotation; update its signature to include an
explicit return type (async def test_mid_loop_8153_breaks_loop(smart_fixture:
respx.Router) -> None:) so it conforms to the typing guideline; verify the
nested helper _dispatch already has its parameter and return types (request:
httpx.Request) -> httpx.Response and only adjust it if any annotations are
missing.
- Around line 192-216: Add explicit typing for the test's return type by
annotating the function signature of test_truncation_warn_when_count_disagrees
to return None (i.e., add "-> None"); keep the existing parameter annotations
smart_fixture: respx.Router and caplog: pytest.LogCaptureFixture as-is and
ensure any required test typing imports are available in the module.

---

Outside diff comments:
In `@pysmarthashtag/tests/test_journal_pagination.py`:
- Around line 231-261: Add missing type hints on the test coroutine: change the
signature of test_no_loop_when_first_page_short to include the fixture type and
a return type, e.g. async def test_no_loop_when_first_page_short(smart_fixture:
respx.Router) -> None:. Keep the nested helper _dispatch as-is (it already has
def _dispatch(request: httpx.Request) -> httpx.Response) and do not modify its
return annotation.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e360742e-133a-45d0-8a05-e956ef0bc19a

📥 Commits

Reviewing files that changed from the base of the PR and between a49f0a6 and c438bc7.

📒 Files selected for processing (3)
  • pysmarthashtag/account.py
  • pysmarthashtag/models.py
  • pysmarthashtag/tests/test_journal_pagination.py

Comment thread pysmarthashtag/account.py
Comment thread pysmarthashtag/tests/test_journal_pagination.py
Comment on lines +141 to +168
@pytest.mark.asyncio
async def test_mid_loop_8153_breaks_loop(smart_fixture: respx.Router):
"""Mid-loop 8153 is benign end-of-data, not an error.

Page 2's ``code: 8153`` ("data unavailable") breaks the loop and
returns what's accumulated; the caller does not see an exception.
"""
page1 = _page_body([_trip(1), _trip(2)], total=10, page_size=2)

def _dispatch(request: httpx.Request) -> httpx.Response:
qs = parse_qs(urlparse(str(request.url)).query)
page_index = int((qs.get("pageIndex") or ["0"])[0])
if page_index == 1:
return httpx.Response(200, json=page1)
return httpx.Response(200, json={"code": "8153", "message": "Connection interruption"})

for base in (API_BASE_URL, API_BASE_URL_V2):
smart_fixture.get(
re.compile(re.escape(base + _JOURNAL_PATH + "TestVIN0000000001") + r".*")
).mock(side_effect=_dispatch)

account = await prepare_account_with_vehicles()
# No raise — 8153 mid-loop is benign end-of-data.
merged = await account.get_trip_journal("TestVIN0000000001", page_size=2)

# Accumulated only page 1's 2 entries; page 2 was 8153.
assert len(merged["data"]["list"]) == 2

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add type hints to the test and nested helper.

Both the test function and the nested _dispatch helper are missing type annotations. As per coding guidelines, Python typing is required for function signatures.

✨ Proposed fix
 `@pytest.mark.asyncio`
-async def test_mid_loop_8153_breaks_loop(smart_fixture: respx.Router):
+async def test_mid_loop_8153_breaks_loop(smart_fixture: respx.Router) -> None:
     """Mid-loop 8153 is benign end-of-data, not an error.
 
     Page 2's ``code: 8153`` ("data unavailable") breaks the loop and
     returns what's accumulated; the caller does not see an exception.
     """
     page1 = _page_body([_trip(1), _trip(2)], total=10, page_size=2)
 
-    def _dispatch(request: httpx.Request) -> httpx.Response:
+    def _dispatch(request: httpx.Request) -> httpx.Response:
         qs = parse_qs(urlparse(str(request.url)).query)

Note: The nested _dispatch already has a return type hint, so only the outer function needs updating.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pysmarthashtag/tests/test_journal_pagination.py` around lines 141 - 168, The
test function test_mid_loop_8153_breaks_loop currently lacks a return type
annotation; update its signature to include an explicit return type (async def
test_mid_loop_8153_breaks_loop(smart_fixture: respx.Router) -> None:) so it
conforms to the typing guideline; verify the nested helper _dispatch already has
its parameter and return types (request: httpx.Request) -> httpx.Response and
only adjust it if any annotations are missing.

Source: Coding guidelines

Comment on lines +170 to +190
@pytest.mark.asyncio
async def test_mid_loop_other_error_propagates(smart_fixture: respx.Router):
"""A non-8153 cloud error mid-loop propagates as ``HTTPStatusError``."""
page1 = _page_body([_trip(1), _trip(2)], total=10, page_size=2)

def _dispatch(request: httpx.Request) -> httpx.Response:
qs = parse_qs(urlparse(str(request.url)).query)
page_index = int((qs.get("pageIndex") or ["0"])[0])
if page_index == 1:
return httpx.Response(200, json=page1)
return httpx.Response(200, json={"code": "5000", "message": "Internal server error"})

for base in (API_BASE_URL, API_BASE_URL_V2):
smart_fixture.get(
re.compile(re.escape(base + _JOURNAL_PATH + "TestVIN0000000001") + r".*")
).mock(side_effect=_dispatch)

account = await prepare_account_with_vehicles()
with pytest.raises(httpx.HTTPStatusError):
await account.get_trip_journal("TestVIN0000000001", page_size=2)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add type hint for the test function.

The test function is missing a return type annotation. As per coding guidelines, Python typing is required for function signatures.

✨ Proposed fix
 `@pytest.mark.asyncio`
-async def test_mid_loop_other_error_propagates(smart_fixture: respx.Router):
+async def test_mid_loop_other_error_propagates(smart_fixture: respx.Router) -> None:
     """A non-8153 cloud error mid-loop propagates as ``HTTPStatusError``."""
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pysmarthashtag/tests/test_journal_pagination.py` around lines 170 - 190, The
test function test_mid_loop_other_error_propagates is missing a return type
annotation; add the Python typing return of None to its signature (i.e., change
"async def test_mid_loop_other_error_propagates(smart_fixture: respx.Router):"
to include "-> None") so the function signature follows project typing
guidelines and tools like mypy; ensure only the signature is changed and no
runtime behavior is altered.

Source: Coding guidelines

Comment on lines +192 to +216
@pytest.mark.asyncio
async def test_truncation_warn_when_count_disagrees(
smart_fixture: respx.Router, caplog: pytest.LogCaptureFixture
):
"""Truncation logs a WARNING by default; loop returns the partial result.

Routine polls keep going so the next iteration can pick up the
missing trips, instead of failing the whole refresh on a mismatch.
"""
# Cloud reports totleSize=10 but only delivers 1 short page → mismatch.
page1 = _page_body([_trip(1)], total=10, page_size=2)
_install_paginated_routes(smart_fixture, {1: page1})

account = await prepare_account_with_vehicles()
with caplog.at_level(logging.WARNING, logger="pysmarthashtag.account"):
merged = await account.get_trip_journal("TestVIN0000000001", page_size=2)

assert len(merged["data"]["list"]) == 1
assert merged["data"]["pagination"]["totleSize"] == 10
assert any(
"page-loop truncation" in record.message
for record in caplog.records
if record.levelno == logging.WARNING
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add type hints for the test function parameters.

The test function is missing type annotations for its parameters. As per coding guidelines, Python typing is required for function signatures.

✨ Proposed fix
 `@pytest.mark.asyncio`
 async def test_truncation_warn_when_count_disagrees(
-    smart_fixture: respx.Router, caplog: pytest.LogCaptureFixture
+    smart_fixture: respx.Router, caplog: pytest.LogCaptureFixture
-):
+) -> None:
     """Truncation logs a WARNING by default; loop returns the partial result.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pysmarthashtag/tests/test_journal_pagination.py` around lines 192 - 216, Add
explicit typing for the test's return type by annotating the function signature
of test_truncation_warn_when_count_disagrees to return None (i.e., add "->
None"); keep the existing parameter annotations smart_fixture: respx.Router and
caplog: pytest.LogCaptureFixture as-is and ensure any required test typing
imports are available in the module.

Source: Coding guidelines

Comment on lines +218 to +229
@pytest.mark.asyncio
async def test_truncation_raises_when_caller_opts_in(smart_fixture: respx.Router):
"""``raise_on_truncation=True`` upgrades the WARNING to a hard failure."""
page1 = _page_body([_trip(1)], total=10, page_size=2)
_install_paginated_routes(smart_fixture, {1: page1})

account = await prepare_account_with_vehicles()
with pytest.raises(JournalTruncationError):
await account.get_trip_journal(
"TestVIN0000000001", page_size=2, raise_on_truncation=True
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add type hint for the test function.

The test function is missing a return type annotation. As per coding guidelines, Python typing is required for function signatures.

✨ Proposed fix
 `@pytest.mark.asyncio`
-async def test_truncation_raises_when_caller_opts_in(smart_fixture: respx.Router):
+async def test_truncation_raises_when_caller_opts_in(smart_fixture: respx.Router) -> None:
     """``raise_on_truncation=True`` upgrades the WARNING to a hard failure."""
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pysmarthashtag/tests/test_journal_pagination.py` around lines 218 - 229, Add
a return type annotation to the test function signature: update the async def
test_truncation_raises_when_caller_opts_in(smart_fixture: respx.Router) to
include -> None so the signature becomes async def
test_truncation_raises_when_caller_opts_in(smart_fixture: respx.Router) -> None,
keeping the rest of the body unchanged; this satisfies the project typing
guideline for test functions.

Source: Coding guidelines

The previous get_trip_journal hard-coded pageIndex=1 and silently
dropped trips beyond the first page when totleSize > page_size. Refactor
the request into a private _fetch_journal_page helper and wrap it in a
loop that fetches pages 2, 3, ... until either:

  * the cloud-reported totleSize is reached, OR
  * the previous page came back short (cloud's "no more pages" signal)

Mid-loop code=8153 ("data unavailable") is treated as benign
end-of-data: break the loop and return what's accumulated. Page 1's
8153 still propagates as HTTPStatusError so existing callers with their
own benign-empty handling (e.g. get_vehicles) keep working unchanged.

After the loop, if the accumulated count disagrees with totleSize:

  * default (raise_on_truncation=False): log a WARNING with both
    numbers and return the partial list — routine polls keep going so
    the next iteration can pick up the missing trips.
  * raise_on_truncation=True: raise JournalTruncationError instead.
    Backfill / archive callers opt into the hard failure since
    silent data loss is worse than a hard failure that forces an
    operator to look.

The returned dict preserves the original first-page response shape
(code/message/data) but with data.list containing the merged entries
from every page; data.pagination.totleSize carries the cloud's most
recent reported total. Existing TripJournal.from_response works
unchanged since it only reads data.list[0] + totleSize.

Adds page_gap_seconds kwarg (default 0.0) so polite-API callers can
insert a sleep between fetches without monkey-patching.
@jusii jusii force-pushed the feat/journal-page-loop branch from c438bc7 to 9beaeb9 Compare June 12, 2026 15:49
@jusii

jusii commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @coderabbitai!

Applied (9beaeb9):

  • pysmarthashtag/account.py:67-68 — hardened _unwrap_journal_page to accept stringified totleSize ("3", " 3 "), reject bool (which is an int subclass and would otherwise coerce True/False to 1/0), reject non-digit strings like "3.0", and preserve the existing len(items)-equivalent fallback path. The verbatim diff in the review used else: total = None, which keeps the current fallback semantics (caller treats None as "no total reported"), but the new branch widens the happy path so a stringified count from the cloud no longer trips the loop's "more pages remain" termination check at the bottom of get_trip_journal.
  • Added a parametrized test in pysmarthashtag/tests/test_journal_pagination.py covering "3", whitespace-padded " 3 ", "3.0" (rejected → None), None (→ None), and True (bool rejected → None). Locks both the new coercion and the bool guard.

Declining (convention):

The -> None annotations on the new async tests in pysmarthashtag/tests/test_journal_pagination.py — flagged at :139, :168, :190, :216, :229 — match the established local style: 15/15 existing async tests across test_actions.py, test_account.py, test_journal.py, and test_journal_control.py omit the return annotation. Matching convention inside a feature PR is the right call here; mixing styles would be the actual inconsistency. Happy to do a suite-wide -> None annotation pass as a separate PR if @DasBasti prefers that style going forward.

Ruff clean, full suite passes.

@coderabbitai

coderabbitai Bot commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

@jusii Thanks for the thorough rundown!

The _unwrap_journal_page hardening looks solid — the bool guard (via isinstance(raw, bool) before the int check) and the str.strip().isdigit() path together cover exactly the edge cases that would silently mis-terminate the page loop, and the parametrized test pins all five variants cleanly.

Your convention argument on -> None is well-taken. With 15/15 existing async tests across the suite omitting the return annotation, adding it only to the new file would be the inconsistency — not the other way around. Withdrawing that suggestion; a suite-wide annotation pass as a standalone PR if the maintainer ever wants to adopt that style is the right way to handle it.

🐇 ✨


✏️ Learnings added
Learnt from: jusii
URL: https://github.com/DasBasti/pySmartHashtag/pull/205

Timestamp: 2026-06-12T15:51:45.115Z
Learning: In pysmarthashtag (GitHub: DasBasti/pySmartHashtag), the established project convention is to omit `-> None` return type annotations on async test functions. All 15+ async tests across test_actions.py, test_account.py, test_journal.py, and test_journal_control.py follow this style. Do not flag missing `-> None` annotations on async test functions in this codebase.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

@DasBasti DasBasti merged commit 6de13a1 into DasBasti:main Jun 13, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants