Deactivation payment/notifications#69
Conversation
…backend into deactivation-payment/notifications
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more 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 (4)
📝 WalkthroughWalkthroughThis PR adds subscription cancellation when client–coach relationships are deleted and updates the refresh_payments endpoint to skip canceled subscriptions. Deactivation notifications now report stopped future payments. Tests verify cancellation behavior and endpoint filtering. ChangesSubscription Cancellation on Relationship Deletion and Refresh Filtering
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/api/roles/shared/account.py (1)
410-447: 💤 Low valueMinor: cancel and side-effects flow could be tightened.
Both branches (client-side and coach-side) duplicate the same shape: fetch requests →
cancel_payments_for_request→ delete relationships → delete request. Extracting the inner per-request loop into a small helper would remove the duplication without changing behavior. Optional only; current code is correct.🤖 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 410 - 447, The delete_client_coach_mappings function duplicates logic for handling each ClientCoachRequest; create a small helper (e.g., process_client_coach_request(db, request)) that takes the Session and a ClientCoachRequest, calls cancel_payments_for_request(db, request), deletes all ClientCoachRelationship rows for request.id, and then deletes the request; replace both per-request for-loops in delete_client_coach_mappings with calls to this helper to remove duplication while preserving behavior.tests/test_shared_account_notifications.py (1)
250-254: ⚡ Quick winUse
monkeypatchforCRON_SECRETto avoid leaking env state across tests.
os.environ["CRON_SECRET"] = "test-cron-secret"mutates the process environment for the rest of the pytest run, which can affect other tests that rely onCRON_SECRETbeing unset (e.g., the 500 path in/refresh_payments). Pytest's built-inmonkeypatchfixture cleans up automatically.♻️ Proposed fix using monkeypatch
def test_refresh_payments_skips_canceled_subscription( test_client, db_session, client_auth_header, + monkeypatch, ): ... - os.environ["CRON_SECRET"] = "test-cron-secret" + monkeypatch.setenv("CRON_SECRET", "test-cron-secret") resp = test_client.post( "/refresh_payments", json={"cron_secret": "test-cron-secret"}, )With this change, the
import osadded on line 3 is no longer needed.🤖 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_shared_account_notifications.py` around lines 250 - 254, Replace the direct environment mutation os.environ["CRON_SECRET"] = "test-cron-secret" with pytest's monkeypatch: add the monkeypatch fixture to the test signature and call monkeypatch.setenv("CRON_SECRET", "test-cron-secret") before invoking test_client.post("/refresh_payments", ...); also remove the now-unnecessary import os if it's unused elsewhere in the file so the test does not leak CRON_SECRET to other tests.
🤖 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/shared/account.py`:
- Around line 364-366: The notification `details` string currently set to
"Subscription canceled" must be updated to include both the substrings "future
payments" and "stopped" (case-insensitive) to satisfy the messaging contract and
tests; locate where `message` and `details` are set (the variables `message` and
`details` in src/api/roles/shared/account.py) and replace the fixed details text
with a phrase such as "Future payments tied to this relationship have been
stopped." so that the `test_account_deactivate_sends_notification` and
`test_account_deactivate_coach_notifies_client` assertions pass.
---
Nitpick comments:
In `@src/api/roles/shared/account.py`:
- Around line 410-447: The delete_client_coach_mappings function duplicates
logic for handling each ClientCoachRequest; create a small helper (e.g.,
process_client_coach_request(db, request)) that takes the Session and a
ClientCoachRequest, calls cancel_payments_for_request(db, request), deletes all
ClientCoachRelationship rows for request.id, and then deletes the request;
replace both per-request for-loops in delete_client_coach_mappings with calls to
this helper to remove duplication while preserving behavior.
In `@tests/test_shared_account_notifications.py`:
- Around line 250-254: Replace the direct environment mutation
os.environ["CRON_SECRET"] = "test-cron-secret" with pytest's monkeypatch: add
the monkeypatch fixture to the test signature and call
monkeypatch.setenv("CRON_SECRET", "test-cron-secret") before invoking
test_client.post("/refresh_payments", ...); also remove the now-unnecessary
import os if it's unused elsewhere in the file so the test does not leak
CRON_SECRET to other tests.
🪄 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: e1165a96-0c52-4a9a-8110-7db81a4797d5
📒 Files selected for processing (4)
src/api/app.pysrc/api/roles/shared/account.pysrc/database/account/models.pytests/test_shared_account_notifications.py
| message = f"{deactivated_account.name} has deactivated their account." | ||
| details = f"{role.capitalize()} account {deactivated_account.id} was deactivated." | ||
| details = "Subscription canceled" | ||
|
|
There was a problem hiding this comment.
Critical: notification details does not match the deactivation messaging contract — failing CI.
The fixed string "Subscription canceled" doesn't contain "future payments" or "stopped", but test_account_deactivate_sends_notification (line 157) and test_account_deactivate_coach_notifies_client (line 295) both assert that details includes both substrings (case-insensitive). This is the root cause of the pr-run-tests-prerequisite / test pipeline failures. The intent (per the AI summary and tests) is for affected users to be informed that future payments tied to the relationship have been stopped, which the current string fails to convey.
🐛 Proposed fix to align details with the messaging contract
- details = "Subscription canceled"
+ details = (
+ "Any future payments tied to this coaching relationship have been stopped."
+ )This satisfies both "future payments" and "stopped" token assertions and reads naturally to the affected client/coach.
📝 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.
| message = f"{deactivated_account.name} has deactivated their account." | |
| details = f"{role.capitalize()} account {deactivated_account.id} was deactivated." | |
| details = "Subscription canceled" | |
| message = f"{deactivated_account.name} has deactivated their account." | |
| details = ( | |
| "Any future payments tied to this coaching relationship have been stopped." | |
| ) | |
🤖 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 364 - 366, The notification
`details` string currently set to "Subscription canceled" must be updated to
include both the substrings "future payments" and "stopped" (case-insensitive)
to satisfy the messaging contract and tests; locate where `message` and
`details` are set (the variables `message` and `details` in
src/api/roles/shared/account.py) and replace the fixed details text with a
phrase such as "Future payments tied to this relationship have been stopped." so
that the `test_account_deactivate_sends_notification` and
`test_account_deactivate_coach_notifies_client` assertions pass.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests