Skip to content
Merged
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
1 change: 1 addition & 0 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ workflows:
only:
- dev
- engagements
- PM-4165_allow-editing-credited-payments
- 'build-prod':
context: org-global
filters:
Expand Down
5 changes: 4 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,10 @@
"**/*.(t|j)s"
],
"coverageDirectory": "../coverage",
"testEnvironment": "node"
"testEnvironment": "node",
"moduleNameMapper": {
"^src/(.*)$": "<rootDir>/$1"
}
},
"prisma": {
"seed": "ts-node prisma/seed.ts"
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
CREATE INDEX idx_winnings_type_created_at ON winnings USING btree (type, created_at DESC);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[⚠️ correctness]
Consider adding a conditional check to ensure that the winnings table exists before attempting to create the index. This can prevent potential errors during the migration if the table is not present.

1 change: 1 addition & 0 deletions prisma/schema.prisma
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ model winnings {

@@index([category, created_at(sort: Desc)], map: "idx_winnings_category_created_at")
@@index([created_at(sort: Desc)], map: "idx_winnings_created_at")
@@index([type, created_at(sort: Desc)], map: "idx_winnings_type_created_at")
@@index([winner_id, created_at(sort: Desc)], map: "idx_winnings_winner_created_at")
@@index([winner_id, winning_id], map: "idx_winnings_winner_id_only")
}
Expand Down
1 change: 1 addition & 0 deletions src/api/admin/admin.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ export class AdminController {
{
includeCount: false,
includePayoutStatus: false,
latestPaymentOnly: true,
},
);

Expand Down
4 changes: 3 additions & 1 deletion src/api/admin/admin.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,8 @@ export class AdminService {
// Update payment amount if requested
if (
body.paymentAmount !== undefined &&
(payment.payment_status === PaymentStatus.OWED ||
(payment.payment_status === PaymentStatus.CREDITED ||
payment.payment_status === PaymentStatus.OWED ||
payment.payment_status === PaymentStatus.ON_HOLD ||
payment.payment_status === PaymentStatus.ON_HOLD_ADMIN)
) {
Expand Down Expand Up @@ -646,6 +647,7 @@ export class AdminService {
version: currentVersion,
payment_status: {
in: [
PaymentStatus.CREDITED,
PaymentStatus.OWED,
PaymentStatus.ON_HOLD,
PaymentStatus.ON_HOLD_ADMIN,
Expand Down
101 changes: 101 additions & 0 deletions src/api/challenge-payments/challenge-payments.service.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
jest.mock('src/shared/global', () => ({
Logger: class {
debug = jest.fn();

error = jest.fn();

info = jest.fn();

log = jest.fn();

warn = jest.fn();
},
}));

jest.mock('src/config', () => ({
ENV_CONFIG: {
TOPCODER_API_V6_BASE_URL: 'https://api.topcoder-dev.com/v6',
},
}));

import { ChallengePaymentsService } from './challenge-payments.service';

describe('ChallengePaymentsService', () => {
let findManyMock: jest.Mock;
let m2mFetchMock: jest.Mock;
let service: ChallengePaymentsService;

beforeEach(() => {
findManyMock = jest.fn().mockResolvedValue([]);
m2mFetchMock = jest.fn();

service = new ChallengePaymentsService(
{
winnings: {
findMany: findManyMock,
},
} as any,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[⚠️ maintainability]
Casting to any can obscure type errors and reduce type safety. Consider using a more specific type or interface for the winnings and m2mFetch objects to enhance maintainability and type safety.

{
m2mFetch: m2mFetchMock,
} as any,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[⚠️ maintainability]
Casting to any can obscure type errors and reduce type safety. Consider using a more specific type or interface for the winnings and m2mFetch objects to enhance maintainability and type safety.

);
});

it('returns all challenge payments for managers', async () => {
m2mFetchMock
.mockResolvedValueOnce([
{
memberHandle: 'manager-user',
memberId: '123',
roleId: 'manager-role-id',
},
])
.mockResolvedValueOnce([
{
fullWriteAccess: false,
id: 'manager-role-id',
name: 'Manager',
},
]);

await service.listChallengePayments({
auth0User: { roles: ['Topcoder User'] },
challengeId: 'challenge-id',
isMachineToken: false,
requestUserId: '123',
winnerOnly: false,
});

const where = findManyMock.mock.calls[0][0].where;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[⚠️ correctness]
Accessing mock.calls[0][0].where directly assumes that the mock has been called at least once and that the structure of the call arguments is as expected. Consider adding checks or using a more robust way to verify the mock call structure to prevent potential runtime errors.

expect(where.winner_id).toBeUndefined();
});

it('keeps winner filtering for users without challenge-wide access', async () => {
m2mFetchMock
.mockResolvedValueOnce([
{
memberHandle: 'reviewer-user',
memberId: '456',
roleId: 'reviewer-role-id',
},
])
.mockResolvedValueOnce([
{
fullWriteAccess: false,
id: 'reviewer-role-id',
name: 'Reviewer',
},
]);

await service.listChallengePayments({
auth0User: { roles: ['Topcoder User'] },
challengeId: 'challenge-id',
isMachineToken: false,
requestUserId: '456',
winnerOnly: false,
});

const where = findManyMock.mock.calls[0][0].where;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[⚠️ correctness]
Accessing mock.calls[0][0].where directly assumes that the mock has been called at least once and that the structure of the call arguments is as expected. Consider adding checks or using a more robust way to verify the mock call structure to prevent potential runtime errors.

expect(where.winner_id).toBe('456');
});
});
38 changes: 25 additions & 13 deletions src/api/challenge-payments/challenge-payments.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,16 +101,17 @@ export class ChallengePaymentsService {
}

try {
const isCopilot = await this.isCopilotForChallenge(
challengeId,
requestUserId,
);
if (isCopilot) {
const hasChallengeWideAccess =

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[💡 readability]
The method name hasChallengeWideAccessForChallenge is somewhat redundant and could be simplified to hasChallengeWideAccess. This would improve readability by reducing verbosity without losing clarity.

await this.hasChallengeWideAccessForChallenge(
challengeId,
requestUserId,
);
if (hasChallengeWideAccess) {
allowAllForChallenge = true;
}
} catch (error) {
this.logger.warn(
`Failed to verify copilot status for user ${requestUserId} on challenge ${challengeId}`,
`Failed to verify challenge-wide payment access for user ${requestUserId} on challenge ${challengeId}`,
error instanceof Error ? error.message : error,
);
}
Expand Down Expand Up @@ -150,7 +151,11 @@ export class ChallengePaymentsService {
roles.push(String(role));
}
});
} else if (value) {
} else if (

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[⚠️ correctness]
The condition checks for typeof value being string, number, or boolean. Consider whether other types (e.g., object, function) should be explicitly handled or if they should trigger a warning or error. This could prevent unexpected behavior if value is of an unhandled type.

typeof value === 'string' ||
typeof value === 'number' ||
typeof value === 'boolean'
) {
roles.push(String(value));
}
});
Expand All @@ -165,7 +170,7 @@ export class ChallengePaymentsService {
return roles;
}

private async isCopilotForChallenge(
private async hasChallengeWideAccessForChallenge(

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[💡 readability]
The method name hasChallengeWideAccessForChallenge is somewhat redundant and could be misleading. Consider renaming it to hasChallengeWideAccess for clarity and maintainability.

challengeId: string,
userId: string,
): Promise<boolean> {
Expand All @@ -181,19 +186,26 @@ export class ChallengePaymentsService {
return false;
}

const copilotRoleIds = new Set(
const challengeWideRoleIds = new Set(
resourceRoles
?.filter((role) =>
role?.name ? role.name.toLowerCase().includes('copilot') : false,
?.filter(
(role) =>
role?.fullWriteAccess === true ||

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[⚠️ correctness]
The condition role?.fullWriteAccess === true is checked before checking the role name. If fullWriteAccess is a boolean, this is fine, but if it can be null or undefined, consider using a more robust check like role?.fullWriteAccess to ensure it handles all falsy values correctly.

(role?.name
? role.name.toLowerCase().includes('copilot') ||

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[💡 readability]
The use of toLowerCase() on role.name is repeated multiple times. Consider extracting this to a variable to improve readability and avoid redundant operations.

role.name.toLowerCase().includes('manager')
: false),
)
.map((role) => role.id),
);

if (copilotRoleIds.size === 0) {
if (challengeWideRoleIds.size === 0) {
return false;
}

return resources.some((resource) => copilotRoleIds.has(resource.roleId));
return resources.some((resource) =>
challengeWideRoleIds.has(resource.roleId),
);
}

private async fetchWinnings(
Expand Down
61 changes: 61 additions & 0 deletions src/api/challenges/challenges.service.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
import { ChallengeStatuses } from '../../dto/challenge.dto';

jest.mock('src/config', () => ({
ENV_CONFIG: {
TGBillingAccounts: [],
TOPCODER_API_V6_BASE_URL: 'https://api.topcoder-dev.com/v6',
},
}));

jest.mock('src/shared/global', () => ({
Logger: class {
debug = jest.fn();

error = jest.fn();

info = jest.fn();

log = jest.fn();

warn = jest.fn();
},
}));

import { ChallengesService } from './challenges.service';

describe('ChallengesService', () => {
it('skips creating payments for fun challenges', async () => {
const prisma = {
challenge_lock: {
create: jest.fn(),
deleteMany: jest.fn(),
},
};
const service = new ChallengesService(
prisma as any,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[⚠️ maintainability]
Using as any for type casting can lead to potential type safety issues. Consider defining proper interfaces or types for the dependencies to ensure type safety.

{} as any,
{} as any,
{} as any,
{} as any,
);
const challenge = {
funChallenge: true,
id: '11111111-1111-1111-1111-111111111111',
name: 'MM 163',
status: ChallengeStatuses.Completed,
};
const createPaymentsSpy = jest
.spyOn(service as any, 'createPayments')

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[⚠️ design]
Spying on private methods (using service as any) can make tests brittle and tightly coupled to the implementation details. Consider testing the public API of the service instead.

.mockResolvedValue(undefined);

jest.spyOn(service, 'getChallenge').mockResolvedValue(challenge as any);

await service.generateChallengePayments(
'11111111-1111-1111-1111-111111111111',
'test-user',
);

expect(createPaymentsSpy).not.toHaveBeenCalled();
expect(prisma.challenge_lock.create).not.toHaveBeenCalled();
});
});
9 changes: 8 additions & 1 deletion src/api/challenges/challenges.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ interface PaymentPayload {
amount: number;
userId: string;
type: WinningsCategory;
currency: string;
currency: PrizeType;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[❗❗ correctness]
The change from string to PrizeType for the currency field suggests a type change. Ensure that PrizeType is an appropriate type for representing currency values and that all usages of PaymentPayload are updated accordingly. If PrizeType is an enum or a complex type, verify that it aligns with expected currency formats and values.

description?: string;
}

Expand Down Expand Up @@ -566,6 +566,13 @@ export class ChallengesService {
throw new Error("Challenge isn't in a payable status!");
}

if (challenge.funChallenge === true) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[⚠️ correctness]
Consider using === instead of == for strict equality check. While funChallenge is likely a boolean, using strict equality ensures type safety and avoids potential issues if the type changes in the future.

this.logger.log(
`Skipping payment generation for fun challenge ${challenge.id} (${challenge.name}).`,
);
return;
}

// need to read for update (LOCK the rows)
this.logger.log(`Attempting to acquire lock for challenge ${challenge.id}`);
try {
Expand Down
2 changes: 2 additions & 0 deletions src/api/challenges/models/challenge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ export interface Challenge {
timelineTemplateId: string;
currentPhaseNames: string[];
wiproAllowed: boolean;
funChallenge?: boolean;
tags: string[];
groups: string[];
submissionStartDate: string;
Expand Down Expand Up @@ -147,6 +148,7 @@ export interface ChallengeResource {
export interface ResourceRole {
id: string;
name: string;
fullWriteAccess?: boolean;

Copy link
Copy Markdown

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 fullWriteAccess property as an optional boolean in the ResourceRole interface could lead to potential issues if not handled properly elsewhere in the codebase. Ensure that all logic that checks or uses this property accounts for the possibility of it being undefined. Consider providing a default value or handling it explicitly in the code that uses this interface.

}

export interface ChallengeReview {
Expand Down
Loading
Loading