-
Notifications
You must be signed in to change notification settings - Fork 101
Fix project delete bug #2203
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
Fix project delete bug #2203
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -304,7 +304,7 @@ export const updateProjectUpdatedByIdQuery = async ( | |
| byTable: | ||
| | "controls" | ||
| | "answers" | ||
| | "projectrisks" | ||
| // | "projectrisks" | ||
| | "vendors" | ||
| | "subclauses" | ||
| | "annexcategories" | ||
|
|
@@ -316,9 +316,9 @@ export const updateProjectUpdatedByIdQuery = async ( | |
| const queryMap = { | ||
| controls: `SELECT pf.project_id as id FROM "${tenant}".controls_eu c JOIN "${tenant}".projects_frameworks pf ON pf.id = c.projects_frameworks_id WHERE c.id = :id;`, | ||
| answers: `SELECT pf.project_id as id FROM "${tenant}".assessments a JOIN "${tenant}".answers_eu ans ON ans.assessment_id = a.id JOIN "${tenant}".projects_frameworks pf ON pf.id = a.projects_frameworks_id WHERE ans.id = :id;`, | ||
| projectrisks: `SELECT p.id FROM | ||
| "${tenant}".projects p JOIN "${tenant}".projectrisks pr ON p.id = pr.project_id | ||
| WHERE pr.id = :id;`, | ||
| // projectrisks: `SELECT p.id FROM | ||
| // "${tenant}".projects p JOIN "${tenant}".projectrisks pr ON p.id = pr.project_id | ||
| // WHERE pr.id = :id;`, | ||
| vendors: `SELECT project_id as id FROM "${tenant}".vendors_projects WHERE vendor_id = :id;`, | ||
| subclauses: `SELECT pf.project_id as id FROM "${tenant}".subclauses_iso sc JOIN "${tenant}".projects_frameworks pf ON pf.id = sc.projects_frameworks_id WHERE sc.id = :id;`, | ||
| annexcategories: `SELECT pf.project_id as id FROM "${tenant}".annexcategories_iso a JOIN "${tenant}".projects_frameworks pf ON pf.id = a.projects_frameworks_id WHERE a.id = :id;`, | ||
|
|
@@ -480,7 +480,8 @@ export const deleteHelper = async ( | |
| let childIds: any = {}; | ||
| if ( | ||
| childTableName !== "projects_members" && | ||
| childTableName !== "projects_frameworks" | ||
| childTableName !== "projects_frameworks" && | ||
| childTableName !== "projects_risks" | ||
| ) { | ||
| if (childTableName === "vendors") { | ||
| childIds = await sequelize.query( | ||
|
|
@@ -555,7 +556,7 @@ export const deleteProjectByIdQuery = async ( | |
| }, | ||
| }, | ||
| { files: { foreignKey: "project_id", model: FileModel } }, | ||
| { projectrisks: { foreignKey: "project_id", model: RiskModel } }, | ||
| { projects_risks: { foreignKey: "project_id", model: RiskModel } }, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion 🧩 Analysis chainCascade deletion updated to projects_risks is right. Deleting join rows by project fixes the prior project delete bug while preserving risks. Confirm foreign key on projects_risks (project_id) is indexed for delete performance: 🏁 Script executed: #!/bin/bash
# Grep migrations for projects_risks FK/index on project_id
rg -nP -C2 'CREATE INDEX.*projects_risks.*project_id|projects_risks.*FOREIGN KEY.*project_id'Length of output: 1364 Add an index on projects_risks.project_id FK exists (projects_risks_project_id_fkey in Servers/database/migrations/20250918172152-migrate-project-risks.js and Servers/scripts/createNewTenant.ts) but no standalone index was found; the PK is (risk_id, project_id) so the btree PK index won’t help lookups by project_id alone — add an index to avoid full-table scans on project deletes. Suggested change (migration + tenant script): |
||
| { | ||
| projects_members: { | ||
| foreignKey: "project_id", | ||
|
|
@@ -604,7 +605,7 @@ export const calculateProjectRisks = async ( | |
| risk_level_autocalculated: string; | ||
| count: string; | ||
| }>( | ||
| `SELECT risk_level_autocalculated, count(*) AS count FROM "${tenant}".projectrisks WHERE project_id = :project_id GROUP BY risk_level_autocalculated`, | ||
| `SELECT risk_level_autocalculated, count(*) AS count FROM "${tenant}".risks r JOIN "${tenant}".projects_risks pr ON r.id = pr.risk_id WHERE pr.project_id = :project_id GROUP BY risk_level_autocalculated`, | ||
| { | ||
| replacements: { project_id }, | ||
| type: QueryTypes.SELECT, | ||
|
|
||
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.
🧩 Analysis chain
Avoid hardcoded framework IDs; align risk-framework association with the project.
Using literal [2, 3] may create inconsistent associations (risk linked to a project that doesn’t have those frameworks). Derive frameworks from the project’s current frameworks for correctness.
Apply this diff:
Add this helper just above the createRiskQuery call:
If the intent was to attach this risk to ISO frameworks, consider associating it with the organizational project instead (and then derive frameworks from that project). Confirm intended behavior.
🏁 Script executed:
Length of output: 1591
🏁 Script executed:
Length of output: 101
🏁 Script executed:
Length of output: 12852
Avoid hardcoded framework IDs — derive frameworks from the project
Hardcoding frameworks: [2, 3] assigns ISO frameworks to project.id (the created project uses different frameworks), causing inconsistent risk→project→framework associations.
Location: Servers/infrastructure.layer/driver/autoDriver.driver.ts — createRiskQuery call (around lines 171–173).
Apply this diff:
Add this helper just above the createRiskQuery call:
If the intent was to attach this risk to ISO frameworks, associate it with the organizational project (projectOrg) instead and derive frameworks from that project.
🤖 Prompt for AI Agents