Yb sep 22 statuses to organizational backend#2193
Yb sep 22 statuses to organizational backend#2193MuhammadKhalilzadeh merged 8 commits intodevelopfrom
Conversation
- add status in project's interface - update project's model to support status
- update status
WalkthroughAdds project status support across backend: new ProjectStatus enum, updated project interface and model column, migration adding public and per-tenant status columns, utility to allow status updates, a new PATCH /:id/status route and controller action that validates and updates status within a transaction (controller implementation duplicated in diff). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client as C
participant Router as R
participant Controller as CTRL
participant DB as DB
C->>R: PATCH /projects/:id/status { status }
R->>CTRL: updateProjectStatus(req, res)
CTRL->>DB: BEGIN TRANSACTION
CTRL->>CTRL: Validate status ∈ ProjectStatus
alt Invalid status
CTRL-->>C: 400 Bad Request
CTRL->>DB: ROLLBACK
else Valid status
CTRL->>DB: SELECT project by id
alt Not found
CTRL-->>C: 404 Not Found
CTRL->>DB: ROLLBACK
else Found
CTRL->>DB: UPDATE projects SET status, last_updated, last_updated_by
alt Update success
DB-->>CTRL: success
CTRL->>DB: COMMIT
CTRL-->>C: 200 OK (updated project)
else Update error
DB-->>CTRL: error
CTRL->>DB: ROLLBACK
CTRL-->>C: 500 Internal Server Error
end
end
end
note over CTRL,DB: Logs emitted at start, not-found, success, and failure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
Servers/utils/project.utils.ts (2)
355-367: Critical: update-path deletes all members when callers pass [] (e.g., status-only update).This block treats the incoming members array as the complete desired set. Callers like updateProjectStatus pass [], which will delete all existing members.
Apply this diff to make membership updates opt‑in only and avoid destructive deletes unless members is explicitly provided:
-export const updateProjectByIdQuery = async ( +export const updateProjectByIdQuery = async ( id: number, - project: Partial<ProjectModel>, - members: number[], + project: Partial<ProjectModel>, + members: number[] | undefined, tenant: string, transaction: Transaction ): Promise<(IProjectAttributes & { members: number[] }) | null> => { - const _currentMembers = await sequelize.query( + const _currentMembers = await sequelize.query( `SELECT user_id FROM "${tenant}".projects_members WHERE project_id = :project_id`, { replacements: { project_id: id }, mapToModel: true, model: ProjectsMembersModel, transaction, } ); const currentMembers = _currentMembers.map((m) => m.user_id); - const deletedMembers = currentMembers.filter((m) => !members.includes(m)); - const newMembers = members.filter((m) => !currentMembers.includes(m)); + const deletedMembers = Array.isArray(members) ? currentMembers.filter((m) => !members.includes(m)) : []; + const newMembers = Array.isArray(members) ? members.filter((m) => !currentMembers.includes(m)) : [];
394-417: Filtering out “falsy” values prevents legitimate updates (e.g., empty string, 0, false).The predicate
project[f] !== undefined && project[f]skips updates to falsy values. Use an undefined‑only check.Apply:
- ] - .filter((f) => { - if ( - project[f as keyof ProjectModel] !== undefined && - project[f as keyof ProjectModel] - ) { - updateProject[f as keyof ProjectModel] = - project[f as keyof ProjectModel]; - return true; - } - }) + ] + .filter((f) => project[f as keyof ProjectModel] !== undefined) + .map((f) => { + updateProject[f as keyof ProjectModel] = project[f as keyof ProjectModel]; + return f; + })Servers/routes/project.route.ts (1)
36-37: Route order bug: '/:id' shadows '/stats/:id'.As registered, GET /stats/123 will match '/:id' first. Move '/:id' after the more specific routes.
Apply:
-router.get("/:id", authenticateJWT, getProjectById); -router.get("/stats/:id", authenticateJWT, getProjectStatsById); +router.get("/stats/:id", authenticateJWT, getProjectStatsById); +router.get("/:id", authenticateJWT, getProjectById);Servers/controllers/project.ctrl.ts (1)
339-353: getProjectStatsById uses unresolved promises (missing await).ownerUser and userWhoUpdated are Promises; properties like name/surname will be undefined at runtime.
Suggested fix (illustrative):
- const ownerUser: any = getUserByIdQuery(project_owner); + const ownerUser: any = await getUserByIdQuery(project_owner); ... - const userWhoUpdated: any = getUserByIdQuery(project_last_updated_by); + const userWhoUpdated: any = await getUserByIdQuery(project_last_updated_by);
🧹 Nitpick comments (5)
Servers/utils/project.utils.ts (1)
419-429: Guard against empty SET clause.If no fields are provided (and members are undefined), this builds
UPDATE ... SET WHERE ..., which is invalid SQL.Suggested minimal guard:
- const query = `UPDATE "${tenant}".projects SET ${setClause} WHERE id = :id RETURNING *;`; + if (!setClause) { + // No field updates; return current row with updated members + const current = await getProjectByIdQuery(id, tenant); + const updatedMembers = await sequelize.query( + `SELECT user_id FROM "${tenant}".projects_members WHERE project_id = :project_id`, + { replacements: { project_id: id }, mapToModel: true, model: ProjectsMembersModel, transaction } + ); + return current + ? { ...(current as any).dataValues ?? current, members: updatedMembers.map((m: any) => m.user_id) } + : null; + } + const query = `UPDATE "${tenant}".projects SET ${setClause} WHERE id = :id RETURNING *;`;Servers/routes/project.route.ts (1)
39-39: Typo in path segment.'complainces' → 'compliances' (introduces a public API surface inconsistency).
Suggested:
-router.get("/complainces/:projid", authenticateJWT, getCompliances); +router.get("/compliances/:projid", authenticateJWT, getCompliances);Servers/domain.layer/interfaces/i.project.ts (1)
19-20: Interface extension looks good; optional on write, present on read.status?: ProjectStatus is fine for create/update. Consider documenting that reads will always include a non‑null status.
Servers/controllers/project.ctrl.ts (2)
329-360: Unrelated but important: fileName typos in logs ('projec.ctrl.ts').Multiple occurrences; keep logs consistent for observability.
410-441: Return codes: consider 200 instead of 202 for GET endpoints.These endpoints return data immediately; 200 is more conventional for successful GETs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Servers/controllers/project.ctrl.ts(2 hunks)Servers/database/migrations/20250922000001-add-project-status.js(1 hunks)Servers/domain.layer/enums/project-status.enum.ts(1 hunks)Servers/domain.layer/interfaces/i.project.ts(2 hunks)Servers/domain.layer/models/project/project.model.ts(2 hunks)Servers/routes/project.route.ts(2 hunks)Servers/utils/project.utils.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
Servers/domain.layer/enums/project-status.enum.ts (1)
Clients/src/application/hooks/useProjectStatus.tsx (1)
ProjectStatus(46-49)
Servers/domain.layer/interfaces/i.project.ts (1)
Clients/src/application/hooks/useProjectStatus.tsx (1)
ProjectStatus(46-49)
Servers/database/migrations/20250922000001-add-project-status.js (1)
Servers/tools/getTenantHash.ts (1)
getTenantHash(3-6)
Servers/domain.layer/models/project/project.model.ts (1)
Clients/src/application/hooks/useProjectStatus.tsx (1)
ProjectStatus(46-49)
Servers/controllers/project.ctrl.ts (2)
Servers/utils/logger/logHelper.ts (3)
logProcessing(22-30)logSuccess(33-49)logFailure(51-68)Servers/utils/project.utils.ts (2)
getProjectByIdQuery(114-159)updateProjectByIdQuery(348-446)
Servers/routes/project.route.ts (1)
Servers/controllers/project.ctrl.ts (1)
updateProjectStatus(730-799)
🔇 Additional comments (8)
Servers/domain.layer/models/project/project.model.ts (2)
12-12: Import is correct and scoped.Matches the enum usage in this file and across the stack.
89-95: Add status column — enums are in sync with the DB migration.
ProjectStatus in Servers/domain.layer/enums/project-status.enum.ts matches Servers/database/migrations/20250922000001-add-project-status.js (ENUM values and default 'Not started').Servers/routes/project.route.ts (2)
69-70: Status route added — good.PATCH /:id/status is a clear, narrowly scoped endpoint.
19-20: Importing updateProjectStatus — OK.Matches the new PATCH route wiring.
Servers/domain.layer/interfaces/i.project.ts (1)
3-3: Import is correct and consistent.Servers/database/migrations/20250922000001-add-project-status.js (1)
26-65: Transactional DDL across all tenants — OK, but monitor lock time.Single long transaction is safer for rollback but may extend locks during peak hours. Plan deployment during low traffic.
Servers/controllers/project.ctrl.ts (1)
28-28: Import of ProjectStatus is correct.Servers/domain.layer/enums/project-status.enum.ts (1)
1-9: Confirm canonical "Closed" wording and adopt machine tokens
- Repo search: no "Closed (archived)" found. Backend enum uses "Closed" (Servers/domain.layer/enums/project-status.enum.ts); client maps Canceled -> "Closed" (Clients/src/domain/enums/mitigitaion.ts:7).
- Recommendation: agree a canonical label in the product spec, persist a machine‑friendly token (e.g., CLOSED) in the DB, and render the human label (e.g., "Closed (archived)") in the UI/i18n layer to avoid future migrations.
| try { | ||
| // Validate status value | ||
| if (!status || !Object.values(ProjectStatus).includes(status)) { | ||
| return res.status(400).json( | ||
| STATUS_CODE[400]({ message: "Valid status is required" }) | ||
| ); | ||
| } | ||
|
|
||
| // Check if project exists | ||
| const existingProject = await getProjectByIdQuery(projectId, req.tenantId!); | ||
| if (!existingProject) { | ||
| await logSuccess({ | ||
| eventType: "Update", | ||
| description: `Project not found for status update: ID ${projectId}`, | ||
| functionName: "updateProjectStatus", | ||
| fileName: "project.ctrl.ts", | ||
| }); | ||
|
|
||
| return res.status(404).json(STATUS_CODE[404]({})); | ||
| } |
There was a problem hiding this comment.
Authorization check missing for status updates.
Endpoint requires JWT but does not verify project ownership/membership/admin role. Add a guard to prevent arbitrary status changes across tenants.
Suggested snippet:
+ // Authorization: only owner, project member, or Admin can update
+ const { userId, role } = req;
+ const isAdmin = role === 'Admin';
+ const isMember = isAdmin ? true : await sequelize.query(
+ `SELECT 1 FROM "${req.tenantId}".projects_members WHERE project_id = :project_id AND user_id = :user_id LIMIT 1`,
+ { replacements: { project_id: projectId, user_id: userId }, type: QueryTypes.SELECT }
+ ).then(r => r.length > 0);
+ if (!isMember) {
+ return res.status(403).json(STATUS_CODE[403]({ message: "Forbidden" }));
+ }📝 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.
| try { | |
| // Validate status value | |
| if (!status || !Object.values(ProjectStatus).includes(status)) { | |
| return res.status(400).json( | |
| STATUS_CODE[400]({ message: "Valid status is required" }) | |
| ); | |
| } | |
| // Check if project exists | |
| const existingProject = await getProjectByIdQuery(projectId, req.tenantId!); | |
| if (!existingProject) { | |
| await logSuccess({ | |
| eventType: "Update", | |
| description: `Project not found for status update: ID ${projectId}`, | |
| functionName: "updateProjectStatus", | |
| fileName: "project.ctrl.ts", | |
| }); | |
| return res.status(404).json(STATUS_CODE[404]({})); | |
| } | |
| try { | |
| // Validate status value | |
| if (!status || !Object.values(ProjectStatus).includes(status)) { | |
| return res.status(400).json( | |
| STATUS_CODE[400]({ message: "Valid status is required" }) | |
| ); | |
| } | |
| // Authorization: only owner, project member, or Admin can update | |
| const { userId, role } = req; | |
| const isAdmin = role === 'Admin'; | |
| const isMember = isAdmin ? true : await sequelize.query( | |
| `SELECT 1 FROM "${req.tenantId}".projects_members WHERE project_id = :project_id AND user_id = :user_id LIMIT 1`, | |
| { replacements: { project_id: projectId, user_id: userId }, type: QueryTypes.SELECT } | |
| ).then(r => r.length > 0); | |
| if (!isMember) { | |
| return res.status(403).json(STATUS_CODE[403]({ message: "Forbidden" })); | |
| } | |
| // Check if project exists | |
| const existingProject = await getProjectByIdQuery(projectId, req.tenantId!); | |
| if (!existingProject) { | |
| await logSuccess({ | |
| eventType: "Update", | |
| description: `Project not found for status update: ID ${projectId}`, | |
| functionName: "updateProjectStatus", | |
| fileName: "project.ctrl.ts", | |
| }); | |
| return res.status(404).json(STATUS_CODE[404]({})); | |
| } |
🤖 Prompt for AI Agents
In Servers/controllers/project.ctrl.ts around lines 741 to 760, the status
update path is missing an authorization guard allowing any authenticated JWT to
change a project's status; add a check that the requester is authorized by
verifying req.tenantId matches the project's tenant and that the user (from
req.user or req.userId/role) is either the project owner, an admin of that
tenant, or a project member with permission to change status; if not authorized
return res.status(403).json(STATUS_CODE[403]({})); include an audit log entry
for denied attempts and proceed only when the authorization check passes.
| const updatedProject = await updateProjectByIdQuery( | ||
| projectId, | ||
| { status, last_updated: new Date(), last_updated_by: req.userId! }, | ||
| [], // no members update | ||
| req.tenantId!, | ||
| transaction | ||
| ); |
There was a problem hiding this comment.
Passing [] for members will purge all project members.
When calling updateProjectByIdQuery with [], the util interprets it as “no members should remain.” Use undefined to signify “do not change members,” paired with the util change.
Apply:
- const updatedProject = await updateProjectByIdQuery(
+ const updatedProject = await updateProjectByIdQuery(
projectId,
{ status, last_updated: new Date(), last_updated_by: req.userId! },
- [], // no members update
+ undefined, // do not modify members
req.tenantId!,
transaction
);And ensure Servers/utils/project.utils.ts is updated per the related comment.
📝 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 updatedProject = await updateProjectByIdQuery( | |
| projectId, | |
| { status, last_updated: new Date(), last_updated_by: req.userId! }, | |
| [], // no members update | |
| req.tenantId!, | |
| transaction | |
| ); | |
| const updatedProject = await updateProjectByIdQuery( | |
| projectId, | |
| { status, last_updated: new Date(), last_updated_by: req.userId! }, | |
| undefined, // do not modify members | |
| req.tenantId!, | |
| transaction | |
| ); |
🤖 Prompt for AI Agents
In Servers/controllers/project.ctrl.ts around lines 763 to 769, the call to
updateProjectByIdQuery currently passes an empty array for members which will
remove all project members; change that argument to undefined to indicate “do
not change members” and leave members untouched, and update
Servers/utils/project.utils.ts so the util treats undefined as no-op (no member
changes) while still treating an empty array as an explicit clear of members.
| @@ -0,0 +1,105 @@ | |||
| 'use strict'; | |||
| const { getTenantHash } = require("../../dist/tools/getTenantHash"); | |||
There was a problem hiding this comment.
Do not require build artifacts from migrations.
Requiring from ../../dist makes migrations environment‑dependent and brittle (dist may not exist in CI/production). Inline the helper or import from a JS util that lives with migrations.
Apply:
-'use strict';
-const { getTenantHash } = require("../../dist/tools/getTenantHash");
+'use strict';
+const crypto = require("crypto");
+function getTenantHash(tenantId) {
+ const hash = crypto.createHash('sha256').update(String(tenantId)).digest('base64');
+ return hash.replace(/[^a-zA-Z0-9]/g, '').slice(0, 10);
+}📝 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 { getTenantHash } = require("../../dist/tools/getTenantHash"); | |
| 'use strict'; | |
| const crypto = require("crypto"); | |
| function getTenantHash(tenantId) { | |
| const hash = crypto.createHash('sha256').update(String(tenantId)).digest('base64'); | |
| return hash.replace(/[^a-zA-Z0-9]/g, '').slice(0, 10); | |
| } |
🤖 Prompt for AI Agents
In Servers/database/migrations/20250922000001-add-project-status.js around line
2, the migration currently requires a helper from ../../dist/tools/getTenantHash
which depends on build artifacts; replace that by either inlining the
getTenantHash implementation directly into this migration or moving the helper
into a source file alongside migrations (e.g.,
Servers/database/migrations/utils/getTenantHash.js) and require that relative JS
module instead; update the require to point to the new local util or remove the
external require and paste the helper code here, ensuring the migration no
longer depends on ../../dist so it can run in unbuilt environments.
| async down(queryInterface, Sequelize) { | ||
| const transaction = await queryInterface.sequelize.transaction(); | ||
| try { | ||
| // Remove status column from public.projects table | ||
| await queryInterface.removeColumn('projects', 'status', { transaction }); | ||
|
|
||
| // Get all organizations to update their tenant schemas | ||
| const organizations = await queryInterface.sequelize.query( | ||
| `SELECT id FROM public.organizations;`, { transaction } | ||
| ); | ||
|
|
||
| // Remove status column from each tenant's projects table | ||
| for (let organization of organizations[0]) { | ||
| const tenantHash = getTenantHash(organization.id); | ||
|
|
||
| await queryInterface.removeColumn( | ||
| { | ||
| tableName: 'projects', | ||
| schema: tenantHash | ||
| }, | ||
| 'status', | ||
| { transaction } | ||
| ); | ||
| } | ||
|
|
||
| await transaction.commit(); | ||
| } catch (error) { | ||
| await transaction.rollback(); | ||
| throw error; | ||
| } | ||
| } | ||
| }; No newline at end of file |
There was a problem hiding this comment.
Drop the generated ENUM types on down().
Sequelize creates enum types (e.g., enum_projects_status) per schema. Removing columns leaves orphaned types.
Apply:
async down(queryInterface, Sequelize) {
const transaction = await queryInterface.sequelize.transaction();
try {
- // Remove status column from public.projects table
+ // Remove status column from public.projects table
await queryInterface.removeColumn('projects', 'status', { transaction });
+ // Drop public enum type if present
+ await queryInterface.sequelize.query('DROP TYPE IF EXISTS "enum_projects_status";', { transaction });
// Get all organizations to update their tenant schemas
const organizations = await queryInterface.sequelize.query(
`SELECT id FROM public.organizations;`, { transaction }
);
// Remove status column from each tenant's projects table
for (let organization of organizations[0]) {
const tenantHash = getTenantHash(organization.id);
-
+
await queryInterface.removeColumn(
{
tableName: 'projects',
schema: tenantHash
},
'status',
{ transaction }
);
+ // Drop tenant-scoped enum type
+ await queryInterface.sequelize.query(
+ `DROP TYPE IF EXISTS "${tenantHash}"."enum_projects_status";`,
+ { transaction }
+ );
}
await transaction.commit();📝 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 down(queryInterface, Sequelize) { | |
| const transaction = await queryInterface.sequelize.transaction(); | |
| try { | |
| // Remove status column from public.projects table | |
| await queryInterface.removeColumn('projects', 'status', { transaction }); | |
| // Get all organizations to update their tenant schemas | |
| const organizations = await queryInterface.sequelize.query( | |
| `SELECT id FROM public.organizations;`, { transaction } | |
| ); | |
| // Remove status column from each tenant's projects table | |
| for (let organization of organizations[0]) { | |
| const tenantHash = getTenantHash(organization.id); | |
| await queryInterface.removeColumn( | |
| { | |
| tableName: 'projects', | |
| schema: tenantHash | |
| }, | |
| 'status', | |
| { transaction } | |
| ); | |
| } | |
| await transaction.commit(); | |
| } catch (error) { | |
| await transaction.rollback(); | |
| throw error; | |
| } | |
| } | |
| }; | |
| async down(queryInterface, Sequelize) { | |
| const transaction = await queryInterface.sequelize.transaction(); | |
| try { | |
| // Remove status column from public.projects table | |
| await queryInterface.removeColumn('projects', 'status', { transaction }); | |
| // Drop public enum type if present | |
| await queryInterface.sequelize.query('DROP TYPE IF EXISTS "enum_projects_status";', { transaction }); | |
| // Get all organizations to update their tenant schemas | |
| const organizations = await queryInterface.sequelize.query( | |
| `SELECT id FROM public.organizations;`, { transaction } | |
| ); | |
| // Remove status column from each tenant's projects table | |
| for (let organization of organizations[0]) { | |
| const tenantHash = getTenantHash(organization.id); | |
| await queryInterface.removeColumn( | |
| { | |
| tableName: 'projects', | |
| schema: tenantHash | |
| }, | |
| 'status', | |
| { transaction } | |
| ); | |
| // Drop tenant-scoped enum type | |
| await queryInterface.sequelize.query( | |
| `DROP TYPE IF EXISTS "${tenantHash}"."enum_projects_status";`, | |
| { transaction } | |
| ); | |
| } | |
| await transaction.commit(); | |
| } catch (error) { | |
| await transaction.rollback(); | |
| throw error; | |
| } | |
| } | |
| }; |
🤖 Prompt for AI Agents
In Servers/database/migrations/20250922000001-add-project-status.js around lines
74 to 105, the down() flow removes the projects.status column but leaves behind
Sequelize-generated ENUM types (e.g., enum_projects_status) in public and tenant
schemas; after removing the column(s) you must explicitly drop the enum type
within the same transaction to avoid orphaned types — run a DROP TYPE IF EXISTS
"enum_projects_status" (qualified in the target schema) for public and for each
tenant schema (use the tenantHash to qualify the schema or run SET search_path)
inside the loop and after the public removeColumn, ensuring all DROP TYPE
queries execute with the same transaction and rollback on error.
|
@Aryanak47 You have a conflict in here now |
…into yb-sep-22-statuses-to-organizational-backend
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Servers/utils/project.utils.ts (2)
348-354: Makemembersoptional to avoid accidental purges.Passing undefined should mean “don’t change members.” Current required param leads to mass‑removals when callers pass [] by default.
Apply:
export const updateProjectByIdQuery = async ( id: number, project: Partial<ProjectModel>, - members: number[], + members?: number[], tenant: string, transaction: Transaction ): Promise<(IProjectAttributes & { members: number[] }) | null> => {
355-392: Guard member updates only whenmembersis provided.Wrap the membership diff logic so that undefined skips updates; empty array still intentionally clears membership.
- const _currentMembers = await sequelize.query( - `SELECT user_id FROM "${tenant}".projects_members WHERE project_id = :project_id`, - { - replacements: { project_id: id }, - mapToModel: true, - model: ProjectsMembersModel, - transaction, - } - ); - const currentMembers = _currentMembers.map((m) => m.user_id); - const deletedMembers = currentMembers.filter((m) => !members.includes(m)); - const newMembers = members.filter((m) => !currentMembers.includes(m)); - - for (let member of deletedMembers) { - await sequelize.query( - `DELETE FROM "${tenant}".projects_members WHERE user_id = :user_id AND project_id = :project_id`, - { - replacements: { user_id: member, project_id: id }, - mapToModel: true, - model: ProjectsMembersModel, - type: QueryTypes.DELETE, - transaction, - } - ); - } - - for (let member of newMembers) { - await sequelize.query( - `INSERT INTO "${tenant}".projects_members (project_id, user_id) VALUES (:project_id, :user_id);`, - { - replacements: { user_id: member, project_id: id }, - mapToModel: true, - model: ProjectsMembersModel, - // type: QueryTypes.INSERT - transaction, - } - ); - } + if (members !== undefined) { + const _currentMembers = await sequelize.query( + `SELECT user_id FROM "${tenant}".projects_members WHERE project_id = :project_id`, + { + replacements: { project_id: id }, + mapToModel: true, + model: ProjectsMembersModel, + transaction, + } + ); + const currentMembers = _currentMembers.map((m) => m.user_id); + const deletedMembers = currentMembers.filter((m) => !members.includes(m)); + const newMembers = members.filter((m) => !currentMembers.includes(m)); + + for (let member of deletedMembers) { + await sequelize.query( + `DELETE FROM "${tenant}".projects_members WHERE user_id = :user_id AND project_id = :project_id`, + { + replacements: { user_id: member, project_id: id }, + mapToModel: true, + model: ProjectsMembersModel, + type: QueryTypes.DELETE, + transaction, + } + ); + } + + for (let member of newMembers) { + await sequelize.query( + `INSERT INTO "${tenant}".projects_members (project_id, user_id) VALUES (:project_id, :user_id);`, + { + replacements: { user_id: member, project_id: id }, + mapToModel: true, + model: ProjectsMembersModel, + // type: QueryTypes.INSERT + transaction, + } + ); + } + }Servers/controllers/project.ctrl.ts (1)
1-30: Add QueryTypes import for authorization query.import { Request, Response } from "express"; +import { QueryTypes } from "sequelize";
🧹 Nitpick comments (1)
Servers/utils/project.utils.ts (1)
406-416: Don’t drop falsy but valid updates (''/0/false/null).Current truthy check skips legitimate updates. Prefer explicit undefined check and presence test.
- .filter((f) => { - if ( - project[f as keyof ProjectModel] !== undefined && - project[f as keyof ProjectModel] - ) { - updateProject[f as keyof ProjectModel] = - project[f as keyof ProjectModel]; - return true; - } - }) + .filter((f) => { + if ( + Object.prototype.hasOwnProperty.call(project, f) && + project[f as keyof ProjectModel] !== undefined + ) { + updateProject[f as keyof ProjectModel] = project[f as keyof ProjectModel]; + return true; + } + return false; + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Servers/controllers/project.ctrl.ts(2 hunks)Servers/utils/project.utils.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Servers/controllers/project.ctrl.ts (2)
Servers/utils/logger/logHelper.ts (3)
logProcessing(22-30)logSuccess(33-49)logFailure(51-68)Servers/utils/project.utils.ts (2)
getProjectByIdQuery(114-159)updateProjectByIdQuery(348-446)
🔇 Additional comments (4)
Servers/utils/project.utils.ts (1)
404-405: Includingstatusin updatable fields looks good.Servers/controllers/project.ctrl.ts (3)
28-28: Import of ProjectStatus enum is correct.
783-785: Passing [] will purge all members; use undefined.- [], // no members update + undefined, // do not modify members
746-816: Missing authorization guard; any authenticated user can update status.Enforce owner/member/Admin check within tenant before updating. Also add a 401 guard and use the transaction for the membership check.
export async function updateProjectStatus(req: Request, res: Response): Promise<any> { const transaction = await sequelize.transaction(); const projectId = parseInt(req.params.id); const { status } = req.body; logProcessing({ description: `starting updateProjectStatus for ID ${projectId}`, functionName: "updateProjectStatus", fileName: "project.ctrl.ts", }); try { + // Require auth context + if (!req.userId || !req.role) { + await logFailure({ + eventType: "Update", + description: "Unauthorized status update attempt", + functionName: "updateProjectStatus", + fileName: "project.ctrl.ts", + error: new Error("Unauthorized"), + }); + await transaction.rollback(); + return res.status(401).json({ message: "Unauthorized" }); + } + // Validate status value if (!status || !Object.values(ProjectStatus).includes(status)) { return res.status(400).json( STATUS_CODE[400]({ message: "Valid status is required" }) ); } // Check if project exists const existingProject = await getProjectByIdQuery(projectId, req.tenantId!); if (!existingProject) { await logSuccess({ eventType: "Update", description: `Project not found for status update: ID ${projectId}`, functionName: "updateProjectStatus", fileName: "project.ctrl.ts", }); return res.status(404).json(STATUS_CODE[404]({})); } + + // Authorization: only Admin, owner, or project member + const isAdmin = req.role === "Admin"; + const isOwner = existingProject.owner === req.userId; + let isMember = false; + if (!isAdmin && !isOwner) { + const membership = await sequelize.query( + `SELECT 1 FROM "${req.tenantId}".projects_members WHERE project_id = :project_id AND user_id = :user_id LIMIT 1`, + { + replacements: { project_id: projectId, user_id: req.userId }, + type: QueryTypes.SELECT, + transaction, + } + ); + isMember = (membership as any[]).length > 0; + } + if (!isAdmin && !isOwner && !isMember) { + await logFailure({ + eventType: "Update", + description: `Forbidden: user ${req.userId} not authorized to update status for project ${projectId}`, + functionName: "updateProjectStatus", + fileName: "project.ctrl.ts", + error: new Error("Forbidden"), + }); + await transaction.rollback(); + return res.status(403).json(STATUS_CODE[403]({ message: "Forbidden" })); + } // Update project status const updatedProject = await updateProjectByIdQuery( projectId, { status, last_updated: new Date(), last_updated_by: req.userId! }, - [], // no members update + undefined, // do not modify members req.tenantId!, transaction );
|
@HarshP4585 Would you please provide your feedback here asap? We need to merge this. It's been open for a while now, |
HarshP4585
left a comment
HarshP4585
left a comment
There was a problem hiding this comment.
Everything looks good, just need to update createNewTenant script to include status field in projects
…into yb-sep-22-statuses-to-organizational-backend
I have just updated 'createNewTenant' script. |
@HarshP4585 Would you do a quick double check please? |
MuhammadKhalilzadeh
left a comment
MuhammadKhalilzadeh
left a comment
There was a problem hiding this comment.
Thank you so much @Aryanak47
MuhammadKhalilzadeh
deleted the
yb-sep-22-statuses-to-organizational-backend
branch
4 participants
Describe your changes
This PR implements a backend for project status tracking system for organizational frameworks. Projects can now have one of seven status values that can be managed through a new API endpoint and will be displayed in a Settings tab within the framework UI.
Write your issue number after "Fixes "
Fixes #2157
Please ensure all items are checked off before requesting a review: