Conversation
Signed-off-by: Alexander Alemayhu <alexander@alemayhu.com>
Signed-off-by: Alexander Alemayhu <alexander@alemayhu.com>
Signed-off-by: Alexander Alemayhu <alexander@alemayhu.com>
|
Warning Rate limit exceeded
⌛ 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. 📝 WalkthroughWalkthroughThe changes introduce subscription management functionality including a new SubscriptionService for handling Stripe subscription cancellations, integrate subscription cleanup into the user deletion workflow, add an API endpoint to check subscription status, and adjust environment-specific configurations for allowed origins and cleanup scheduling. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Controller as UsersController
participant SubService as SubscriptionService
participant Stripe as Stripe API
participant DB as Database
User->>Controller: DELETE /users/{id}
Controller->>DB: Fetch user by owner ID
Controller->>SubService: cancelUserSubscriptions(user.email)
SubService->>DB: Query active subscriptions for email
SubService->>Stripe: Cancel subscription (per subscription)
Stripe-->>SubService: Confirmation
SubService->>DB: Mark subscription inactive
SubService-->>Controller: Cleanup complete
Controller->>DB: Delete user account
DB-->>Controller: User deleted
Controller-->>User: 200 OK
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/constants.ts (1)
5-28:⚠️ Potential issue | 🟡 MinorTrailing slashes in origin URLs may cause CORS mismatches.
Lines 13 and 15 have trailing slashes (
https://notion.2anki.net/,https://templates.2anki.net/). BrowserOriginheaders never include a trailing slash, so these entries will never match a CORS check. This appears to be a pre-existing issue, but worth fixing while you're touching this block.The conditional composition via
LOCAL_DEVlooks good overall.Proposed fix
- 'https://notion.2anki.net/', + 'https://notion.2anki.net', 'https://staging.2anki.net', - 'https://templates.2anki.net/', + 'https://templates.2anki.net',src/controllers/UsersControllers.ts (1)
251-256:⚠️ Potential issue | 🟡 MinorPre-existing: guard condition may allow execution with a falsy
owner.The condition
!owner && req.body.confirmed === trueon line 253 only guards against the case whereowneris falsy andconfirmedistrue. Ifowneris falsy andconfirmedis nottrue, execution falls through togetUserById(owner)with a falsy value. Consider whether the guard should be!owneralone (or!owner || !req.body.confirmed).This is pre-existing, but the new code on line 259 now relies on
ownerbeing valid, making this gap slightly more impactful.
🤖 Fix all issues with AI agents
In `@src/controllers/StripeController/StripeController.ts`:
- Around line 96-98: Remove the dead wrapper method
StripeController.cancelUserSubscriptions: locate the async method
cancelUserSubscriptions in class StripeController and delete it, ensuring no
other code references it (replace any calls with direct
SubscriptionService.cancelUserSubscriptions if found); also remove any related
imports or tests that only exist for this method and run the test/compile to
confirm no remaining references.
In `@src/services/SubscriptionService.ts`:
- Around line 43-50: The current sequence calls stripe.subscriptions.cancel(...)
then updates the local record via database('subscriptions').where({ id:
subscription.id }).update({ active: false }), which can leave the DB active if
the update fails; fix by making the operation atomic: update the local record
first (set active=false or set a pending_cancellation flag) using
database('subscriptions').where({ id: subscription.id }).update(...), then call
stripe.subscriptions.cancel(stripeSubscription.id); if the Stripe call fails,
either revert the DB change or persist a pending_cancellation marker and enqueue
a retry/job to re-attempt cancellation, or alternatively implement a short
targeted retry loop around the DB update when stripe.subscriptions.cancel
succeeds so the two states cannot diverge.
- Line 19: The log in SubscriptionService currently prints the raw userEmail
(console.log(`Found ${subscriptions.length} active subscriptions for user
${userEmail}`)), which is PII; change it to avoid logging the plain email by
either using an opaque identifier (e.g., userId if available) or a
redacted/hashed form of userEmail (implement a small helper like redactEmail or
hashEmail and call it here), and update the message to reference subscriptions
and the safe identifier (e.g., `Found ${subscriptions.length} active
subscriptions for user <redacted>` or using userId) so no raw email is emitted
from the method in SubscriptionService.
- Around line 78-84: Make deactivateSubscription a static method to match the
rest of the API: change the declaration to "static async
deactivateSubscription(subscriptionId: number): Promise<void>" in
SubscriptionService, ensure it doesn't reference instance state (no use of
this), keep the same internals (calling getDatabase() and updating
'subscriptions'), and update any callers to call
SubscriptionService.deactivateSubscription(...) rather than instantiating the
class.
🧹 Nitpick comments (1)
src/controllers/StripeController/StripeController.ts (1)
62-70: Service instantiation is duplicated fromgetSuccessfulCheckout.Lines 62-70 are nearly identical to lines 22-30. Consider extracting a private helper (e.g.,
getAuthService()) to reduce duplication and keep the two methods in sync.Example extraction
+ private getServices() { + const database = getDatabase(); + const emailService = useDefaultEmailService(); + const userRepository = new UsersRepository(database); + const usersService = new UsersService(userRepository, emailService); + const tokenRepository = new TokenRepository(database); + const authService = new AuthenticationService(tokenRepository, userRepository); + return { database, usersService, authService }; + }
| async cancelUserSubscriptions(userEmail: string): Promise<void> { | ||
| return SubscriptionService.cancelUserSubscriptions(userEmail); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'cancelUserSubscriptions' --type=ts -C2Repository: 2anki/server
Length of output: 1266
Remove dead code: StripeController.cancelUserSubscriptions is an unused wrapper.
The method only proxies to SubscriptionService.cancelUserSubscriptions, but the only caller in the codebase (UsersControllers.ts line 265) calls SubscriptionService directly. This wrapper adds no value and should be removed.
🤖 Prompt for AI Agents
In `@src/controllers/StripeController/StripeController.ts` around lines 96 - 98,
Remove the dead wrapper method StripeController.cancelUserSubscriptions: locate
the async method cancelUserSubscriptions in class StripeController and delete
it, ensuring no other code references it (replace any calls with direct
SubscriptionService.cancelUserSubscriptions if found); also remove any related
imports or tests that only exist for this method and run the test/compile to
confirm no remaining references.
| }) | ||
| .andWhere({ active: true }); | ||
|
|
||
| console.log(`Found ${subscriptions.length} active subscriptions for user ${userEmail}`); |
There was a problem hiding this comment.
Logging user email is a PII/compliance risk.
console.log on line 19 includes the raw user email. This can end up in log aggregation systems and may violate GDPR/CCPA requirements. Redact or omit the email, or use an opaque user identifier instead.
Proposed fix
- console.log(`Found ${subscriptions.length} active subscriptions for user ${userEmail}`);
+ console.log(`Found ${subscriptions.length} active subscriptions for user`);📝 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.
| console.log(`Found ${subscriptions.length} active subscriptions for user ${userEmail}`); | |
| console.log(`Found ${subscriptions.length} active subscriptions for user`); |
🤖 Prompt for AI Agents
In `@src/services/SubscriptionService.ts` at line 19, The log in
SubscriptionService currently prints the raw userEmail (console.log(`Found
${subscriptions.length} active subscriptions for user ${userEmail}`)), which is
PII; change it to avoid logging the plain email by either using an opaque
identifier (e.g., userId if available) or a redacted/hashed form of userEmail
(implement a small helper like redactEmail or hashEmail and call it here), and
update the message to reference subscriptions and the safe identifier (e.g.,
`Found ${subscriptions.length} active subscriptions for user <redacted>` or
using userId) so no raw email is emitted from the method in SubscriptionService.
| if (stripeSubscription?.id) { | ||
| console.log(`Cancelling Stripe subscription ${stripeSubscription.id}`); | ||
| await stripe.subscriptions.cancel(stripeSubscription.id); | ||
|
|
||
| // Update local database to mark as inactive | ||
| await database('subscriptions') | ||
| .where({ id: subscription.id }) | ||
| .update({ active: false }); |
There was a problem hiding this comment.
Stripe cancellation and DB update are not atomic — local record stays active if the DB update fails.
If stripe.subscriptions.cancel succeeds on line 45 but the DB update on lines 48-50 throws, the outer catch (line 56) logs and continues, leaving the local record marked active while Stripe has already cancelled the subscription. Consider swapping the order (mark inactive first, then cancel in Stripe) or adding a targeted retry for the DB update.
🤖 Prompt for AI Agents
In `@src/services/SubscriptionService.ts` around lines 43 - 50, The current
sequence calls stripe.subscriptions.cancel(...) then updates the local record
via database('subscriptions').where({ id: subscription.id }).update({ active:
false }), which can leave the DB active if the update fails; fix by making the
operation atomic: update the local record first (set active=false or set a
pending_cancellation flag) using database('subscriptions').where({ id:
subscription.id }).update(...), then call
stripe.subscriptions.cancel(stripeSubscription.id); if the Stripe call fails,
either revert the DB change or persist a pending_cancellation marker and enqueue
a retry/job to re-attempt cancellation, or alternatively implement a short
targeted retry loop around the DB update when stripe.subscriptions.cancel
succeeds so the two states cannot diverge.
| async deactivateSubscription(subscriptionId: number): Promise<void> { | ||
| const database = getDatabase(); | ||
|
|
||
| await database('subscriptions') | ||
| .where({ id: subscriptionId }) | ||
| .update({ active: false }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
deactivateSubscription is an instance method while the rest are static — inconsistent API.
cancelUserSubscriptions and getUserActiveSubscriptions are static, but deactivateSubscription is an instance method. Callers would need to instantiate SubscriptionService only for this one method, which is awkward. Make it static for consistency.
Proposed fix
- async deactivateSubscription(subscriptionId: number): Promise<void> {
+ static async deactivateSubscription(subscriptionId: number): Promise<void> {📝 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.
| async deactivateSubscription(subscriptionId: number): Promise<void> { | |
| const database = getDatabase(); | |
| await database('subscriptions') | |
| .where({ id: subscriptionId }) | |
| .update({ active: false }); | |
| } | |
| static async deactivateSubscription(subscriptionId: number): Promise<void> { | |
| const database = getDatabase(); | |
| await database('subscriptions') | |
| .where({ id: subscriptionId }) | |
| .update({ active: false }); | |
| } |
🤖 Prompt for AI Agents
In `@src/services/SubscriptionService.ts` around lines 78 - 84, Make
deactivateSubscription a static method to match the rest of the API: change the
declaration to "static async deactivateSubscription(subscriptionId: number):
Promise<void>" in SubscriptionService, ensure it doesn't reference instance
state (no use of this), keep the same internals (calling getDatabase() and
updating 'subscriptions'), and update any callers to call
SubscriptionService.deactivateSubscription(...) rather than instantiating the
class.
Signed-off-by: Alexander Alemayhu <alexander@alemayhu.com>
Will be used to purge old accounts Signed-off-by: Alexander Alemayhu <alexander@alemayhu.com>
Signed-off-by: Alexander Alemayhu <alexander@alemayhu.com>
Signed-off-by: Alexander Alemayhu <alexander@alemayhu.com>
|
1 participant
This addresses most of the manual cases I recieve on a weekly basis:
Summary by CodeRabbit
New Features
Chores