-
Notifications
You must be signed in to change notification settings - Fork 6
Submissions api integration #13
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.
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
Conversation
ff6b4bf
to
c02734e
Compare
pnpm-lock.yaml
Outdated
[email protected]: | ||
resolution: {integrity: sha512-wkvS7mL/JMugcup3/rMitHmd9ecIGd2lhFhK9N3UUQ450h66d1r3Y9nvXzQAW1Lq+wyx61k/1pfKS5KuKiyEbg==} | ||
engines: {node: '>=0.4.x'} | ||
deprecated: The querystring API is considered Legacy. new code should use the URLSearchParams API instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The querystring
package is marked as deprecated. Consider replacing it with the URLSearchParams
API to ensure future compatibility and to adhere to best practices.
return numA - numB; | ||
}; | ||
|
||
function convertSubmissionES(esData): any { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifying any
as the return type for convertSubmissionES
can lead to potential type safety issues. Consider defining a more specific return type to improve type safety and maintainability.
if (source['resource'] === 'submission') { | ||
await handleElasticSearchSubmission(source); | ||
} | ||
} catch { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Catching errors without specifying the error variable can make debugging difficult. Consider including the error variable in the catch block to log or handle specific errors more effectively.
}, | ||
}); | ||
} | ||
} catch { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Catching errors without specifying the error object can make debugging difficult. Consider adding the error object back to the catch block to log or handle specific errors.
await prisma.upload.createMany({ | ||
data: uploadDataList, | ||
}); | ||
} catch { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding error handling logic to the catch block to handle specific errors or log additional information. Currently, the catch block is empty, which might make it difficult to diagnose issues.
for (const u of uploadDataList) { | ||
try { | ||
await prisma.upload.create({ data: u }); | ||
} catch { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding error handling logic to the catch block to handle specific errors or log additional information. Currently, the catch block is empty, which might make it difficult to diagnose issues.
await prisma.submission.createMany({ | ||
data: submissionDataList, | ||
}); | ||
} catch { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider logging the error object in the catch block to provide more context about the failure. This can help with debugging and understanding why the submission creation failed.
// collect data to uploadSubmissionMap | ||
if ( | ||
dbData.status === SubmissionStatus.ACTIVE && | ||
dbData.legacyUploadId != null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using strict equality checks (===
and !==
) instead of loose equality checks (==
and !=
) for better type safety and to avoid unexpected type coercion.
const existing = uploadSubmissionMap[dbData.legacyUploadId]; | ||
if ( | ||
!existing.score || | ||
item.created.getTime() > existing.created.getTime() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that item.created
and existing.created
are both valid Date objects before calling getTime()
to prevent potential runtime errors.
async function doImportResourceSubmission() { | ||
try { | ||
await prisma.resourceSubmission.createMany({ | ||
data: resourceSubmissions, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding an error parameter to the catch block to log more detailed error information. This can help in diagnosing the issue more effectively.
await prisma.resourceSubmission.create({ | ||
data: rs, | ||
}); | ||
} catch { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding an error parameter to the catch block to log more detailed error information. This can help in diagnosing the issue more effectively.
@@ -1,4 +1,4 @@ | |||
import { Controller, Post, Body } from '@nestjs/common'; | |||
import { Controller, Post, Body, Req } from '@nestjs/common'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Req
import from @nestjs/common
is added, but it is not used in the current code. Consider removing it if it is not needed to avoid unnecessary imports.
@@ -16,15 +16,20 @@ import { | |||
mapContactRequestToDto, | |||
} from 'src/dto/contactRequest.dto'; | |||
import { PrismaService } from '../../shared/modules/global/prisma.service'; | |||
import { ResourceApiService } from 'src/shared/modules/global/resource.service'; | |||
import { JwtUser } from 'src/shared/modules/global/jwt.service'; | |||
|
|||
@ApiTags('Contact Requests') | |||
@ApiBearerAuth() | |||
@Controller('/') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change from @Controller('/api')
to @Controller('/')
might affect the routing of the API endpoints. Ensure that this change is intentional and that it aligns with the overall API structure.
|
||
@Post('/contact-requests') | ||
@Roles(UserRole.Submitter, UserRole.Reviewer) | ||
@Roles(UserRole.User, UserRole.Talent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The roles have been changed from UserRole.Submitter, UserRole.Reviewer
to UserRole.User, UserRole.Talent
. Verify that these roles are correct for the intended functionality and that they have the necessary permissions for creating contact requests.
@Body() body: ContactRequestDto, | ||
): Promise<ContactRequestResponseDto> { | ||
const authUser: JwtUser = req['user'] as JwtUser; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding error handling for the case where req['user']
might not be defined or might not be of type JwtUser
. This will help prevent runtime errors if the request does not contain a valid user object.
@Body() body: ContactRequestDto, | ||
): Promise<ContactRequestResponseDto> { | ||
const authUser: JwtUser = req['user'] as JwtUser; | ||
await this.resourceApiService.validateResourcesRoles( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method validateResourcesRoles
should ideally handle exceptions or return a meaningful error if the validation fails. Ensure that any errors thrown by this method are caught and handled appropriately to provide a clear response to the client.
return data; | ||
} | ||
|
||
private buildResponse(data: any): SubmissionResponseDto { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider specifying a more precise type than any
for the data
parameter to improve type safety and maintainability.
@@ -7,4 +7,5 @@ export enum UserRole { | |||
Reviewer = 'reviewer', | |||
Submitter = 'Submitter', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using consistent casing for enum values. The value 'Submitter' should be changed to 'submitter' to match the casing of other values like 'reviewer' and 'Talent'.
@@ -7,4 +7,5 @@ export enum UserRole { | |||
Reviewer = 'reviewer', | |||
Submitter = 'Submitter', | |||
User = 'Topcoder User', | |||
Talent = 'Topcoder Talent', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that the new enum value 'Talent' is used consistently across the application. Verify that any logic dependent on user roles is updated to include this new role.
/** | ||
* Fetch list of resource roles | ||
* | ||
* @returns resolves to list of resouce role |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in the comment: 'resouce' should be 'resource'.
/** | ||
* Fetch list of resource | ||
* | ||
* @returns resolves to list of resouce info |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in the comment: 'resouce' should be 'resource'.
} | ||
|
||
/** | ||
* Validate resource fole |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in the comment: 'fole' should be 'role'.
/** | ||
* Validate resource fole | ||
* | ||
* @param requiredRoles list of require roles |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in the comment: 'require' should be 'required'.
); | ||
} catch (e) { | ||
if (e instanceof AxiosError) { | ||
this.logger.error(`Http Error: ${e.message}`, e.response?.data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using a more specific error type or message for the thrown error to provide more context about the failure.
} catch (e) { | ||
if (e instanceof AxiosError) { | ||
this.logger.error(`Http Error: ${e.message}`, e.response?.data); | ||
throw new Error('Cannot get data from Resource API.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using a more specific error type or message for the thrown error to provide more context about the failure.
No description provided.