[CORRUPTED] Synthetic Benchmark PR #25312 - fix: break circular dependency between reminderScheduler and credit-service

[tomerqodo]

User description

Benchmark PR calcom#25312

Type: Corrupted (contains bugs)

Original PR Title: fix: break circular dependency between reminderScheduler and credit-service
Original PR Description: ## What does this PR do?

• Removes the circular dep between packages/features/ee/workflows/lib/reminders/reminderScheduler.ts and packages/features/ee/billing/credit-service.ts
• I created WorkflowReminderRepository for prisma queries and made cancelScheduledMessagesAndScheduleEmails accept pre-computed userIdsWithNoCredits parameter

Mandatory Tasks (DO NOT REMOVE)

• I have self-reviewed the code
• I have updated the developer docs in /docs if this PR makes changes that would require a documentation change. N/A - internal refactoring only
• I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

• Covered by existing tests
  Original PR URL: fix: break circular dependency between reminderScheduler and credit-service calcom/cal.com#25312

---

PR Type

Bug fix, Enhancement

---

Description

• Breaks circular dependency between reminderScheduler and credit-service

• Moves credit-checking logic to CreditService with new private method

• Creates WorkflowReminderRepository to encapsulate Prisma queries

• Refactors cancelScheduledMessagesAndScheduleEmails to accept pre-computed userIdsWithNoCredits parameter

• Updates tests to pass userIdsWithNoCredits instead of computing internally

---

Diagram Walkthrough

flowchart LR
  CS["CreditService"]
  RS["reminderScheduler"]
  WRR["WorkflowReminderRepository"]
  
  CS -- "calls with userIdsWithNoCredits" --> RS
  RS -- "queries via" --> WRR
  CS -- "computes credits" --> CS
  
  style CS fill:#e1f5ff
  style RS fill:#f3e5f5
  style WRR fill:#e8f5e9

Loading

File Walkthrough

	Relevant files
Enhancement	credit-service.ts Add credit-checking logic to CreditService                              packages/features/ee/billing/credit-service.ts Added private method _getUserIdsWithoutCredits to compute user IDs without available credits Modified call to cancelScheduledMessagesAndScheduleEmails to pass pre-computed userIdsWithNoCredits parameter Removed direct userId parameter from the function call Moved credit-checking logic from reminderScheduler to CreditService +41/-5    WorkflowReminderRepository.ts Create WorkflowReminderRepository for database operations packages/features/ee/workflows/repositories/WorkflowReminderRepository.ts Created new repository class to encapsulate workflow reminder database operations Implemented findScheduledMessagesToCancel method to query reminders for users without credits Implemented updateRemindersToEmail method to convert SMS reminders to email Extracted complex Prisma query logic from reminderScheduler Uses WorkflowMethods enum for method filtering +76/-0
Bug fix	reminderScheduler.ts Refactor to use WorkflowReminderRepository and accept pre-computed credits packages/features/ee/workflows/lib/reminders/reminderScheduler.ts Removed circular import of CreditService and credit-checking logic Changed function signature to accept userIdsWithNoCredits parameter instead of computing it Extracted Prisma queries to WorkflowReminderRepository Removed unused WorkflowMethods import and simplified method filtering Delegated database operations to repository methods +12/-88
Tests	reminderScheduler.test.ts Update tests to use userIdsWithNoCredits parameter              packages/features/ee/workflows/lib/reminders/reminderScheduler.test.ts Updated test calls to pass userIdsWithNoCredits parameter instead of userId or teamId Removed unnecessary prismaMock.membership.findMany mock setup Updated test assertions to verify correct userIdsWithNoCredits are used in queries Simplified test setup by removing credit-checking mock logic +6/-7

---

[github-actions]

Hey there and thank you for opening this pull request! 👋🏼

We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted.

Details:

