Fix Onboard API: Add optional OpenAI key and enforce transactional consistency #337
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a transactional onboarding flow: new OnboardingRequest/OnboardingResponse models with defaults, a CRUD Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant API_Route as "Onboarding Route"
participant CRUD as "crud.onboard_project"
participant DB as "Database"
Note over Client,API_Route: POST /onboard (superuser)
Client->>API_Route: OnboardingRequest
API_Route->>CRUD: onboard_project(onboard_in, session)
CRUD->>DB: fetch or create Organization
CRUD->>DB: get_project_by_name(name, org_id)
alt project exists
CRUD-->>API_Route: raise HTTP 409
API_Route-->>Client: 409 Conflict
else create project
CRUD->>DB: create Project
CRUD->>DB: fetch or create User (hash password if new)
CRUD->>DB: generate raw API key -> store encrypted APIKey
opt openai_api_key provided
CRUD->>DB: encrypt & store Credential(provider="openai")
end
CRUD->>DB: commit transaction
CRUD-->>API_Route: OnboardingResponse (plaintext api_key)
API_Route-->>Client: 201 APIResponse{ success: true, data: ... }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (5)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (15)
backend/app/api/docs/onboarding/onboarding.md (1)
27-30: Document error codes and rollback behavior.Consider adding explicit status codes (e.g., 201 Created on success, 409 on conflict, 422 on validation error) and that any server-side errors result in a full rollback within a single DB transaction.
backend/app/models/__init__.py (1)
66-66: Silence Ruff F401 for re-exported onboarding types.This package’s init aggregates public exports; Ruff flags these imports as unused. Mark the line with a per-line ignore to keep the aggregator pattern without introducing a massive all.
Apply this diff:
-from .onboarding import OnboardingRequest, OnboardingResponse +from .onboarding import OnboardingRequest, OnboardingResponse # noqa: F401backend/app/crud/project.py (1)
31-37: Type hint modernization and data integrity recommendation.
- Prefer
Project | NoneoverOptional[Project](PEP 604) for consistency with modern typing.- Relying on a pre-insert lookup to enforce uniqueness is race-prone. Recommend a DB-level unique constraint on
(organization_id, name)to guarantee integrity under concurrency; keep the lookup for fast-path and to return 409 gracefully.Apply this diff to modernize the return type:
-def get_project_by_name( - *, session: Session, project_name: str, organization_id: int -) -> Optional[Project]: +def get_project_by_name( + *, session: Session, project_name: str, organization_id: int +) -> Project | None:If helpful, I can propose the
UniqueConstraintchange inbackend/app/models/project.pyand a constraint-aware insert pattern with exception-to-HTTP 409 mapping.backend/app/tests/crud/test_project.py (1)
47-70: LGTM: solid coverage for name + organization filters.Both the positive and not-found paths look good and exercise the new CRUD method well.
Optional: Add a test asserting that the same project name under a different organization doesn’t collide (documents the intended uniqueness scope).
backend/app/crud/__init__.py (1)
22-24: Silence Ruff F401 for re-exported CRUD functions.These imports are for public re-exports; add per-line ignores to avoid F401 while keeping the aggregator pattern consistent with the rest of this file.
Apply this diff:
from .project import ( create_project, get_project_by_id, - get_project_by_name, - get_projects_by_organization, - validate_project, + get_project_by_name, # noqa: F401 + get_projects_by_organization, # noqa: F401 + validate_project, # noqa: F401 ) from .api_key import ( - create_api_key, - generate_api_key, + create_api_key, # noqa: F401 + generate_api_key, # noqa: F401 get_api_key, get_api_key_by_value, get_api_keys_by_project, get_api_key_by_project_user, delete_api_key, get_api_key_by_user_id, ) @@ -from .onboarding import onboard_project +from .onboarding import onboard_project # noqa: F401If you prefer, we can introduce an explicit
__all__for the whole module to centralize export intent and avoid per-line ignores.Also applies to: 29-29, 70-70
backend/app/crud/onboarding.py (3)
25-27: Optional: wrap the whole workflow in a transaction context for automatic rollback.While
commit()is called once at the end, wrapping withsession.begin()ensures a rollback is issued automatically on any exception and keeps the function self-contained.Example:
def onboard_project(session: Session, onboard_in: OnboardingRequest) -> OnboardingResponse: with session.begin(): # ... do all the work without explicit commit # outside context, build and return OnboardingResponse
101-113: Consider not echoing the OpenAI API key back in the response.Returning provider secrets in responses increases the chance of accidental leakage via logs/analytics. Prefer returning a boolean flag (e.g.,
openai_credential_linked) or a masked value.Potential change (requires model/test updates):
- Replace
openai_api_key: str | NoneinOnboardingResponsewithopenai_credential_linked: bool.- In the response, set
openai_credential_linked=bool(onboard_in.openai_api_key).
76-89: Optional: link the user to the project via ProjectUser if that association is expected.Currently, the flow creates a User and an API key but doesn’t attach the user to the project through the join table. If the rest of the system relies on
ProjectUser, add that link.Can you confirm whether
ProjectUserassociation is required for permissions elsewhere? If yes, I can propose a precise snippet.backend/app/api/routes/onboarding.py (1)
20-23: Silence the linter for unusedcurrent_userby renaming to_current_user.The dependency is used for access control side-effects; the variable isn’t used directly.
Apply this diff:
-def onboard_project_route( - onboard_in: OnboardingRequest, - session: SessionDep, - current_user: User = Depends(get_current_active_superuser), -): +def onboard_project_route( + onboard_in: OnboardingRequest, + session: SessionDep, + _current_user: User = Depends(get_current_active_superuser), +):backend/app/tests/crud/test_onboarding.py (2)
3-3: Remove unused import.
from requests import sessionis unused.Apply this diff:
-from requests import session
84-93: Strengthen assertion: verify stored OpenAI credentials are encrypted.You already check that a credential row exists. Add an assertion that the stored value differs from the plaintext key to confirm encryption.
Apply this diff:
assert credential is not None + # Ensure we didn't store the plaintext key + assert credential.credential != openai_keybackend/app/models/onboarding.py (1)
84-107: Optional: forbid extra fields to catch mismatches early.To prevent accidental fields (e.g.,
user_namein responses) from slipping through, consider setting Pydantic v2 config to forbid extras.Outside this range, add to the class:
from pydantic import ConfigDict class OnboardingResponse(SQLModel): model_config = ConfigDict(extra="forbid") # existing fields...backend/app/tests/api/routes/test_onboarding.py (3)
6-6: Remove unused import.
OnboardingRequestis not used in these API tests.Apply this diff:
-from app.models import OnboardingRequest
9-9: Remove unused import.
create_random_useris not used in this file.Apply this diff:
-from app.tests.utils.user import create_random_user
136-156: Silence unuseddbfixture where it’s intentionally injected for isolation.If the fixture is injected for DB isolation but unused in the test body, you can add a no-op to keep Ruff happy.
Apply this minimal diff to the start of the function body:
def test_onboard_project_without_openai_key( client: TestClient, superuser_token_headers: dict[str, str], db: Session ) -> None: """Test onboarding without OpenAI API key.""" + del db # fixture used for DB isolation; variable intentionally unusedRepeat similarly for other tests in this file where
dbis unused.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
backend/app/api/docs/onboarding/onboarding.md(1 hunks)backend/app/api/routes/onboarding.py(2 hunks)backend/app/crud/__init__.py(2 hunks)backend/app/crud/onboarding.py(1 hunks)backend/app/crud/project.py(1 hunks)backend/app/models/__init__.py(1 hunks)backend/app/models/onboarding.py(1 hunks)backend/app/tests/api/routes/test_onboarding.py(1 hunks)backend/app/tests/crud/test_onboarding.py(1 hunks)backend/app/tests/crud/test_project.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (9)
backend/app/crud/project.py (4)
backend/app/models/project.py (2)
Project(28-54)ProjectCreate(16-17)backend/app/crud/organization.py (2)
get_organization_by_name(34-36)get_organization_by_id(29-31)backend/app/seed_data/seed_data.py (1)
create_project(96-122)backend/app/tests/utils/utils.py (1)
get_project(70-89)
backend/app/models/__init__.py (1)
backend/app/models/onboarding.py (2)
OnboardingRequest(7-81)OnboardingResponse(84-107)
backend/app/tests/crud/test_onboarding.py (9)
backend/app/crud/onboarding.py (1)
onboard_project(25-126)backend/app/crud/organization.py (2)
get_organization_by_name(34-36)get_organization_by_id(29-31)backend/app/crud/project.py (1)
get_project_by_name(31-37)backend/app/crud/user.py (1)
get_user_by_email(34-37)backend/app/models/onboarding.py (2)
OnboardingRequest(7-81)OnboardingResponse(84-107)backend/app/models/credentials.py (1)
Credential(50-94)backend/app/tests/utils/utils.py (2)
random_lower_string(21-22)random_email(29-30)backend/app/tests/utils/test_data.py (2)
create_test_organization(23-31)create_test_project(34-50)backend/app/tests/conftest.py (1)
db(18-35)
backend/app/tests/crud/test_project.py (4)
backend/app/crud/project.py (1)
get_project_by_name(31-37)backend/app/tests/conftest.py (1)
db(18-35)backend/app/tests/utils/test_data.py (2)
create_test_project(34-50)create_test_organization(23-31)backend/app/tests/utils/utils.py (1)
random_lower_string(21-22)
backend/app/crud/onboarding.py (10)
backend/app/crud/organization.py (1)
get_organization_by_name(34-36)backend/app/crud/project.py (1)
get_project_by_name(31-37)backend/app/crud/user.py (1)
get_user_by_email(34-37)backend/app/models/api_key.py (1)
APIKey(28-38)backend/app/models/credentials.py (1)
Credential(50-94)backend/app/models/onboarding.py (2)
OnboardingRequest(7-81)OnboardingResponse(84-107)backend/app/models/organization.py (1)
Organization(35-58)backend/app/models/project.py (1)
Project(28-54)backend/app/models/user.py (1)
User(48-60)backend/app/alembic/versions/60b6c511a485_add_project_id_in_api_key_table.py (1)
upgrade(20-47)
backend/app/models/onboarding.py (3)
backend/app/models/organization.py (1)
Organization(35-58)backend/app/alembic/versions/99f4fc325617_add_organization_project_setup.py (1)
upgrade(20-72)backend/app/models/project.py (1)
ProjectCreate(16-17)
backend/app/crud/__init__.py (3)
backend/app/crud/project.py (1)
get_project_by_name(31-37)backend/app/crud/onboarding.py (1)
onboard_project(25-126)backend/app/crud/api_key.py (2)
get_api_keys_by_project(149-164)create_api_key(25-57)
backend/app/api/routes/onboarding.py (2)
backend/app/crud/onboarding.py (1)
onboard_project(25-126)backend/app/models/onboarding.py (2)
OnboardingRequest(7-81)OnboardingResponse(84-107)
backend/app/tests/api/routes/test_onboarding.py (4)
backend/app/models/onboarding.py (1)
OnboardingRequest(7-81)backend/app/tests/utils/utils.py (2)
random_email(29-30)random_lower_string(21-22)backend/app/tests/utils/test_data.py (1)
create_test_organization(23-31)backend/app/tests/crud/test_onboarding.py (4)
test_onboard_project_new_organization_project_user(27-92)test_onboard_project_existing_organization(95-122)test_onboard_project_without_openai_key(179-195)test_onboard_project_with_auto_generated_defaults(198-217)
🪛 Ruff (0.12.2)
backend/app/crud/project.py
33-33: Use X | None for type annotations
Convert to X | None
(UP045)
backend/app/models/__init__.py
66-66: .onboarding.OnboardingRequest imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
66-66: .onboarding.OnboardingResponse imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
backend/app/tests/crud/test_onboarding.py
3-3: requests.session imported but unused
Remove unused import: requests.session
(F401)
backend/app/crud/__init__.py
22-22: .project.get_project_by_name imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
23-23: .project.get_projects_by_organization imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
24-24: .project.validate_project imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
28-28: .api_key.create_api_key imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
29-29: .api_key.generate_api_key imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
70-70: .onboarding.onboard_project imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
backend/app/api/routes/onboarding.py
22-22: Unused function argument: current_user
(ARG001)
backend/app/tests/api/routes/test_onboarding.py
6-6: app.models.OnboardingRequest imported but unused
Remove unused import: app.models.OnboardingRequest
(F401)
9-9: app.tests.utils.user.create_random_user imported but unused
Remove unused import: app.tests.utils.user.create_random_user
(F401)
13-13: Unused function argument: db
(ARG001)
96-96: Unused function argument: db
(ARG001)
136-136: Unused function argument: db
(ARG001)
165-165: Unused function argument: db
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (4)
backend/app/api/routes/onboarding.py (1)
15-18: LGTM: standardized response shape and 201 semantics.Delegating to CRUD and returning
APIResponse[OnboardingResponse]with 201 aligns with the PR goals. The docstring loader is a nice touch.Also applies to: 24-25
backend/app/tests/crud/test_onboarding.py (1)
179-196: Nice coverage for absence of OpenAI key.Validates the optional behavior and aligns with the schema defaults.
backend/app/models/onboarding.py (1)
69-81: Defaults viamodel_validatorare clear and safe.Good use of post-validation to guarantee
user_name,passwordexist by the time CRUD runs, minimizing downstream checks.backend/app/tests/api/routes/test_onboarding.py (1)
12-23: Endpoint happy-path test looks solid and validates the standardized envelope.Good assertions on 201,
success, and nesteddatawith IDs and credentials.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (19)
backend/app/api/docs/onboarding/onboarding.md (2)
9-12: Clarify success status code for project creation.Explicitly documenting 201 vs. 409 improves client expectations.
## 📂 Project Handling -- If `project_name` does **not exist** in the organization, it will be created. -- If the project already exists in the same organization, the API will return **409 Conflict**. +- If `project_name` does **not exist** in the organization, it will be created and the API responds with **201 Created**. +- If the project already exists in the same organization, the API returns **409 Conflict**.
25-30: Document generated defaults and validation to reduce surprises for clients.The API generates safe defaults for several fields; make this explicit.
--- ## 🔄 Transactional Guarantee The onboarding process is **all-or-nothing**: - If any step fails (e.g., invalid password), **no organization, project, or user will be persisted**. + +--- + +## ✅ Defaults & Validation +- `user_name`: Defaults to "<project_name> User" if not provided. +- `email`: Auto-generated if not provided, using a normalized username and random suffix. +- `password`: Auto-generated secure value if not provided; must be 8–128 characters when supplied.backend/app/models/__init__.py (1)
66-66: Mark re-exports to satisfy Ruff F401.This file serves as the package's public API surface; add a
noqato avoid false positives for re-exports.-from .onboarding import OnboardingRequest, OnboardingResponse +from .onboarding import OnboardingRequest, OnboardingResponse # noqa: F401 - re-exported via package __init__backend/app/crud/project.py (3)
31-37: Modernize return type toProject | None.Prefer the
X | Noneunion form for consistency with the rest of the codebase and Ruff UP045.-def get_project_by_name( - *, session: Session, project_name: str, organization_id: int -) -> Optional[Project]: +def get_project_by_name( + *, session: Session, project_name: str, organization_id: int +) -> Project | None:
31-37: Add DB-level uniqueness on (organization_id, name) to avoid race-condition duplicates.You guard duplicates in code, but concurrent requests can still insert the same (org, name) pair unless the DB enforces it. Add a unique constraint/index.
Proposed change in backend/app/models/project.py (outside this diff):
from sqlmodel import SQLModel, Field, UniqueConstraint class Project(ProjectBase, table=True): __table_args__ = ( UniqueConstraint("organization_id", "name", name="uq_project_org_name"), ) ...Also consider an index on ("organization_id", "name") if you perform frequent lookups by both.
45-49: Fix grammar in docstring.def validate_project(session: Session, project_id: int) -> Project: """ - Ensures that an project exists and is active. + Ensures that a project exists and is active. """backend/app/crud/__init__.py (1)
22-24: Silence Ruff F401 for re-exports in package init.These imports intentionally re-export symbols. Avoid per-line noise by disabling F401 for this module.
Add this at the top of the file (outside this hunk):
# ruff: noqa: F401 # This module re-exports CRUD symbols for the package API.Also applies to: 28-29, 70-70
backend/app/crud/onboarding.py (1)
24-46: Transactional behavior: ok as-is; considersession.begin()for automatic rollback semantics.You’re doing a single commit at the end (good). As an optional improvement, wrap the body in
with session.begin():to guarantee rollback on any exception without relying on outer layers.Also applies to: 114-115
backend/app/models/onboarding.py (2)
83-107: Align response contract vs. CRUD: either adduser_nameor keep it out.Your CRUD currently attempted to include
user_namein the response (which would fail). You have two choices:
- Keep the lean response (current model). Then remove
user_namefrom the constructor (see CRUD comment).- Or add it here if you want to return it.
If you choose to include it, add:
class OnboardingResponse(SQLModel): @@ user_email: EmailStr = Field( description="Email of the user.", ) + user_name: str | None = Field( + default=None, + description="Full name of the user, if available.", + ) api_key: str = Field(description="Generated internal API key for the project.")
35-56: Optional: mark secrets as SecretStr to reduce accidental exposure.For password and openai_api_key, consider
SecretStrto avoid leaking values in logs/tracebacks.backend/app/tests/crud/test_onboarding.py (1)
3-3: Remove unused import (requests.session).This import is unused and flagged by Ruff (F401).
Apply:
-from requests import sessionbackend/app/tests/api/routes/test_onboarding.py (7)
3-3: Remove unused importSession.After dropping the unused
dbfixture parameter, this import becomes unused.Apply:
-from sqlmodel import Session
6-6: Drop unused imports.These are flagged by Ruff (F401).
Apply:
-from app.models import OnboardingRequest @@ -from app.tests.utils.user import create_random_userAlso applies to: 9-9
12-16: Remove unuseddb: Sessionparam from test signature.The test doesn't use the
dbfixture (Ruff ARG001).Apply:
-def test_onboard_project_new_organization_project_user( - client: TestClient, superuser_token_headers: dict[str, str], db: Session -) -> None: +def test_onboard_project_new_organization_project_user( + client: TestClient, superuser_token_headers: dict[str, str] +) -> None:
58-61: Remove unuseddb: Sessionparam from test signature.Apply:
-def test_onboard_project_existing_organization( - client: TestClient, superuser_token_headers: dict[str, str], db: Session -) -> None: +def test_onboard_project_existing_organization( + client: TestClient, superuser_token_headers: dict[str, str] +) -> None:
95-98: Remove unuseddb: Sessionparam from test signature.Apply:
-def test_onboard_project_duplicate_project_in_organization( - client: TestClient, superuser_token_headers: dict[str, str], db: Session -) -> None: +def test_onboard_project_duplicate_project_in_organization( + client: TestClient, superuser_token_headers: dict[str, str] +) -> None:
136-139: Remove unuseddb: Sessionparam from test signature.Apply:
-def test_onboard_project_without_openai_key( - client: TestClient, superuser_token_headers: dict[str, str], db: Session -) -> None: +def test_onboard_project_without_openai_key( + client: TestClient, superuser_token_headers: dict[str, str] +) -> None:
165-168: Remove unuseddb: Sessionparam from test signature.Apply:
-def test_onboard_project_with_auto_generated_defaults( - client: TestClient, superuser_token_headers: dict[str, str], db: Session -) -> None: +def test_onboard_project_with_auto_generated_defaults( + client: TestClient, superuser_token_headers: dict[str, str] +) -> None:backend/app/api/routes/onboarding.py (1)
22-22: Silence Ruff ARG001 for dependency-only parameter.
current_useris intentionally unused; it enforces the superuser dependency. Rename to_to please Ruff.Apply:
- current_user: User = Depends(get_current_active_superuser), + _: User = Depends(get_current_active_superuser),Alternatively, keep the name and add
# noqa: ARG001.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
backend/app/api/docs/onboarding/onboarding.md(1 hunks)backend/app/api/routes/onboarding.py(2 hunks)backend/app/crud/__init__.py(2 hunks)backend/app/crud/onboarding.py(1 hunks)backend/app/crud/project.py(1 hunks)backend/app/models/__init__.py(1 hunks)backend/app/models/onboarding.py(1 hunks)backend/app/tests/api/routes/test_onboarding.py(1 hunks)backend/app/tests/crud/test_onboarding.py(1 hunks)backend/app/tests/crud/test_project.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (9)
backend/app/crud/project.py (5)
backend/app/models/project.py (2)
Project(28-54)ProjectCreate(16-17)backend/app/seed_data/seed_data.py (1)
create_project(96-122)backend/app/crud/organization.py (1)
get_organization_by_name(34-36)backend/app/tests/utils/utils.py (1)
get_project(70-89)backend/app/tests/utils/test_data.py (1)
create_test_project(34-50)
backend/app/crud/onboarding.py (7)
backend/app/core/security.py (3)
encrypt_api_key(106-122)encrypt_credentials(144-161)get_password_hash(93-103)backend/app/crud/api_key.py (1)
generate_api_key(18-22)backend/app/crud/organization.py (1)
get_organization_by_name(34-36)backend/app/crud/project.py (1)
get_project_by_name(31-37)backend/app/crud/user.py (1)
get_user_by_email(34-37)backend/app/models/credentials.py (1)
Credential(50-94)backend/app/models/onboarding.py (2)
OnboardingRequest(7-81)OnboardingResponse(84-107)
backend/app/tests/crud/test_onboarding.py (10)
backend/app/crud/onboarding.py (1)
onboard_project(25-126)backend/app/crud/organization.py (2)
get_organization_by_name(34-36)get_organization_by_id(29-31)backend/app/crud/project.py (1)
get_project_by_name(31-37)backend/app/crud/user.py (1)
get_user_by_email(34-37)backend/app/models/onboarding.py (2)
OnboardingRequest(7-81)OnboardingResponse(84-107)backend/app/models/api_key.py (1)
APIKey(28-38)backend/app/models/credentials.py (1)
Credential(50-94)backend/app/tests/utils/utils.py (2)
random_lower_string(21-22)random_email(29-30)backend/app/tests/utils/test_data.py (2)
create_test_organization(23-31)create_test_project(34-50)backend/app/tests/conftest.py (1)
db(18-35)
backend/app/tests/crud/test_project.py (3)
backend/app/crud/project.py (1)
get_project_by_name(31-37)backend/app/tests/utils/test_data.py (2)
create_test_project(34-50)create_test_organization(23-31)backend/app/tests/utils/utils.py (1)
random_lower_string(21-22)
backend/app/crud/__init__.py (3)
backend/app/crud/project.py (2)
get_project_by_name(31-37)validate_project(45-62)backend/app/crud/api_key.py (2)
create_api_key(25-57)generate_api_key(18-22)backend/app/crud/onboarding.py (1)
onboard_project(25-126)
backend/app/models/__init__.py (1)
backend/app/models/onboarding.py (2)
OnboardingRequest(7-81)OnboardingResponse(84-107)
backend/app/models/onboarding.py (2)
backend/app/models/user.py (3)
NewPassword(37-39)UserUpdateMe(32-34)UserRegister(20-23)backend/app/api/routes/private.py (1)
PrivateUserCreate(16-20)
backend/app/tests/api/routes/test_onboarding.py (5)
backend/app/models/onboarding.py (1)
OnboardingRequest(7-81)backend/app/tests/utils/utils.py (2)
random_email(29-30)random_lower_string(21-22)backend/app/tests/utils/test_data.py (1)
create_test_organization(23-31)backend/app/tests/crud/test_onboarding.py (4)
test_onboard_project_new_organization_project_user(27-92)test_onboard_project_existing_organization(95-122)test_onboard_project_without_openai_key(179-195)test_onboard_project_with_auto_generated_defaults(198-217)backend/app/tests/conftest.py (2)
superuser_token_headers(53-54)db(18-35)
backend/app/api/routes/onboarding.py (4)
backend/app/crud/onboarding.py (1)
onboard_project(25-126)backend/app/models/onboarding.py (2)
OnboardingRequest(7-81)OnboardingResponse(84-107)backend/app/utils.py (3)
APIResponse(27-48)load_description(204-209)success_response(34-37)backend/app/api/deps.py (1)
get_current_active_superuser(137-142)
🪛 Ruff (0.12.2)
backend/app/crud/project.py
33-33: Use X | None for type annotations
Convert to X | None
(UP045)
backend/app/tests/crud/test_onboarding.py
3-3: requests.session imported but unused
Remove unused import: requests.session
(F401)
backend/app/crud/__init__.py
22-22: .project.get_project_by_name imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
23-23: .project.get_projects_by_organization imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
24-24: .project.validate_project imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
28-28: .api_key.create_api_key imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
29-29: .api_key.generate_api_key imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
70-70: .onboarding.onboard_project imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
backend/app/models/__init__.py
66-66: .onboarding.OnboardingRequest imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
66-66: .onboarding.OnboardingResponse imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
backend/app/tests/api/routes/test_onboarding.py
6-6: app.models.OnboardingRequest imported but unused
Remove unused import: app.models.OnboardingRequest
(F401)
9-9: app.tests.utils.user.create_random_user imported but unused
Remove unused import: app.tests.utils.user.create_random_user
(F401)
13-13: Unused function argument: db
(ARG001)
96-96: Unused function argument: db
(ARG001)
136-136: Unused function argument: db
(ARG001)
165-165: Unused function argument: db
(ARG001)
backend/app/api/routes/onboarding.py
22-22: Unused function argument: current_user
(ARG001)
🔇 Additional comments (6)
backend/app/crud/project.py (1)
31-37: LGTM: Helper correctly scopes to org and returns first match.The query composes the AND predicate as intended and returns a single project or None.
backend/app/tests/crud/test_project.py (2)
47-58: Good coverage for happy-path retrieval by name.Test asserts id/name/org match. Clear and deterministic.
60-70: Good negative test.Confirms None when name is absent in org scope. Nicely isolates org to avoid false positives.
backend/app/crud/onboarding.py (1)
4-4: Drop reversible encryption for API keys; store only the hashEncrypting API keys for storage is reversible and introduces unnecessary risk—
generate_api_key()already returns a secure hash suitable for “compare-and-verify” flows. Please update onboarding to use that hash and remove the unused encryption import:• File:
backend/app/crud/onboarding.py
Change imports:- from app.core.security import encrypt_api_key, encrypt_credentials, get_password_hash + from app.core.security import encrypt_credentials, get_password_hashAnd replace key assignment:
- raw_key, _ = generate_api_key() - encrypted_key = encrypt_api_key(raw_key) - api_key = APIKey(key=encrypted_key, ...) + raw_key, hashed_key = generate_api_key() + api_key = APIKey(key=hashed_key, ...)• Verify your authentication logic:
- Locate where
APIKey.keyis read and compared (e.g. in your auth endpoint or middleware).- Confirm it uses hash-based comparison (no
decrypt_api_keycalls).- Adjust any validators to use
verify_passwordor equivalent against the stored hash.backend/app/models/onboarding.py (1)
69-81: Default generation looks solid.Generating user_name, email, and password defaults here keeps CRUD slim and ensures earlier validation. No issues spotted.
backend/app/api/routes/onboarding.py (1)
13-25: Route refactor looks good.Thin route delegating to CRUD, consistent response wrapper, 201 status, and doc loading are all solid.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
backend/app/tests/api/routes/test_onboarding.py (7)
8-8: Remove unused import create_random_user.This import is not used and is flagged by Ruff (F401).
-from app.tests.utils.user import create_random_user
11-13: Silence unused fixture argument by renaming db -> _db (keeps fixture side-effects).The test doesn’t use the variable directly, but you likely want the DB fixture’s setup/teardown. Renaming to underscore satisfies Ruff (ARG001) without removing the fixture.
-def test_onboard_project_new_organization_project_user( - client: TestClient, superuser_token_headers: dict[str, str], db: Session -) -> None: +def test_onboard_project_new_organization_project_user( + client: TestClient, superuser_token_headers: dict[str, str], _db: Session +) -> None:
94-96: Silence unused fixture argument by renaming db -> _db (duplicate-project test).This test doesn’t use the variable directly; keep fixture effects, avoid ARG001.
-def test_onboard_project_duplicate_project_in_organization( - client: TestClient, superuser_token_headers: dict[str, str], db: Session -) -> None: +def test_onboard_project_duplicate_project_in_organization( + client: TestClient, superuser_token_headers: dict[str, str], _db: Session +) -> None:
134-136: Silence unused fixture argument by renaming db -> _db (without-openai-key test).Aligns with Ruff’s ARG001 while preserving fixture behavior.
-def test_onboard_project_without_openai_key( - client: TestClient, superuser_token_headers: dict[str, str], db: Session -) -> None: +def test_onboard_project_without_openai_key( + client: TestClient, superuser_token_headers: dict[str, str], _db: Session +) -> None:
163-165: Silence unused fixture argument by renaming db -> _db (auto-generated defaults test).Same rationale as above to satisfy ARG001.
-def test_onboard_project_with_auto_generated_defaults( - client: TestClient, superuser_token_headers: dict[str, str], db: Session -) -> None: +def test_onboard_project_with_auto_generated_defaults( + client: TestClient, superuser_token_headers: dict[str, str], _db: Session +) -> None:
118-131: Optionally assert the standard envelope on error responses for consistency.If your API wraps errors similarly, consider checking success == False and absence of data to prevent regressions in error payload shape.
Example:
error_response = response.json() assert error_response.get("success") is False assert "data" not in error_response or error_response["data"] is None assert "error" in error_response and "Project already exists" in error_response["error"]
15-21: Consider randomizing names to avoid cross-test coupling.Using constant org/project names across tests can cause flakiness if DB isolation ever changes. Appending a short random suffix improves resilience.
Example:
org_name = f"TestOrgOnboard-{random_lower_string()[:8]}" project_name = f"TestProjectOnboard-{random_lower_string()[:8]}"Also applies to: 64-68, 98-102, 138-142, 167-169
backend/app/tests/crud/test_onboarding.py (3)
3-3: Remove unused import requests.session.Not used; flagged by Ruff (F401).
-from requests import session
194-197: Optionally also verify that no OpenAI credential is created when key is omitted.Strengthens the negative-path assertion to ensure DB doesn’t get an unused provider record.
# After asserting response.openai_api_key is None: org = get_organization_by_name(session=db, name=org_name) project = get_project_by_name(session=db, project_name=project_name, organization_id=org.id) cred = db.exec( select(Credential).where( Credential.organization_id == org.id, Credential.project_id == project.id, Credential.provider == "openai", Credential.is_active.is_(True), ) ).first() assert cred is None
126-146: Optionally assert existing user’s password isn’t mutated.Since onboarding should reuse existing users, capturing and re-reading hashed_password verifies we don’t overwrite credentials.
existing_user = create_random_user(db) prev_hash = existing_user.hashed_password # ... run onboarding ... refetched_user = get_user_by_email(session=db, email=existing_user.email) assert refetched_user.hashed_password == prev_hash
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
backend/app/api/docs/onboarding/onboarding.md(1 hunks)backend/app/crud/onboarding.py(1 hunks)backend/app/tests/api/routes/test_onboarding.py(1 hunks)backend/app/tests/crud/test_onboarding.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/app/crud/onboarding.py
- backend/app/api/docs/onboarding/onboarding.md
🧰 Additional context used
🧬 Code Graph Analysis (2)
backend/app/tests/api/routes/test_onboarding.py (5)
backend/app/utils.py (1)
mask_string(158-167)backend/app/tests/utils/utils.py (2)
random_email(29-30)random_lower_string(21-22)backend/app/tests/utils/test_data.py (1)
create_test_organization(23-31)backend/app/tests/conftest.py (2)
superuser_token_headers(53-54)db(18-35)backend/app/api/routes/onboarding.py (2)
onboard_user(78-145)OnboardingResponse(66-70)
backend/app/tests/crud/test_onboarding.py (9)
backend/app/utils.py (1)
mask_string(158-167)backend/app/crud/onboarding.py (1)
onboard_project(26-125)backend/app/crud/organization.py (2)
get_organization_by_name(34-36)get_organization_by_id(29-31)backend/app/crud/project.py (1)
get_project_by_name(31-37)backend/app/crud/user.py (1)
get_user_by_email(34-37)backend/app/models/onboarding.py (2)
OnboardingRequest(7-81)OnboardingResponse(84-107)backend/app/tests/utils/utils.py (2)
random_lower_string(21-22)random_email(29-30)backend/app/tests/utils/test_data.py (2)
create_test_organization(23-31)create_test_project(34-50)backend/app/tests/conftest.py (1)
db(18-35)
🪛 Ruff (0.12.2)
backend/app/tests/api/routes/test_onboarding.py
8-8: app.tests.utils.user.create_random_user imported but unused
Remove unused import: app.tests.utils.user.create_random_user
(F401)
12-12: Unused function argument: db
(ARG001)
95-95: Unused function argument: db
(ARG001)
135-135: Unused function argument: db
(ARG001)
164-164: Unused function argument: db
(ARG001)
backend/app/tests/crud/test_onboarding.py
3-3: requests.session imported but unused
Remove unused import: requests.session
(F401)
287-287: No newline at end of file
Add trailing newline
(W292)
🪛 GitHub Actions: AI Platform CI
backend/app/tests/crud/test_onboarding.py
[error] 1-1: Pre-commit end-of-file-fixer modified backend/app/tests/crud/test_onboarding.py to add a trailing newline.
🔇 Additional comments (3)
backend/app/tests/api/routes/test_onboarding.py (1)
37-55: API envelope and masked OpenAI key checks look correct.201 status, success flag, data envelope, and masked openai_api_key match the updated API contract and utils.mask_string behavior.
backend/app/tests/crud/test_onboarding.py (2)
233-255: Good coverage of API key generation and encryption invariant.Asserting a non-empty public key and that the stored key differs from the response verifies encryption-at-rest without leaking sensitive data.
287-287: Ensure a trailing newline at EOF to satisfy pre-commit.CI already fixed this via end-of-file-fixer; just confirm the change is committed to avoid repeated pipeline noise.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
backend/app/tests/api/routes/test_onboarding.py (6)
10-28: Stabilize test data and silence unused fixture warning
- Use unique org/project names per test to avoid cross-test collisions if DB isolation ever regresses or tests run in parallel.
- The
dbfixture isn’t used in this test; rename to_dbto satisfy Ruff ARG001 without changing fixture behavior.Apply this diff:
-def test_onboard_project_new_organization_project_user( - client: TestClient, superuser_token_headers: dict[str, str], db: Session -) -> None: +def test_onboard_project_new_organization_project_user( + client: TestClient, superuser_token_headers: dict[str, str], _db: Session +) -> None: @@ - org_name = "TestOrgOnboard" - project_name = "TestProjectOnboard" + org_name = f"TestOrgOnboard-{random_lower_string()}" + project_name = f"TestProjectOnboard-{random_lower_string()}"
56-91: Add wrapper assertions for consistencyYou already extract
response_data["data"]but don’t assert the wrapper fields here. Add a minimal shape check to keep tests consistent across scenarios.Apply this diff:
@@ - response_data = response.json() + response_data = response.json() + assert "success" in response_data and response_data["success"] is True + assert "data" in response_data
133-160: Randomize names and silence unuseddbfixtureSame rationale as above: avoid flakiness and address Ruff ARG001.
-def test_onboard_project_without_openai_key( - client: TestClient, superuser_token_headers: dict[str, str], db: Session -) -> None: +def test_onboard_project_without_openai_key( + client: TestClient, superuser_token_headers: dict[str, str], _db: Session +) -> None: @@ - org_name = "TestOrgOnboard" - project_name = "TestProjectOnboard" + org_name = f"TestOrgOnboard-{random_lower_string()}" + project_name = f"TestProjectOnboard-{random_lower_string()}"
162-192: Strengthen isolation and lint hygiene for defaults test
- Randomize org/project to prevent cross-test contamination.
- Rename
dbto_dbto satisfy Ruff.-def test_onboard_project_with_auto_generated_defaults( - client: TestClient, superuser_token_headers: dict[str, str], db: Session -) -> None: +def test_onboard_project_with_auto_generated_defaults( + client: TestClient, superuser_token_headers: dict[str, str], _db: Session +) -> None: @@ - org_name = "TestOrgOnboard" - project_name = "TestProjectOnboard" + org_name = f"TestOrgOnboard-{random_lower_string()}" + project_name = f"TestProjectOnboard-{random_lower_string()}"
10-28: Consider adding a rollback/invariance test for transactional onboardingGiven the PR goal (single transaction, all-or-nothing), add a test where the request passes early validations but fails mid-way (e.g., simulate an encryption failure via monkeypatch) and assert no partial org/project/user persists afterward.
I can provide a test using monkeypatch to raise in
encrypt_credentialsorencrypt_api_keyand then assert via the DB that nothing was committed. Want me to draft it?Also applies to: 56-74, 93-108, 133-147, 162-175
93-131: Exception Handler Verified:errorEnvelope Present
- Confirmed in
backend/app/main.pythat
register_exception_handlers(app)is called and bothHTTPExceptionandRequestValidationErrorare mapped toAPIResponse.failure_response, producing a JSON with an"error"field.- Your test’s assertions on
response.json()["error"]for status 409 will pass as intended.Optional refactors to improve test hygiene and lint compliance:
- Rename the
db: Sessionfixture parameter to_db: Session.- Append a random suffix to
org_nameandproject_nameto isolate tests:Suggested diff in backend/app/tests/api/routes/test_onboarding.py:
--- a/backend/app/tests/api/routes/test_onboarding.py +++ b/backend/app/tests/api/routes/test_onboarding.py @@ -def test_onboard_project_duplicate_project_in_organization( - client: TestClient, superuser_token_headers: dict[str, str], db: Session -) -> None: +def test_onboard_project_duplicate_project_in_organization( + client: TestClient, superuser_token_headers: dict[str, str], _db: Session +) -> None: @@ - org_name = "TestOrgOnboard" - project_name = "TestProjectOnboard" + org_name = f"TestOrgOnboard-{random_lower_string()}" + project_name = f"TestProjectOnboard-{random_lower_string()}"backend/app/api/routes/onboarding.py (2)
23-23: Silence unused dependency argument
current_useris intentionally unused (guard-only). Rename to_to satisfy Ruff ARG001 without changing behavior.- current_user: User = Depends(get_current_active_superuser), + _: User = Depends(get_current_active_superuser),
14-19: Optional: consider setting a Location header for 201 responsesSince this is a create endpoint, returning a Location pointing to the project (or onboarding result) is a nice-to-have REST touch. Not blocking.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
backend/app/api/routes/onboarding.py(1 hunks)backend/app/crud/onboarding.py(1 hunks)backend/app/tests/api/routes/test_onboarding.py(1 hunks)backend/app/tests/crud/test_onboarding.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/app/tests/crud/test_onboarding.py
- backend/app/crud/onboarding.py
🧰 Additional context used
🧬 Code Graph Analysis (2)
backend/app/tests/api/routes/test_onboarding.py (4)
backend/app/utils.py (1)
mask_string(158-167)backend/app/tests/utils/utils.py (2)
random_email(29-30)random_lower_string(21-22)backend/app/tests/utils/test_data.py (1)
create_test_organization(23-31)backend/app/tests/conftest.py (2)
superuser_token_headers(53-54)db(18-35)
backend/app/api/routes/onboarding.py (5)
backend/app/api/deps.py (1)
get_current_active_superuser(137-142)backend/app/crud/onboarding.py (1)
onboard_project(29-138)backend/app/models/onboarding.py (2)
OnboardingRequest(7-81)OnboardingResponse(84-107)backend/app/models/user.py (1)
User(48-60)backend/app/utils.py (3)
APIResponse(27-48)load_description(204-209)success_response(34-37)
🪛 Ruff (0.12.2)
backend/app/tests/api/routes/test_onboarding.py
11-11: Unused function argument: db
(ARG001)
94-94: Unused function argument: db
(ARG001)
134-134: Unused function argument: db
(ARG001)
163-163: Unused function argument: db
(ARG001)
backend/app/api/routes/onboarding.py
23-23: Unused function argument: current_user
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (2)
backend/app/tests/api/routes/test_onboarding.py (1)
39-54: Good coverage of new response contractYou validate the 201 + APIResponse wrapper and the masked OpenAI key, and assert IDs/api_key presence. Looks solid.
backend/app/api/routes/onboarding.py (1)
14-26: Global exception handlers ensure consistent APIResponse envelopesA brief check confirms that in
backend/app/core/exception_handlers.py, handlers forHTTPException,RequestValidationError, and genericExceptionall returnAPIResponse.failure_response(...).model_dump(), andregister_exception_handlers(app)is invoked inbackend/app/main.py. No changes to this route are needed.
| - User: | ||
| - Create and link if `email` does not exist. | ||
| - If exists, attach to project. |
There was a problem hiding this comment.
mention create user with username, email and generated password as those are also added
…nsistency (#337) * add support of openai key * Fix immature entries in onboard api * add unit test for onboarding api * unit test for get_project_by_name * add logs
3 participants
Summary
Target issue: #338
This PR improves the Onboard API to better handle optional parameters and ensure data consistency.
Optional OpenAI API Key
Transactional Safety
Improved Validation
Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Notes
Please add here if any other information is required for the reviewer.
Summary by CodeRabbit
New Features
Documentation
Tests