-
Notifications
You must be signed in to change notification settings - Fork 9
feat(PM-2177): modified schema to support vote tracking #167
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
base: develop
Are you sure you want to change the base?
Conversation
|
|
||
| -- CreateTable | ||
| CREATE TABLE "aiWorkflowRunItemVote" ( | ||
| "id" VARCHAR(14) NOT NULL DEFAULT nanoid(), |
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.
[design]
Consider using a UUID type for the id column instead of VARCHAR(14) with nanoid(). UUIDs are a more standard approach for unique identifiers and can improve interoperability and consistency across different systems.
|
|
||
| -- CreateTable | ||
| CREATE TABLE "aiWorkflowRunItemCommentVote" ( | ||
| "id" VARCHAR(14) NOT NULL DEFAULT nanoid(), |
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.
[design]
Consider using a UUID type for the id column instead of VARCHAR(14) with nanoid(). UUIDs are a more standard approach for unique identifiers and can improve interoperability and consistency across different systems.
| "workflowRunItemId" VARCHAR(14) NOT NULL, | ||
| "voteType" "VoteType" NOT NULL, | ||
| "createdAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| "createdBy" TEXT, |
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.
[💡 design]
The createdBy column is defined as TEXT, which might not be optimal if this field is intended to store user identifiers. Consider using a more specific data type, such as VARCHAR with a defined length, to ensure data consistency and potentially improve performance.
| "workflowRunItemCommentId" VARCHAR(14) NOT NULL, | ||
| "voteType" "VoteType" NOT NULL, | ||
| "createdAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| "createdBy" TEXT, |
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.
[💡 design]
The createdBy column is defined as TEXT, which might not be optimal if this field is intended to store user identifiers. Consider using a more specific data type, such as VARCHAR with a defined length, to ensure data consistency and potentially improve performance.
| } | ||
|
|
||
| model aiWorkflowRunItemVote { | ||
| id String @id @default(dbgenerated("nanoid()")) @db.VarChar(14) |
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.
[💡 maintainability]
Consider using a more descriptive name for the id field, such as voteId, to improve clarity and maintainability. This can help distinguish it from other id fields in different models.
| } | ||
|
|
||
| model aiWorkflowRunItemCommentVote { | ||
| id String @id @default(dbgenerated("nanoid()")) @db.VarChar(14) |
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.
[💡 maintainability]
Consider using a more descriptive name for the id field, such as commentVoteId, to improve clarity and maintainability. This can help distinguish it from other id fields in different models.
| @@index([workflowRunItemCommentId]) | ||
| } | ||
|
|
||
| enum VoteType { |
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.
[💡 design]
The VoteType enum currently only includes UPVOTE and DOWNVOTE. Consider whether additional vote types might be needed in the future, such as NEUTRAL, to accommodate potential changes in voting logic.
|
|
||
| const allowedFields = ['content', 'upVotes', 'downVotes']; | ||
| // Handle vote updates | ||
| if (patchData.upVote !== undefined || patchData.downVote !== undefined) { |
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.
[correctness]
Consider using a transaction for the vote update operations to ensure atomicity. If any part of the vote update process fails, it could leave the database in an inconsistent state.
|
|
||
| // Update properties which can be updated only via m2m | ||
| if (patchData.upVote !== undefined || patchData.downVote !== undefined) { | ||
| // Remove existing votes by this user for this item |
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.
[correctness]
Consider using a transaction for the vote update operations to ensure atomicity. If any part of the vote update process fails, it could leave the database in an inconsistent state.
| delete patchData.upVote; | ||
| } | ||
|
|
||
| // No other fields to update apart from likes |
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.
[💡 performance]
The check for Object.keys(patchData).length === 0 could be moved before the vote handling logic to avoid unnecessary database operations if there are no updates to process.
| } | ||
|
|
||
| // Update properties which can be updated only via m2m | ||
| if (patchData.upVote !== undefined || patchData.downVote !== undefined) { |
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.
[💡 performance]
The check for patchData.upVote !== undefined || patchData.downVote !== undefined could be moved before the vote handling logic to avoid unnecessary database operations if there are no updates to process.
| @IsInt() | ||
| @Min(0) | ||
| upVotes?: number; | ||
| @IsBoolean() |
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.
[design]
Changing upVotes from an integer to a boolean may limit future functionality, such as tracking the number of upvotes. Consider if this change aligns with the intended use case.
| @Min(0) | ||
| downVotes?: number; | ||
| @IsBoolean() | ||
| downVote?: boolean; |
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.
[design]
Changing downVotes from an integer to a boolean may limit future functionality, such as tracking the number of downvotes. Consider if this change aligns with the intended use case.
| @IsInt() | ||
| @IsOptional() | ||
| upVotes?: number; | ||
| upVote?: number; |
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.
[❗❗ correctness]
The upVote field is still defined as an integer in UpdateRunItemCommentDto, whereas it was changed to a boolean in CreateAiWorkflowRunItemDto. Ensure consistency across DTOs unless there's a specific reason for this difference.
| @IsInt() | ||
| @IsOptional() | ||
| downVotes?: number; | ||
| downVote?: number; |
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.
[❗❗ correctness]
The downVote field is still defined as an integer in UpdateRunItemCommentDto, whereas it was changed to a boolean in CreateAiWorkflowRunItemDto. Ensure consistency across DTOs unless there's a specific reason for this difference.
What's in this PR?
Ticket link - https://topcoder.atlassian.net/browse/PM-2177
PS: Once the schema changes is approved, I will further update the logics in the API to create votes.