boundaries on which we split to be fetched from mdms #982
Conversation
|
Warning Review failedThe pull request is closed. WalkthroughWalkthroughThis update enhances the configuration capabilities in the project-factory utility by adding a new property to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
utilities/project-factory/src/server/utils/campaignUtils.ts (3)
Line range hint
1791-1807: Introduce a new async function to fetch boundary split configuration.The new function
getBoundaryOnWhichWeSplitis introduced to fetch configuration for splitting boundaries based onmasterNameForSplitBoundariesOnfrom MDMS data. This is a crucial addition to dynamically determine the boundary split logic based on external configuration.Ensure proper error handling to manage cases where the MDMS response might not contain the expected data or the data might not be active. This will prevent runtime errors that could crash the server.
+ if (!mdmsResponse || mdmsResponse.length === 0) { + throw new Error('No valid configuration found for splitting boundaries.'); + } + return mdmsResponse.filter((item: any) => item.isActive).map((item: any) => item.splitBoundariesOn);
Line range hint
502-502: Potential TypeError due to unsafe optional chaining.The use of optional chaining in this context could lead to a TypeError if it short-circuits to
undefined. Consider adding checks or default values to ensure stability.- const value = someObject?.someProperty?.value; + const value = someObject?.someProperty?.value ?? defaultValue; // Add a default value or handle the potential undefined case
Line range hint
759-761: Redundant else clause after early exit.The else block is unnecessary here because the preceding if condition ends with a return statement. Simplifying this by removing the else clause can enhance readability and maintainability.
- if (condition) { - return value; - } else { - // some code - } + if (condition) { + return value; + } + // some code
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- utilities/project-factory/src/server/config/index.ts (1 hunks)
- utilities/project-factory/src/server/utils/campaignUtils.ts (2 hunks)
Files skipped from review due to trivial changes (1)
- utilities/project-factory/src/server/config/index.ts
Additional context used
Biome
utilities/project-factory/src/server/utils/campaignUtils.ts
[error] 415-415: This var should be declared at the root of the enclosing function.
The var is accessible in the whole body of the enclosing function.
To avoid confusion, it should be declared at the root of the enclosing function.(lint/correctness/noInnerDeclarations)
[error] 451-451: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
[error] 502-502: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
[error] 545-545: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 574-574: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 576-576: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 614-614: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 616-616: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 617-617: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 632-635: This var should be declared at the root of the enclosing function.
The var is accessible in the whole body of the enclosing function.
To avoid confusion, it should be declared at the root of the enclosing function.(lint/correctness/noInnerDeclarations)
[error] 649-649: This var should be declared at the root of the enclosing function.
The var is accessible in the whole body of the enclosing function.
To avoid confusion, it should be declared at the root of the enclosing function.(lint/correctness/noInnerDeclarations)
[error] 759-761: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 874-874: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 881-881: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 882-882: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 883-883: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 884-884: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 1078-1078: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
[error] 1100-1103: This var should be declared at the root of the enclosing function.
The var is accessible in the whole body of the enclosing function.
To avoid confusion, it should be declared at the root of the enclosing function.(lint/correctness/noInnerDeclarations)
[error] 1104-1104: This var should be declared at the root of the enclosing function.
The var is accessible in the whole body of the enclosing function.
To avoid confusion, it should be declared at the root of the enclosing function.(lint/correctness/noInnerDeclarations)
Additional comments not posted (2)
utilities/project-factory/src/server/utils/campaignUtils.ts (2)
Line range hint
545-545: Optimize by avoiding the delete operator.Using the delete operator can lead to performance issues due to deoptimization of the underlying JavaScript engine's optimizations. Consider setting properties to
undefinedor restructuring how data is managed to avoid the need to delete properties.- delete object.property; + object.property = undefined; // This avoids the performance hit from using deleteAlso applies to: 574-574, 576-576, 614-614, 616-616, 617-617, 874-874, 881-881, 882-882, 883-883, 884-884
1792-1792: Check for potential type errors due to optional chaining.The code uses optional chaining in a context where it might result in
undefinedbeing used in a way that could cause runtime errors.
| @@ -1799,6 +1800,11 @@ async function getDifferentTabGeneratedBasedOnConfig(request: any, boundaryDataG | |||
| return boundaryDataGeneratedAfterDifferentTabSeparation; | |||
| } | |||
|
|
|||
| async function getBoundaryOnWhichWeSplit(request: any) { | |||
| const mdmsResponse = await getMDMSV1Data(request, config?.values?.moduleName, config?.masterNameForSplitBoundariesOn, request?.query?.tenantId); | |||
| return mdmsResponse.filter((item: any) => item.isActive).map((item: any) => item.splitBoundariesOn); | |||
There was a problem hiding this comment.
i think isactive flag should be implemented in UI
and in API it should filter based on
campaigntype
So if request is for ADMIN, u should take admin's splitBoundariesOn
There was a problem hiding this comment.
ok then will chnage the logic to fetch the hierarchy type from campaign object
No description provided.