-
Notifications
You must be signed in to change notification settings - Fork 9
Further fixes for /v6/my-reviews for big accounts #142
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
Further fixes for /v6/my-reviews for big accounts
| rv."resourceId", | ||
| COUNT(*) AS "pendingAppealCount", | ||
| now() | ||
| FROM reviews.review rv |
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 now() function is used multiple times in the SQL statements. Consider using a single variable to store the current timestamp to ensure consistency across the transaction, especially if the transaction takes a non-trivial amount of time to execute.
| AFTER INSERT OR UPDATE OR DELETE ON reviews."appealResponse" | ||
| FOR EACH ROW | ||
| EXECUTE FUNCTION reviews.handle_appeal_response_change(); | ||
|
|
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.
[💡 style]
There is no newline at the end of the file. While this is a minor issue, it is generally a good practice to include a newline at the end of files to avoid potential issues with some tools and to adhere to POSIX standards.
| } | ||
|
|
||
| model reviewPendingSummary { | ||
| resourceId String @id |
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 adding a @db.VarChar annotation to resourceId to explicitly define its length in the database schema. This can help prevent potential issues with varying string lengths across different database systems.
| hasIncompleteReviews: boolean | null; | ||
| incompletePhaseName: string | null; | ||
| hasPendingAppealResponses: boolean | null; | ||
| hasPendingAppealResponses: 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.
[❗❗ correctness]
The type change from boolean | null to boolean for hasPendingAppealResponses may introduce issues if the previous logic depended on distinguishing between false and null. Ensure that the rest of the codebase correctly handles this change.
| deliverable_reviews."hasIncompleteReviews" AS "hasIncompleteReviews", | ||
| deliverable_reviews."incompletePhaseName" AS "incompletePhaseName", | ||
| pending_appeals."hasPendingAppealResponses" AS "hasPendingAppealResponses", | ||
| COALESCE(pending_appeals."pendingAppealCount" > 0, FALSE) AS "hasPendingAppealResponses", |
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 use of COALESCE(pending_appeals."pendingAppealCount" > 0, FALSE) assumes that pendingAppealCount is always non-null. If pendingAppealCount can be null, this logic may not behave as expected. Consider verifying the nullability of pendingAppealCount.
No description provided.