-
Notifications
You must be signed in to change notification settings - Fork 38
add team id for octokit instance when saving pr user data in mongo #154
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
This comment has been minimized.
This comment has been minimized.
[], | ||
[], | ||
platformType, | ||
organizationAndTeamData?.[0], |
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.
organizationAndTeamData[0],
The optional chaining on organizationAndTeamData?.[0]
is unnecessary. The guard clause on line 78 (if (!configs || !configs.length)
) ensures that the configs
array is non-empty at this point in the code. Consequently, organizationAndTeamData
will also be a non-empty array, meaning organizationAndTeamData[0]
will always be defined. Removing the optional chaining (?.
) would make the code cleaner and more accurately reflect the established logic that this value cannot be undefined
.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
src/core/application/use-cases/pullRequests/update-pull-request-to-new-format.use-case.ts
Show resolved
Hide resolved
const organizationId = | ||
organizationAndTeamData?.organizationId ?? ''; |
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.
const organizationId = organizationAndTeamData?.organizationId;
if (!organizationId) {
this.logger.error({
message: `organizationId is missing in organizationAndTeamData for PR #${pullRequest?.number}`,
context: PullRequestsService.name,
metadata: {
pullRequestNumber: pullRequest?.number,
repositoryName: repository?.name,
organizationAndTeamData,
},
});
return null;
}
The current implementation defaults organizationId
to an empty string if organizationAndTeamData
or its organizationId
property is missing. This can lead to creating pull request records with an invalid organizationId
, causing data integrity issues. It's better to fail early by adding a guard clause to validate that organizationId
is present and logging an error if it's missing. The error log has been enhanced to include organizationAndTeamData
in the metadata for better traceability, adhering to company logging standards.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
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.
Found critical issues please review the requested changes
Kody Review CompleteGreat news! 🎉 Keep up the excellent work! 🚀 Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
This pull request addresses an issue where pull request user data (authors, reviewers, assignees) was not being saved correctly.
The core change involves modifying the data structures and method signatures across the application to consistently pass an
OrganizationAndTeamData
object, which now includes an optionalteamId
alongside theorganizationId
.Key changes include:
teamId
: TheteamId
is now extracted from the request body in theUpdatePullRequestToNewFormatUseCase
and passed down through thePullRequestsService
and its helper methods.extractUser
,extractUsers
) now receive theOrganizationAndTeamData
object. This ensures that user lookups (e.g., via the underlying code management client like Octokit) are performed with the correct team context, facilitating the accurate saving of user information./update-pull-requests
endpoint's DTO (updatePullRequestDto
) has been updated to accept an optionalteamId
parameter.