058 Dec 19 Calculate Assessment and Compliance Tracker progress #367
Conversation
WalkthroughThis pull request introduces several new utility functions and an API endpoint for calculating user project progress. Changes are made across multiple files in the Servers directory, including the user controller, routes, Swagger specification, and utility functions. The key addition is the Changes
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
Servers/controllers/user.ctrl.ts (1)
342-420: Use consistent naming convention for enumerations/status checks.
Right now, subControl status is compared to the string "Done" (line 368). This is fine for an MVP. However, consider extracting status to a constant or enum (like SUBCONTROL_STATUS.Done) to avoid typos or to manage new statuses easily in the future.Servers/swagger.yaml (1)
3858-3941: Ensure route documentation covers all realistic responses.
The new /users/{id}/calculate-progress endpoint only defines 200 and 500 errors in the spec. Consider adding 404 if the user does not exist or 401 if authentication fails. This promotes a robust and clear API contract.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Servers/controllers/user.ctrl.ts(3 hunks)Servers/routes/user.route.ts(2 hunks)Servers/swagger.yaml(1 hunks)Servers/utils/user.utils.ts(1 hunks)
🔇 Additional comments (3)
Servers/controllers/user.ctrl.ts (2)
342-420: Check for potential null or empty array returns.
The for-loops assume that userProjects (line 358), controlcategories (line 361), controls (line 363), subControls (line 365), etc. will return valid arrays. If any of these is empty or null, the logic still works without errors, but consider gracefully handling or logging edge cases where data might be absent.
Would you like to add explicit checks (e.g. if length is 0 => skip) or do you prefer continuing to rely on the for-of loops which safely handle empty arrays?
432-432: All necessary exports are present.
Exporting calculateProgress here ensures the route can use it. Looks good.
Servers/utils/user.utils.ts (1)
232-295: Review each new SQL statement for correctness and performance.
You’ve added multiple small functions for retrieving data (getUserProjects, getControlCategoriesForProject, etc.). Each function is parameterized with ID in the WHERE clause (good for preventing SQL injection). Ensure that the correct columns and table references exist and that indexes are present on these columns for performance if needed.
Servers/controllers/user.ctrl.ts
Outdated
| async function calculateProgress( | ||
| req: Request, | ||
| res: Response | ||
| ): Promise<Response> { | ||
| try { | ||
| const id = parseInt(req.params.id) | ||
| const userProjects = await getUserProjects(id) | ||
|
|
||
| let questionsMetadata = [] | ||
| let allTotalQuestions = 0 | ||
| let allDoneQuestions = 0 | ||
|
|
||
| let controlsMetadata = [] | ||
| let allTotalSubControls = 0 | ||
| let allDoneSubControls = 0 | ||
|
|
||
| for (const userProject of userProjects) { | ||
| let totalSubControls = 0 | ||
| let doneSubControls = 0 | ||
| const controlcategories = await getControlCategoriesForProject(userProject.id) | ||
| for (const controlcategory of controlcategories) { | ||
| const controls = await getControlForControlCategory(controlcategory.id) | ||
| for (const control of controls) { | ||
| const subControls = await getSubControlForControl(control.id) | ||
| for (const subControl of subControls) { | ||
| totalSubControls++; | ||
| if (subControl.status === "Done") { | ||
| doneSubControls++; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| allTotalSubControls += totalSubControls | ||
| allDoneSubControls += doneSubControls | ||
| controlsMetadata.push({ projectId: userProject.id, totalSubControls, doneSubControls }) | ||
|
|
||
| let totalQuestions = 0 | ||
| let doneQuestions = 0 | ||
| const assessments = await getAssessmentsForProject(userProject.id) | ||
| for (const assessment of assessments) { | ||
| const topics = await getTopicsForAssessment(assessment.id) | ||
| for (const topic of topics) { | ||
| const subTopics = await getSubTopicsForTopic(topic.id) | ||
| for (const subTopic of subTopics) { | ||
| const questions = await getQuestionsForSubTopic(subTopic.id) | ||
| for (const question of questions) { | ||
| totalQuestions++; | ||
| if (question.answer) { | ||
| doneQuestions++ | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| allTotalQuestions += totalQuestions | ||
| allDoneQuestions += doneQuestions | ||
| questionsMetadata.push({ projectId: userProject.id, totalQuestions, doneQuestions }) | ||
| } | ||
|
|
||
| const response = { | ||
| controls: { | ||
| projects: controlsMetadata, | ||
| totalSubControls: allTotalSubControls, | ||
| doneSubControls: allDoneSubControls, | ||
| percentageComplete: Number(((allDoneSubControls / allTotalSubControls) * 100).toFixed(2)) | ||
| }, | ||
| questions: { | ||
| projects: questionsMetadata, | ||
| totalQuestions: allTotalQuestions, | ||
| doneQuestions: allDoneQuestions, | ||
| percentageComplete: Number(((allDoneQuestions / allTotalQuestions) * 100).toFixed(2)) | ||
| } | ||
| } | ||
| return res.status(200).json(response) | ||
| } catch (error) { | ||
| console.log(error); | ||
| return res.status(500).json({ message: "Internal server error" }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Validate numeric calculations for percentageComplete.
If there are zero total sub-controls or questions, the code divides by zero on lines 406 and 412. Consider adding a safety check to avoid NaN or Infinity values.
- percentageComplete: Number(((allDoneSubControls / allTotalSubControls) * 100).toFixed(2))
+ percentageComplete: allTotalSubControls !== 0
+ ? Number(((allDoneSubControls / allTotalSubControls) * 100).toFixed(2))
+ : 0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function calculateProgress( | |
| req: Request, | |
| res: Response | |
| ): Promise<Response> { | |
| try { | |
| const id = parseInt(req.params.id) | |
| const userProjects = await getUserProjects(id) | |
| let questionsMetadata = [] | |
| let allTotalQuestions = 0 | |
| let allDoneQuestions = 0 | |
| let controlsMetadata = [] | |
| let allTotalSubControls = 0 | |
| let allDoneSubControls = 0 | |
| for (const userProject of userProjects) { | |
| let totalSubControls = 0 | |
| let doneSubControls = 0 | |
| const controlcategories = await getControlCategoriesForProject(userProject.id) | |
| for (const controlcategory of controlcategories) { | |
| const controls = await getControlForControlCategory(controlcategory.id) | |
| for (const control of controls) { | |
| const subControls = await getSubControlForControl(control.id) | |
| for (const subControl of subControls) { | |
| totalSubControls++; | |
| if (subControl.status === "Done") { | |
| doneSubControls++; | |
| } | |
| } | |
| } | |
| } | |
| allTotalSubControls += totalSubControls | |
| allDoneSubControls += doneSubControls | |
| controlsMetadata.push({ projectId: userProject.id, totalSubControls, doneSubControls }) | |
| let totalQuestions = 0 | |
| let doneQuestions = 0 | |
| const assessments = await getAssessmentsForProject(userProject.id) | |
| for (const assessment of assessments) { | |
| const topics = await getTopicsForAssessment(assessment.id) | |
| for (const topic of topics) { | |
| const subTopics = await getSubTopicsForTopic(topic.id) | |
| for (const subTopic of subTopics) { | |
| const questions = await getQuestionsForSubTopic(subTopic.id) | |
| for (const question of questions) { | |
| totalQuestions++; | |
| if (question.answer) { | |
| doneQuestions++ | |
| } | |
| } | |
| } | |
| } | |
| } | |
| allTotalQuestions += totalQuestions | |
| allDoneQuestions += doneQuestions | |
| questionsMetadata.push({ projectId: userProject.id, totalQuestions, doneQuestions }) | |
| } | |
| const response = { | |
| controls: { | |
| projects: controlsMetadata, | |
| totalSubControls: allTotalSubControls, | |
| doneSubControls: allDoneSubControls, | |
| percentageComplete: Number(((allDoneSubControls / allTotalSubControls) * 100).toFixed(2)) | |
| }, | |
| questions: { | |
| projects: questionsMetadata, | |
| totalQuestions: allTotalQuestions, | |
| doneQuestions: allDoneQuestions, | |
| percentageComplete: Number(((allDoneQuestions / allTotalQuestions) * 100).toFixed(2)) | |
| } | |
| } | |
| return res.status(200).json(response) | |
| } catch (error) { | |
| console.log(error); | |
| return res.status(500).json({ message: "Internal server error" }); | |
| } | |
| } | |
| async function calculateProgress( | |
| req: Request, | |
| res: Response | |
| ): Promise<Response> { | |
| try { | |
| const id = parseInt(req.params.id) | |
| const userProjects = await getUserProjects(id) | |
| let questionsMetadata = [] | |
| let allTotalQuestions = 0 | |
| let allDoneQuestions = 0 | |
| let controlsMetadata = [] | |
| let allTotalSubControls = 0 | |
| let allDoneSubControls = 0 | |
| for (const userProject of userProjects) { | |
| let totalSubControls = 0 | |
| let doneSubControls = 0 | |
| const controlcategories = await getControlCategoriesForProject(userProject.id) | |
| for (const controlcategory of controlcategories) { | |
| const controls = await getControlForControlCategory(controlcategory.id) | |
| for (const control of controls) { | |
| const subControls = await getSubControlForControl(control.id) | |
| for (const subControl of subControls) { | |
| totalSubControls++; | |
| if (subControl.status === "Done") { | |
| doneSubControls++; | |
| } | |
| } | |
| } | |
| } | |
| allTotalSubControls += totalSubControls | |
| allDoneSubControls += doneSubControls | |
| controlsMetadata.push({ projectId: userProject.id, totalSubControls, doneSubControls }) | |
| let totalQuestions = 0 | |
| let doneQuestions = 0 | |
| const assessments = await getAssessmentsForProject(userProject.id) | |
| for (const assessment of assessments) { | |
| const topics = await getTopicsForAssessment(assessment.id) | |
| for (const topic of topics) { | |
| const subTopics = await getSubTopicsForTopic(topic.id) | |
| for (const subTopic of subTopics) { | |
| const questions = await getQuestionsForSubTopic(subTopic.id) | |
| for (const question of questions) { | |
| totalQuestions++; | |
| if (question.answer) { | |
| doneQuestions++ | |
| } | |
| } | |
| } | |
| } | |
| } | |
| allTotalQuestions += totalQuestions | |
| allDoneQuestions += doneQuestions | |
| questionsMetadata.push({ projectId: userProject.id, totalQuestions, doneQuestions }) | |
| } | |
| const response = { | |
| controls: { | |
| projects: controlsMetadata, | |
| totalSubControls: allTotalSubControls, | |
| doneSubControls: allDoneSubControls, | |
| percentageComplete: allTotalSubControls !== 0 | |
| ? Number(((allDoneSubControls / allTotalSubControls) * 100).toFixed(2)) | |
| : 0 | |
| }, | |
| questions: { | |
| projects: questionsMetadata, | |
| totalQuestions: allTotalQuestions, | |
| doneQuestions: allDoneQuestions, | |
| percentageComplete: Number(((allDoneQuestions / allTotalQuestions) * 100).toFixed(2)) | |
| } | |
| } | |
| return res.status(200).json(response) | |
| } catch (error) { | |
| console.log(error); | |
| return res.status(500).json({ message: "Internal server error" }); | |
| } | |
| } |
| getAssessmentsForProject, | ||
| getControlCategoriesForProject, | ||
| getControlForControlCategory, | ||
| getQuestionsForSubTopic, | ||
| getSubControlForControl, | ||
| getSubTopicsForTopic, | ||
| getTopicsForAssessment, |
There was a problem hiding this comment.
💡 Codebase verification
Missing test coverage for critical utility functions
The utility functions imported from Servers/utils/user.utils.ts lack test coverage. These functions are used in critical progress calculation logic but:
- No test files (*.test.ts, *.spec.ts) were found in the Servers directory
- The functions are only referenced in their implementation file and the controller
Action items:
- Add unit tests for all utility functions in
Servers/utils/user.utils.ts, particularly:- getControlCategoriesForProject
- getAssessmentsForProject
- getControlForControlCategory
- getQuestionsForSubTopic
- getSubControlForControl
- getSubTopicsForTopic
- getTopicsForAssessment
🔗 Analysis chain
Ensure all newly added utility functions are tested and consistently reused.
You have introduced several function imports at lines 7-13 and 16 (e.g. getControlCategoriesForProject, getUserProjects, etc.). Verify these are tested thoroughly, as they appear critical to the progress calculation in the new controller method.
Also applies to: 16-16
Let me gather more information about the test coverage for these utility functions.
Let me check for test files specifically in the Servers directory since we found the utility functions in Servers/utils/user.utils.ts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Automated check to find references to these utility functions in the codebase (other than these lines).
# This helps ensure they are indeed used consistently and tested in related unit tests or integration tests.
ast-grep --pattern $'getControlCategoriesForProject($_)'
ast-grep --pattern $'getUserProjects($_)'
# Repeat for the other added utility functions...
Length of output: 310
Script:
#!/bin/bash
# Search for test files that might contain these utility functions
rg -l "getControlCategoriesForProject|getAssessmentsForProject|getControlForControlCategory|getQuestionsForSubTopic|getSubControlForControl|getSubTopicsForTopic|getTopicsForAssessment" --type=test
# Look for the actual implementation of these functions to understand their source
fd -e ts -e js | xargs rg "export (const|function) (getControlCategoriesForProject|getAssessmentsForProject|getControlForControlCategory|getQuestionsForSubTopic|getSubControlForControl|getSubTopicsForTopic|getTopicsForAssessment)"
Length of output: 1274
Script:
#!/bin/bash
# Search for test files in the Servers directory
fd -e test.ts -e spec.ts -e test.js -e spec.js . Servers/
# Look for any test files that might reference these utilities
rg -l "user\.utils" Servers/
Length of output: 151
| router.get("/:id/calculate-progress", calculateProgress) | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Authentication middleware is missing for sensitive user progress data
Looking at the route patterns in user.route.ts, it's clear that several sensitive endpoints are protected with authenticateJWT middleware, including user deletion and email-based retrieval. The newly added progress calculation endpoint lacks this protection, while being user-specific (:id parameter). This could expose sensitive user progress data.
Servers/routes/user.route.ts: AddauthenticateJWTmiddleware to the progress calculation route:
router.get("/:id/calculate-progress", authenticateJWT, calculateProgress)🔗 Analysis chain
Consider authentication for progress route.
Unlike some other GET routes, this added endpoint (line 152) is not behind authenticateJWT. If the progress data is sensitive, you may want to protect it with authentication and/or role checks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching the route usage and verifying if we have an associated test that expects 401 or 403 for unauthenticated requests
rg -A 5 '"\/:id\/calculate-progress"'
Length of output: 134
Script:
#!/bin/bash
# Search for the route pattern with proper escaping
rg -A 5 "/:id/calculate-progress"
# Check if calculateProgress function has any authentication checks
ast-grep --pattern 'function calculateProgress($_) { $$$ }'
# Look for other routes in the same file to compare authentication patterns
rg "router\.(get|post|put|delete)" "Servers/routes/user.route.ts"
Length of output: 781
|
If "questions" in the response is from assessments page, can you change it to "assessments"? We can only fix this area and the rest (output) can stay the same: 
|
@gorkem-bwl Done |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
Servers/controllers/user.ctrl.ts (1)
358-399: Consider refactoring nested loops for better maintainabilityThe deep nesting of loops (4 levels deep) makes the code harder to maintain and potentially impacts performance. Consider extracting the inner loops into separate async functions.
+async function calculateControlProgress(projectId: number) { + let totalSubControls = 0; + let doneSubControls = 0; + const controlcategories = await getControlCategoriesForProject(projectId); + for (const controlcategory of controlcategories) { + const controls = await getControlForControlCategory(controlcategory.id); + for (const control of controls) { + const subControls = await getSubControlForControl(control.id); + for (const subControl of subControls) { + totalSubControls++; + if (subControl.status === "Done") { + doneSubControls++; + } + } + } + } + return { totalSubControls, doneSubControls }; +} +async function calculateAssessmentProgress(projectId: number) { + let totalAssessments = 0; + let doneAssessments = 0; + const assessments = await getAssessmentsForProject(projectId); + for (const assessment of assessments) { + const topics = await getTopicsForAssessment(assessment.id); + for (const topic of topics) { + const subTopics = await getSubTopicsForTopic(topic.id); + for (const subTopic of subTopics) { + const questions = await getQuestionsForSubTopic(subTopic.id); + for (const question of questions) { + totalAssessments++; + if (question.answer) { + doneAssessments++; + } + } + } + } + } + return { totalAssessments, doneAssessments }; +}Servers/swagger.yaml (1)
3858-3941: Add missing error response codesThe endpoint documentation should include additional error responses to match the implementation:
- 400 for invalid user ID
- 404 for when no projects are found
responses: "200": description: user content: application/json: type: object properties: controls: type: object # ... existing properties ... + "400": + description: invalid request + content: + application/json: + type: object + properties: + message: + type: string + example: "Invalid user ID" + "404": + description: no content to display + content: + application/json: + type: object + properties: + message: + type: string + example: "No projects found for user" "500": description: internal server error # ... existing properties ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Servers/controllers/user.ctrl.ts(3 hunks)Servers/swagger.yaml(1 hunks)
🔇 Additional comments (2)
Servers/controllers/user.ctrl.ts (2)
7-13: Missing test coverage for critical utility functions
The utility functions imported from Servers/utils/user.utils.ts lack test coverage. These functions are used in critical progress calculation logic but:
- No test files (*.test.ts, *.spec.ts) were found in the Servers directory
- The functions are only referenced in their implementation file and the controller
Also applies to: 16-16
432-432: LGTM!
The export statement correctly includes the new calculateProgress function.
| projects: controlsMetadata, | ||
| totalSubControls: allTotalSubControls, | ||
| doneSubControls: allDoneSubControls, | ||
| percentageComplete: Number(((allDoneSubControls / allTotalSubControls) * 100).toFixed(2)) |
There was a problem hiding this comment.
Add safety check for division by zero
The percentage calculations could throw errors if there are no controls or assessments.
-percentageComplete: Number(((allDoneSubControls / allTotalSubControls) * 100).toFixed(2))
+percentageComplete: allTotalSubControls > 0 ? Number(((allDoneSubControls / allTotalSubControls) * 100).toFixed(2)) : 0
-percentageComplete: Number(((allDoneAssessments / allTotalAssessments) * 100).toFixed(2))
+percentageComplete: allTotalAssessments > 0 ? Number(((allDoneAssessments / allTotalAssessments) * 100).toFixed(2)) : 0Also applies to: 412-412
| const id = parseInt(req.params.id) | ||
| const userProjects = await getUserProjects(id) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for user ID
The function should validate that the user ID is positive and exists before proceeding with calculations.
const id = parseInt(req.params.id)
+if (isNaN(id) || id <= 0) {
+ return res.status(400).json({ message: "Invalid user ID" });
+}
const userProjects = await getUserProjects(id)
+if (!userProjects?.length) {
+ return res.status(404).json({ message: "No projects found for user" });
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const id = parseInt(req.params.id) | |
| const userProjects = await getUserProjects(id) | |
| const id = parseInt(req.params.id) | |
| if (isNaN(id) || id <= 0) { | |
| return res.status(400).json({ message: "Invalid user ID" }); | |
| } | |
| const userProjects = await getUserProjects(id) | |
| if (!userProjects?.length) { | |
| return res.status(404).json({ message: "No projects found for user" }); | |
| } |
| } catch (error) { | ||
| console.log(error); | ||
| return res.status(500).json({ message: "Internal server error" }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling
The catch block should distinguish between different types of errors and return appropriate status codes.
} catch (error) {
console.log(error);
- return res.status(500).json({ message: "Internal server error" });
+ if (error instanceof TypeError) {
+ return res.status(400).json({ message: "Invalid data structure" });
+ }
+ return res.status(500).json({
+ message: "Internal server error",
+ error: error instanceof Error ? error.message : "Unknown error"
+ });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (error) { | |
| console.log(error); | |
| return res.status(500).json({ message: "Internal server error" }); | |
| } | |
| } catch (error) { | |
| console.log(error); | |
| if (error instanceof TypeError) { | |
| return res.status(400).json({ message: "Invalid data structure" }); | |
| } | |
| return res.status(500).json({ | |
| message: "Internal server error", | |
| error: error instanceof Error ? error.message : "Unknown error" | |
| }); | |
| } |
Describe your changes
/usersrouteIssue number
Please ensure all items are checked off before requesting a review:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation