Activate/deactive account#57
Merged
Merged
Conversation
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughDeactivated-account handling tightened to return HTTP 403; account deactivation now computes affected accounts, creates notifications, and deletes client/coach mappings. Client and coach role APIs expanded (availability, per-client telemetry, simplified request lists, stricter request validation). Tests and test utilities added/updated for these flows. ChangesAccount Deactivation & Coach/Client APIs
Sequence DiagramsequenceDiagram
participant Client
participant CoachSystem as Server
participant Database
participant Notifications
participant Coach
Client->>CoachSystem: POST /roles/client/coach_requests (coach_id)
CoachSystem->>Database: validate coach exists & is active/verified
alt coach invalid
CoachSystem-->>Client: 404 Not Found
else coach valid
CoachSystem->>Database: create ClientCoachRequest
CoachSystem->>Database: fetch coach Account
CoachSystem->>Notifications: create Notification for coach
Notifications->>Database: persist Notification
CoachSystem-->>Client: 200 OK (request_id)
end
Coach->>CoachSystem: GET /client_requests
CoachSystem->>Database: fetch pending ClientCoachRequests
CoachSystem-->>Coach: 200 [client_id, request_id]
Coach->>CoachSystem: GET /my_clients
CoachSystem->>Database: fetch ClientCoachRelationships + client Accounts
CoachSystem->>Database: fetch per-client telemetry
CoachSystem-->>Coach: 200 [clients + telemetry]
Note over Client,Notifications: On account deactivation
Client->>CoachSystem: POST /roles/shared/account/deactivate
CoachSystem->>Database: get_affected_accounts(deactivated_account)
CoachSystem->>Notifications: create deactivated notifications for affected accounts
CoachSystem->>Database: delete_client_coach_mappings(deactivated_account)
CoachSystem->>Database: commit changes
CoachSystem-->>Client: 200 OK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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 unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/roles/coach/coach.py (1)
799-827:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAuthorization helper ignores blocked relationships.
prescribe_workout_plan(lines 324-329) requiresis_active=True AND coach_blocked=False AND client_blocked=False, but_authorize_coach_for_clientonly checksis_active=True. As a result, a coach who is blocked by a client (or who has blocked the client) can still read the client's telemetry, progress pictures, meals, plans, and availability through every endpoint that calls this helper.🔒 Suggested fix
rel = db.exec( select(ClientCoachRelationship).where( ClientCoachRelationship.request_id == accepted.id, ClientCoachRelationship.is_active == True, + ClientCoachRelationship.coach_blocked == False, + ClientCoachRelationship.client_blocked == False, ) ).first()🤖 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 799 - 827, The _authorize_coach_for_client helper currently only checks for an active relationship but ignores blocking flags; update its relationship lookup (the select against ClientCoachRelationship when accepted is found) to require rel.is_active == True AND rel.coach_blocked == False AND rel.client_blocked == False (or use .is_(False) if these are nullable booleans) so it only authorizes when neither party has blocked the other, keeping the existing pending-request check and same HTTPException behavior.
🧹 Nitpick comments (2)
tests/test_client_routes.py (1)
6-10: 💤 Low valueAvoid hardcoding payment card data, even test PANs.
Static analysis flagged the literal as a credit-card number. While
4111111111111111is a well-known Visa test PAN and not a real card, leaving raw card-shaped strings in source triggers PCI/DLP scanners and can fail compliance gates. Consider moving the value to a fixture/constants module (e.g.TEST_CARD = "4111" + "1" * 12) so the literal is not flagged on every scan, and to centralize test-only PAN choices.🤖 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 6 - 10, The test currently embeds a raw PAN string in the payment_information dict (the "ccnum" value), which triggers scanners; extract the test card into a centralized test constant (e.g. TEST_CARD) in a fixtures/constants module and build it without a single literal (for example by concatenating smaller pieces) and replace the inline "ccnum" usage with an import of TEST_CARD; update any tests that reference the payment_information dict to import the constant so the literal no longer appears in test_client_routes.py.tests/test_client_coach_availability.py (1)
119-127: 💤 Low value
datetime.utcnow()is deprecated in Python 3.12+.
datetime.utcnow()emits aDeprecationWarningon Python 3.12+ and produces a naive datetime. Usedatetime.now(timezone.utc)instead for a timezone-aware UTC value.♻️ Suggested change
-from datetime import datetime +from datetime import datetime, timezone @@ - created_at=datetime.utcnow(), + created_at=datetime.now(timezone.utc),🤖 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_coach_availability.py` around lines 119 - 127, Replace the deprecated naive timestamp usage in the ClientCoachRelationship creation: instead of datetime.utcnow() use a timezone-aware UTC timestamp (datetime.now(timezone.utc)); update the imports in the test file (e.g., ensure timezone is imported from datetime or use from datetime import datetime, timezone) and replace the created_at value in the ClientCoachRelationship instantiation accordingly.
🤖 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 209-227: The handler get_coach_availability_for_client currently
only checks existence and verification but does not reject deactivated coaches
and allows coach.coach_availability==None to produce a NULL filter; update the
logic to (1) enforce the coach is active (e.g., check coach.active or the
appropriate deactivation flag and return 404 if false) and (2) guard against a
missing availability id by returning an empty CoachAvailabilityResponse or a 404
when coach.coach_availability is None instead of running the select; only
execute select(Availability).where(Availability.coach_availability_id ==
coach.coach_availability) when coach.coach_availability is a valid id.
In `@src/api/roles/coach/coach.py`:
- Around line 138-154: The code assumes db.get(Coach, coach_acc.coach_id) always
returns a Coach; add a guard after that call to handle None (e.g., raise a
404/NotFound HTTP exception or return an error) before accessing
coach.coach_availability or mutating coach. Specifically, check the result of
db.get(Coach, coach_acc.coach_id) (the variable coach) and bail out with a clear
404 if coach is None so the subsequent logic that uses coach,
coach_availability_id, coach.coach_availability, and db.add(coach) is safe.
- Around line 680-795: The get_my_clients endpoint is causing an N+1 query
problem by loading Client, Account, ClientTelemetry and many telemetry child
tables inside per-client loops (see get_my_clients, ClientCoachRequest,
ClientCoachRelationship, Client, Account, ClientTelemetry, HealthMetrics,
StepCount, DailyMoodSurvey, DailyWorkoutSurvey, DailyBodyMetricsSurvey,
DailyStepsSurvey, DailyMealSurvey, CompletedMealActivity, CompletedWorkout). Fix
by replacing per-row queries with either eager-loading (use
select().options(selectinload/joinedload(...) on
ClientCoachRequest->Client/Account->telemetry->child relations) or by batching:
collect request.client_id and telemetry ids then issue one IN(...) query per
telemetry type and group results in memory), and limit telemetry returned (add
pagination/limit or return a summary and link to existing per-client telemetry
endpoints). Also declare a response_model Pydantic schema for the endpoint to
make the shape explicit and validate serialization.
In `@src/api/roles/shared/account.py`:
- Around line 368-401: The delete_client_coach_mappings function currently
hard-deletes ClientCoachRequest and ClientCoachRelationship rows, causing
irreversible loss of coach/client history; change it to perform a soft-delete or
mark-as-inactive instead: update ClientCoachRequest and ClientCoachRelationship
records (e.g., set an existing soft-delete flag like is_active=false or
deleted_at=now() or status='inactive') rather than calling db.delete, and ensure
the function updates both the ClientCoachRequest (found via
ClientCoachRequest.client_id or coach_id) and associated ClientCoachRelationship
(found via request.id) so relationships are preserved for reactivation; modify
delete_client_coach_mappings to use update operations and commit the changes,
and add checks that rely on those soft-delete fields elsewhere where queries
currently assume hard-deleted rows.
In `@tests/test_client_routes.py`:
- Around line 29-71: The tests (test_get_my_coach, test_get_coach_profile,
test_get_progress_pictures, test_get_my_clients) are using overly permissive
assertions like assert response.status_code in (200, 404) or (200, 409); make
them deterministic by preparing the preconditions and asserting a single
expected status: update the tests that call make_client_profile (or add a new
fixture that creates no profile) so one test asserts the happy path (assert ==
200) when the client/profile/coach exists and a separate test asserts the
missing-resource path (assert == 404 or == 409) when the profile/assignment is
absent; use the existing helper make_client_profile and
coach_auth_header/auth_header to drive state and split each route into two
focused tests (happy and not-found) rather than accepting multiple statuses in
one assertion.
---
Outside diff comments:
In `@src/api/roles/coach/coach.py`:
- Around line 799-827: The _authorize_coach_for_client helper currently only
checks for an active relationship but ignores blocking flags; update its
relationship lookup (the select against ClientCoachRelationship when accepted is
found) to require rel.is_active == True AND rel.coach_blocked == False AND
rel.client_blocked == False (or use .is_(False) if these are nullable booleans)
so it only authorizes when neither party has blocked the other, keeping the
existing pending-request check and same HTTPException behavior.
---
Nitpick comments:
In `@tests/test_client_coach_availability.py`:
- Around line 119-127: Replace the deprecated naive timestamp usage in the
ClientCoachRelationship creation: instead of datetime.utcnow() use a
timezone-aware UTC timestamp (datetime.now(timezone.utc)); update the imports in
the test file (e.g., ensure timezone is imported from datetime or use from
datetime import datetime, timezone) and replace the created_at value in the
ClientCoachRelationship instantiation accordingly.
In `@tests/test_client_routes.py`:
- Around line 6-10: The test currently embeds a raw PAN string in the
payment_information dict (the "ccnum" value), which triggers scanners; extract
the test card into a centralized test constant (e.g. TEST_CARD) in a
fixtures/constants module and build it without a single literal (for example by
concatenating smaller pieces) and replace the inline "ccnum" usage with an
import of TEST_CARD; update any tests that reference the payment_information
dict to import the constant so the literal no longer appears in
test_client_routes.py.
🪄 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: 72f14a53-bcca-40d1-bff7-5ad153d3b8b3
📒 Files selected for processing (11)
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_coach_availability.pytests/test_client_routes.pytests/test_hirable_coaches.pytests/test_shared_account_activation.pytests/test_shared_account_delete.pytests/test_shared_account_notifications.py
Comment on lines
+368
to
+401
| def delete_client_coach_mappings(db: Session, account: Account): | ||
| if account.client_id is not None: | ||
| requests = db.exec( | ||
| select(ClientCoachRequest) | ||
| .where(ClientCoachRequest.client_id == account.client_id) | ||
| ).all() | ||
|
|
||
| for request in requests: | ||
| relationships = db.exec( | ||
| select(ClientCoachRelationship) | ||
| .where(ClientCoachRelationship.request_id == request.id) | ||
| ).all() | ||
|
|
||
| for relationship in relationships: | ||
| db.delete(relationship) | ||
|
|
||
| db.delete(request) | ||
|
|
||
| if account.coach_id is not None: | ||
| requests = db.exec( | ||
| select(ClientCoachRequest) | ||
| .where(ClientCoachRequest.coach_id == account.coach_id) | ||
| ).all() | ||
|
|
||
| for request in requests: | ||
| relationships = db.exec( | ||
| select(ClientCoachRelationship) | ||
| .where(ClientCoachRelationship.request_id == request.id) | ||
| ).all() | ||
|
|
||
| @router.post("/deactivate", response_model=DeactivateAccountResponse) | ||
| def deactivate_account( | ||
| for relationship in relationships: | ||
| db.delete(relationship) | ||
|
|
||
| db.delete(request) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Keep soft deactivation non-destructive.
This helper permanently deletes every ClientCoachRequest and ClientCoachRelationship, so /activate brings the account back without any of its prior coach/client links. For a deactivate/reactivate flow, that is irreversible data loss rather than a reversible status change.
🤖 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/shared/account.py` around lines 368 - 401, The
delete_client_coach_mappings function currently hard-deletes ClientCoachRequest
and ClientCoachRelationship rows, causing irreversible loss of coach/client
history; change it to perform a soft-delete or mark-as-inactive instead: update
ClientCoachRequest and ClientCoachRelationship records (e.g., set an existing
soft-delete flag like is_active=false or deleted_at=now() or status='inactive')
rather than calling db.delete, and ensure the function updates both the
ClientCoachRequest (found via ClientCoachRequest.client_id or coach_id) and
associated ClientCoachRelationship (found via request.id) so relationships are
preserved for reactivation; modify delete_client_coach_mappings to use update
operations and commit the changes, and add checks that rely on those soft-delete
fields elsewhere where queries currently assume hard-deleted rows.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/api/roles/coach/coach.py (2)
848-862:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAuthorize against an accepted request, not an arbitrary historical one.
This query grabs the first request for the coach/client pair with no
is_acceptedfilter. If the pair has an older denied request plus a newer accepted one,first()can select the denied row and incorrectly block every telemetry endpoint behind this helper.Suggested fix
accepted = db.exec( select(ClientCoachRequest).where( ClientCoachRequest.client_id == client_id, ClientCoachRequest.coach_id == coach_id, + ClientCoachRequest.is_accepted == True, ) ).first()🤖 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 848 - 862, The current authorization selects the first ClientCoachRequest for the client/coach pair without checking acceptance, which can pick a denied historical request; update the query that assigns accepted (select(ClientCoachRequest)...) to include a filter for ClientCoachRequest.is_accepted == True and (optionally) order by a timestamp or id desc to prefer the latest accepted request so the subsequent ClientCoachRelationship check uses an actually accepted request.
524-581:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winReject already-resolved requests before creating billing records.
This endpoint can be called again for the same
request_idbecause it never checks that the request is still pending. Re-accepting an already accepted request will create another relationship, subscription, billing cycle, and invoice.Suggested fix
if request.coach_id != acc.coach_id: raise HTTPException(403, detail="Not authorized to accept this request") + + if request.is_accepted is not None: + raise HTTPException(409, detail="This request has already been resolved") # update existing request rather than inserting a new row with the same PK request.is_accepted = True🤖 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 524 - 581, Before creating relationship/subscription/billing, verify the ClientCoachRequest is still pending by checking request.is_accepted (or status) and reject the call if it's already accepted/resolved (e.g., raise HTTPException 400/409); additionally, guard against duplicates by checking for an existing ClientCoachRelationship (query ClientCoachRelationship where request_id == request.id) and/or an existing Subscription for request.client_id and request.coach_id and return the existing resource or raise, instead of creating new Relationship/Subscription/BillingCycle/Invoice. Update the accept handler around the section that sets request.is_accepted and the subsequent creation steps (symbols: ClientCoachRequest, request.is_accepted, ClientCoachRelationship, Subscription, BillingCycle, Invoice) to perform these checks and early-return or error before adding anything to the DB.
♻️ Duplicate comments (1)
src/api/roles/shared/account.py (1)
374-407:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftKeep deactivate/reactivate reversible.
This still hard-deletes
ClientCoachRequestandClientCoachRelationshiprows, so/activatebrings the account back without its prior coach/client links or history. If deactivation is meant to be reversible, these records need an inactive/soft-delete transition instead ofdb.delete(...).🤖 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/shared/account.py` around lines 374 - 407, The delete_client_coach_mappings function currently hard-deletes ClientCoachRequest and ClientCoachRelationship rows, breaking reversible deactivate/reactivate; change it to perform a soft-delete by updating a flag/timestamp on those models instead of calling db.delete: for each ClientCoachRequest and its ClientCoachRelationship(s) found by request.client_id or request.coach_id, set an "is_active" boolean to False or populate a "deleted_at" timestamp (add such fields to ClientCoachRequest and ClientCoachRelationship if they don't exist), persist the updates with db.add/db.merge (or equivalent) so records remain retrievable on account reactivation, and retain existing selection logic in delete_client_coach_mappings to locate affected rows.
🧹 Nitpick comments (1)
tests/test_client_routes.py (1)
4-24: ⚡ Quick winReuse the shared client payload builder here.
This helper redefines the initial survey payload even though
tests.payload_tools.client.build_client_init_payload()already exists. The two copies will drift on required fields and test defaults.Suggested refactor
-from tests.payload_tools.constants import TEST_CARD_NUMBER +from tests.payload_tools.client import build_client_init_payload def make_client_profile(test_client, auth_header): - payload = { - "fitness_goals": { - "goal_enum": "weight loss" - }, - "payment_information": { - "ccnum": TEST_CARD_NUMBER, - "cv": "123", - "exp_date": "2026-12-31" - }, - "availabilities": [ - { - "weekday": "monday", - "start_time": "08:00:00", - "end_time": "10:00:00" - } - ], - "initial_health_metric": { - "weight": 180 - } - } + payload = build_client_init_payload(weight=180) response = test_client.post( "/roles/client/initial_survey",🤖 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 4 - 24, The helper make_client_profile duplicates the initial survey payload; replace the hardcoded payload with a call to the shared builder tests.payload_tools.client.build_client_init_payload(), then modify or extend the returned dict as needed (e.g., set payment_information, availabilities, initial_health_metric) before sending. Update make_client_profile to import/consume build_client_init_payload() and apply only the test-specific overrides so the canonical payload lives in one place.
🤖 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 680-706: The current logic selects the first accepted
ClientCoachRequest and then any ClientCoachRelationship for it, which can return
stale or inactive relationships; update the query flow in the /my_coach handler
(symbols: ClientCoachRequest, coach_request, ClientCoachRelationship,
relationship, Coach, coach_account) to (1) select the most recent accepted
ClientCoachRequest (order_by last_updated descending) and (2) when fetching
ClientCoachRelationship filter for the active/current state (e.g.,
relationship.is_active == True and relationship.is_blocked == False or the
project’s equivalent flags) and tie it to coach_request.id so only an active,
unblocked relationship is returned; if no active relationship exists, raise the
404 as before.
In `@src/api/roles/coach/coach.py`:
- Around line 706-718: The query building the relationships for my_clients uses
offset/limit without an order_by, causing nondeterministic pagination; modify
the select(...) call that populates relationships (selecting ClientCoachRequest
and ClientCoachRelationship) to include a stable order_by (e.g., order by a
monotonic column such as ClientCoachRelationship.id or a created_at timestamp on
ClientCoachRelationship/ClientCoachRequest) and a deterministic direction (ASC
or DESC) before applying .offset(pagination.skip).limit(pagination.limit) so
pages are stable and rows don’t shift or duplicate across requests.
In `@tests/payload_tools/client.py`:
- Around line 12-15: The test helper under the "payment_information" block
currently sets exp_date to a hardcoded date (str(date(2026, 12, 31))) which will
expire and cause failures; update the exp_date for this test payload (near
TEST_ALT_CARD_NUMBER usage) to a far-future date or compute it dynamically
(e.g., use date.today() plus several years) so the test card does not age out;
modify the code that builds the payment payload (the exp_date assignment in
tests/payload_tools/client.py) accordingly.
---
Outside diff comments:
In `@src/api/roles/coach/coach.py`:
- Around line 848-862: The current authorization selects the first
ClientCoachRequest for the client/coach pair without checking acceptance, which
can pick a denied historical request; update the query that assigns accepted
(select(ClientCoachRequest)...) to include a filter for
ClientCoachRequest.is_accepted == True and (optionally) order by a timestamp or
id desc to prefer the latest accepted request so the subsequent
ClientCoachRelationship check uses an actually accepted request.
- Around line 524-581: Before creating relationship/subscription/billing, verify
the ClientCoachRequest is still pending by checking request.is_accepted (or
status) and reject the call if it's already accepted/resolved (e.g., raise
HTTPException 400/409); additionally, guard against duplicates by checking for
an existing ClientCoachRelationship (query ClientCoachRelationship where
request_id == request.id) and/or an existing Subscription for request.client_id
and request.coach_id and return the existing resource or raise, instead of
creating new Relationship/Subscription/BillingCycle/Invoice. Update the accept
handler around the section that sets request.is_accepted and the subsequent
creation steps (symbols: ClientCoachRequest, request.is_accepted,
ClientCoachRelationship, Subscription, BillingCycle, Invoice) to perform these
checks and early-return or error before adding anything to the DB.
---
Duplicate comments:
In `@src/api/roles/shared/account.py`:
- Around line 374-407: The delete_client_coach_mappings function currently
hard-deletes ClientCoachRequest and ClientCoachRelationship rows, breaking
reversible deactivate/reactivate; change it to perform a soft-delete by updating
a flag/timestamp on those models instead of calling db.delete: for each
ClientCoachRequest and its ClientCoachRelationship(s) found by request.client_id
or request.coach_id, set an "is_active" boolean to False or populate a
"deleted_at" timestamp (add such fields to ClientCoachRequest and
ClientCoachRelationship if they don't exist), persist the updates with
db.add/db.merge (or equivalent) so records remain retrievable on account
reactivation, and retain existing selection logic in
delete_client_coach_mappings to locate affected rows.
---
Nitpick comments:
In `@tests/test_client_routes.py`:
- Around line 4-24: The helper make_client_profile duplicates the initial survey
payload; replace the hardcoded payload with a call to the shared builder
tests.payload_tools.client.build_client_init_payload(), then modify or extend
the returned dict as needed (e.g., set payment_information, availabilities,
initial_health_metric) before sending. Update make_client_profile to
import/consume build_client_init_payload() and apply only the test-specific
overrides so the canonical payload lives in one place.
🪄 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: 2955dfa1-1403-4072-a00e-6ad43eccb0a4
📒 Files selected for processing (7)
src/api/roles/client/client.pysrc/api/roles/coach/coach.pysrc/api/roles/shared/account.pytests/payload_tools/client.pytests/payload_tools/constants.pytests/test_client_coach_availability.pytests/test_client_routes.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/api/roles/shared/account.py (1)
376-409:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftHard-deleting relationships on deactivate is irreversible.
delete_client_coach_mappingsis invoked from the/deactivateflow (line 491) anddb.delete(...)s everyClientCoachRequestandClientCoachRelationshiprow tied to the account. When the user later hits/activate,is_activeflips back toTruebut their coach/client links are permanently gone — what is advertised as a reversible deactivation is functionally a partial delete.Either soft-deactivate the rows (e.g., flip
ClientCoachRelationship.is_active=Falseand an analogous flag onClientCoachRequest) so they can be restored on/activate, or only calldelete_client_coach_mappingsfrom/deleteand skip it during/deactivate. The latter is the smaller change if you want to ship now — affected counterparts are still notified and the deactivated account itself can no longer authenticate, so the active links naturally become inert.🤖 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/shared/account.py` around lines 376 - 409, The current delete_client_coach_mappings function hard-deletes ClientCoachRequest and ClientCoachRelationship rows, which makes /deactivate irreversible; either change delete_client_coach_mappings to soft-delete (set an is_active=False flag on ClientCoachRequest and ClientCoachRelationship rows instead of db.delete(...) and commit) so rows can be restored on /activate, or remove the call to delete_client_coach_mappings from the /deactivate flow and only invoke it from the account deletion flow (/delete) so deactivation merely disables the account; locate references to delete_client_coach_mappings and update the function to update the is_active fields (or update the deactivate route handler to stop calling it) while keeping restoration logic in the /activate handler to flip is_active back to True.
🧹 Nitpick comments (2)
src/api/roles/shared/account.py (2)
302-342: ⚡ Quick winCollapse the per-relationship
Accountlookups into a single join.Both branches issue one
SELECT Account ...per relationship row, which is an N+1 over the user's active mappings. The whole thing can be a single query per role using the same joins you already have, and it also drops the unusedrelationshiploop variable that Ruff is flagging (B007).♻️ Proposed refactor
- # If the deactivated account is a client, notify their active coach(es) - if account.client_id is not None: - relationships = db.exec( - select(ClientCoachRequest, ClientCoachRelationship) - .join( - ClientCoachRelationship, - ClientCoachRelationship.request_id == ClientCoachRequest.id, - ) - .where( - ClientCoachRequest.client_id == account.client_id, - ClientCoachRelationship.is_active == True, - ) - ).all() - - for request, relationship in relationships: - coach_account = db.exec( - select(Account).where(Account.coach_id == request.coach_id) - ).first() - - add_affected_account(coach_account) - - # If the deactivated account is a coach, notify their active client(s) - if account.coach_id is not None: - relationships = db.exec( - select(ClientCoachRequest, ClientCoachRelationship) - .join( - ClientCoachRelationship, - ClientCoachRelationship.request_id == ClientCoachRequest.id, - ) - .where( - ClientCoachRequest.coach_id == account.coach_id, - ClientCoachRelationship.is_active == True, - ) - ).all() - - for request, relationship in relationships: - client_account = db.exec( - select(Account).where(Account.client_id == request.client_id) - ).first() - - add_affected_account(client_account) + if account.client_id is not None: + coach_accounts = db.exec( + select(Account) + .join(ClientCoachRequest, ClientCoachRequest.coach_id == Account.coach_id) + .join( + ClientCoachRelationship, + ClientCoachRelationship.request_id == ClientCoachRequest.id, + ) + .where( + ClientCoachRequest.client_id == account.client_id, + ClientCoachRelationship.is_active.is_(True), + ) + ).all() + for coach_account in coach_accounts: + add_affected_account(coach_account) + + if account.coach_id is not None: + client_accounts = db.exec( + select(Account) + .join(ClientCoachRequest, ClientCoachRequest.client_id == Account.client_id) + .join( + ClientCoachRelationship, + ClientCoachRelationship.request_id == ClientCoachRequest.id, + ) + .where( + ClientCoachRequest.coach_id == account.coach_id, + ClientCoachRelationship.is_active.is_(True), + ) + ).all() + for client_account in client_accounts: + add_affected_account(client_account)🤖 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/shared/account.py` around lines 302 - 342, The current logic does an N+1 by querying Account inside the for-loop; instead expand the initial queries to join Account so you retrieve Account rows in the same DB call and remove the unused loop variable. For the client branch (when account.client_id is not None) change the db.exec(select(...)) to also select/join Account (join ClientCoachRelationship on ClientCoachRelationship.request_id == ClientCoachRequest.id, then join Account on Account.coach_id == ClientCoachRequest.coach_id) and iterate the returned Account results calling add_affected_account(account); do the symmetric join for the coach branch (join Account on Account.client_id == ClientCoachRequest.client_id) and remove the per-relationship select(Account) inside the loop and the unused relationship loop variable to eliminate the N+1 and the B007 warning while still using db.exec, select, ClientCoachRequest, ClientCoachRelationship, Account, and add_affected_account.
355-373: 💤 Low valueCompute
role/message strings once, not per recipient.Nothing inside the role/
detailsderivation depends onaffected_account, so the branches re-execute for every notification. Hoist them above the loop for clarity and a tiny perf win.♻️ Proposed refactor
- for affected_account in affected_accounts: - if affected_account.id is None: - continue - - role = "account" - if deactivated_account.client_id is not None: - role = "client" - elif deactivated_account.coach_id is not None: - role = "coach" - - db.add( - Notification( - account_id=affected_account.id, - fav_category="account_deactivated", - message=f"{deactivated_account.name} has deactivated their account.", - details=f"{role.capitalize()} account {deactivated_account.id} was deactivated.", - is_read=False, - ) - ) + if deactivated_account.client_id is not None: + role = "client" + elif deactivated_account.coach_id is not None: + role = "coach" + else: + role = "account" + + message = f"{deactivated_account.name} has deactivated their account." + details = f"{role.capitalize()} account {deactivated_account.id} was deactivated." + + for affected_account in affected_accounts: + if affected_account.id is None: + continue + db.add( + Notification( + account_id=affected_account.id, + fav_category="account_deactivated", + message=message, + details=details, + is_read=False, + ) + )Note: if a single
Accountever has bothclient_idandcoach_idpopulated, thisif/elif(and the original) silently picksclient— confirm that's the intended labeling.🤖 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/shared/account.py` around lines 355 - 373, Compute the role and the derived message/details strings once before iterating affected_accounts instead of inside the loop: determine role from deactivated_account (use deactivated_account.client_id / deactivated_account.coach_id logic), build role_cap = role.capitalize(), build message = f"{deactivated_account.name} has deactivated their account." and details = f"{role_cap} account {deactivated_account.id} was deactivated.", then inside the for loop just create the Notification with those precomputed values; also verify whether client/coach precedence (client wins over coach) is intended.
🤖 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.
Duplicate comments:
In `@src/api/roles/shared/account.py`:
- Around line 376-409: The current delete_client_coach_mappings function
hard-deletes ClientCoachRequest and ClientCoachRelationship rows, which makes
/deactivate irreversible; either change delete_client_coach_mappings to
soft-delete (set an is_active=False flag on ClientCoachRequest and
ClientCoachRelationship rows instead of db.delete(...) and commit) so rows can
be restored on /activate, or remove the call to delete_client_coach_mappings
from the /deactivate flow and only invoke it from the account deletion flow
(/delete) so deactivation merely disables the account; locate references to
delete_client_coach_mappings and update the function to update the is_active
fields (or update the deactivate route handler to stop calling it) while keeping
restoration logic in the /activate handler to flip is_active back to True.
---
Nitpick comments:
In `@src/api/roles/shared/account.py`:
- Around line 302-342: The current logic does an N+1 by querying Account inside
the for-loop; instead expand the initial queries to join Account so you retrieve
Account rows in the same DB call and remove the unused loop variable. For the
client branch (when account.client_id is not None) change the
db.exec(select(...)) to also select/join Account (join ClientCoachRelationship
on ClientCoachRelationship.request_id == ClientCoachRequest.id, then join
Account on Account.coach_id == ClientCoachRequest.coach_id) and iterate the
returned Account results calling add_affected_account(account); do the symmetric
join for the coach branch (join Account on Account.client_id ==
ClientCoachRequest.client_id) and remove the per-relationship select(Account)
inside the loop and the unused relationship loop variable to eliminate the N+1
and the B007 warning while still using db.exec, select, ClientCoachRequest,
ClientCoachRelationship, Account, and add_affected_account.
- Around line 355-373: Compute the role and the derived message/details strings
once before iterating affected_accounts instead of inside the loop: determine
role from deactivated_account (use deactivated_account.client_id /
deactivated_account.coach_id logic), build role_cap = role.capitalize(), build
message = f"{deactivated_account.name} has deactivated their account." and
details = f"{role_cap} account {deactivated_account.id} was deactivated.", then
inside the for loop just create the Notification with those precomputed values;
also verify whether client/coach precedence (client wins over coach) is
intended.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ca9b31cb-2652-4abb-b74b-4dca753c5063
📒 Files selected for processing (2)
src/api/roles/shared/account.pytests/test_shared_account_delete.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_shared_account_delete.py
This was referenced May 7, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests