cal/major-domo-database
1
0
Fork 0
Code Issues 25 Pull Requests 25 Actions Packages Projects Releases Wiki Activity

fix: preserve total_count in get_totalstats instead of overwriting with page length (#101) #102

Open
Claude wants to merge 1 commits from issue/101-fieldingstats-get-totalstats-total-count-overwritt into main
pull from: issue/101-fieldingstats-get-totalstats-total-count-overwritt
merge into: cal:main
Conversation 0 Commits 1 Files Changed 1 -1
Claude commented 2026-04-01 23:01:57 +00:00
Collaborator
Copy Link

Closes #101

Problem

get_totalstats correctly computed total_count = all_stats.count() before pagination (line 247) and set it in return_stats["count"] at initialization (line 250), but then overwrote it at the end:

return_stats["count"] = len(return_stats["stats"])

This replaced the pre-pagination total with the current page's post-filter length, making pagination metadata incorrect.

Fix

Removed the return_stats["count"] = len(return_stats["stats"]) line. The total_count value (set before pagination is applied) is already correctly placed in return_stats["count"] at initialization and is preserved throughout.

Files Changed

  • app/routers_v3/fieldingstats.py — removed 1 line
Closes #101 ## Problem `get_totalstats` correctly computed `total_count = all_stats.count()` before pagination (line 247) and set it in `return_stats["count"]` at initialization (line 250), but then overwrote it at the end: ```python return_stats["count"] = len(return_stats["stats"]) ``` This replaced the pre-pagination total with the current page's post-filter length, making pagination metadata incorrect. ## Fix Removed the `return_stats["count"] = len(return_stats["stats"])` line. The `total_count` value (set before pagination is applied) is already correctly placed in `return_stats["count"]` at initialization and is preserved throughout. ## Files Changed - `app/routers_v3/fieldingstats.py` — removed 1 line
Claude added 1 commit 2026-04-01 23:01:58 +00:00
fix: preserve total_count in get_totalstats instead of overwriting with page length (#101)
All checks were successful
Build Docker Image / build (pull_request) Successful in 1m57s
Details
a68e4216d6 
Closes #101

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude added the
ai-reviewing
 label 2026-04-01 23:15:32 +00:00
Claude reviewed 2026-04-01 23:16:51 +00:00
Claude left a comment
Author
Collaborator
Copy Link

AI Code Review

Files Reviewed

  • app/routers_v3/fieldingstats.py (modified)

Findings

Correctness

  • Fix is correct. total_count = all_stats.count() is called at line 247 before pagination (all_stats.offset(offset).limit(limit) at line 248), and return_stats["count"] is initialized to that pre-pagination value at line 250. Removing the overwrite on the old line 284 preserves the semantically correct value.
  • This mirrors the fix already applied to get_fieldingstats (the non-totals endpoint in the same file), which correctly sets total_count before the limit and never overwrites it. PR #99 fixed get_fieldingstats; this PR completes the same fix for get_totalstats.
  • Empty result edge case is safe: total_count = 0 is set at line 247 → propagated to count: 0 at line 250 → loop body never executes → count: 0 preserved correctly after the fix.
  • One pre-existing observation (out of scope, not blocking): The Python-level filter inside the loop at line 253 (if x.sum_xch + x.sum_sbc <= 0: continue) is effectively dead code when min_ch >= 1 (the default), because the HAVING sum_xch >= min_ch clause already ensures sum_xch >= 1, making sum_xch + sum_sbc > 0 always true. In the edge case where min_ch=0 is passed, the Python filter could make the page's actual stats count lower than limit without that being reflected in count. This is not introduced by this PR.

Security

  • No security implications.

Style & Conventions

  • Single-line deletion, no style issues. Consistent with the existing pattern in get_fieldingstats.

Suggestions

  • None.

Verdict: APPROVED

Minimal, correct one-line fix. The pre-pagination total_count was already correctly computed and stored — the overwrite was the sole defect. Fix is consistent with the companion fix already applied to get_fieldingstats in PR #99.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/routers_v3/fieldingstats.py` (modified) ### Findings #### Correctness - Fix is correct. `total_count = all_stats.count()` is called at line 247 *before* pagination (`all_stats.offset(offset).limit(limit)` at line 248), and `return_stats["count"]` is initialized to that pre-pagination value at line 250. Removing the overwrite on the old line 284 preserves the semantically correct value. - This mirrors the fix already applied to `get_fieldingstats` (the non-totals endpoint in the same file), which correctly sets `total_count` before the limit and never overwrites it. PR #99 fixed `get_fieldingstats`; this PR completes the same fix for `get_totalstats`. - Empty result edge case is safe: `total_count = 0` is set at line 247 → propagated to `count: 0` at line 250 → loop body never executes → `count: 0` preserved correctly after the fix. - One pre-existing observation (out of scope, not blocking): The Python-level filter inside the loop at line 253 (`if x.sum_xch + x.sum_sbc <= 0: continue`) is effectively dead code when `min_ch >= 1` (the default), because the `HAVING sum_xch >= min_ch` clause already ensures `sum_xch >= 1`, making `sum_xch + sum_sbc > 0` always true. In the edge case where `min_ch=0` is passed, the Python filter could make the page's actual `stats` count lower than `limit` without that being reflected in `count`. This is not introduced by this PR. #### Security - No security implications. #### Style & Conventions - Single-line deletion, no style issues. Consistent with the existing pattern in `get_fieldingstats`. #### Suggestions - None. ### Verdict: APPROVED Minimal, correct one-line fix. The pre-pagination `total_count` was already correctly computed and stored — the overwrite was the sole defect. Fix is consistent with the companion fix already applied to `get_fieldingstats` in PR #99. --- *Automated review by Claude PR Reviewer*
Claude added
ai-reviewed
 and removed
ai-reviewing
 labels 2026-04-01 23:17:25 +00:00
All checks were successful
Build Docker Image / build (pull_request) Successful in 1m57s
Details
This pull request doesn't have enough approvals yet. 0 of 1 approvals granted.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin issue/101-fieldingstats-get-totalstats-total-count-overwritt:issue/101-fieldingstats-get-totalstats-total-count-overwritt
git checkout issue/101-fieldingstats-get-totalstats-total-count-overwritt
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
ai-changes-requested

   
ai-pr-opened

   
ai-reviewed

   
ai-reviewing

   
ai-reviewing

   
ai-working
No Label
ai-changes-requested
ai-pr-opened
ai-reviewed
ai-reviewing
ai-reviewing
ai-working
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: cal/major-domo-database#102
No description provided.
Delete Branch "issue/101-fieldingstats-get-totalstats-total-count-overwritt"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?