Skip to content

Refactor Onboarding Endpoint to Make User Context Optional #335

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
AkhileshNegi merged 5 commits into from
Aug 18, 2025
Merged

Refactor Onboarding Endpoint to Make User Context Optional #335

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1cd2d01
email and password optional in onboarding api request
avirajsingh7 Aug 18, 2025
a229095
add suffix to email autogenerated
avirajsingh7 Aug 18, 2025
2f2f09b
make username optional
avirajsingh7 Aug 18, 2025
77dbd0b
rename method
avirajsingh7 Aug 18, 2025
b24059e
refactor username
avirajsingh7 Aug 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 32 additions & 6 deletions backend/app/api/routes/onboarding.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import uuid
import re
import secrets

from fastapi import APIRouter, HTTPException, Depends
from pydantic import BaseModel, EmailStr
from pydantic import BaseModel, EmailStr, model_validator, field_validator
from sqlmodel import Session

from app.crud import (
Expand Down Expand Up @@ -32,9 +33,34 @@
class OnboardingRequest(BaseModel):
organization_name: str
project_name: str
email: EmailStr
password: str
user_name: str
email: EmailStr | None = None
password: str | None = None
user_name: str | None = None
Comment on lines +36 to +38
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Aug 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Making email/password/user_name optional — LGTM; ensure tests cover no-email/no-password paths

This aligns with the PR objective. Please confirm there are tests that omit each of these fields to exercise default generation.

You can verify coverage with:

🏁 Script executed:

#!/bin/bash
# Search onboarding tests for payloads that omit email or password
rg -n -C2 --type=py 'client\.post\(.+/onboard' backend/app/tests | sed -n '1,200p'
rg -n -C3 --type=py '"email":' backend/app/tests/api/routes/test_onboarding.py

Length of output: 1076

Add tests for missing optional fields

I reviewed test_onboarding.py and all existing payloads include email, password, and user_name. We need explicit coverage for cases where each of these is omitted to exercise default generation.

Please add tests that:

  • Omit email and assert a generated email is assigned
  • Omit password and assert a generated password is assigned
  • Omit user_name and assert a generated user name is assigned

File to update:

  • backend/app/tests/api/routes/test_onboarding.py
🤖 Prompt for AI Agents 
In backend/app/api/routes/onboarding.py around lines 36 to 38, the Pydantic
model marks email, password, and user_name as optional, but tests never cover
omitted-field behavior; add three new tests in
backend/app/tests/api/routes/test_onboarding.py that POST to the onboarding
endpoint with payloads that (1) omit email and assert the response contains a
non-empty, valid-looking generated email, (2) omit password and assert the
response contains a non-empty generated password (and not null), and (3) omit
user_name and assert the response contains a non-empty generated user_name; each
test should use the same client fixture and status code assertions as existing
tests and only assert presence/format of the generated field (not exact values).


@staticmethod
def _clean_username(raw: str, max_len: int = 200) -> str:
"""
Normalize a string into a safe username that can also be used
as the local part of an email address.
"""
username = re.sub(r"[^A-Za-z0-9._]", "_", raw.strip().lower())
username = re.sub(r"[._]{2,}", "_", username) # collapse repeats
username = username.strip("._") # remove leading/trailing
return username[:max_len]

@model_validator(mode="after")
def set_defaults(self):
if self.user_name is None:
self.user_name = self.project_name + " User"

if self.email is None:
local_part = self._clean_username(self.user_name, max_len=200)
suffix = secrets.token_hex(3)
self.email = f"{local_part}.{suffix}@kaapi.org"

Comment on lines +56 to +60
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Aug 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Fix: Ensure auto-generated email local-part is valid and within RFC length (64).

Two issues can cause user creation to fail (likely as 500 from UserCreate validation):

  • If _clean_username(...) yields an empty string (e.g., input is all invalid/non-ASCII), you'll generate emails like .abcdef@kaapi.org which are invalid.
  • The local-part can exceed 64 chars (you currently allow up to 200), which is invalid and will be rejected by EmailStr.

Harden the generation by providing a fallback base and truncating to fit 64 chars (leaving room for the dot and suffix).

Apply this diff:

-        if self.email is None:
-            local_part = self._clean_username(self.user_name, max_len=200)
-            suffix = secrets.token_hex(3)
-            self.email = f"{local_part}.{suffix}@kaapi.org"
+        if self.email is None:
+            suffix = secrets.token_hex(3)  # 6 hex chars
+            base = self._clean_username(self.user_name or self.project_name, max_len=200).strip("._")
+            if not base:
+                base = "user"
+            # RFC local-part max length is 64; reserve 1 for the dot and len(suffix)
+            allowed = max(1, 64 - 1 - len(suffix))
+            local_part = base[:allowed]
+            self.email = f"{local_part}.{suffix}@kaapi.org"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if self.email is None:
local_part = self._clean_username(self.user_name, max_len=200)
suffix = secrets.token_hex(3)
self.email = f"{local_part}.{suffix}@kaapi.org"
if self.email is None:
suffix = secrets.token_hex(3) # 6 hex chars
base = self._clean_username(self.user_name or self.project_name, max_len=200).strip("._")
if not base:
base = "user"
# RFC local-part max length is 64; reserve 1 for the dot and len(suffix)
allowed = max(1, 64 - 1 - len(suffix))
local_part = base[:allowed]
self.email = f"{local_part}.{suffix}@kaapi.org"
🤖 Prompt for AI Agents 
In backend/app/api/routes/onboarding.py around lines 56 to 60, the
auto-generated email local-part can be empty or exceed the 64-character RFC
limit; ensure you provide a safe fallback when _clean_username returns an empty
string and truncate the local-part to fit within 64 characters leaving room for
the dot and suffix. After calling _clean_username, if the result is empty set a
fallback like "user", compute max_local = 64 - 1 - len(suffix) (1 for the dot),
truncate the cleaned local-part to max_local before composing
f"{local}.{suffix}@kaapi.org", and keep the suffix length small enough (e.g.,
token_hex(3)) so the calculation always produces a positive max_local. Ensure no
additional characters are added that could push the local-part over 64.

if self.password is None:
self.password = secrets.token_urlsafe(12)
return self
Comment on lines +51 to +63
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Aug 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Please add tests that omit optional fields to exercise default generation.

Current tests historically always provided email, password, and user_name. We need coverage for:

  • Omitted email → response user/email created and looks valid
  • Omitted password → non-empty generated password (len >= 12)
  • Omitted user_name → generated from project_name

If possible, also add a test omitting all three.

You can use this script to quickly inspect existing onboarding tests for omitted fields:

🏁 Script executed:

#!/bin/bash
# Show onboarding POST payloads and presence of optional keys
rg -n -C2 --type=py -P 'client\.post\(.+?/onboard' backend/app/tests
echo '--- email keys ---'
rg -n -C0 --type=py -P '"email"\s*:' backend/app/tests/api/routes/test_onboarding.py || true
echo '--- password keys ---'
rg -n -C0 --type=py -P '"password"\s*:' backend/app/tests/api/routes/test_onboarding.py || true
echo '--- user_name keys ---'
rg -n -C0 --type=py -P '"user_name"\s*:' backend/app/tests/api/routes/test_onboarding.py || true

Length of output: 954

Add tests for default generation of optional onboarding fields

Current tests in backend/app/tests/api/routes/test_onboarding.py always include "email", "password", and "user_name", so the model_validator branches that set defaults are never exercised. Please add tests that cover:

  • Omitting email
    • Verify that the response’s user.email is generated in the form <clean_username>.<hex>@kaapi.org
  • Omitting password
    • Verify that user.password is non-empty, URL-safe, and at least 12 characters long
  • Omitting user_name
    • Verify that user.user_name defaults to <project_name> User
  • (Optional) Omitting all three fields in one request to ensure they all default correctly
🤖 Prompt for AI Agents 
In backend/app/api/routes/onboarding.py around lines 51 to 63, the
model_validator that generates defaults for user_name, email, and password is
never exercised by tests; add new tests in
backend/app/tests/api/routes/test_onboarding.py that post onboarding requests
omitting each optional field to exercise those branches: (1) omit "email" and
assert response JSON user.email matches
"<cleaned_user_name>.<6-hex-chars>@kaapi.org" (cleaning uses the same rules as
_clean_username), (2) omit "password" and assert response JSON user.password is
non-empty, URL-safe (contains only URL-safe characters), and length >= 12, (3)
omit "user_name" and assert response JSON user.user_name equals "<project_name>
User", and (4) optionally send a request omitting all three and assert all
defaults are set accordingly; ensure tests use the same project_name input and
validate patterns/lengths rather than exact random values.



class OnboardingResponse(BaseModel):
Expand Down Expand Up @@ -82,7 +108,7 @@ def onboard_user(request: OnboardingRequest, session: SessionDep):
user = existing_user
else:
user_create = UserCreate(
name=request.user_name,
full_name=request.user_name,
email=request.email,
password=request.password,
)
Expand Down
8 changes: 4 additions & 4 deletions backend/app/tests/api/routes/test_onboarding.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ def test_onboard_user(client, db: Session, superuser_token_headers: dict[str, st
"project_name": "TestProject",
"email": random_email(),
"password": "testpassword123",
"user_name": "Test User",
"user_name": "test_user",
}

response = client.post(
Expand Down Expand Up @@ -65,7 +65,7 @@ def test_create_user_existing_email(
"project_name": "TestProject",
"email": random_email(),
"password": "testpassword123",
"user_name": "Test User",
"user_name": "test_user",
}

client.post(
Expand All @@ -89,7 +89,7 @@ def test_is_superuser_flag(
"project_name": "TestProjects",
"email": random_email(),
"password": "testpassword123",
"user_name": "Test User",
"user_name": "test_user",
}

response = client.post(
Expand All @@ -112,7 +112,7 @@ def test_organization_and_project_creation(
"project_name": "NewProject",
"email": random_email(),
"password": "newpassword123",
"user_name": "New User",
"user_name": "new_user",
}

response = client.post(
Expand Down