Activate/deactivate only#62
Conversation
📝 WalkthroughWalkthroughThis PR implements a unified account deactivation workflow with consistent authorization enforcement. It strengthens auth dependencies to return HTTP 403 for inactive accounts, adds helper functions for affected account notification and relationship cleanup, introduces new coach discovery and client listing endpoints with enhanced validation, and adds comprehensive integration tests. ChangesAccount Deactivation and Coach-Client Relationship Management
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@src/api/roles/client/client.py`:
- Around line 645-670: The response dict in get_coach_profile currently returns
"base_account" (Account) and "coach_account" (Coach) which is misleading; change
the key "coach_account" to a clear name like "coach" or "coach_details" in the
return value from get_coach_profile (and update any callers of
client.get_coach_profile accordingly), and add or update a response_model/schema
for get_coach_profile so the contract enforces the types/fields (Account vs
Coach) to prevent consumers from expecting account fields under the coach key.
- Around line 705-738: There is a duplicate route registration for
`@router.get`("/my_coach") — remove the older get_my_coach implementation that
returns MyCoachResponse so the new get_my_coach (the handler that queries
ClientCoachRequest and ClientCoachRelationship) becomes the sole route; ensure
the remaining handler declares a response_model (create or reuse MyCoachResponse
or a compatible Pydantic model) and keep the intended semantics: require Account
via Depends(get_client_account), return 403 when acc.client_id is None, return
404 when no active non-blocked relationship is found, and return the
relationship payload with the fields currently produced by the new handler.
In `@src/api/roles/coach/coach.py`:
- Around line 559-623: The code in the get-my-clients flow uses N+1 queries
inside the relationships loop (variables/queries around relationships, fetching
Client, Account, ClientTelemetry, then HealthMetrics/StepCount/Daily* and
Completed* per telemetry) — collect IDs up front and batch-query each table with
IN(...) and then assemble in memory. Specifically: after computing relationships
from ClientCoachRequest/ClientCoachRelationship, gather all client_ids and
telemetry_ids, then replace the per-relationship db.get(Account) with a single
select(Account).where(Account.client_id.in_(client_ids)), replace the
per-request ClientTelemetry selects with one
select(ClientTelemetry).where(ClientTelemetry.client_id.in_(client_ids)), and
replace each child-table select (HealthMetrics, StepCount, DailyMoodSurvey,
DailyWorkoutSurvey, DailyBodyMetricsSurvey, DailyStepsSurvey, DailyMealSurvey,
CompletedMealActivity, CompletedWorkout) with single selects using
client_telemetry_id.in_(telemetry_ids); build maps by id (client->account,
client->telemetries list, telemetry->child lists) and assemble the clients list
from those maps instead of querying inside the loops.
- Around line 640-647: The response payload builds safe_account including a
non-existent field account.status which will raise at runtime; remove the
"status" entry from the safe_account dict (the block that constructs
safe_account in the function that checks if account:) so the keys only reference
real Account attributes (id, name, email, is_active, gender, etc.), and run
tests to ensure no other code expects safe_account["status"].
In `@tests/test_client_routes.py`:
- Around line 7-9: The test fixture contains a raw PAN-like card number under
the "ccnum" field which can trigger security scanners; replace the hardcoded
literal with a non-PAN token or a shared test helper constant (e.g., use a
TEST_CARD_TOKEN or a get_test_card_payload() helper) and update any tests
referencing "ccnum" to use that tokenized placeholder instead; ensure the "cv"
and "exp_date" fields remain realistic but not real credentials and centralize
the placeholder in a test helper/module so future fixtures reuse it (locate the
JSON in tests/test_client_routes.py around the "ccnum"/"cv"/"exp_date" keys).
🪄 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: da732890-0e0d-4aad-9291-b75dc71522cf
📒 Files selected for processing (9)
src/api/dependencies.pysrc/api/roles/client/client.pysrc/api/roles/coach/coach.pysrc/api/roles/shared/account.pysrc/database/account/models.pytests/test_client_routes.pytests/test_hirable_coaches.pytests/test_shared_account_activation.pytests/test_shared_account_notifications.py
| return { | ||
| "base_account": { | ||
| "id": coach_account.id, | ||
| "name": coach_account.name, | ||
| "email": coach_account.email, | ||
| "is_active": coach_account.is_active, | ||
| "gender": coach_account.gender, | ||
| "bio": coach_account.bio, | ||
| "age": coach_account.age, | ||
| "pfp_url": coach_account.pfp_url, | ||
| "client_id": coach_account.client_id, | ||
| "coach_id": coach_account.coach_id, | ||
| "admin_id": coach_account.admin_id, | ||
| "created_at": coach_account.created_at, | ||
| }, | ||
| "coach_account": coach, | ||
| "specialties": coach.specialties, | ||
| "certifications": certifications, | ||
| "experiences": experiences, | ||
| "pricing_plan": pricing_plan, | ||
| "availability": availability, | ||
| "rating_summary": { | ||
| "rating_count": int(rating_summary.rating_count or 0), | ||
| "avg_rating": float(rating_summary.avg_rating) if rating_summary.avg_rating is not None else None, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Misleading coach_account field — returns a Coach, not an Account.
In the get_coach_profile response, "base_account" holds the Account row while "coach_account" is set to the Coach model instance. The field naming is reversed from what consumers will expect and will likely cause integration bugs (e.g., a frontend reading coach_account.email would get nothing because email lives on Account). Rename it (e.g., "coach" or "coach_details") and consider a response_model so the contract is enforced.
🛠️ Suggested rename
return {
"base_account": {
...
},
- "coach_account": coach,
+ "coach": coach,
"specialties": coach.specialties,🤖 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 `@src/api/roles/client/client.py` around lines 645 - 670, The response dict in
get_coach_profile currently returns "base_account" (Account) and "coach_account"
(Coach) which is misleading; change the key "coach_account" to a clear name like
"coach" or "coach_details" in the return value from get_coach_profile (and
update any callers of client.get_coach_profile accordingly), and add or update a
response_model/schema for get_coach_profile so the contract enforces the
types/fields (Account vs Coach) to prevent consumers from expecting account
fields under the coach key.
| @router.get("/my_coach") | ||
| def get_my_coach(db = Depends(get_session), acc: Account = Depends(get_client_account)): | ||
| """ | ||
| Returns the active coach relationship for the logged-in client. | ||
| """ | ||
|
|
||
| if acc.client_id is None: | ||
| raise HTTPException(403, detail="Client profile required") | ||
|
|
||
| result = db.exec( | ||
| select(ClientCoachRequest, ClientCoachRelationship) | ||
| .join(ClientCoachRelationship, ClientCoachRelationship.request_id == ClientCoachRequest.id) | ||
| .where( | ||
| ClientCoachRequest.client_id == acc.client_id, | ||
| ClientCoachRequest.is_accepted == True, | ||
| ClientCoachRelationship.is_active == True, | ||
| ClientCoachRelationship.client_blocked == False, | ||
| ClientCoachRelationship.coach_blocked == False, | ||
| ) | ||
| ).first() | ||
|
|
||
| if result is None: | ||
| raise HTTPException(404, detail="No active coach relationship found") | ||
|
|
||
| request, relationship = result | ||
|
|
||
| return { | ||
| "relationship_id": relationship.id, | ||
| "request_id": request.id, | ||
| "client_id": request.client_id, | ||
| "coach_id": request.coach_id, | ||
| "created_at": relationship.created_at, | ||
| "is_active": relationship.is_active, | ||
| } |
There was a problem hiding this comment.
Duplicate /my_coach route — new handler is unreachable.
@router.get("/my_coach") is already registered at line 556 (get_my_coach returning MyCoachResponse). FastAPI dispatches to the first-matched route, so this new handler at 705 is dead code and the PR's intended supersede behavior (relationship metadata, 403/404 semantics, active+non-blocked filtering) will never run. The two implementations also disagree on response shape (MyCoachResponse vs. raw dict) and on the missing-client-id semantics (404 vs. 403), which would silently drift between docs and behavior.
Per the PR summary, the older handler was meant to be removed. Delete lines 556–578 so this new one is the only registration (or merge them into one), and ideally add a response_model to keep the API contract documented.
🛠️ Suggested cleanup
Remove the previous get_my_coach (lines 556–578):
-@router.get("/my_coach", response_model=MyCoachResponse)
-def get_my_coach(db = Depends(get_session), acc: Account = Depends(get_client_account)):
- """
- Returns the coach of a specific client
- """
-
- if acc is None:
- raise HTTPException(404, detail="Account not found")
-
- coach_request = db.query(ClientCoachRequest).filter(ClientCoachRequest.client_id == acc.client_id).first()
-
- # If no request found or the request hasn't been accepted, surface as not found.
- if coach_request is None or not getattr(coach_request, "is_accepted", False):
- raise HTTPException(404, detail="No active coach relationship found")
-
- relationship = db.query(ClientCoachRelationship).filter(ClientCoachRelationship.request_id == coach_request.id).first()
-
- if relationship is None:
- raise HTTPException(404, detail="Relationship not Found")
-
- coach = db.query(Coach).filter(Coach.id == coach_request.coach_id).first()
-
- return MyCoachResponse(coach = coach)🧰 Tools
🪛 Ruff (0.15.12)
[warning] 706-706: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
[warning] 706-706: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
[error] 719-719: Avoid equality comparisons to True; use ClientCoachRequest.is_accepted: for truth checks
Replace with ClientCoachRequest.is_accepted
(E712)
[error] 720-720: Avoid equality comparisons to True; use ClientCoachRelationship.is_active: for truth checks
Replace with ClientCoachRelationship.is_active
(E712)
[error] 721-721: Avoid equality comparisons to False; use not ClientCoachRelationship.client_blocked: for false checks
Replace with not ClientCoachRelationship.client_blocked
(E712)
[error] 722-722: Avoid equality comparisons to False; use not ClientCoachRelationship.coach_blocked: for false checks
Replace with not ClientCoachRelationship.coach_blocked
(E712)
🤖 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 `@src/api/roles/client/client.py` around lines 705 - 738, There is a duplicate
route registration for `@router.get`("/my_coach") — remove the older get_my_coach
implementation that returns MyCoachResponse so the new get_my_coach (the handler
that queries ClientCoachRequest and ClientCoachRelationship) becomes the sole
route; ensure the remaining handler declares a response_model (create or reuse
MyCoachResponse or a compatible Pydantic model) and keep the intended semantics:
require Account via Depends(get_client_account), return 403 when acc.client_id
is None, return 404 when no active non-blocked relationship is found, and return
the relationship payload with the fields currently produced by the new handler.
| relationships = db.exec( | ||
| select(ClientCoachRequest, ClientCoachRelationship) | ||
| .join(ClientCoachRelationship, ClientCoachRelationship.request_id == ClientCoachRequest.id) | ||
| .where( | ||
| ClientCoachRequest.coach_id == acc.coach_id, | ||
| ClientCoachRequest.is_accepted == True, | ||
| ClientCoachRelationship.is_active == True, | ||
| ClientCoachRelationship.client_blocked == False, | ||
| ClientCoachRelationship.coach_blocked == False, | ||
| ) | ||
| ).all() | ||
|
|
||
| clients = [] | ||
|
|
||
| for request, relationship in relationships: | ||
| client = db.get(Client, request.client_id) | ||
|
|
||
| account = db.exec( | ||
| select(Account).where(Account.client_id == request.client_id) | ||
| ).first() | ||
|
|
||
| telemetry_records = db.exec( | ||
| select(ClientTelemetry) | ||
| .where(ClientTelemetry.client_id == request.client_id) | ||
| .order_by(ClientTelemetry.date.desc()) | ||
| ).all() | ||
|
|
||
| telemetry = [] | ||
|
|
||
| for t in telemetry_records: | ||
| health_metrics = db.exec( | ||
| select(HealthMetrics).where(HealthMetrics.client_telemetry_id == t.id) | ||
| ).all() | ||
|
|
||
| step_counts = db.exec( | ||
| select(StepCount).where(StepCount.client_telemetry_id == t.id) | ||
| ).all() | ||
|
|
||
| mood_surveys = db.exec( | ||
| select(DailyMoodSurvey).where(DailyMoodSurvey.client_telemetry_id == t.id) | ||
| ).all() | ||
|
|
||
| workout_surveys = db.exec( | ||
| select(DailyWorkoutSurvey).where(DailyWorkoutSurvey.client_telemetry_id == t.id) | ||
| ).all() | ||
|
|
||
| body_metrics_surveys = db.exec( | ||
| select(DailyBodyMetricsSurvey).where(DailyBodyMetricsSurvey.client_telemetry_id == t.id) | ||
| ).all() | ||
|
|
||
| steps_surveys = db.exec( | ||
| select(DailyStepsSurvey).where(DailyStepsSurvey.client_telemetry_id == t.id) | ||
| ).all() | ||
|
|
||
| meal_surveys = db.exec( | ||
| select(DailyMealSurvey).where(DailyMealSurvey.client_telemetry_id == t.id) | ||
| ).all() | ||
|
|
||
| completed_meals = db.exec( | ||
| select(CompletedMealActivity).where(CompletedMealActivity.client_telemetry_id == t.id) | ||
| ).all() | ||
|
|
||
| completed_workouts = db.exec( | ||
| select(CompletedWorkout).where(CompletedWorkout.client_telemetry_id == t.id) | ||
| ).all() |
There was a problem hiding this comment.
/my_clients currently has a heavy N+1 query pattern.
This endpoint issues many per-relationship and per-telemetry queries (client, account, then multiple child tables per telemetry row). With real data, this can become a latency hotspot and cause request timeouts.
Consider batching by IDs (IN (...)) per table and assembling results in memory, instead of querying inside nested loops.
🧰 Tools
🪛 Ruff (0.15.12)
[error] 564-564: Avoid equality comparisons to True; use ClientCoachRequest.is_accepted: for truth checks
Replace with ClientCoachRequest.is_accepted
(E712)
[error] 565-565: Avoid equality comparisons to True; use ClientCoachRelationship.is_active: for truth checks
Replace with ClientCoachRelationship.is_active
(E712)
[error] 566-566: Avoid equality comparisons to False; use not ClientCoachRelationship.client_blocked: for false checks
Replace with not ClientCoachRelationship.client_blocked
(E712)
[error] 567-567: Avoid equality comparisons to False; use not ClientCoachRelationship.coach_blocked: for false checks
Replace with not ClientCoachRelationship.coach_blocked
(E712)
🤖 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 `@src/api/roles/coach/coach.py` around lines 559 - 623, The code in the
get-my-clients flow uses N+1 queries inside the relationships loop
(variables/queries around relationships, fetching Client, Account,
ClientTelemetry, then HealthMetrics/StepCount/Daily* and Completed* per
telemetry) — collect IDs up front and batch-query each table with IN(...) and
then assemble in memory. Specifically: after computing relationships from
ClientCoachRequest/ClientCoachRelationship, gather all client_ids and
telemetry_ids, then replace the per-relationship db.get(Account) with a single
select(Account).where(Account.client_id.in_(client_ids)), replace the
per-request ClientTelemetry selects with one
select(ClientTelemetry).where(ClientTelemetry.client_id.in_(client_ids)), and
replace each child-table select (HealthMetrics, StepCount, DailyMoodSurvey,
DailyWorkoutSurvey, DailyBodyMetricsSurvey, DailyStepsSurvey, DailyMealSurvey,
CompletedMealActivity, CompletedWorkout) with single selects using
client_telemetry_id.in_(telemetry_ids); build maps by id (client->account,
client->telemetries list, telemetry->child lists) and assemble the clients list
from those maps instead of querying inside the loops.
| if account: | ||
| safe_account = { | ||
| "id": account.id, | ||
| "name": account.name, | ||
| "email": account.email, | ||
| "is_active": account.is_active, | ||
| "status": account.status, | ||
| "gender": account.gender, |
There was a problem hiding this comment.
Remove access to non-existent Account.status in the response payload.
Line 646 references account.status, but Account does not define that field. This will raise at runtime when a related account exists.
Suggested fix
if account:
safe_account = {
"id": account.id,
"name": account.name,
"email": account.email,
"is_active": account.is_active,
- "status": account.status,
"gender": account.gender,
"bio": account.bio,
"age": account.age,
"pfp_url": account.pfp_url,📝 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.
| if account: | |
| safe_account = { | |
| "id": account.id, | |
| "name": account.name, | |
| "email": account.email, | |
| "is_active": account.is_active, | |
| "status": account.status, | |
| "gender": account.gender, | |
| if account: | |
| safe_account = { | |
| "id": account.id, | |
| "name": account.name, | |
| "email": account.email, | |
| "is_active": account.is_active, | |
| "gender": account.gender, |
🤖 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 `@src/api/roles/coach/coach.py` around lines 640 - 647, The response payload
builds safe_account including a non-existent field account.status which will
raise at runtime; remove the "status" entry from the safe_account dict (the
block that constructs safe_account in the function that checks if account:) so
the keys only reference real Account attributes (id, name, email, is_active,
gender, etc.), and run tests to ensure no other code expects
safe_account["status"].
| "ccnum": "4111111111111111", | ||
| "cv": "123", | ||
| "exp_date": "2026-12-31" |
There was a problem hiding this comment.
Avoid committing raw PAN-like card numbers in test fixtures.
Line 7 hardcodes a full card-number literal. This commonly triggers security/compliance scanners and can block CI, even for test code.
Use a non-PAN tokenized placeholder (or shared test payload helper) so card-like numbers are not stored directly in source.
🧰 Tools
🪛 Microsoft Presidio Analyzer (2.2.362)
[high] 7-7: Detected payment card number with confidence 1.00. Remove the card number or use only payment-provider test PANs (e.g. 4242 4242 4242 4242).
(CREDIT_CARD)
🪛 OpenGrep (1.20.0)
[ERROR] 7-7: Possible credit card number (PAN) detected in source code. Credit card numbers should never be hardcoded or stored in source files. Use a secrets manager or tokenization service instead.
(coderabbit.pii.credit-card-number)
🤖 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 `@tests/test_client_routes.py` around lines 7 - 9, The test fixture contains a
raw PAN-like card number under the "ccnum" field which can trigger security
scanners; replace the hardcoded literal with a non-PAN token or a shared test
helper constant (e.g., use a TEST_CARD_TOKEN or a get_test_card_payload()
helper) and update any tests referencing "ccnum" to use that tokenized
placeholder instead; ensure the "cv" and "exp_date" fields remain realistic but
not real credentials and centralize the placeholder in a test helper/module so
future fixtures reuse it (locate the JSON in tests/test_client_routes.py around
the "ccnum"/"cv"/"exp_date" keys).
2 participants
Summary by CodeRabbit
New Features
Improvements