Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions apps/api/src/database/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,18 @@ export type PaymentRowWithDetails = WithCompositions<
{ paymentDetails: RegistrationFeeRow[] }
>

export type PaymentWithPlayerDetails = {
paymentId: number
paymentCode: string
paymentAmount: string
playerName: string
fees: {
registrationFeeId: number
amount: string
isPaid: number
}[]
}

// =============================================================================
// Common Composition Patterns - Scores
// =============================================================================
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
import { Observable } from "rxjs"
import { map } from "rxjs/operators"

import {
Body,
Controller,
Expand All @@ -8,10 +11,12 @@ import {
ParseIntPipe,
Post,
Query,
Sse,
} from "@nestjs/common"
import type {
AdminRegistration,
AvailableSlotGroup,
BulkRefundPreview,
RefundRequest,
PlayerQuery,
RegisteredPlayer,
Expand All @@ -27,6 +32,7 @@ import type {

import { Admin } from "../../auth"
import { AdminRegistrationService } from "../services/admin-registration.service"
import { BulkRefundProgressTracker } from "../services/bulk-refund-progress-tracker"
import { PlayerService } from "../services/player.service"
import { RefundService } from "../services/refund.service"
import { RegistrationService } from "../services/registration.service"
Expand All @@ -42,6 +48,7 @@ export class AdminRegistrationController {
@Inject(PlayerService) private readonly adminRegisterService: PlayerService,
@Inject(RefundService) private readonly refundService: RefundService,
@Inject(RegistrationService) private readonly registrationService: RegistrationService,
@Inject(BulkRefundProgressTracker) private readonly progressTracker: BulkRefundProgressTracker,
) {}

@Get("players")
Expand Down Expand Up @@ -189,4 +196,34 @@ export class AdminRegistrationController {
await this.refundService.processRefunds(refundRequests, issuerId)
return { success: true }
}

@Get(":eventId/bulk-refund-preview")
async getBulkRefundPreview(
@Param("eventId", ParseIntPipe) eventId: number,
): Promise<BulkRefundPreview> {
return this.refundService.getBulkRefundPreview(eventId)
}

@Sse(":eventId/bulk-refund")
bulkRefund(@Param("eventId", ParseIntPipe) eventId: number): Observable<{ data: string }> {
// Check if operation is already running
const existing = this.progressTracker.getProgressObservable(eventId)
if (existing) {
return existing.pipe(
map((progress) => ({
data: JSON.stringify(progress),
})),
)
}

// Start new bulk refund and return progress stream
const issuerId = 1 // TODO: change issuer to a string
const observable = this.refundService.processBulkRefundsStream(eventId, issuerId)
Comment on lines +219 to +221

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# First, let's verify the file exists and examine the specific lines
if [ -f "apps/api/src/registration/controllers/admin-registration.controller.ts" ]; then
  echo "=== File content around lines 219-221 ==="
  sed -n '210,230p' apps/api/src/registration/controllers/admin-registration.controller.ts
else
  echo "File not found"
fi

Repository: finleysg/bhmc-admin

Length of output: 597


🏁 Script executed:

#!/bin/bash
# Search for issuerId usage patterns in the api directory
echo "=== issuerId usage in API code ==="
rg -n "issuerId" apps/api/src --context 2

echo ""
echo "=== Admin identity/user context patterns ==="
rg -n "req\.user|request\.user|@Req\(" apps/api/src/registration/controllers -A 2 -B 2

Repository: finleysg/bhmc-admin

Length of output: 17426


🏁 Script executed:

#!/bin/bash
# Search for refundService.processBulkRefundsStream to understand the method signature
echo "=== processBulkRefundsStream method signature ==="
rg -n "processBulkRefundsStream" apps/api/src -B 5 -A 10

Repository: finleysg/bhmc-admin

Length of output: 2877


🏁 Script executed:

#!/bin/bash
# Look for other admin-related endpoints to find the pattern
echo "=== Admin context patterns in other controllers ==="
rg -n "@Controller|@Post|@Get" apps/api/src/registration/controllers/admin-registration.controller.ts -A 3

Repository: finleysg/bhmc-admin

Length of output: 2861


🏁 Script executed:

#!/bin/bash
# Quick check on the method signature for getUser/extracting admin identity
echo "=== Checking AuthenticatedRequest type ==="
rg -n "interface AuthenticatedRequest|type AuthenticatedRequest" apps/api/src -A 5

echo ""
echo "=== Checking for admin identity extraction patterns ==="
rg -n "req\.user\.id" apps/api/src/registration/controllers -B 2 -A 1

Repository: finleysg/bhmc-admin

Length of output: 2181


🏁 Script executed:

#!/bin/bash
# Check if there's a request object available in the controller context
echo "=== Full admin-registration.controller.ts first 50 lines ==="
head -50 apps/api/src/registration/controllers/admin-registration.controller.ts

Repository: finleysg/bhmc-admin

Length of output: 1553


Replace hardcoded issuerId with authenticated admin identity from request context.

Hardcoding issuerId = 1 breaks auditability—all refunds are incorrectly attributed to user ID 1 regardless of who issued them. Inject @Req() req: AuthenticatedRequest and use req.user.id instead. This same issue exists at line 195 in the processRefunds method. The pattern is established elsewhere in the codebase (user-registration.controller.ts, user-payments.controller.ts).

🤖 Prompt for AI Agents
In `@apps/api/src/registration/controllers/admin-registration.controller.ts`
around lines 219 - 221, Replace the hardcoded issuerId = 1 with the
authenticated admin ID from the request: inject `@Req`() req: AuthenticatedRequest
into the controller methods and use req.user.id when calling
refundService.processBulkRefundsStream and refundService.processRefunds (the two
locations where issuerId is currently set). Update the parameter list for the
methods that create the observable/handle refunds to accept Req and pass
req.user.id into the refundService calls so refunds are attributed to the actual
authenticated admin.


return observable.pipe(
map((progress) => ({
data: JSON.stringify(progress),
})),
)
}
}
1 change: 1 addition & 0 deletions apps/api/src/registration/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ export { PaymentsService } from "./services/payments.service"
export { RegistrationService } from "./services/registration.service"
export { CleanupService } from "./services/cleanup.service"
export { RefundService } from "./services/refund.service"
export { BulkRefundProgressTracker } from "./services/bulk-refund-progress-tracker"
export { RegistrationRepository } from "./repositories/registration.repository"
export { PaymentsRepository } from "./repositories/payments.repository"
export { RegistrationCleanupCron } from "./cron/registration-cleanup.cron"
Expand Down
3 changes: 3 additions & 0 deletions apps/api/src/registration/registration.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
RefundService,
PlayerService,
CleanupService,
BulkRefundProgressTracker,
} from "./"

@Module({
Expand All @@ -39,6 +40,7 @@ import {
],
providers: [
AdminRegistrationService,
BulkRefundProgressTracker,
CleanupService,
PaymentsRepository,
PaymentsService,
Expand All @@ -52,6 +54,7 @@ import {
],
exports: [
AdminRegistrationService,
BulkRefundProgressTracker,
CleanupService,
PaymentsService,
PlayerService,
Expand Down
60 changes: 59 additions & 1 deletion apps/api/src/registration/repositories/payments.repository.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
import { eq, inArray } from "drizzle-orm"
import { and, eq, inArray, like } from "drizzle-orm"

import { Inject, Injectable } from "@nestjs/common"

import {
DrizzleService,
payment,
PaymentRowWithDetails,
PaymentWithPlayerDetails,
player,
refund,
registrationFee,
registrationSlot,
Expand Down Expand Up @@ -70,6 +72,62 @@ export class PaymentsRepository {
}
}

async findConfirmedPaymentsByEventWithDetails(
eventId: number,
): Promise<PaymentWithPlayerDetails[]> {
const results = await this.drizzle.db
.select({
paymentId: payment.id,
paymentCode: payment.paymentCode,
paymentAmount: payment.paymentAmount,
playerFirstName: player.firstName,
playerLastName: player.lastName,
registrationFeeId: registrationFee.id,
feeAmount: registrationFee.amount,
isPaid: registrationFee.isPaid,
})
.from(payment)
.innerJoin(registrationFee, eq(payment.id, registrationFee.paymentId))
.innerJoin(registrationSlot, eq(registrationFee.registrationSlotId, registrationSlot.id))
.innerJoin(player, eq(registrationSlot.playerId, player.id))
.where(
and(
eq(payment.eventId, eventId),
eq(payment.confirmed, 1),
like(payment.paymentCode, "pi_%"),
),
)

// Group by payment ID
const paymentsMap = new Map<number, PaymentWithPlayerDetails>()
for (const row of results) {
const existing = paymentsMap.get(row.paymentId)
if (existing) {
existing.fees.push({
registrationFeeId: row.registrationFeeId,
amount: row.feeAmount,
isPaid: row.isPaid,
})
} else {
paymentsMap.set(row.paymentId, {
paymentId: row.paymentId,
paymentCode: row.paymentCode,
paymentAmount: row.paymentAmount,
playerName: `${row.playerFirstName} ${row.playerLastName}`,
fees: [
{
registrationFeeId: row.registrationFeeId,
amount: row.feeAmount,
isPaid: row.isPaid,
},
],
})
}
Comment on lines +75 to +125

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# Find schema definitions
fd -type f -name '*.ts' apps/api/src/database | head -20

Repository: finleysg/bhmc-admin

Length of output: 233


🏁 Script executed:

# Search for schema definitions with payment, registrationFee, and relevant tables
rg -n "export const payment|export const registrationFee|export const registrationSlot|export const player" apps/api/src --type ts -A 10 | head -100

Repository: finleysg/bhmc-admin

Length of output: 8491


🏁 Script executed:

# Look for the payments.repository.ts file and examine its context
cat -n apps/api/src/registration/repositories/payments.repository.ts | head -150

Repository: finleysg/bhmc-admin

Length of output: 5261


🏁 Script executed:

# Search for how payments are created/inserted to understand cardinality
rg -n "paymentId|registrationFee.*insert|payment.*insert" apps/api/src/registration -g '*.ts' -B 2 -A 5 | head -100

Repository: finleysg/bhmc-admin

Length of output: 8531


🏁 Script executed:

# Confirm the schema for registrationSlot and its FK to player
cat -n apps/api/src/database/schema/registration.schema.ts | sed -n '100,150p'

Repository: finleysg/bhmc-admin

Length of output: 2021


🏁 Script executed:

# Check registrationFee schema for paymentId and registrationSlotId
cat -n apps/api/src/database/schema/registration.schema.ts | sed -n '125,145p'

Repository: finleysg/bhmc-admin

Length of output: 882


Payment may span multiple players; playerName field will only reflect the first player.

The cardinality allows a single payment to have multiple registrationFee records pointing to different registrationSlots (each with its own player). The grouping logic stores only the first player's name per payment, but the fees array can include fees from other players. Either return an array of distinct player names or attach playerName to each individual fee.

🤖 Prompt for AI Agents
In `@apps/api/src/registration/repositories/payments.repository.ts` around lines
75 - 125, The current findConfirmedPaymentsByEventWithDetails logic writes a
single playerName per payment in paymentsMap but a payment can have fees for
multiple players; update the model and grouping to preserve per-fee player info
or collect distinct player names: modify the PaymentWithPlayerDetails type and
the loop in findConfirmedPaymentsByEventWithDetails so each fee entry includes
playerFirstName/playerLastName (or playerName) OR add a players: string[] (or
players: {firstName,lastName}[]) on the payment and, when iterating results,
either push a fee object that includes the
row.playerFirstName/row.playerLastName or maintain a Set to collect unique
player names and assign players array before setting/returning the map values
(adjust paymentsMap handling and the initial object creation accordingly).

}

return Array.from(paymentsMap.values())
}

async createPayment(data: PaymentInsert): Promise<number> {
const [result] = await this.drizzle.db.insert(payment).values(data)
return Number(result.insertId)
Expand Down
122 changes: 122 additions & 0 deletions apps/api/src/registration/services/bulk-refund-progress-tracker.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
import { Observable, Subject } from "rxjs"

import { Injectable, Logger } from "@nestjs/common"
import { BulkRefundProgressEvent, BulkRefundResponse } from "@repo/domain/types"

const PROGRESS_CLEANUP_MS = 5 * 60 * 1000 // 5 minutes

@Injectable()
export class BulkRefundProgressTracker {
private readonly logger = new Logger(BulkRefundProgressTracker.name)

private readonly activeOperations = new Map<number, Subject<BulkRefundProgressEvent>>()

/**
* Start tracking a new bulk refund operation and return the progress subject
*/
startTracking(eventId: number): Subject<BulkRefundProgressEvent> {
if (this.activeOperations.has(eventId)) {
throw new Error(`Bulk refund operation already in progress for event ${eventId}`)
}

const subject = new Subject<BulkRefundProgressEvent>()
this.activeOperations.set(eventId, subject)

// Auto-cleanup after timeout
setTimeout(() => {
this.cleanupOperation(eventId)
}, PROGRESS_CLEANUP_MS)

Comment on lines +17 to +29

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find and read the bulk-refund-progress-tracker.ts file
fd "bulk-refund-progress-tracker.ts" --type f | head -1 | xargs cat -n

Repository: finleysg/bhmc-admin

Length of output: 4005


🏁 Script executed:

#!/bin/bash
# Also check for PROGRESS_CLEANUP_MS constant definition
rg "PROGRESS_CLEANUP_MS" --type ts -B 2 -A 2

Repository: finleysg/bhmc-admin

Length of output: 986


🏁 Script executed:

#!/bin/bash
# Search for any PRD or documentation files related to bulk refund
fd -e "md" -e "json" -e "txt" | xargs grep -l "bulk.refund\|bulk-refund" 2>/dev/null | head -5

Repository: finleysg/bhmc-admin

Length of output: 89


🏁 Script executed:

#!/bin/bash
cat plans/bulk-refund-prd.json | jq '.[] | select(.property | contains("duration") or contains("timeout") or contains("batch") or contains("limit"))' 2>/dev/null || cat plans/bulk-refund-prd.json | head -100

Repository: finleysg/bhmc-admin

Length of output: 5687


🏁 Script executed:

#!/bin/bash
# Get the rest of the PRD file to check for batch size or duration limits
tail -n +100 plans/bulk-refund-prd.json | head -200

Repository: finleysg/bhmc-admin

Length of output: 6041


🏁 Script executed:

#!/bin/bash
# Check if there are any batch size limits or expected operation duration comments in the code
rg "batch|limit|duration|timeout|refund.*timeout|refund.*delay" apps/api/src/registration --type ts -i -B 2 -A 2

Repository: finleysg/bhmc-admin

Length of output: 14044


🏁 Script executed:

#!/bin/bash
# Check the refund service to see how payments are processed
fd "refund.service.ts" --type f | head -1 | xargs wc -l

Repository: finleysg/bhmc-admin

Length of output: 119


🏁 Script executed:

#!/bin/bash
# Check the refund service implementation to understand processing model
cat -n apps/api/src/registration/services/refund.service.ts | sed -n '1,50p'

Repository: finleysg/bhmc-admin

Length of output: 2312


🏁 Script executed:

#!/bin/bash
# Look for the processBulkRefundsStream method specifically
rg -A 30 "processBulkRefundsStream" apps/api/src/registration/services/refund.service.ts

Repository: finleysg/bhmc-admin

Length of output: 1176


🏁 Script executed:

#!/bin/bash
# Check if there are any batch size limits documented or enforced
rg "MAX.*PAYMENT|MAX.*BATCH|batch.*size" apps/api/src/registration -i -B 2 -A 2

Repository: finleysg/bhmc-admin

Length of output: 45


🏁 Script executed:

#!/bin/bash
# Check the rest of processBulkRefundsStream to see completion flow
rg -A 60 "processBulkRefundsStream" apps/api/src/registration/services/refund.service.ts | tail -40

Repository: finleysg/bhmc-admin

Length of output: 1407


🏁 Script executed:

#!/bin/bash
# Check for TypeScript strict mode compliance and any type safety issues in the file
head -20 apps/api/src/registration/services/bulk-refund-progress-tracker.ts

Repository: finleysg/bhmc-admin

Length of output: 802


🏁 Script executed:

#!/bin/bash
# Check tsconfig for strict mode settings
rg "strict" apps/api/tsconfig.json

Repository: finleysg/bhmc-admin

Length of output: 45


🏁 Script executed:

#!/bin/bash
# Find tsconfig files
fd "tsconfig" --type f | grep -E "api|root" | head -3

Repository: finleysg/bhmc-admin

Length of output: 85


🏁 Script executed:

#!/bin/bash
# Check for any usage of 'any' type in the bulk-refund-progress-tracker file
rg ": any" apps/api/src/registration/services/bulk-refund-progress-tracker.ts

Repository: finleysg/bhmc-admin

Length of output: 45


🏁 Script executed:

#!/bin/bash
# Check tsconfig strict mode
cat apps/api/tsconfig.json | grep -A 5 '"strict"'

Repository: finleysg/bhmc-admin

Length of output: 45


🏁 Script executed:

#!/bin/bash
# Find tsconfig and check strict mode
find . -name "tsconfig*.json" -type f | head -5 | xargs grep -l "strict" 2>/dev/null

Repository: finleysg/bhmc-admin

Length of output: 115


🏁 Script executed:

#!/bin/bash
# Check the root tsconfig
cat tsconfig.json 2>/dev/null | head -30

Repository: finleysg/bhmc-admin

Length of output: 572


Unconditional 5-minute cleanup timeout can terminate long-running bulk refund operations.

The startTracking method sets a fixed 5-minute timeout without any reset mechanism. Since emitProgress does not refresh the timer, events with many payments could exceed the timeout mid-stream, causing subscribers to miss the final progress/completion events. With no batch size limits documented in the PRD, this becomes a real concern for large events.

Implement a refreshable cleanup timer that resets on each progress emission and is explicitly cleared on completion or error. See suggested fix below.

🧹 Suggested fix (refreshable cleanup timer)
 export class BulkRefundProgressTracker {
 	private readonly logger = new Logger(BulkRefundProgressTracker.name)
 
 	private readonly activeOperations = new Map<number, Subject<BulkRefundProgressEvent>>()
+	private readonly cleanupTimers = new Map<number, ReturnType<typeof setTimeout>>()
+
+	private scheduleCleanup(eventId: number, delayMs = PROGRESS_CLEANUP_MS): void {
+		const existing = this.cleanupTimers.get(eventId)
+		if (existing) {
+			clearTimeout(existing)
+		}
+		this.cleanupTimers.set(
+			eventId,
+			setTimeout(() => this.cleanupOperation(eventId), delayMs),
+		)
+	}
 
 	startTracking(eventId: number): Subject<BulkRefundProgressEvent> {
 		if (this.activeOperations.has(eventId)) {
 			throw new Error(`Bulk refund operation already in progress for event ${eventId}`)
 		}
 
 		const subject = new Subject<BulkRefundProgressEvent>()
 		this.activeOperations.set(eventId, subject)
-		setTimeout(() => {
-			this.cleanupOperation(eventId)
-		}, PROGRESS_CLEANUP_MS)
+		this.scheduleCleanup(eventId)
 
 		return subject
 	}
 
 	emitProgress(eventId: number, current: number, total: number, playerName: string): void {
 		const subject = this.activeOperations.get(eventId)
 		if (subject) {
 			subject.next({
 				status: "processing",
 				current,
 				total,
 				playerName,
 			})
+			this.scheduleCleanup(eventId)
 		}
 	}
 
 	completeOperation(eventId: number, result: BulkRefundResponse): void {
 		const subject = this.activeOperations.get(eventId)
 		if (subject) {
 			subject.next({
 				status: "complete",
 				current: result.refundedCount + result.failedCount,
 				total: result.refundedCount + result.failedCount + result.skippedCount,
 				result,
 			})
 		}
 
-		setTimeout(() => {
-			this.cleanupOperation(eventId)
-		}, 1000)
+		this.scheduleCleanup(eventId, 1000)
 	}
 
 	errorOperation(eventId: number, error: string): void {
 		const subject = this.activeOperations.get(eventId)
 		if (subject) {
 			subject.next({
 				status: "error",
 				current: 0,
 				total: 0,
 				error,
 			})
 		}
 
-		setTimeout(() => {
-			this.cleanupOperation(eventId)
-		}, 1000)
+		this.scheduleCleanup(eventId, 1000)
 	}
 
 	private cleanupOperation(eventId: number): void {
+		const timer = this.cleanupTimers.get(eventId)
+		if (timer) {
+			clearTimeout(timer)
+			this.cleanupTimers.delete(eventId)
+		}
 		const subject = this.activeOperations.get(eventId)
 		if (subject) {
 			subject.complete()
 			this.activeOperations.delete(eventId)
 			this.logger.debug(`Cleaned up bulk refund operation for event ${eventId}`)
 		}
 	}

Also applies to: lines 43–53, 58–73, 78–93, 105–112

🤖 Prompt for AI Agents
In `@apps/api/src/registration/services/bulk-refund-progress-tracker.ts` around
lines 17 - 29, The current startTracking creates an unrefreshable timeout that
can kill long-running operations; change the activeOperations map to store an
object { subject, timeoutId } instead of just Subject, create the cleanup timer
in startTracking using PROGRESS_CLEANUP_MS and save its id, then update
emitProgress to clear and rearm (reset) that timer on every emission, and ensure
cleanupOperation (and any completion/error methods like
completeTracking/failTracking) clear the timer and remove the entry so the
timeout is not left running.

return subject
}

/**
* Get the progress observable for an active operation
*/
getProgressObservable(eventId: number): Observable<BulkRefundProgressEvent> | null {
return this.activeOperations.get(eventId)?.asObservable() ?? null
}

/**
* Emit progress update for a payment being processed
*/
emitProgress(eventId: number, current: number, total: number, playerName: string): void {
const subject = this.activeOperations.get(eventId)
if (subject) {
subject.next({
status: "processing",
current,
total,
playerName,
})
}
}

/**
* Mark operation as complete with final results
*/
completeOperation(eventId: number, result: BulkRefundResponse): void {
const subject = this.activeOperations.get(eventId)
if (subject) {
subject.next({
status: "complete",
current: result.refundedCount + result.failedCount,
total: result.refundedCount + result.failedCount + result.skippedCount,
result,
})
}

// Cleanup after short delay to allow final event to be sent
setTimeout(() => {
this.cleanupOperation(eventId)
}, 1000)
}

/**
* Mark operation as failed with error
*/
errorOperation(eventId: number, error: string): void {
const subject = this.activeOperations.get(eventId)
if (subject) {
subject.next({
status: "error",
current: 0,
total: 0,
error,
})
}

// Cleanup after short delay
setTimeout(() => {
this.cleanupOperation(eventId)
}, 1000)
}

/**
* Check if an operation is currently active
*/
isOperationActive(eventId: number): boolean {
return this.activeOperations.has(eventId)
}

/**
* Clean up resources for an operation
*/
private cleanupOperation(eventId: number): void {
const subject = this.activeOperations.get(eventId)
if (subject) {
subject.complete()
this.activeOperations.delete(eventId)
this.logger.debug(`Cleaned up bulk refund operation for event ${eventId}`)
}
}

/**
* Force cleanup of all operations (for testing/shutdown)
*/
cleanupAll(): void {
for (const eventId of this.activeOperations.keys()) {
this.cleanupOperation(eventId)
}
}
}
Loading