Validate project roster people#296
Conversation
…-picker # Conflicts: # tests/unit/test_backend_api.py
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis PR extends project roster management across the dashboard and backend to support validated ChangesProject Roster Member Management with Verified Candidate Selection
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
This PR tightens project roster writes by requiring dashboard users to select a validated @508.dev person candidate (from CRM/ERPNext lookups) when adding ERP roster users, and adds dashboard + backend support for removing ERP roster users and local historical roster members.
Changes:
- Require
candidate_idwhen adding ERP project users; add a new dashboard endpoint to fetch validated roster candidates. - Add backend endpoints and shared helpers for removing ERP roster users (via ERPNext Project.users updates) and deleting local historical roster members.
- Update admin dashboard UI to use verified-candidate selection for adds and to provide “Remove” actions with confirmation; expand unit tests for new behaviors.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_erpnext_client.py | Adds unit coverage for ERPNext Project.users removal behavior. |
| tests/unit/test_backend_api.py | Updates/adds API tests for candidate validation and new remove endpoints. |
| packages/shared/src/five08/projects.py | Adds remove_project_roster_member() helper for deleting local roster rows. |
| packages/shared/src/five08/clients/erpnext.py | Adds remove_project_user() to update ERPNext Project.users by removing a user. |
| apps/api/src/five08/backend/api.py | Adds candidate lookup endpoint, enforces candidate validation for adds, and introduces removal endpoints. |
| apps/admin_dashboard/src/main.tsx | Updates UI flow to require verified-candidate selection and adds removal actions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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)
apps/admin_dashboard/src/main.tsx (1)
1010-1011: ⚡ Quick winUse
showErrorfor remove failures to preserve diagnostic context.Line 1010 and Line 1076 bypass the shared API error path (
showError), so DEV diagnostics (DashboardDevErrors) won’t capture these failures consistently.Suggested patch
- } catch (error) { - showToast(error instanceof Error ? error.message : "Unable to remove project user", "error") + } catch (error) { + showError(error, "Unable to remove project user") return false } finally { setBusy(`project:${projectId}:user`, false) } @@ - } catch (error) { - showToast( - error instanceof Error ? error.message : "Unable to remove historical member", - "error", - ) + } catch (error) { + showError(error, "Unable to remove historical member") return false } finally { setBusy(`project:${projectId}:historical`, false) }Also applies to: 1076-1079
🤖 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 `@apps/admin_dashboard/src/main.tsx` around lines 1010 - 1011, Replace the direct showToast calls used for the remove-project-user failure path with the shared showError helper so the failure is routed through the DashboardDevErrors diagnostics path; specifically, where showToast(error instanceof Error ? error.message : "Unable to remove project user", "error") is used (in the remove-user handler in main.tsx), call showError with the Error object or with both a message and the Error (e.g., showError(error) or showError("Unable to remove project user", error)) and keep the existing return false; do the same for the other occurrence around the 1076–1079 remove flow.
🤖 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 `@packages/shared/src/five08/clients/erpnext.py`:
- Around line 286-315: remove_project_user currently returns the original
project when no user matched, making callers unable to distinguish "not found"
from a successful removal; modify remove_project_user to raise an explicit
ERPNextAPIError (e.g., ERPNextAPIError(f"Project user not found: {user} for
project {project_id}")) when removed is False after iterating, while keeping the
existing behavior of calling update_project(project_id, {"users": next_users})
when a removal did occur; keep get_project and update_project calls unchanged
and include the normalized_user and project_id in the error text for clear
diagnostics.
In `@packages/shared/src/five08/projects.py`:
- Around line 819-822: The DELETE currently interpolates Python into SQL via the
f-string using the variable where_clause; replace this with a static
parameterized SQL string and pass values via the DB parameter API instead of
formatting into the query. Locate the code that constructs the DELETE for
project_roster_members (the usage of where_clause in
packages.shared.src.five08.projects.py) and rewrite it to build a fixed SQL
statement (e.g., "DELETE FROM project_roster_members WHERE <column>=%s" or
similar) and supply the corresponding parameters in the execute call rather than
injecting where_clause via f-string.
---
Nitpick comments:
In `@apps/admin_dashboard/src/main.tsx`:
- Around line 1010-1011: Replace the direct showToast calls used for the
remove-project-user failure path with the shared showError helper so the failure
is routed through the DashboardDevErrors diagnostics path; specifically, where
showToast(error instanceof Error ? error.message : "Unable to remove project
user", "error") is used (in the remove-user handler in main.tsx), call showError
with the Error object or with both a message and the Error (e.g.,
showError(error) or showError("Unable to remove project user", error)) and keep
the existing return false; do the same for the other occurrence around the
1076–1079 remove flow.
🪄 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: 161679fd-e7aa-488a-8b2e-542ef40a02ce
📒 Files selected for processing (11)
apps/admin_dashboard/src/main.tsxapps/api/src/five08/backend/api.pyapps/api/src/five08/backend/static/dashboard/.vite/manifest.jsonapps/api/src/five08/backend/static/dashboard/assets/index--kTKJbUI.jsapps/api/src/five08/backend/static/dashboard/assets/index-E1H3CuXy.cssapps/api/src/five08/backend/static/dashboard/assets/index-WxptdPpM.jsapps/api/src/five08/backend/static/dashboard/index.htmlpackages/shared/src/five08/clients/erpnext.pypackages/shared/src/five08/projects.pytests/unit/test_backend_api.pytests/unit/test_erpnext_client.py
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10884ecf9f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a075577b7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) | ||
| elif exc.code == "person_not_found": | ||
| detail = ( | ||
| f'No active CRM person or ERPNext user with a @508.dev email matched "{user}". ' |
| return JSONResponse( | ||
| {"error": "erpnext_project_user_remove_failed", "detail": str(exc)}, |
Summary
Verification
uv run ruff check apps/api/src/five08/backend/api.py tests/unit/test_backend_api.py packages/shared/src/five08/projects.py packages/shared/src/five08/clients/erpnext.py tests/unit/test_erpnext_client.pyuv run pytest tests/unit/test_backend_api.py::test_dashboard_add_project_user_uses_erpnext_record_id tests/unit/test_backend_api.py::test_dashboard_add_project_user_requires_verified_candidate tests/unit/test_backend_api.py::test_dashboard_project_member_candidates_returns_verified_508_people tests/unit/test_backend_api.py::test_dashboard_add_project_historical_member_returns_person_not_found_detail tests/unit/test_backend_api.py::test_dashboard_remove_project_user_updates_erp_roster tests/unit/test_backend_api.py::test_dashboard_remove_project_historical_member_deletes_local_roster tests/unit/test_backend_api.py::test_project_roster_user_candidates_require_508_email tests/unit/test_erpnext_client.py::test_remove_project_user_updates_project_usersbun run lintinapps/admin_dashboardSummary by CodeRabbit
Release Notes