Skip to content

Fix transaction #104

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 24, 2025
Merged

Fix transaction #104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
54f823d
Drop the db transaction when creating winnings
vas3a Oct 24, 2025
d7d3012
Properly use db transaction context
vas3a Oct 24, 2025
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
22 changes: 13 additions & 9 deletions src/api/repository/identity-verification.repo.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { Injectable } from '@nestjs/common';
import { verification_status } from '@prisma/client';
import { Prisma, verification_status } from '@prisma/client';
import { PrismaService } from 'src/shared/global/prisma.service';

@Injectable()
Expand All @@ -12,14 +12,18 @@ export class IdentityVerificationRepository {
* @param userId - The unique identifier of the user.
* @returns A promise that resolves to `true` if the user has at least one active identity verification association, otherwise `false`.
*/
async completedIdentityVerification(userId: string): Promise<boolean> {
const count =
await this.prisma.user_identity_verification_associations.count({
where: {
user_id: userId,
verification_status: verification_status.ACTIVE,
},
});
async completedIdentityVerification(
userId: string,
tx?: Prisma.TransactionClient,
Copy link

Choose a reason for hiding this comment

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

[⚠️ maintainability]
Consider documenting the tx parameter to clarify its purpose and usage. This will improve the maintainability and readability of the function, especially for developers unfamiliar with transaction handling.

): Promise<boolean> {
const count = await (
tx || this.prisma
).user_identity_verification_associations.count({
where: {
user_id: userId,
verification_status: verification_status.ACTIVE,
},
});

return count > 0;
}
Expand Down
22 changes: 14 additions & 8 deletions src/api/repository/paymentMethod.repo.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
import { Injectable } from '@nestjs/common';
import { payment_method_status, user_payment_methods } from '@prisma/client';
import {
payment_method_status,
Prisma,
user_payment_methods,
} from '@prisma/client';
import { PrismaService } from 'src/shared/global/prisma.service';

@Injectable()
Expand All @@ -14,14 +18,16 @@ export class PaymentMethodRepository {
*/
async getConnectedPaymentMethod(
userId: string,
tx?: Prisma.TransactionClient,
): Promise<user_payment_methods | null> {
const connectedUserPaymentMethod =
await this.prisma.user_payment_methods.findFirst({
where: {
user_id: userId,
status: payment_method_status.CONNECTED,
},
});
const connectedUserPaymentMethod = await (
tx || this.prisma
Copy link

Choose a reason for hiding this comment

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

[💡 readability]
The use of (tx || this.prisma) is technically correct, but it could be improved for clarity. Consider explicitly checking if tx is defined and using it, otherwise default to this.prisma. This can help avoid potential confusion about the precedence of the operations.

).user_payment_methods.findFirst({
where: {
user_id: userId,
status: payment_method_status.CONNECTED,
},
});

return connectedUserPaymentMethod;
}
Expand Down
9 changes: 6 additions & 3 deletions src/api/repository/taxForm.repo.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { Injectable } from '@nestjs/common';
import { tax_form_status } from '@prisma/client';
import { Prisma, tax_form_status } from '@prisma/client';
import { PrismaService } from 'src/shared/global/prisma.service';

@Injectable()
Expand All @@ -12,8 +12,11 @@ export class TaxFormRepository {
* @param userId user id
* @returns true if user has active tax form
*/
async hasActiveTaxForm(userId: string): Promise<boolean> {
const count = await this.prisma.user_tax_form_associations.count({
async hasActiveTaxForm(
userId: string,
tx?: Prisma.TransactionClient,
): Promise<boolean> {
const count = await (tx || this.prisma).user_tax_form_associations.count({
Copy link

Choose a reason for hiding this comment

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

[💡 readability]
The use of (tx || this.prisma) is correct for supporting transactions, but it could be improved for clarity. Consider explicitly checking if tx is defined and using it, otherwise defaulting to this.prisma. This makes the intention clearer and avoids potential confusion.

where: {
user_id: userId,
tax_form_status: tax_form_status.ACTIVE,
Expand Down
22 changes: 16 additions & 6 deletions src/api/winnings/winnings.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,13 @@ export class WinningsService {
}
}

private async setPayrollPaymentMethod(userId: string) {
const payrollPaymentMethod = await this.prisma.payment_method.findFirst({
private async setPayrollPaymentMethod(
userId: string,
tx?: Prisma.TransactionClient,
Copy link

Choose a reason for hiding this comment

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

[⚠️ maintainability]
The addition of the optional tx parameter allows for transaction handling, which is beneficial for atomic operations. However, ensure that all calls to setPayrollPaymentMethod are reviewed to pass the transaction client where necessary, to maintain consistency and avoid potential issues with transaction management.

) {
const payrollPaymentMethod = await (
tx || this.prisma
).payment_method.findFirst({
where: {
payment_method_type: 'Wipro Payroll',
},
Expand All @@ -101,7 +106,7 @@ export class WinningsService {
}

if (
await this.prisma.user_payment_methods.findFirst({
await (tx || this.prisma).user_payment_methods.findFirst({
Copy link

Choose a reason for hiding this comment

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

[❗❗ correctness]
The use of (tx || this.prisma) suggests that a transaction object tx might be passed in some cases. Ensure that tx is always a valid Prisma transaction object when provided, as misuse could lead to unexpected behavior or errors. Consider adding validation or type-checking if this is not guaranteed by the surrounding logic.

where: {
user_id: userId,
payment_method_id: payrollPaymentMethod.payment_method_id,
Expand Down Expand Up @@ -180,13 +185,18 @@ export class WinningsService {

const hasActiveTaxForm = await this.taxFormRepo.hasActiveTaxForm(
body.winnerId,
tx,
Copy link

Choose a reason for hiding this comment

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

[⚠️ correctness]
Passing the transaction object tx to hasActiveTaxForm is a good practice for ensuring that all database operations are part of the same transaction. However, ensure that the hasActiveTaxForm method is designed to handle transactions correctly and that it doesn't introduce any side effects or unexpected behavior.

);
const hasConnectedPaymentMethod = Boolean(
await this.paymentMethodRepo.getConnectedPaymentMethod(body.winnerId),
await this.paymentMethodRepo.getConnectedPaymentMethod(
body.winnerId,
tx,
Copy link

Choose a reason for hiding this comment

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

[⚠️ correctness]
Ensure that the getConnectedPaymentMethod method is capable of handling the transaction object tx correctly. This is important to maintain transactional integrity and avoid potential issues with database operations.

),
);
const isIdentityVerified =
await this.identityVerificationRepo.completedIdentityVerification(
body.winnerId,
tx,
Copy link

Choose a reason for hiding this comment

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

[⚠️ correctness]
Verify that the completedIdentityVerification method is designed to work with the transaction object tx. This ensures that the identity verification check is consistent with the transactional context.

);

for (const detail of body.details || []) {
Expand All @@ -213,7 +223,7 @@ export class WinningsService {
`Payroll payment detected. Setting payment status to PAID for user ${body.winnerId}`,
);
paymentModel.payment_status = PaymentStatus.PAID;
await this.setPayrollPaymentMethod(body.winnerId);
await this.setPayrollPaymentMethod(body.winnerId, tx);
Copy link

Choose a reason for hiding this comment

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

[❗❗ correctness]
The addition of the tx parameter to setPayrollPaymentMethod suggests that this method now requires a transaction context. Ensure that tx is correctly initialized and managed to prevent potential issues with transaction handling, such as incomplete or failed transactions.

}

winningModel.payment.create.push(paymentModel);
Expand All @@ -223,7 +233,7 @@ export class WinningsService {
}

this.logger.debug('Attempting to create winning with nested payments.');
const createdWinning = await this.prisma.winnings.create({
const createdWinning = await tx.winnings.create({
Copy link

Choose a reason for hiding this comment

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

[❗❗ correctness]
Switching from this.prisma.winnings.create to tx.winnings.create suggests a transaction context is being used. Ensure that tx is correctly initialized and managed to avoid potential transaction-related issues, such as incomplete or failed transactions.

data: winningModel as any,
});

Expand Down