[codex] Add ERPNext engineer onboarding flows#292
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis pull request implements a complete engineer onboarding workflow for ERPNext with optional activity cost configuration. It introduces shared orchestration for creating and linking User/Employee/Supplier records, backend API support for dashboard engineer setup and project roster updates with activity cost details, Discord slash commands for both workflows, and React dashboard components for engineer setup and project roster activity rate management. ChangesEngineer Onboarding and Activity Cost Configuration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 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 docstrings
🧪 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
Adds an end-to-end ERPNext engineer onboarding flow that creates/links User, Employee, and Supplier records, optionally configures project Activity Cost (bill/cost rates), and surfaces the flow in both the dashboard UI/API and Discord Steering Committee commands. Shared orchestration logic lives in a new engineer_onboarding module, backed by new generic CRUD helpers on ERPNextClient and unit tests.
Changes:
- New
five08.engineer_onboardingmodule orchestrating User/Employee/Supplier/Activity Cost setup with duplicate-name detection and @508.dev email enforcement. - Generic
get/create/update_recordandcall_methodhelpers added toERPNextClient. - Dashboard API endpoint + UI panel and a Discord cog with
setup-engineer/add-engineer-to-projectcommands, all role-gated and audited.
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/shared/src/five08/engineer_onboarding.py | New orchestration module for engineer onboarding and project rate config. |
| packages/shared/src/five08/clients/erpnext.py | Adds generic record CRUD + Frappe method helpers used by the new module. |
| apps/api/src/five08/backend/api.py | New dashboard endpoint for engineer setup; extends add-project-user with optional Activity Cost rates. |
| apps/discord_bot/src/five08/discord_bot/cogs/engineer_onboarding.py | New Discord cog exposing engineer setup and project assignment commands. |
| apps/admin_dashboard/src/main.tsx | Adds Engineer Setup panel and roster add form with optional rate fields. |
| apps/api/src/five08/backend/static/dashboard/{index.html,.vite/manifest.json,assets/index-Bu9GOZtL.css} | Rebuilt dashboard bundle assets. |
| tests/unit/test_engineer_onboarding.py | Unit tests covering setup, duplicate-name guard, email validation, and project add with Activity Cost. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: 698b44b77d
ℹ️ 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".
michaelmwu
force-pushed
the
michaelmwu/engineer-onboarding-flow
branch
from
698b44b to
9a29b69
Compare
michaelmwu
force-pushed
the
michaelmwu/engineer-onboarding-flow
branch
from
9a29b69 to
0a1ef92
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a1ef925b7
ℹ️ 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".
michaelmwu
force-pushed
the
michaelmwu/engineer-onboarding-flow
branch
from
0a1ef92 to
6bf1e4f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6bf1e4f13c
ℹ️ 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".
…nboarding-flow # Conflicts: # apps/admin_dashboard/src/main.tsx # apps/api/src/five08/backend/static/dashboard/.vite/manifest.json # apps/api/src/five08/backend/static/dashboard/assets/index-Bu9GOZtL.css # apps/api/src/five08/backend/static/dashboard/index.html
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92456610c3
ℹ️ 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".
| def _is_not_found_error(client: ERPNextClient, _exc: ERPNextAPIError) -> bool: | ||
| return getattr(client, "status_code", None) == 404 |
There was a problem hiding this comment.
_is_not_found_error ignores the raised exception and relies only on client.status_code, which can be stale. In ERPNextClient.request, status_code is only updated after receiving an HTTP response; transport failures (timeouts/TLS/DNS) keep the previous code. If a prior call set status_code to 404, later network errors will be misclassified as "not found" by paths like ensure_user/supplier_by_id, leading onboarding to create new User/Supplier records instead of failing fast, which can introduce duplicate ERP records during transient outages.
Useful? React with 👍 / 👎.
| "source": "dashboard", | ||
| "user": normalized_user, | ||
| "candidate_id": payload.candidate_id, | ||
| "activity_cost": project_user_result.get("activity_cost"), | ||
| "activity_cost_error": project_user_result.get("activity_cost_error"), | ||
| "local_project_id": project_id, |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81797b0669
ℹ️ 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".
| "activity_cost": project_user_result.get("activity_cost"), | ||
| "activity_cost_error": project_user_result.get("activity_cost_error"), | ||
| "local_project_id": project_id, |
There was a problem hiding this comment.
In dashboard_add_project_user_handler, the new partial-success path is surfaced via activity_cost_error, but this audit call still records the action as success, so downstream audit consumers that key on result will miss failed Activity Cost writes even though billing/costing were not applied. When project_user_result contains partial_success/activity_cost_error, record a non-success audit result to keep operational monitoring accurate.
Useful? React with 👍 / 👎.
| except ERPNextAPIError: | ||
| fields.pop("role_profile_name", None) | ||
| fields["roles"] = [{"role": EMPLOYEE_ROLE}] | ||
| return client.create_record("User", fields), True |
There was a problem hiding this comment.
ensure_user catches any ERPNextAPIError from the first create_record("User", ...) attempt and immediately retries with a different payload. This broad retry can turn transient/transport failures into duplicate-create attempts and partial onboarding side effects (e.g., first create may have succeeded server-side, then the fallback attempt fails with duplicate state). Only fallback when the first failure is specifically the role-profile incompatibility case.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9dd63b7a4
ℹ️ 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".
| if not user_exists: | ||
| ensure_no_similar_engineer_name(client, email=email, full_name=full_name) |
There was a problem hiding this comment.
The duplicate-name check is skipped whenever the ERPNext User record already exists, but setup_engineer still resolves/creates Supplier by employee/full name afterward. In the common pre-provisioned-user case (existing User, no Employee yet), this can silently attach the engineer to an unrelated existing Supplier with the same name and append their portal user, creating incorrect cross-linking and access side effects instead of returning a conflict for operator confirmation.
Useful? React with 👍 / 👎.
| except ERPNextAPIError: | ||
| pass |
| const preferredEmailOptions = ["Company Email", "Personal Email", "User ID"] | ||
|
|
||
| function todayInputDate() { | ||
| return new Date().toISOString().slice(0, 10) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@apps/admin_dashboard/src/main.tsx`:
- Line 5404: Remove fabricated defaults and stop sending gender/dateOfBirth when
the operator hasn’t provided them: delete or stop using the hardcoded constant
defaultEmployeeDateOfBirth and the hardcoded gender "Male", and update the
payload construction for EngineerSetupRequest (the form submit/handleSubmit code
that currently includes dateOfBirth and gender on lines around the submission)
to only include dateOfBirth and gender keys if the user explicitly entered
values (e.g., check for non-empty/undefined form fields before adding them to
the request object). Ensure default constants are not used as fallbacks that
cause unedited fields to be submitted.
- Line 5460: The current flow auto-populates dateOfJoining (state managed by
dateOfJoining and setDateOfJoining) with todayInputDate() and then overwrites it
with person.created_at, causing incorrect date_of_joining to be submitted;
change the initial state to empty (null/""), remove or guard the effect that
assigns person.created_at to dateOfJoining unless that value is a verified join
date or the operator explicitly toggles/accepts it in Advanced options, and
update the submit logic so the payload only includes date_of_joining when
dateOfJoining is non-empty/explicitly provided by the user (do not always
include person.created_at).
In `@apps/api/src/five08/backend/api.py`:
- Around line 4453-4461: The audit call is currently persisting potentially
identifying duplicate-match payloads by putting exc.matches into the audit
metadata; update the call to _audit_dashboard_engineer_setup (the
AuditResult.DENIED branch) to stop saving exc.matches directly and instead store
only minimal diagnostics (e.g., "matches_count": len(exc.matches) or a redacted
summary) in the metadata, while leaving the full exc.matches data only in the
HTTP response logic. Modify the metadata dict passed to
_audit_dashboard_engineer_setup to replace "matches": exc.matches with a
non-identifying field such as "matches_count" or a masked summary.
🪄 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: 5ddf5f78-005e-43d5-bf46-2373872534db
📒 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-Cn-kslAB.cssapps/api/src/five08/backend/static/dashboard/assets/index-DuHixsFk.jsapps/api/src/five08/backend/static/dashboard/index.htmlapps/discord_bot/src/five08/discord_bot/cogs/engineer_onboarding.pypackages/shared/src/five08/clients/erpnext.pypackages/shared/src/five08/engineer_onboarding.pytests/unit/test_backend_api.pytests/unit/test_engineer_onboarding.py
✅ Files skipped from review due to trivial changes (1)
- apps/api/src/five08/backend/static/dashboard/.vite/manifest.json
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb5dcc1c21
ℹ️ 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".
| function todayInputDate() { | ||
| return new Date().toISOString().slice(0, 10) |
There was a problem hiding this comment.
todayInputDate() derives the default from new Date().toISOString().slice(0, 10), which is UTC-based rather than local time. In US time zones this flips to the next day during evening hours, so the Engineer Setup form can prefill date_of_joining as tomorrow and submit an incorrect future join date if the operator accepts defaults.
Useful? React with 👍 / 👎.
…nboarding-flow # Conflicts: # apps/api/src/five08/backend/static/dashboard/.vite/manifest.json # apps/api/src/five08/backend/static/dashboard/assets/index-Cn-kslAB.css # apps/api/src/five08/backend/static/dashboard/index.html
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 622cfd994c
ℹ️ 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".
| if preflight_employee is None: | ||
| ensure_no_similar_engineer_name(client, email=email, full_name=full_name) |
There was a problem hiding this comment.
Fresh evidence from this revision: the new gate at setup_engineer only runs ensure_no_similar_engineer_name when preflight_employee is None. If an Employee already exists but has no supplier link, the flow proceeds to ensure_supplier_preconditions/ensure_supplier using employee_name, and ensure_supplier will attach portal access to the first Supplier name match. In environments with duplicate human names, this can silently link the engineer to an unrelated Supplier and grant unintended portal access, so conflict/ownership checks must also run for existing-employee paths (at least when Employee.supplier is empty).
Useful? React with 👍 / 👎.
Summary
Notes
@508.devemail and uses that email as the idempotency key.Validation
uv run pytest tests/unituv run ruff check packages/shared/src/five08/engineer_onboarding.py packages/shared/src/five08/clients/erpnext.py apps/api/src/five08/backend/api.py apps/discord_bot/src/five08/discord_bot/cogs/engineer_onboarding.py tests/unit/test_engineer_onboarding.pyuv run mypy packages/shared/src/five08/engineer_onboarding.py apps/discord_bot/src/five08/discord_bot/cogs/engineer_onboarding.py apps/api/src/five08/backend/api.pybun run lint && bun run typecheck && bun run testinapps/admin_dashboardbun run buildinapps/admin_dashboardSummary by CodeRabbit
Engineer Onboarding and Project Costing
New Features
Tests