No release type found in pull request title "[CORRUPTED] Synthetic Benchmark PR #25312 - fix: break circular dependency between reminderScheduler and credit-service". Add a prefix to indicate what kind of release this pull request corresponds to. For reference, see https://www.conventionalcommits.org/

Available types:
 - feat: A new feature
 - fix: A bug fix
 - docs: Documentation only changes
 - style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
 - refactor: A code change that neither fixes a bug nor adds a feature
 - perf: A code change that improves performance
 - test: Adding missing tests or correcting existing tests
 - build: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
 - ci: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
 - chore: Other changes that don't modify src or test files
 - revert: Reverts a previous commit

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Missing audit logs: The new credit-related action to cancel scheduled messages due to no credits is not accompanied by an explicit audit log entry capturing who triggered it, when, and the outcome. Referred Code cancelScheduledMessagesAndScheduleEmails({ teamId: result.teamId, userIdsWithNoCredits: await this._getUserIdsWithoutCredits({ teamId: result.teamId ?? null, userId: result.userId ?? null, }), }).catch((error) => { log.error("Failed to cancel scheduled messages", error, { result }); }) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Partial error context: The catch logs errors when cancelling scheduled messages but does not include sufficient operation context (e.g., teamId, computed userIdsWithNoCredits) or ensure fallback behavior if cancellation fails. Referred Code cancelScheduledMessagesAndScheduleEmails({ teamId: result.teamId, userIdsWithNoCredits: await this._getUserIdsWithoutCredits({ teamId: result.teamId ?? null, userId: result.userId ?? null, }), }).catch((error) => { log.error("Failed to cancel scheduled messages", error, { result }); }) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Potential info leak: Logging the raw error object when cancellation fails may expose internal details; verify logs are not user-facing and are properly sanitized. Referred Code }).catch((error) => { log.error("Failed to cancel scheduled messages", error, { result }); }) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Unsanitized logging: The error log includes the entire result object, which may contain sensitive identifiers; ensure sensitive fields are excluded or redacted. Referred Code }).catch((error) => { log.error("Failed to cancel scheduled messages", error, { result }); }) Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Inefficiently checks credits causing performance issues The _getUserIdsWithoutCredits method inefficiently checks credits for each team member individually, causing an N+1 query problem. This should be refactored into a single bulk database operation to avoid performance issues with large teams. Examples: packages/features/ee/billing/credit-service.ts [851-856] await Promise.all( teamMembers.map(async (member) => { const hasCredits = await this.hasAvailableCredits({ userId: member.userId }); return { userId: member.userId, hasCredits }; }) ) Solution Walkthrough: Before: private async _getUserIdsWithoutCredits({ teamId, userId }) { if (teamId) { const teamMembers = await prisma.membership.findMany({ where: { teamId }, }); // Loop over each member, causing an N+1 query problem const results = await Promise.all( teamMembers.map(async (member) => { // `hasAvailableCredits` makes at least one DB query per member const hasCredits = await this.hasAvailableCredits({ userId: member.userId }); return { userId: member.userId, hasCredits }; }) ); return results.filter(({ hasCredits }) => !hasCredits).map(({ userId }) => userId); } // ... } After: private async _getUserIdsWithoutCredits({ teamId, userId }) { if (teamId) { const teamMembers = await prisma.membership.findMany({ where: { teamId }, select: { userId: true }, }); const memberUserIds = teamMembers.map(member => member.userId); // Perform a single, bulk operation to find users without credits // This is a conceptual example; the actual query would be more complex const usersWithoutCredits = await findUsersWithoutCreditsInBulk(memberUserIds); return usersWithoutCredits.map(user => user.id); } // ... } Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a critical N+1 query problem in the new _getUserIdsWithoutCredits method, which would cause severe performance degradation for teams with many members.	High
Possible issue	Fix N+1 query in credit check Refactor the _getUserIdsWithoutCredits method to eliminate the N+1 query problem by batching database reads instead of querying for each user's credit status individually in a loop. packages/features/ee/billing/credit-service.ts [850-859] -userIdsWithNoCredits = ( - await Promise.all( - teamMembers.map(async (member) => { - const hasCredits = await this.hasAvailableCredits({ userId: member.userId }); - return { userId: member.userId, hasCredits }; - }) - ) -) - .filter(({ hasCredits }) => !hasCredits) - .map(({ userId }) => userId); +const teamMemberUserIds = teamMembers.map((member) => member.userId); +// Find all users who have personal credits +const usersWithPersonalCredits = await prisma.credit.findMany({ + where: { + userId: { in: teamMemberUserIds }, + balance: { gt: 0 }, + }, + select: { userId: true }, +}); +const userIdsWithPersonalCredits = new Set(usersWithPersonalCredits.map((c) => c.userId).filter(Boolean)); + +// Find all teams that members belong to +const allMemberships = await prisma.membership.findMany({ + where: { + userId: { in: teamMemberUserIds }, + accepted: true, + }, + select: { userId: true, teamId: true }, +}); + +const allTeamIds = [...new Set(allMemberships.map((m) => m.teamId))]; + +// Find all teams/orgs with credits +const teamsWithCredits = await prisma.team.findMany({ + where: { + id: { in: allTeamIds }, + OR: [ + { credits: { some: { balance: { gt: 0 } } } }, // Team has credits + { parent: { credits: { some: { balance: { gt: 0 } } } } }, // Org has credits + ], + }, + select: { id: true }, +}); +const teamIdsWithCredits = new Set(teamsWithCredits.map((t) => t.id)); + +// Map users to their teams +const userToTeamsMap = new Map<number, number[]>(); +allMemberships.forEach((m) => { + if (!userToTeamsMap.has(m.userId)) { + userToTeamsMap.set(m.userId, []); + } + userToTeamsMap.get(m.userId)?.push(m.teamId); +}); + +// Determine which users have access to team credits +const userIdsWithTeamCredits = new Set<number>(); +for (const [userId, teams] of userToTeamsMap.entries()) { + if (teams.some((teamId) => teamIdsWithCredits.has(teamId))) { + userIdsWithTeamCredits.add(userId); + } +} + +// A user has credits if they have personal credits OR belong to a team with credits. +userIdsWithNoCredits = teamMemberUserIds.filter( + (userId) => !userIdsWithPersonalCredits.has(userId) && !userIdsWithTeamCredits.has(userId) +); + Apply / Chat Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a significant N+1 query performance issue in the newly added _getUserIdsWithoutCredits method, which would degrade performance for large teams. The proposed fix correctly suggests batching database reads to resolve this.	High
	Include WhatsApp reminders in cancellation logic Update the findScheduledMessagesToCancel query to also cancel WhatsApp reminders (WorkflowMethods.WHATSAPP) in addition to SMS reminders when credits are depleted. packages/features/ee/workflows/repositories/WorkflowReminderRepository.ts [31-33] method: { - equals: WorkflowMethods.SMS, + in: [WorkflowMethods.SMS, WorkflowMethods.WHATSAPP], }, Apply / Chat Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a functional regression introduced in the PR. The original code handled both SMS and WhatsApp reminders, but the refactoring dropped WhatsApp, which this suggestion correctly proposes to re-add.	High
General	Clear referenceId when updating reminder method In the updateRemindersToEmail method, set referenceId to null along with updating the method to EMAIL to maintain data integrity. packages/features/ee/workflows/repositories/WorkflowReminderRepository.ts [64-75] static async updateRemindersToEmail({ reminderIds }: { reminderIds: number[] }): Promise<void> { await prisma.workflowReminder.updateMany({ where: { id: { in: reminderIds, }, }, data: { method: WorkflowMethods.EMAIL, + referenceId: null, }, }); } Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly points out that the refactoring in the PR missed clearing the referenceId when converting a reminder to email. This is a data integrity issue and a regression from the previous implementation.	Medium
More