Issue #251045 feat: Hierarchical Categories Implementation#668
Issue #251045 feat: Hierarchical Categories Implementation#668vaivk369 wants to merge 10 commits intoELEVATE-Project:developfrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughAdds hierarchical project library categories (parent/child, sequence, keywords), new delete/details endpoints, validation and helper logic for hierarchy management, schema/index updates, API response messages, and extends project templates listing to support category filtering/grouping and task-detail enrichment. Changes
Sequence DiagramssequenceDiagram
participant Client
participant Controller as LibraryCategoriesController
participant Helper as LibraryCategoriesHelper
participant DB as Database
Client->>Controller: DELETE /project/v1/library/categories/delete/:id
Controller->>Helper: delete({ _id: id, tenantId }, userDetails)
Helper->>DB: Find category by id & tenant
DB-->>Helper: category
alt not found
Helper-->>Controller: error (not found)
Controller-->>Client: 404
else found
Helper->>DB: Count child categories
DB-->>Helper: childCount
alt childCount > 0
Helper-->>Controller: error (has children)
Controller-->>Client: 400
else no children
Helper->>DB: Count associated projects
DB-->>Helper: projectCount
alt projectCount > 0
Helper-->>Controller: error (has projects)
Controller-->>Client: 400
else safe to delete
Helper->>DB: Soft-delete category (isDeleted)
DB-->>Helper: updatedCategory
Helper->>DB: Update parent hasChildCategories if needed
DB-->>Helper: parentUpdated
Helper-->>Controller: success(result)
Controller-->>Client: 200
end
end
end
sequenceDiagram
participant Client
participant Controller as ProjectTemplatesController
participant Helper as ProjectTemplatesHelper
participant DB as Database
Client->>Controller: GET /project/v1/project-templates?categoryIds=...&groupByCategory=true
Controller->>Helper: list(pageNo, pageSize, searchText, currentOrgOnly, userDetails, categoryIds, groupByCategory, taskDetails)
Helper->>DB: Query templates with optional category filter
DB-->>Helper: paginatedTemplates
alt groupByCategory == true
Helper->>Helper: Group templates by categoryId
alt taskDetails == true
Helper->>Helper: Enrich templates with tasks and subtasks
end
Helper-->>Controller: groupedResult
else
alt taskDetails == true
Helper->>Helper: Enrich templates with tasks
end
Helper-->>Controller: paginatedResult
end
Controller-->>Client: 200 result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 review |
✅ Actions performedReview triggered.
|
![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)
controllers/v1/project/templates.js (2)
815-828: Ensure query params passed toprojectTemplatesHelper.listare normalized to booleans
currentOrgOnly,categoryIds(string), andgroupByCategoryare passed straight fromreq.query. The helper already normalizescurrentOrgOnly, butgroupByCategoryis consumed as‑is, so values like?groupByCategory=falsewill still be treated as truthy.If you don’t normalize inside the controller, make sure the helper does (see suggested change in
list), so that:
"true"/"false"/true/falseall behave as expected.- Omitting the param defaults to
false.Given the existing pattern in this file (booleans normalized in helpers), fixing this centrally in
ProjectTemplatesHelper.listis preferable to duplicating parsing logic here.
903-913: NormalizeisExternalProgrambefore callingcreateChildProjectTemplate
req.query.isExternalProgramarrives as a string and is passed directly without type normalization. While the downstreamsolutionsUtils.createSolutiondoes callUTILS.convertStringToBoolean(isExternalProgram)internally, normalizing at the controller entry point is cleaner and aligns with patterns elsewhere in the codebase (e.g.,controllers/v1/userProjects.js).Convert it in the controller:
const isExternalProgram = req.query.isExternalProgram !== undefined ? UTILS.convertStringToBoolean(req.query.isExternalProgram) : trueThis ensures the boolean type is correct from the outset and keeps query parameter handling consistent across the API.
module/project/templates/helper.js (1)
611-735: Fix critical bugs increateChildProjectTemplate: wrong response property and severely misaligned cleanup parametersThree high-impact correctness issues:
Use
createdSolution.datainstead ofcreatedSolution.result(lines 673, 677)
The functionhandleDuplicateTemplateTaskcorrectly accesses the same helper's response asnewSolution.data._id. Using.resulthere causesundefinedvalues, breaking cleanup logic.Fix
deletedResourceDetailscall with correct parameter alignment (lines 726–731)
The signature expects(resourceId, resourceType, isAPrivateProgram, tenantId, orgId, deletedBy), but the call passes(resourceId, resourceType, tenantId, tenantAndOrgInfo[0], userId). This misalignment breaks the cleanup logic entirely:
tenantIdplaced in position 3 (expects boolean)tenantAndOrgInfo[0]isundefined(array indexing on object)userIdplaced whereorgIdbelongsdeletedBynot passedUse the established pattern from admin.js: pass
tenantAndOrgInfo.orgId[0]for the org parameter, anduserIdas the sixth argument.Normalize
isExternalProgramfrom string to boolean
Query parameters are strings. Convert usingUTILS.convertStringToBoolean()(see admin.js:171 pattern) or ensure the helper handles string inputs safely.Corrected parameter order for deletedResourceDetails
- await adminHelper.deletedResourceDetails( - _id, - CONSTANTS.common.SOLUTION, - userDetails.tenantAndOrgInfo.tenantId, - userDetails.tenantAndOrgInfo[0], - userDetails.userInformation.userId - ) + await adminHelper.deletedResourceDetails( + _id, + CONSTANTS.common.SOLUTION, + false, // isAPrivateProgram + userDetails.tenantAndOrgInfo.tenantId, + userDetails.tenantAndOrgInfo.orgId[0], + userDetails.userInformation.userId // deletedBy + )module/library/categories/validator/v1.js (1)
32-34: Add validation for updatable fields in the update method.The update method only validates the
_idparameter but doesn't validate body fields. If fields likedescription,keywords,parent_id, orsequenceNumbercan be updated, they should have the same validation rules as in the create method.♻️ Suggested enhancement
update: function () { req.checkParams('_id').exists().withMessage('required category id') + req.checkBody('description').optional().isString().withMessage('description must be a string') + req.checkBody('keywords').optional().isArray().withMessage('keywords must be an array') + if (req.body.keywords) { + req.body.keywords.forEach((keyword, index) => { + req.checkBody(`keywords[${index}]`) + .isString() + .withMessage(`keyword at index ${index} must be a string`) + }) + } + req.checkBody('parent_id').optional().isMongoId().withMessage('parent_id must be a valid ObjectId') + req.checkBody('hasChildCategories') + .not() + .exists() + .withMessage('hasChildCategories cannot be set in request body') + req.checkBody('sequenceNumber') + .optional() + .isInt({ min: 0 }) + .withMessage('sequenceNumber must be a non-negative integer') },Note: Consider extracting common validation logic into a shared function to avoid duplication.
🤖 Fix all issues with AI agents
In @models/project-categories.js:
- Around line 69-94: The compound index keys use parent_id but the schema
defines parentId, so update the compound index definition in
models/project-categories.js to use parentId (and keep tenantId and
sequenceNumber unchanged), and then audit and fix all usages in
LibraryCategoriesHelper (create/update/list/delete) to consistently reference
parentId (not parent_id) and the existing hasChildCategories and sequenceNumber
fields so queries and updates use the same field names as the schema.
In @module/library/categories/helper.js:
- Around line 1114-1215: In the static delete method, the check for associated
templates uses categoryData.noOfProjects but treats it like an array
(associatedTemplates.length), which always fails; change the logic to treat
noOfProjects as a number (e.g., const associatedTemplates =
categoryData.noOfProjects || 0 and then if (associatedTemplates > 0) throw the
bad_request error) so deletion is blocked when noOfProjects is greater than
zero; update the check around categoryData.noOfProjects and ensure the existing
error payload is thrown when the numeric condition is met.
- Around line 910-951: create currently mixes categoryData.parentId and
categoryData.parent_id which causes validateParentId, sequenceNumber calculation
and DB queries (projectCategoriesQueries.categoryDocuments) to operate on
different fields leading to incorrect sibling lookups and inconsistent stored
documents; normalize to a single canonical field (use parentId): derive const
parentId = categoryData.parentId || categoryData.parent_id || null, set
categoryData.parentId = parentId and delete categoryData.parent_id, call
this.validateParentId(parentId, ...) and use parentId for all sequenceNumber
logic and for the query filters passed to
projectCategoriesQueries.categoryDocuments, ensuring sequenceNumber assignment
(sequenceNumber variable) uses the normalized parentId for both sibling and root
queries and store only parentId in the created document.
In @module/project/templates/helper.js:
- Around line 1985-1993: In the static list(...) method normalize
groupByCategory exactly like currentOrgOnly (convert req.query string
"true"/"false" to a boolean) right after the currentOrgOnly conversion so
"false" doesn’t evaluate truthy; and when grouping results (the loop around
categories in list), precompute a Set of requested categoryIds
(parsed/normalized to the same _id type as categories) and only add a template
to groups for categories whose _id exists in that Set so templates aren’t placed
into categories that weren’t requested.
🧹 Nitpick comments (5)
module/library/categories/helper.js (3)
37-94: Hierarchy helpers are solid, but consider guardingisDescendantagainst cyclesThe new
validateParentId,isDescendant, andgetHierarchyDepthhelpers add good safeguards (self-parenting and descendant checks, tenant scoping).One minor robustness concern:
isDescendantwalksparentIdup the tree with awhile (currentId)loop and no depth/visited limit.- If historical data already contains a cycle,
isDescendantcan loop indefinitely, repeatedly querying the same nodes.Given
getHierarchyDepthusesdepth < 10as a safety cap, consider applying a similar depth or visited-set guard inisDescendantas a defensive measure. This isn’t a blocker but will make the hierarchy routines more resilient to bad data.Also applies to: 105-130
655-685: Parent update logic: good overall, but keepparentIdhandling consistentThe
updateflow now:
- Validates
updateData.parentIdviavalidateParentId.- Adjusts
hasChildCategorieson the old parent if the category moves.- Sets
hasChildCategories: trueon the new parent when applicable.This correctly maintains the parent’s leaf/branch status.
However:
validateParentIdreturnsparentCategory, but the localparentCategoryvariable is never used; you can drop it to avoid confusion.- Ensure that everywhere else in the codebase you rely on the same field name (
parentId, notparent_id) for parent relations; some parts ofcreateand the model still refer toparent_id, which will bypass this logic.Not a blocker, but cleaning up these minor inconsistencies will avoid subtle bugs later.
Also applies to: 721-750
1123-1222: Async Promise executor lint warnings can likely be ignored for consistencyThe new
deleteanddetailsmethods follow the existing pattern in this service:static delete(filterQuery, userDetails) { return new Promise(async (resolve, reject) => { // ... }) }Biome flags
noAsyncPromiseExecutor, but this pattern is used widely across the codebase for helpers/controllers and is an intentional architectural choice. Changing just these two methods would introduce inconsistency without real benefit.Unless you’re planning a broader refactor away from this pattern, it’s reasonable to keep the current style and adjust linter configuration instead.
Also applies to: 1233-1293
controllers/v1/library/categories.js (1)
181-230: Updated list documentation still references old response shapeThe
@apiexample under/library/categories/liststill shows the older flat response structure (name,type,projectsCount,url). The helper now returns richer category objects (includingdescription,keywords,parentId,hasChildCategories,sequenceNumber, etc.).Not strictly required for functionality, but updating the example to reflect the current shape will make the API docs less confusing for consumers.
module/library/categories/validator/v1.js (1)
14-21: Consider additional validation for keywords array.The current validation checks if keywords is an array and if each item is a string. Consider adding:
- Minimum/maximum array length to prevent abuse
- Non-empty string validation for each keyword
- Duplicate detection within the array
- Optional: trim whitespace from keywords
♻️ Enhanced validation example
req.checkBody('keywords') .optional() .custom((value) => { if (!Array.isArray(value)) { throw new Error('keywords must be an array') } if (value.length > 50) { throw new Error('keywords array cannot exceed 50 items') } if (!value.every(item => typeof item === 'string' && item.trim().length > 0)) { throw new Error('All keywords must be non-empty strings') } const uniqueKeywords = new Set(value.map(k => k.toLowerCase().trim())) if (uniqueKeywords.size !== value.length) { throw new Error('keywords must be unique') } return true })
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
constants/interface-routes/elevate-project/configs.jsoncontrollers/v1/library/categories.jscontrollers/v1/project/templates.jsgenerics/constants/api-responses.jsmodels/project-categories.jsmodule/library/categories/helper.jsmodule/library/categories/validator/v1.jsmodule/project/templates/helper.js
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-08T11:43:01.267Z
Learnt from: Prajwal17Tunerlabs
Repo: ELEVATE-Project/project-service PR: 565
File: constants/interface-routes/elevate-project/configs.json:4050-4104
Timestamp: 2025-09-08T11:43:01.267Z
Learning: For route configurations in constants/interface-routes/elevate-project/configs.json, suggestions about controller/handler implementation mismatches may not be applicable as this file only declares route mappings and the actual implementation details are handled at the application level.
Applied to files:
constants/interface-routes/elevate-project/configs.json
📚 Learning: 2025-09-08T11:40:02.328Z
Learnt from: Prajwal17Tunerlabs
Repo: ELEVATE-Project/project-service PR: 565
File: constants/interface-routes/elevate-project/configs.json:4036-4048
Timestamp: 2025-09-08T11:40:02.328Z
Learning: For route configurations in constants/interface-routes/elevate-project/configs.json, security suggestions like adding rate limiting or requiresCustomHandling may not be applicable or may be handled differently in the system architecture.
Applied to files:
constants/interface-routes/elevate-project/configs.json
📚 Learning: 2025-09-09T03:37:46.179Z
Learnt from: Prajwal17Tunerlabs
Repo: ELEVATE-Project/project-service PR: 565
File: constants/interface-routes/elevate-project/configs.json:4036-4048
Timestamp: 2025-09-09T03:37:46.179Z
Learning: The requiresCustomHandling key is not needed for route configurations in constants/interface-routes/elevate-project/configs.json.
Applied to files:
constants/interface-routes/elevate-project/configs.json
📚 Learning: 2025-08-01T12:05:55.953Z
Learnt from: MallanagoudaB
Repo: ELEVATE-Project/project-service PR: 542
File: module/admin/helper.js:386-540
Timestamp: 2025-08-01T12:05:55.953Z
Learning: The deleteAssociatedResources method in module/admin/helper.js uses the async Promise executor pattern "return new Promise(async (resolve, reject) => {" intentionally for architectural uniformity across the ELEVATE-Project/project-service codebase, consistent with the established pattern used throughout the service.
Applied to files:
controllers/v1/library/categories.js
📚 Learning: 2025-08-11T11:22:52.598Z
Learnt from: Prajwal17Tunerlabs
Repo: ELEVATE-Project/project-service PR: 550
File: api-doc/api-doc.yaml:1683-1685
Timestamp: 2025-08-11T11:22:52.598Z
Learning: In the ELEVATE-Project/project-service codebase, for the api-doc.yaml file, it is acceptable to have query parameters with descriptions mentioning "in the path". This is an intentional design choice and should not be flagged as a mismatch.
Applied to files:
controllers/v1/project/templates.js
🧬 Code graph analysis (2)
module/project/templates/helper.js (2)
module/scp/helper.js (1)
template(133-152)module/library/categories/helper.js (2)
category(113-120)category(146-153)
module/library/categories/helper.js (4)
generics/middleware/authenticator.js (1)
tenantId(665-665)module/project/templates/helper.js (1)
projectCategoriesQueries(25-25)module/userProjects/helper.js (1)
projectCategoriesQueries(13-13)generics/helpers/utils.js (1)
query(113-113)
🪛 Biome (2.1.2)
controllers/v1/library/categories.js
[error] 267-284: Promise executor functions should not be async.
(lint/suspicious/noAsyncPromiseExecutor)
module/library/categories/helper.js
[error] 1123-1222: Promise executor functions should not be async.
(lint/suspicious/noAsyncPromiseExecutor)
[error] 1234-1293: Promise executor functions should not be async.
(lint/suspicious/noAsyncPromiseExecutor)
🔇 Additional comments (7)
generics/constants/api-responses.js (1)
79-84: New delete response constants are consistent and ready to use
PROJECT_CATEGORIES_DELETEDandPROJECT_CATEGORIES_NOT_DELETEDfollow the existing naming/message conventions and are already referenced from the new delete helper logic, so there’s nothing to change here.module/library/categories/helper.js (1)
1011-1077: New list filters are useful; just ensure search and keyword filters compose as intendedThe extended
listimplementation adds:
- Stronger base filters:
status: ACTIVE_STATUS,isDeleted: false.- Optional
parentIdhandling (including'null'for root).- Optional keyword filtering via
keywords: { $in: [...] }.- Case-insensitive search on
name,description, andexternalIdvia$or.The shape of the resulting Mongo query (
ANDbetweenkeywordsand$orsearch clauses) looks correct for “must match keywords and search term”. No code change is strictly required here; mainly:
- Be aware that providing both
keywordsandsearchTextwill narrow results (not union them).- If you want “either matches keywords OR search”, you’d need to wrap them in a higher-level
$or.As implemented, this is a reasonable, predictable default.
Also applies to: 1078-1090
controllers/v1/library/categories.js (1)
266-285:deletecontroller method is wired correctly to the helperThe new
deletecontroller:
- Builds a simple filter
{ _id: req.params._id }.- Delegates to
libraryCategoriesHelper.delete.- Maps
datatoresultin the standard controller pattern and returns structured errors.This aligns with existing
create/update/listpatterns and matches the new DELETE route configuration. No changes needed here.constants/interface-routes/elevate-project/configs.json (1)
895-921: New DELETE route for library categories is consistent with existing routesThe route:
{ "sourceRoute": "/project/v1/library/categories/delete/:id", "type": "DELETE", "priority": "MUST_HAVE", "inSequence": false, "orchestrated": false, "targetPackages": [{ "basePackageName": "project", "packageName": "elevate-project" }], "service": "project" }is consistent with the existing library category routes (
create,update,list) and correctly targets theprojectservice. Assuming the router maps this path toLibraryCategories.delete, no further changes are required.module/library/categories/validator/v1.js (3)
13-13: LGTM! Clean validation rules.The validation for
description,hasChildCategories, andsequenceNumberis well-implemented:
descriptionas optional string is straightforward and correcthasChildCategoriesis properly forbidden in the request body (likely a computed field)sequenceNumbercorrectly validates as a non-negative integer for orderingAlso applies to: 23-30
22-22: Validation is properly separated: format in validator, business logic in helper.The
parent_idvalidator correctly checks only the MongoDB ObjectId format. The helper layer (module/library/categories/helper.js) handles all business logic validation:
- Circular reference detection via
isDescendant()check (line 83)- Parent existence validation via
validateParentId()(called during create and update operations)- Hierarchy depth tracking via
getHierarchyDepth()methodAll edge cases are properly covered.
14-21: The forEach pattern with dynamicreq.checkBody()calls is working correctly and is used elsewhere in the codebase. The validator middleware (generics/middleware/validator/index.js) calls the validator function during middleware execution (beforenext()), so allreq.checkBody()calls register in time for validation. This same pattern is used successfully inmodule/userExtension/validator/v1.jswith even more complex nesting.No changes needed.
Likely an incorrect or invalid review comment.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @controllers/v1/library/categories.js:
- Around line 258-265: Update the JSDoc block above the delete method so the
@name tag correctly reflects the method name (replace `@name list` with `@name
delete`) and adjust the description if needed to match "delete a library
category"; locate the JSDoc that documents the delete function (the comment
starting "delete a library category") and ensure tags like @method, @name,
@param, and @returns accurately describe the `delete` method signature.
In @module/library/categories/helper.js:
- Around line 1017-1022: The code in the list(searchText, params, userDetails)
function incorrectly references req.userDetails.userInformation.organizationId
which is out of scope; replace uses of req with the passed-in userDetails object
(e.g., access userDetails.userInformation.organizationId) so the currentOrgOnly
branch builds query['orgId'] = { $in: ['ALL',
userDetails.userInformation.organizationId] } (ensure
UTILS.convertStringToBoolean(params['currentOrgOnly']) stays as-is and handle
the case where userDetails or userDetails.userInformation may be undefined
before reading organizationId).
In @module/project/templates/helper.js:
- Around line 2065-2088: groupedTemplates is referenced but never declared in
the groupByCategory block causing a runtime ReferenceError; declare and
initialize groupedTemplates before use (e.g., const groupedTemplates = {} or let
groupedTemplates = {} placed immediately inside the if (groupByCategory) block,
above the paginatedResults.forEach loop) so that the subsequent assignments to
groupedTemplates[catId].push(template) and the final paginatedResults =
groupedTemplates work correctly; ensure the chosen declaration (const vs let)
matches any later reassignments to paginatedResults.
- Around line 2066-2086: The grouping logic assumes each entry in
template.categories is an ObjectId, but categories are objects with an _id
field; update the loop in the paginatedResults handling (where
template.categories is iterated and catId computed) to extract the real id:
derive the id value as const idVal = categoryId && categoryId._id ?
categoryId._id : categoryId, then call idVal.toString() for the key and use that
string for membership checks against categoryIdArray and for groupedTemplates
keys (i.e., replace categoryId.toString() with the normalized id string); ensure
you handle null/undefined categoryId safely before calling toString().
🧹 Nitpick comments (3)
module/project/templates/helper.js (1)
2051-2063: Performance: N+1 query pattern in taskDetails loop.When
taskDetailsistrue, this performs a database query for each template inpaginatedResults. Consider batching these queries or using aggregation to reduce database round-trips.module/library/categories/helper.js (2)
105-130: Consider adding a safety limit to prevent infinite loops.Unlike
getHierarchyDepthwhich has adepth < 10safety limit,isDescendantcould loop indefinitely if there's corrupted data with a circular reference already in the database.♻️ Suggested improvement
static async isDescendant(potentialDescendantId, ancestorId, tenantId) { let currentId = potentialDescendantId + let iterations = 0 + const MAX_ITERATIONS = 100 // Safety limit - while (currentId) { + while (currentId && iterations < MAX_ITERATIONS) { + iterations++ if (currentId.toString() === ancestorId.toString()) { return true }
1243-1304:detailsmethod implementation is correct.The method properly:
- Fetches category by ID with tenant scoping
- Optionally retrieves immediate children
- Returns structured response
Note: The
organizationIdvariable at line 1247 is declared but never used - consider removing it.♻️ Remove unused variable
static details(filterQuery, userDetails) { return new Promise(async (resolve, reject) => { try { let tenantId = userDetails.userInformation.tenantId - let organizationId = userDetails.userInformation.organizationId
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
constants/interface-routes/elevate-project/configs.jsoncontrollers/v1/library/categories.jscontrollers/v1/project/templates.jsmodels/project-categories.jsmodule/library/categories/helper.jsmodule/project/templates/helper.js
🚧 Files skipped from review as they are similar to previous changes (2)
- constants/interface-routes/elevate-project/configs.json
- controllers/v1/project/templates.js
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-01T11:59:41.053Z
Learnt from: MallanagoudaB
Repo: ELEVATE-Project/project-service PR: 542
File: databaseQueries/projectTemplates.js:158-172
Timestamp: 2025-08-01T11:59:41.053Z
Learning: In the ELEVATE-Project/project-service codebase, the async Promise executor pattern "return new Promise(async (resolve, reject) => {" is intentionally used consistently across all database query methods in the databaseQueries directory for architectural uniformity, despite static analysis tools flagging it as an anti-pattern.
Applied to files:
controllers/v1/library/categories.js
📚 Learning: 2025-09-08T11:55:24.922Z
Learnt from: Prajwal17Tunerlabs
Repo: ELEVATE-Project/project-service PR: 565
File: databaseQueries/organizationExtension.js:70-84
Timestamp: 2025-09-08T11:55:24.922Z
Learning: Database query helper files in this project use Promise wrapper patterns (with new Promise and async executors) to maintain consistency across all helper functions, even when async/await could be used directly.
Applied to files:
controllers/v1/library/categories.js
📚 Learning: 2025-08-06T06:10:37.743Z
Learnt from: MallanagoudaB
Repo: ELEVATE-Project/project-service PR: 542
File: module/admin/helper.js:337-337
Timestamp: 2025-08-06T06:10:37.743Z
Learning: The ELEVATE-Project/project-service codebase has inconsistent MongoDB $in operator usage patterns. While some files correctly use arrays (e.g., `{ $in: [value] }`), others incorrectly pass variables directly (e.g., `{ $in: variable }`). This inconsistency exists particularly between main application files (mostly correct) and migration files (mostly incorrect), and should be standardized to prevent runtime errors.
Applied to files:
module/project/templates/helper.jsmodels/project-categories.jsmodule/library/categories/helper.js
🧬 Code graph analysis (2)
controllers/v1/library/categories.js (3)
module/project/templates/helper.js (1)
libraryCategoriesHelper(17-17)module/userProjects/helper.js (1)
libraryCategoriesHelper(9-9)generics/helpers/utils.js (1)
filterQuery(965-965)
module/project/templates/helper.js (1)
module/wishlist/helper.js (2)
startIndex(251-251)endIndex(252-252)
🪛 Biome (2.1.2)
controllers/v1/library/categories.js
[error] 267-284: Promise executor functions should not be async.
(lint/suspicious/noAsyncPromiseExecutor)
module/library/categories/helper.js
[error] 1133-1232: Promise executor functions should not be async.
(lint/suspicious/noAsyncPromiseExecutor)
[error] 1244-1303: Promise executor functions should not be async.
(lint/suspicious/noAsyncPromiseExecutor)
🔇 Additional comments (8)
models/project-categories.js (2)
69-98: LGTM! Schema fields support hierarchical categories well.The new fields (
parentId,hasChildCategories,sequenceNumber,keywords,description,metaInformation) provide solid support for hierarchical categorization. The indexes on these fields will enable efficient hierarchy queries.Minor note:
descriptionhas bothrequired: trueanddefault: 'default'- the default ensures the required constraint is always satisfied, which works but means documents will always have this field populated automatically.
100-111: Compound indexes are well-designed for hierarchy queries.The indexes on
{ parentId: 1, tenantId: 1, sequenceNumber: 1 }and{ tenantId: 1, hasChildCategories: 1 }will efficiently support:
- Fetching children of a parent in sequence order
- Finding leaf/parent categories by tenant
module/library/categories/helper.js (3)
37-94: Well-implemented parent validation with circular reference detection.The
validateParentIdmethod properly:
- Validates ObjectId format
- Checks parent exists in same tenant
- Prevents self-parenting
- Detects circular references via
isDescendant
1133-1232: Async promise executor pattern is intentional.Static analysis flagged this as an anti-pattern, but based on learnings, this codebase intentionally uses this pattern across database query methods for architectural uniformity.
The
deletemethod implementation is solid:
- Validates category exists
- Checks for child categories before deletion
- Checks for associated projects
- Performs soft delete
- Updates parent's
hasChildCategoriesflag
910-951: Sequence number auto-assignment logic is correct.The implementation properly finds the maximum sequence number among siblings (under the same parent or at root level) and increments by 1. This ensures unique ordering within each hierarchy level.
controllers/v1/library/categories.js (3)
240-256: Updatedlistmethod correctly integrates with new helper signature.The method now passes
req.searchText,req.query, andreq.userDetailsto the helper, and properly mapsdatatoresultfor API response consistency.
266-285:deletecontroller method implementation is correct.The async promise executor pattern is intentional in this codebase per learnings. The implementation properly:
- Constructs filter query from request params
- Delegates to helper
- Maps response for API consistency
- Handles errors appropriately
295-315:detailsmethod correctly handlesgetChildrenquery parameter.The boolean conversion
req.query.getChildren === 'true'is appropriate for query string parameters. The implementation follows the established controller patterns.
aks30
left a comment
aks30
left a comment
There was a problem hiding this comment.
Reviewed and added comments for all except - module/library/categories/helper.js, will do that later.
| name: { externalId: 1, tenantId: 1 }, | ||
| indexType: { unique: true }, | ||
| }, | ||
| { |
| type: String, | ||
| index: true, | ||
| }, | ||
| name: String, |
There was a problem hiding this comment.
if category name is updated, the name in project-templates become stale and then we would need to write sync job to update the names in project templates and projects. As we are alredy haaving id and external Id of the category, we can safely remove name.
| _id: req.params._id, | ||
| getChildren: req.query.getChildren === 'true', | ||
| } | ||
| let projectCategories = await libraryCategoriesHelper.details(filterQuery, req.userDetails) |
| return new Promise(async (resolve, reject) => { | ||
| try { | ||
| let projectCategories = await libraryCategoriesHelper.list(req) | ||
| let projectCategories = await libraryCategoriesHelper.list(req.searchText, req.query, req.userDetails) |
There was a problem hiding this comment.
@vaivk369 why do we need user detail to get category listing?
There was a problem hiding this comment.
we should get list of categories for the tenant user is belonging to right?
user categories model has tenant.
Also we should fetch categories which are having visibleToOrganizations set to orgId of the user
2 participants
Description
These recommendations are intended to promote code quality and team communication during software development. They cover a variety of topics, including ensuring that pull requests are submitted to the correct branch, documenting new methods, preserving consistency across many services, and avoiding typical blunders like accessing APIs or DB queries within loops. Sensitive data should not be uploaded, and changes to environment variables or database models should be executed consistently. Teams may work more effectively and develop higher-quality software by adhering to these standards.
Type of change
Please choose appropriate options.
Checklist
Summary by CodeRabbit
New Features
Other
✏️ Tip: You can customize this high-level summary in your review settings.