moving-changes-to-main#754
Open
MallanagoudaB wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughThe PR adds an ChangesPrivate Program Deletion Flag Propagation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
module/admin/helper.js (2)
267-276:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
projecFilterhardcodesisAPrivateProgram: false— will orphan projects of private programs/solutions when they are deleted.The
isAPrivateProgramparameter is available in thedeletedResourceDetails()function and is conditionally used when deleting solutions and programs themselves (lines 188–195), but the project deletion filters at lines 267–276 and 359–366 hardcode it tofalse. When a private program or solution is deleted, private projects linked to it will not be matched by the filter and will be left orphaned with dangling parent references.The parameter should be passed through to the project deletion filter:
Fix for both locations
let projecFilter = { solutionId: { $in: solutionIds }, - isAPrivateProgram: false, + isAPrivateProgram: isAPrivateProgram, tenantId: tenantId, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@module/admin/helper.js` around lines 267 - 276, The project deletion filter currently hardcodes the isAPrivateProgram flag (projecFilter) which will skip private projects and leave them orphaned; update the filter used before calling projectQueries.delete so it uses the isAPrivateProgram parameter passed into deletedResourceDetails() (or the surrounding function) instead of always false; locate the projecFilter construction(s) and replace the hardcoded isAPrivateProgram: false with the variable/parameter (e.g., isAPrivateProgram) so both project deletion sites mirror the same privacy-aware behavior as deletedResourceDetails(), ensuring private projects tied to deleted solutions/programs are matched and removed.
437-446:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix JSDoc type and parameter descriptions; clarify privacy-scoping intent for templates/tasks.
Three issues to address:
JSDoc corrections needed:
isAPrivateProgramtype should be{Boolean}, not{String}.deletedBydescription should read "User ID (or 'SYSTEM') that triggered the deletion; forwarded to the survey service" (not "Auth token").- Add
@returns {Promise<Object>}documenting the aggregated deletion counts and success flag.Local DB filters (
projectTemplateFilter,taskFilter) are scoped only bytenantId—they do not incorporate any privacy distinction. Note thatisAPrivateProgramis forwarded tosurveyService.deleteSolutionResource()but not used in your template/task deletion logic.Schema context:
projectTemplatesdocuments carry anisPrivatefield (notisAPrivateProgram), andprojectTemplateTasksdocuments do not include either field. Verify whether:
- Templates and tasks should inherit privacy scoping from their parent program/solution (currently only tenant-scoped), or
- The
isAPrivateProgramparameter should be used in these filters if privacy separation is required at this layer (e.g., by cross-referencing with the program or solution record).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@module/admin/helper.js` around lines 437 - 446, Update the JSDoc on deleteAssociatedResources: change isAPrivateProgram type to {Boolean}, change deletedBy description to "User ID (or 'SYSTEM') that triggered the deletion; forwarded to the survey service", and add `@returns` {Promise<Object>} describing aggregated deletion counts and success flag. In the function body, ensure projectTemplateFilter and taskFilter are adjusted to account for privacy scoping: either add an isPrivate filter that matches isAPrivateProgram (or query the parent program/solution to derive isPrivate) so templates/tasks are filtered by privacy as well as tenantId, or explicitly document and enforce that privacy is inherited from the program/solution before calling surveyService.deleteSolutionResource; confirm use of projectTemplates.isPrivate and that projectTemplateTasks currently have no privacy field and therefore must be scoped via their parent template/solution lookup if required.
🧹 Nitpick comments (3)
module/admin/helper.js (2)
186-199: 💤 Low valueCollapse the duplicated
programFilterbranches.Both branches are identical except for the boolean value, so the
if/elseis redundant.♻️ Proposed simplification
- let programFilter - if (isAPrivateProgram) { - programFilter = { - _id: resourceId, - tenantId: tenantId, - isAPrivateProgram: true, - } - } else { - programFilter = { - _id: resourceId, - tenantId: tenantId, - isAPrivateProgram: false, - } - } + const programFilter = { + _id: resourceId, + tenantId: tenantId, + isAPrivateProgram: Boolean(isAPrivateProgram), + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@module/admin/helper.js` around lines 186 - 199, The if/else that builds programFilter is duplicated; simplify by creating a single object and set the boolean from isAPrivateProgram directly. Replace the conditional block that assigns programFilter with one assignment using keys _id: resourceId, tenantId: tenantId and isAPrivateProgram: isAPrivateProgram (or shorthand) so the logic uses the existing resourceId/tenantId variables and removes the redundant branches.
144-164: 💤 Low valueRealign the JSDoc with the new signature.
A few drift issues introduced/preserved while threading
isAPrivateProgramthrough:
@param {Object} isAPrivateProgramshould be{Boolean}.userTokenis still documented but it isn't part of the signature.isAPrivateProgram = falseis given a default while the params after it (tenantId,orgId) are required. Putting an optional in the middle is a footgun; consider moving it to the tail or making all callers pass it explicitly.📝 Proposed JSDoc fix
- * `@param` {Object} isAPrivateProgram - If Program is Private `true` else `false`. + * `@param` {Boolean} isAPrivateProgram - `true` if the program is private, otherwise `false`. * `@param` {String} tenantId - Tenant identifier for multitenancy. * `@param` {String} orgId - Organization ID performing the operation. * `@param` {String} [deletedBy='SYSTEM'] - User ID or system name that triggered the deletion. - * `@param` {String} userToken - Auth token used for downstream service calls (e.g., survey service). * * `@returns` {Promise<Object>} - Result object summarizing deletion impact. */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@module/admin/helper.js` around lines 144 - 164, The JSDoc for deletedResourceDetails is out of sync: change `@param` {Object} isAPrivateProgram to `@param` {Boolean} isAPrivateProgram, remove the undocumented userToken param from the docblock, and fix the optional/default placement (either remove the default and require callers to pass isAPrivateProgram explicitly or move isAPrivateProgram to the end of the parameter list) so required params tenantId and orgId are not after an optional; update the doc examples and description to match the final signature of the deletedResourceDetails method.generics/services/survey.js (1)
328-339: 💤 Low valueJSDoc is out of sync with the updated signature.
While editing this block to add
isAPrivateProgram, please also realign the rest:
@param {String} isAPrivateProgramshould be{Boolean}(callers passtrue/false).- The block still documents
tokenandsolutionId, but the actual signature is(solutionIds, resourceType, tenantId, orgId, userId, isAPrivateProgram)—tokendoesn't exist andsolutionIdsis plural.userIdis not documented at all.📝 Proposed JSDoc fix
/** * Calls the Survey Service to delete a survey or observation resource linked to a solution. * - * `@param` {String} token - The user authentication token. - * `@param` {String} solutionId - The ID of the solution (survey/observation) to be deleted. + * `@param` {Array<String>} solutionIds - IDs of the solutions (survey/observation) to be deleted. * `@param` {String} resourceType - Type of the resource being deleted ('solution', 'program', etc.). * `@param` {String} tenantId - Tenant identifier (used for multi-tenancy). * `@param` {String} orgId - Organization ID from where the deletion is triggered. - * `@param` {String} isAPrivateProgram - If Program is Private `true` else `false`. + * `@param` {String} userId - User ID that triggered the deletion (used as `deletedBy`). + * `@param` {Boolean} isAPrivateProgram - `true` if the program is private, otherwise `false`. * `@returns` {Promise<Object>} - Result indicating success/failure and optional response data. */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@generics/services/survey.js` around lines 328 - 339, Update the JSDoc for deleteSolutionResource to match the function signature: remove the obsolete `token` and `solutionId` entries, document `solutionIds` (Array or String depending on callers) instead of `solutionId`, add a `@param {String} userId` entry, and change `@param {isAPrivateProgram}` type to `Boolean`; also update the function description to reference deleting multiple solution(s) when `solutionIds` is used and keep the other params (resourceType, tenantId, orgId) accurate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@module/admin/helper.js`:
- Around line 267-276: The project deletion filter currently hardcodes the
isAPrivateProgram flag (projecFilter) which will skip private projects and leave
them orphaned; update the filter used before calling projectQueries.delete so it
uses the isAPrivateProgram parameter passed into deletedResourceDetails() (or
the surrounding function) instead of always false; locate the projecFilter
construction(s) and replace the hardcoded isAPrivateProgram: false with the
variable/parameter (e.g., isAPrivateProgram) so both project deletion sites
mirror the same privacy-aware behavior as deletedResourceDetails(), ensuring
private projects tied to deleted solutions/programs are matched and removed.
- Around line 437-446: Update the JSDoc on deleteAssociatedResources: change
isAPrivateProgram type to {Boolean}, change deletedBy description to "User ID
(or 'SYSTEM') that triggered the deletion; forwarded to the survey service", and
add `@returns` {Promise<Object>} describing aggregated deletion counts and success
flag. In the function body, ensure projectTemplateFilter and taskFilter are
adjusted to account for privacy scoping: either add an isPrivate filter that
matches isAPrivateProgram (or query the parent program/solution to derive
isPrivate) so templates/tasks are filtered by privacy as well as tenantId, or
explicitly document and enforce that privacy is inherited from the
program/solution before calling surveyService.deleteSolutionResource; confirm
use of projectTemplates.isPrivate and that projectTemplateTasks currently have
no privacy field and therefore must be scoped via their parent template/solution
lookup if required.
---
Nitpick comments:
In `@generics/services/survey.js`:
- Around line 328-339: Update the JSDoc for deleteSolutionResource to match the
function signature: remove the obsolete `token` and `solutionId` entries,
document `solutionIds` (Array or String depending on callers) instead of
`solutionId`, add a `@param {String} userId` entry, and change `@param
{isAPrivateProgram}` type to `Boolean`; also update the function description to
reference deleting multiple solution(s) when `solutionIds` is used and keep the
other params (resourceType, tenantId, orgId) accurate.
In `@module/admin/helper.js`:
- Around line 186-199: The if/else that builds programFilter is duplicated;
simplify by creating a single object and set the boolean from isAPrivateProgram
directly. Replace the conditional block that assigns programFilter with one
assignment using keys _id: resourceId, tenantId: tenantId and isAPrivateProgram:
isAPrivateProgram (or shorthand) so the logic uses the existing
resourceId/tenantId variables and removes the redundant branches.
- Around line 144-164: The JSDoc for deletedResourceDetails is out of sync:
change `@param` {Object} isAPrivateProgram to `@param` {Boolean} isAPrivateProgram,
remove the undocumented userToken param from the docblock, and fix the
optional/default placement (either remove the default and require callers to
pass isAPrivateProgram explicitly or move isAPrivateProgram to the end of the
parameter list) so required params tenantId and orgId are not after an optional;
update the doc examples and description to match the final signature of the
deletedResourceDetails method.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cdd74fea-5412-4827-a9bd-e60b108d0831
📒 Files selected for processing (2)
generics/services/survey.jsmodule/admin/helper.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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