PY: fix admin error - #16827
Conversation
Signed-off-by: Jin Hai <haijin.chn@gmail.com>
📝 WalkthroughWalkthroughThe admin client now requires a newer ChangesAdmin authentication and account controls
Admin client dependency baseline
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
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. Comment |
dosubot
Bot
added
the
size:S
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
admin/server/routes.py (1)
156-157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the new self-status rejection path.
This newly added flow currently returns without logging. Add a structured warning/info log containing the actor and target identifiers according to the repository’s Python logging guideline, while avoiding unnecessary sensitive data.
🤖 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 `@admin/server/routes.py` around lines 156 - 157, The self-status rejection in the current-user status update handler returns without an audit log. Add a structured warning or info log immediately before the error response, including the actor and target identifiers using the repository’s Python logging conventions while excluding unnecessary sensitive data.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 `@admin/client/pyproject.toml`:
- Line 11: Regenerate admin/client/uv.lock with `uv lock` so the beartype
resolution and metadata satisfy the updated constraint in pyproject.toml, then
verify the lockfile reflects a version at least 0.22.9.
In `@admin/server/routes.py`:
- Around line 156-157: Update error_response to accept a separate HTTP-status
parameter defaulting to its existing status, then use that parameter when
constructing the response. In the current-user check in the relevant route, call
error_response with both the JSON code and HTTP status set to 409 so the client
receives an actual HTTP 409 response.
---
Nitpick comments:
In `@admin/server/routes.py`:
- Around line 156-157: The self-status rejection in the current-user status
update handler returns without an audit log. Add a structured warning or info
log immediately before the error response, including the actor and target
identifiers using the repository’s Python logging conventions while excluding
unnecessary sensitive data.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 44f9fb0a-d24e-4d29-b891-4c8496aa4892
📒 Files selected for processing (3)
admin/client/pyproject.tomladmin/server/auth.pyadmin/server/routes.py
| dependencies = [ | ||
| "requests>=2.30.0,<3.0.0", | ||
| "beartype>=0.20.0,<1.0.0", | ||
| "beartype>=0.22.9,<1.0.0", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚖️ Poor tradeoff
Lockfile is out of sync with the new beartype constraint.
admin/client/uv.lock currently pins beartype at 0.22.6, which no longer satisfies the new >=0.22.9 floor. The lockfile must be regenerated (uv lock) so the resolved version and metadata reflect the tightened requirement. Otherwise CI or local installs that rely on the lockfile will either fail or install a version that violates the declared constraint.
As per coding guidelines, "Run the narrowest relevant test, lint, or build command after a change." — regenerating the lockfile is the equivalent build step here.
🔧 Regenerate the lockfile
cd admin/client && uv lock🤖 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 `@admin/client/pyproject.toml` at line 11, Regenerate admin/client/uv.lock with
`uv lock` so the beartype resolution and metadata satisfy the updated constraint
in pyproject.toml, then verify the lockfile reflects a version at least 0.22.9.
| if current_user.email == username: | ||
| return error_response(f"can't alter current user status: {username}", 409) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Return an actual HTTP 409 response.
error_response(..., 409) only sets the JSON code; admin/server/responses.py:23-24 always returns HTTP 400. This violates the intended conflict response and can break clients branching on the HTTP status.
Add a separate HTTP-status parameter to error_response while preserving its existing default, then call it with both body code and HTTP status set to 409.
🤖 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 `@admin/server/routes.py` around lines 156 - 157, Update error_response to
accept a separate HTTP-status parameter defaulting to its existing status, then
use that parameter when constructing the response. In the current-user check in
the relevant route, call error_response with both the JSON code and HTTP status
set to 409 so the client receives an actual HTTP 409 response.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #16827 +/- ##
==========================================
- Coverage 94.56% 93.16% -1.40%
==========================================
Files 10 10
Lines 717 717
Branches 118 118
==========================================
- Hits 678 668 -10
- Misses 25 29 +4
- Partials 14 20 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Summary
Sync code from EE