Skip to content
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.

Sign up for GitHub

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

058 Dec 19 Calculate Assessment and Compliance Tracker progress #367

Merged
merged 2 commits into from
Dec 23, 2024
Merged

058 Dec 19 Calculate Assessment and Compliance Tracker progress #367

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a195924
added code to calculate the assessment and compliance tracker proghress
HarshP4585 Dec 19, 2024
43f358d
changes questions to assessments in response
HarshP4585 Dec 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 89 additions & 0 deletions Servers/controllers/user.ctrl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,16 @@ import {
createNewUserQuery,
deleteUserByIdQuery,
getAllUsersQuery,
getAssessmentsForProject,
getControlCategoriesForProject,
getControlForControlCategory,
getQuestionsForSubTopic,
getSubControlForControl,
getSubTopicsForTopic,
getTopicsForAssessment,
Comment on lines +7 to +13
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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

getUserByEmailQuery,
getUserByIdQuery,
getUserProjects,
resetPasswordQuery,
updateUserByIdQuery,
} from "../utils/user.utils";
Expand Down Expand Up @@ -331,6 +339,86 @@ async function checkUserExists(
}
}

async function calculateProgress(
req: Request,
res: Response
): Promise<Response> {
try {
const id = parseInt(req.params.id)
const userProjects = await getUserProjects(id)
Comment on lines +347 to +348
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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" });
}


let assessmentsMetadata = []
let allTotalAssessments = 0
let allDoneAssessments = 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 totalAssessments = 0
let doneAssessments = 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) {
totalAssessments++;
if (question.answer) {
doneAssessments++
}
}
}
}
}
allTotalAssessments += totalAssessments
allDoneAssessments += doneAssessments
assessmentsMetadata.push({ projectId: userProject.id, totalAssessments, doneAssessments })
}

const response = {
controls: {
projects: controlsMetadata,
totalSubControls: allTotalSubControls,
doneSubControls: allDoneSubControls,
percentageComplete: Number(((allDoneSubControls / allTotalSubControls) * 100).toFixed(2))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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)) : 0

Also applies to: 412-412

},
assessments: {
projects: assessmentsMetadata,
totalAssessments: allTotalAssessments,
doneAssessments: allDoneAssessments,
percentageComplete: Number(((allDoneAssessments / allTotalAssessments) * 100).toFixed(2))
}
}
return res.status(200).json(response)
} catch (error) {
console.log(error);
return res.status(500).json({ message: "Internal server error" });
}
Comment on lines +416 to +419
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
} 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"
});
}

}

export {
getAllUsers,
getUserByEmail,
Expand All @@ -341,4 +429,5 @@ export {
updateUserById,
deleteUserById,
checkUserExists,
calculateProgress
};
3 changes: 3 additions & 0 deletions Servers/routes/user.route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
loginUser,
resetPassword,
updateUserById,
calculateProgress
} from "../controllers/user.ctrl";
import authenticateJWT from "../middleware/auth.middleware";

Expand Down Expand Up @@ -148,4 +149,6 @@ router.delete("/:id", authenticateJWT, deleteUserById);
*/
router.get("/check/exists", checkUserExists);

router.get("/:id/calculate-progress", calculateProgress)

Comment on lines +152 to +153
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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: Add authenticateJWT middleware 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

export default router;
84 changes: 84 additions & 0 deletions Servers/swagger.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3855,6 +3855,90 @@ paths:
example: internal server error
error:
type: object
/users/{id}/calculate-progress:
get:
tags: [users]
security:
- JWTAuth: []
parameters:
- in: path
name: id
schema:
type: integer
required: true
description: id of the user
responses:
"200":
description: user
content:
application/json:
type: object
properties:
controls:
type: object
properties:
projects:
type: array
items:
type: object
properties:
projectId:
type: integer
example: 1
totalSubControls:
type: integer
example: 12
doneSubControls:
type: integer
example: 1
totalSubControls:
type: integer
example: 12
doneSubControls:
type: integer
example: 1
percentageComplete:
type: number
format: float
example: 8.33
assessments:
type: object
properties:
projects:
type: array
items:
type: object
properties:
projectId:
type: integer
example: 1
totalAssessments:
type: integer
example: 34
doneAssessments:
type: integer
example: 0
totalAssessments:
type: integer
example: 34
doneAssessments:
type: integer
example: 0
percentageComplete:
type: number
format: float
example: 0
"500":
description: internal server error
content:
application/json:
type: object
properties:
message:
type: string
example: internal server error
error:
type: object

/roles:
get:
Expand Down
64 changes: 64 additions & 0 deletions Servers/utils/user.utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,3 +229,67 @@ export const checkUserExistsQuery = async (): Promise<boolean> => {
throw error;
}
};

export const getUserProjects = async (id: number) => {
const result = await pool.query(
"SELECT id FROM projects WHERE id = $1",
[id]
)
return result.rows
}

export const getControlCategoriesForProject = async (id: number) => {
const result = await pool.query(
"SELECT id FROM controlcategories WHERE project_id = $1",
[id]
)
return result.rows
}

export const getControlForControlCategory = async (id: number) => {
const result = await pool.query(
"SELECT id FROM controls WHERE control_group = $1",
[id]
)
return result.rows
}

export const getSubControlForControl = async (id: number) => {
const result = await pool.query(
"SELECT * FROM subcontrols WHERE control_id = $1",
[id]
)
return result.rows
}

export const getAssessmentsForProject = async (id: number) => {
const result = await pool.query(
"SELECT id FROM assessments WHERE project_id = $1",
[id]
)
return result.rows
}

export const getTopicsForAssessment = async (id: number) => {
const result = await pool.query(
"SELECT id FROM topics WHERE assessment_id = $1",
[id]
)
return result.rows
}

export const getSubTopicsForTopic = async (id: number) => {
const result = await pool.query(
"SELECT id FROM subtopics WHERE topic_id = $1",
[id]
)
return result.rows
}

export const getQuestionsForSubTopic = async (id: number) => {
const result = await pool.query(
"SELECT * FROM questions WHERE subtopic_id = $1",
[id]
)
return result.rows
}