Fix whitespace-only bounty sort defaults#470
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe ChangesWhitespace-only sort parameter handling
Possibly related PRs
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 04b9fcb3-38e8-45f2-be79-fab0bf9e81b8
📒 Files selected for processing (2)
app/bounty_sorting.pytests/test_bounty_pages.py
weilixiong
left a comment
There was a problem hiding this comment.
LGTM — whitespace-only sort defaults, well tested.
Verified:
- python -m py_compile app/bounty_sorting.py: OK
- pytest tests/test_bounty_pages.py -k bounties_page_and_api_sort: 1/1 passed (1.71s)
- git diff: 2 files, +17/-1 lines
- Whitespace " " correctly falls through to "newest" default
- Test covers: API whitespace sort → "newest", page HTML whitespace sort → "newest" selected, page sort order correct
jtc268
left a comment
There was a problem hiding this comment.
Evidence checked:
- Inspected app/bounty_sorting.py: normalize_bounty_sort now trims/lowercases, defaults None/empty/whitespace to newest, and still rejects non-empty invalid values through BOUNTY_SORT_ERROR.
-
- Traced callers in app/bounty_api.py and app/public_routes.py so the same normalization reaches /api/v1/bounties and /bounties.
-
-
- Inspected tests/test_bounty_pages.py: whitespace-only sort now covers API order plus public page order and selected option.
-
-
-
-
- Checked the prior CodeRabbit inline thread; the missing order assertion is present on the latest head and the thread is resolved.
-
-
-
-
-
-
- Current GitHub checks are green: CI success and CodeRabbit success.
Local validation:
- Current GitHub checks are green: CI success and CodeRabbit success.
-
-
-
- ./.venv/bin/python -m pytest tests/test_bounty_pages.py::test_bounties_page_and_api_sort_public_rows tests/test_public_routes.py -q -> 2 passed
-
- ./.venv/bin/python -m ruff check app/bounty_sorting.py tests/test_bounty_pages.py -> passed
-
-
- ./.venv/bin/python -m mypy app -> success
-
-
-
-
- git diff --check origin/main...HEAD -> clean
No requested changes from me.
- git diff --check origin/main...HEAD -> clean
-
-
rebel117
left a comment
There was a problem hiding this comment.
Reviewed PR #470 at current head for Bounty #447.
Checked files: app/bounty_sorting.py, tests/test_bounty_pages.py.
Evidence:
- Inspected the diff: the old code
(sort or "newest").strip().lower()would return"newest"forNonebut would pass a whitespace-only string through to the validation check (since.strip()turns" "to"", and"" not in BOUNTY_SORT_OPTIONSwould raiseValueError). - The new code splits into: default empty string → strip → if empty, return "newest" → otherwise validate against options. This correctly treats whitespace-only sort as "use default" rather than rejecting it.
- Confirmed the test additions: API route returns 200 with default ordering for
sort=" ", HTML page renders with"newest" selectedoption. - Cross-referenced with
BOUNTY_SORT_OPTIONSinapp/bounty_sorting.pyto confirm the valid set is unchanged. - Checked that the test uses
params={"sort": " "}which preserves the whitespace through query-string parsing.
Verdict: Small but correct fix. Whitespace-only sort values should fall back to default rather than error — this matches how most query parameter handlers work. Tests cover both API and HTML paths. No blockers.
Summary
sortquery values as omitted/defaultnewest.400response.Bounty #406
Evidence
./.venv/bin/python -m pytest tests/test_bounty_pages.py::test_bounties_page_and_api_sort_public_rows -qreturned400for/bountieswith a whitespace-onlysortquery.GET https://api.mrwk.ltclab.site/api/v1/bounties/66/attempts?include_expired=falsereturnedattempts: [].Validation
./.venv/bin/python -m pytest tests/test_bounty_pages.py::test_bounties_page_and_api_sort_public_rows -q-> 1 passed./.venv/bin/python -m pytest tests/test_bounty_pages.py tests/test_bounty_api_routes.py -q-> 14 passed./.venv/bin/python -m pytest -q-> 414 passed./.venv/bin/python -m ruff check .-> passed./.venv/bin/python -m ruff format --check .-> 79 files already formatted./.venv/bin/python -m mypy app-> successgit diff --check-> cleangitleaks detect --no-banner --redact --source . --log-opts upstream/main..HEAD --exit-code 1-> no leaks foundSummary by CodeRabbit
Bug Fixes
Tests