enhancement/organization-apis#77
Conversation
…9/organization_apis
There was a problem hiding this comment.
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- src/docs/swagger.json: Language not supported
Comments suppressed due to low confidence (2)
src/routes/organization.routes.js:38
- Removal of verifyAdminPermission middleware from the update route might allow unauthorized organization updates. Please confirm that this change is intentional and that updateOrganization is sufficiently protected by other mechanisms.
verifyAdminPermission,
src/middlewares/verifyUserPermission.middleware.js:2
- The removal of the email check (req.user.email === req.params.email) may reduce flexibility in permission verification. Confirm that this simplified logic meets all intended use cases.
if (req.params.id === req.user.id || req.user.role === 'ADMIN') {
| const parts = url.pathname.split('/'); | ||
|
|
||
| // Cloudinary's publicId includes folders, so we remove `/image/upload/` and get everything after | ||
| const uploadIndex = parts.findIndex((part) => part === 'upload'); |
There was a problem hiding this comment.
Potential bug: If 'upload' is not found in the URL pathname, uploadIndex will be -1 leading to incorrect publicId extraction. Consider validating uploadIndex before slicing the parts array.
| const uploadIndex = parts.findIndex((part) => part === 'upload'); | |
| const uploadIndex = parts.findIndex((part) => part === 'upload'); | |
| if (uploadIndex === -1) { | |
| throw new Error('Invalid image URL: "upload" segment not found in pathname'); | |
| } |
| // Rest of the function remains the same... | ||
| // Handle boolean filter | ||
| if (filters.isVerified !== undefined) { | ||
| where.isVerified = ['true', '1', true].includes(filters.isVerified); |
There was a problem hiding this comment.
Potential bug: The boolean conversion for filters.isVerified does not explicitly handle false-equivalent values (e.g., 'false', '0'). Consider expanding the logic to clearly determine both true and false conditions.
| where.isVerified = ['true', '1', true].includes(filters.isVerified); | |
| if (['true', '1', true].includes(filters.isVerified)) { | |
| where.isVerified = true; | |
| } else if (['false', '0', false].includes(filters.isVerified)) { | |
| where.isVerified = false; | |
| } |
| const isMember = organization.users.length > 0; | ||
|
|
There was a problem hiding this comment.
Bug: The membership check only verifies that there are users in the organization, rather than confirming that the current user is a member. Consider checking if organization.users includes the current user's id.
| const isMember = organization.users.length > 0; | |
| const isMember = organization.users.some( | |
| (user) => user.id === req.user.id, | |
| ); |
PR Checklist (required)
Please check if your PR fulfills the following requirements:
The commit message follows our guidelines.
Tests for the changes have been added (for bug fixes / features)
PR Type
What kind of change does this PR introduce?
Related Issue
Closes #29