Task #217774 Resolve conflicts of AclHelper#1061
Task #217774 Resolve conflicts of AclHelper#1061YoginiTayade wants to merge 2 commits intotekdi:feat-aclfrom
Conversation
WalkthroughThe project updates encompass a comprehensive overhaul aimed at enhancing system functionality. Changes include version upgrades for dependencies, introduction of new methods and endpoints, and logic refinements to boost performance and maintainability. These updates span across middleware, service, and controller layers, reflecting a concerted effort to improve the system's capabilities. Changes
Recent Review DetailsConfiguration used: .coderabbit.yaml Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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: 40
Out of diff range and nitpick comments (15)
src/src/common/middlewares/cohort.middleware.ts (1)
Line range hint
32-54: Refactor to improve error handling and reduce redundancy.- if (req?.headers?.['x-program-id']) { - req.mw_program_id = req.headers['x-program-id']; - const program_id = parseInt(req.mw_program_id, 10); - if (isNaN(program_id)) { - throw new BadRequestException('Invalid Program Id'); - } - const hasAccess = await this.method.isUserHasAccessForProgram(req); - if (!hasAccess) { - return res.json({ - success: false, - message: 'User does not have access to this Program Id', - }); - } - } else { - return res.status(403).send({ - success: false, - message: 'Program Id is required', - }); - } + try { + const program_id = parseInt(req.headers['x-program-id'], 10); + if (isNaN(program_id)) throw new BadRequestException('Invalid Program Id'); + const hasAccess = await this.method.isUserHasAccessForProgram(req); + if (!hasAccess) throw new BadRequestException('User does not have access to this Program Id'); + } catch (error) { + return res.status(403).json({ success: false, message: error.message }); + }This refactoring simplifies the logic by using a try-catch block, improving readability and error handling.
src/src/helper/queryGenerator.service.ts (1)
Line range hint
351-423: Optimize thequerymethod by modularizing the construction of filter conditions.This method is quite complex due to the dynamic construction of the query string. Consider extracting the logic for building filter conditions into a separate function. This will not only improve readability but also make the method easier to test and maintain.
src/src/cron/prepareCertificateHtml.cron.ts (1)
10-10: Remove unused importjsonfrom 'stream/consumers'.The
jsonimport does not appear to be used in this file. Removing unused imports can help keep the code clean and maintainable.src/src/lms/lms.service.ts (1)
Line range hint
572-615: Consider using template literals for dynamic string construction to enhance readability and prevent potential errors in query construction.- let filterQuery = [`context: {_eq: ${context}}`]; + let filterQuery = [`context: {_eq: "${context}"}`];This change ensures that the context value is correctly interpreted as a string within the query.
src/src/camp/camp.core.service.ts (1)
Line range hint
81-159: Ensure consistent use of template literals for string construction in GraphQL queries to enhance readability and prevent errors.- let filterQueryArray = [`{group_users: {member_type: {_eq: "owner"}, group: {program_id: {_eq:${program_id}}, academic_year_id: {_eq:${academic_year_id}}},user:{program_faciltators:{parent_ip:{_eq:"${parent_ip_id}"}}}}}`]; + let filterQueryArray = [`{group_users: {member_type: {_eq: "owner"}, group: {program_id: {_eq: "${program_id}"}, academic_year_id: {_eq: "${academic_year_id}"}}, user: {program_faciltators: {parent_ip: {_eq: "${parent_ip_id}"}}}}}`];This change ensures that all dynamic values are correctly encapsulated as strings within the query.
src/src/modules/auth/auth.service.ts (1)
Line range hint
800-821: Optimize the data structure and query handling.The method
newCreateconstructs a large query with many fields. Consider breaking down the query into smaller, more manageable parts or functions to improve readability and maintainability.src/src/user/user.service.ts (1)
1789-1789: Consider adding a comment to describe the purpose of therole_slugfield.Adding a brief comment explaining the purpose of
role_slugcan improve code readability and maintainability.src/src/userauth/userauth.service.ts (1)
153-155: Avoid commented-out code in production as it can lead to confusion and clutter.Consider removing or replacing the commented-out console logs with proper logging mechanisms if needed.
src/src/observations/observations.service.ts (2)
473-560: Improve logging for debugging and monitoring.Consider adding logging statements in the
updateFieldsmethod to log key actions and decisions. This will help in debugging issues and monitoring the flow of data through the method, especially in production environments.
650-734: Use appropriate HTTP status codes.In the
updateFieldResponsesmethod, consider using more appropriate HTTP status codes for different types of responses. For example, use 400 for bad requests, 404 for not found, and 500 for internal server errors. This helps clients handle responses more effectively.src/src/camp/camp.service.ts (5)
39-39: Consider adding a comment describing the purpose ofreturnFieldscampsfor better code readability.
52-52: Consider adding a comment describing the purpose ofreturnFieldsGroupsfor better code readability.
197-197: Thetypefield is hardcoded as 'pcr'. If this is intended to be dynamic or configurable, consider passing it as a parameter or fetching it from a configuration file.
Line range hint
387-431: The GraphQL query insidecampListmethod is complex and handles multiple conditions. Consider breaking it down into smaller, more manageable functions or queries to improve readability and maintainability.
Line range hint
2064-2126: The pagination logic ingetConsentBenficiariesseems to be manually implemented. Consider using existing libraries or frameworks that provide pagination features to reduce boilerplate code and improve reliability.
| const { mw_roles } = header; | ||
| const authToken = header.header('authorization'); | ||
| const decoded: any = jwt_decode(authToken); | ||
| let keycloak_id = decoded.sub; | ||
| console.log('keycloak_id', keycloak_id); | ||
|
|
There was a problem hiding this comment.
Consider using a more secure method to extract the JWT token.
The current method directly accesses the header which can be prone to security risks. It's recommended to use a dedicated method or middleware to safely extract and verify the JWT token.
| !body.role_fields.academic_year_id || | ||
| !body.role_fields.role_slug |
There was a problem hiding this comment.
Validate the presence of required fields before proceeding.
The code checks for the presence of several fields but does not handle the case where these fields might be missing or null, which could lead to runtime errors. Consider adding null checks or default values.
| if (body.role_fields.role_slug) { | ||
| body.role_slug = body.role_fields.role_slug; | ||
| } |
There was a problem hiding this comment.
Use consistent field handling for role-specific data.
The handling of role_fields is inconsistent. For example, role_slug is directly assigned without checking its existence, unlike other fields. Refactor to ensure consistent handling:
- body.role_slug = body.role_fields.role_slug;
+ if (body.role_fields.role_slug) {
+ body.role_slug = body.role_fields.role_slug;
+ }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.
| if (body.role_fields.role_slug) { | |
| body.role_slug = body.role_fields.role_slug; | |
| } | |
| if (body.role_fields.role_slug) { | |
| body.role_slug = body.role_fields.role_slug; | |
| } |
|
|
||
| if (req.mw_roles?.includes('program_owner')) { | ||
| req.parent_ip_id = req.mw_ip_user_id; | ||
| } else { | ||
| const user = await this.userService.ipUserInfo(req); | ||
| if (req.mw_roles?.includes('staff')) { | ||
| req.parent_ip_id = | ||
| user?.data?.program_users?.[0]?.organisation_id; | ||
| } else if (req.mw_roles?.includes('facilitator')) { | ||
| req.parent_ip_id = user?.data?.program_faciltators?.parent_ip; | ||
| } | ||
| } | ||
| if (!req.parent_ip_id) { | ||
| return resp.status(404).send({ | ||
| success: false, | ||
| message: 'Invalid User', | ||
| message: 'Invalid Ip', |
There was a problem hiding this comment.
Ensure proper role handling and error messaging for unauthorized access.
- return resp.status(404).send({
+ return resp.status(403).send({
success: false,
message: 'Unauthorized access',
data: {},
});This change ensures that the correct HTTP status code (403 Forbidden) is used for unauthorized access instead of 404 Not Found, which can be misleading as it typically indicates that a resource does not exist. The error message has also been updated for clarity.
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.
| if (req.mw_roles?.includes('program_owner')) { | |
| req.parent_ip_id = req.mw_ip_user_id; | |
| } else { | |
| const user = await this.userService.ipUserInfo(req); | |
| if (req.mw_roles?.includes('staff')) { | |
| req.parent_ip_id = | |
| user?.data?.program_users?.[0]?.organisation_id; | |
| } else if (req.mw_roles?.includes('facilitator')) { | |
| req.parent_ip_id = user?.data?.program_faciltators?.parent_ip; | |
| } | |
| } | |
| if (!req.parent_ip_id) { | |
| return resp.status(404).send({ | |
| success: false, | |
| message: 'Invalid User', | |
| message: 'Invalid Ip', | |
| if (req.mw_roles?.includes('program_owner')) { | |
| req.parent_ip_id = req.mw_ip_user_id; | |
| } else { | |
| const user = await this.userService.ipUserInfo(req); | |
| if (req.mw_roles?.includes('staff')) { | |
| req.parent_ip_id = | |
| user?.data?.program_users?.[0]?.organisation_id; | |
| } else if (req.mw_roles?.includes('facilitator')) { | |
| req.parent_ip_id = user?.data?.program_faciltators?.parent_ip; | |
| } | |
| } | |
| if (!req.parent_ip_id) { | |
| return resp.status(403).send({ | |
| success: false, | |
| message: 'Unauthorized access', | |
| data: {}, | |
| }); |
| if (req.mw_roles?.includes('program_owner')) { | ||
| req.parent_ip_id = req.mw_ip_user_id; | ||
| } else { | ||
| const user = await this.userService.ipUserInfo(req); | ||
| if (req.mw_roles?.includes('staff')) { | ||
| req.parent_ip_id = | ||
| user?.data?.program_users?.[0]?.organisation_id; | ||
| } else if (req.mw_roles?.includes('facilitator')) { | ||
| req.parent_ip_id = user?.data?.program_faciltators?.parent_ip; | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider refactoring the role-based assignment of parent_ip_id to a separate method to improve readability and maintainability.
- if (req.mw_roles?.includes('program_owner')) {
- req.parent_ip_id = req.mw_ip_user_id;
- } else {
- const user = await this.userService.ipUserInfo(req);
- if (req.mw_roles?.includes('staff')) {
- req.parent_ip_id = user?.data?.program_users?.[0]?.organisation_id;
- } else if (req.mw_roles?.includes('facilitator')) {
- req.parent_ip_id = user?.data?.program_faciltators?.parent_ip;
- }
- }
+ req.parent_ip_id = this.determineParentIpId(req);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.
| if (req.mw_roles?.includes('program_owner')) { | |
| req.parent_ip_id = req.mw_ip_user_id; | |
| } else { | |
| const user = await this.userService.ipUserInfo(req); | |
| if (req.mw_roles?.includes('staff')) { | |
| req.parent_ip_id = | |
| user?.data?.program_users?.[0]?.organisation_id; | |
| } else if (req.mw_roles?.includes('facilitator')) { | |
| req.parent_ip_id = user?.data?.program_faciltators?.parent_ip; | |
| } | |
| } | |
| req.parent_ip_id = this.determineParentIpId(req); |
| @Post('admin/multiple_end_pcr') | ||
| @UseGuards(new AuthGuard()) | ||
| multiplePcrCampEnd( | ||
| @Req() request: any, | ||
| @Body() body: any, | ||
| @Res() response: any, | ||
| ) { | ||
| return this.campService.multiplePcrCampEnd(body, request, response); |
There was a problem hiding this comment.
Ensure consistent use of AuthGuard instantiation.
The method multiplePcrCampEnd incorrectly uses new AuthGuard(). Update this to @UseGuards(AuthGuard) to align with NestJS best practices and ensure consistent guard handling.
| public async updateWithVariable( | ||
| id: number, | ||
| tableName: String, | ||
| item: Object, | ||
| onlyFields: any = [], | ||
| fields: any = [], | ||
| props: any = {}, | ||
| ) { | ||
| return this.getResponce( | ||
| await lastValueFrom( | ||
| this.httpService | ||
| .post( | ||
| this.url, | ||
|
|
||
| this.qgService.updateWithVariable( | ||
| id, | ||
| tableName, | ||
| item, | ||
| onlyFields, | ||
| fields, | ||
| props, | ||
| ), | ||
|
|
||
| { | ||
| headers: { | ||
| 'x-hasura-admin-secret': | ||
| process.env.HASURA_ADMIN_SECRET, | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| }, | ||
| ) | ||
| .pipe(map((res) => res.data)), | ||
| ), | ||
| tableName, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Ensure proper error handling in asynchronous operations.
The method updateWithVariable lacks proper error handling. Currently, it logs the error but does not handle it in a way that could be useful for the caller. Consider throwing an exception or returning a specific error response to allow the caller to handle the error appropriately.
| // if (roles.includes('program_owner')) { | ||
| // if (req?.headers && req?.headers?.['x-ip-org-id']) { | ||
| // userId = req.headers['x-ip-org-id']; | ||
| // const result = await this.userService.getIPuser( | ||
| // req, | ||
| // res, | ||
| // ); | ||
| // const data = result?.data?.program_users?.[0]; | ||
| // req.mw_userid = data?.user_id; | ||
| // } else if (['/users/ip_user_info'].includes(req.baseUrl)) { | ||
| // // const user = await this.userService.ipUserInfo(req); | ||
| // userId = await this.userService.getUserIdFromKeycloakId( | ||
| // keycloak_id, | ||
| // ); | ||
| // req.mw_userid = userId; | ||
| // } | ||
| // req.mw_roles = roles; //pass role if x-ip-org-id is not send | ||
| // } else { | ||
| // const user = await this.userService.ipUserInfo(req); | ||
| userId = await this.userService.getUserIdFromKeycloakId( | ||
| keycloak_id, | ||
| ); | ||
| req.mw_userid = userId; | ||
| // } |
There was a problem hiding this comment.
Commented out role-based user ID handling logic.
The removal of this block of code might affect functionalities that depend on role-based access control. Verify that the new implementation covers all required functionalities and that no security loopholes have been introduced.
| updateWithVariable( | ||
| id: number, | ||
| tName: String, | ||
| item: any, | ||
| onlyFields: any = [], | ||
| fields: any = [], | ||
| props: any = {}, | ||
| ) { | ||
| let tableName = `update_${tName}`; | ||
| const keys = Object.keys(item); | ||
| let resultObject = {}; | ||
| let params = ''; | ||
| const { variable } = props; | ||
|
|
||
| if (Array.isArray(variable) && variable?.length > 0) { | ||
| params = `(${variable | ||
| .filter((key) => item[key.key]) | ||
| .map((newD) => `$${newD.key}: ${newD?.type}`) | ||
| .join(', ')})`; | ||
|
|
||
| let vData = {}; | ||
| variable.forEach((e) => { | ||
| if (item?.[e.key]) { | ||
| vData = { ...vData, [e.key]: item?.[e.key] }; | ||
| } | ||
| }); | ||
| resultObject = { ...resultObject, variables: vData }; | ||
| } | ||
| const getObjStr = (item: any, type: String = '') => { | ||
| let str = `where: ${ | ||
| props?.where ? props?.where : `{id: {_eq: ${id}}}` | ||
| }, _set: {`; | ||
| let strArr = []; | ||
| keys.forEach((e, index) => { | ||
| if ( | ||
| e !== 'id' && | ||
| (onlyFields.length < 1 || onlyFields.includes(e)) | ||
| ) { | ||
| const data = variable.map((e) => e.key).filter((e) => e); | ||
| if (data.includes(e)) { | ||
| strArr = [...strArr, `${e}:$${e}`]; | ||
| } else { | ||
| strArr = [...strArr, `${e}:"${item[e]}"`]; | ||
| } | ||
| } | ||
| }); | ||
| str += strArr.join(); | ||
| str += `}`; | ||
| return str; | ||
| }; | ||
|
|
||
| resultObject = { | ||
| query: `mutation MyQuery${params} { | ||
| ${tableName}(${getObjStr(item)}) { | ||
| affected_rows | ||
| returning { | ||
| ${this.getParam( | ||
| fields && fields.length > 0 | ||
| ? fields | ||
| : onlyFields | ||
| ? onlyFields | ||
| : keys, | ||
| )} | ||
| } | ||
| } | ||
| } | ||
| `, | ||
| ...resultObject, | ||
| }; | ||
|
|
||
| let coreQuery = `${tableName}(${getObjStr(item)}) { | ||
| affected_rows | ||
| returning { | ||
| ${this.getParam( | ||
| fields && fields.length > 0 | ||
| ? fields | ||
| : onlyFields | ||
| ? onlyFields | ||
| : keys, | ||
| )} | ||
| } | ||
| }`; | ||
| if (props?.isCore === true) { | ||
| return coreQuery; | ||
| } | ||
|
|
||
| resultObject = { | ||
| query: `mutation MyQuery { | ||
| ${coreQuery} | ||
| }`, | ||
| ...resultObject, | ||
| }; | ||
|
|
||
| return resultObject; | ||
| } |
There was a problem hiding this comment.
Refactor the updateWithVariable method to improve readability and reduce complexity.
Consider breaking down this method into smaller functions, particularly separating the construction of the GraphQL query string from the logic that handles the variables. This will make the code easier to maintain and understand.
| quit: update_camp_days_activities_tracker(where: {start_date: {_gte: "${yesterdayStartTime}", _lte: "${yesterdayEndTime}"}, end_date: {_is_null: true}, attendances: {status: {_neq: "present"},context:{_eq:"camp_days_activities_tracker"}}}, _set: {end_camp_marked_by: "quit", end_date: "${today}"}) { | ||
| affected_rows | ||
| } | ||
| } | ||
| system: update_camp_days_activities_tracker(where: {start_date: {_gte: "${yesterdayStartTime}", _lte: "${yesterdayEndTime}"}, end_date: {_is_null: true}}, _set: {end_camp_marked_by: "system", end_date: "${today}"}) { | ||
| affected_rows | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider optimizing the GraphQL mutation to reduce redundancy.
- let updateQuery = `mutation MyMutation {
- quit: update_camp_days_activities_tracker(where: {start_date: {_gte: "${yesterdayStartTime}", _lte: "${yesterdayEndTime}"}, end_date: {_is_null: true}, attendances: {status: {_neq: "present"},context:{_eq:"camp_days_activities_tracker"}}}, _set: {end_camp_marked_by: "quit", end_date: "${today}"}) {
- affected_rows
- }
- system: update_camp_days_activities_tracker(where: {start_date: {_gte: "${yesterdayStartTime}", _lte: "${yesterdayEndTime}"}, end_date: {_is_null: true}}, _set: {end_camp_marked_by: "system", end_date: "${today}"}) {
- affected_rows
- }
- }`;
+ let updateQuery = `mutation MyMutation {
+ update_camp_days_activities_tracker(where: {start_date: {_gte: "${yesterdayStartTime}", _lte: "${yesterdayEndTime}"}, end_date: {_is_null: true}}, _set: {end_camp_marked_by: "system", end_date: "${today}"}) {
+ affected_rows
+ }
+ }`;This change consolidates the two updates into a single operation, assuming that the differentiation between 'quit' and 'system' as end_camp_marked_by is not required based on the current logic. If differentiation is needed, consider adding a condition within the where clause.
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.
| quit: update_camp_days_activities_tracker(where: {start_date: {_gte: "${yesterdayStartTime}", _lte: "${yesterdayEndTime}"}, end_date: {_is_null: true}, attendances: {status: {_neq: "present"},context:{_eq:"camp_days_activities_tracker"}}}, _set: {end_camp_marked_by: "quit", end_date: "${today}"}) { | |
| affected_rows | |
| } | |
| } | |
| system: update_camp_days_activities_tracker(where: {start_date: {_gte: "${yesterdayStartTime}", _lte: "${yesterdayEndTime}"}, end_date: {_is_null: true}}, _set: {end_camp_marked_by: "system", end_date: "${today}"}) { | |
| affected_rows | |
| } | |
| } | |
| update_camp_days_activities_tracker(where: {start_date: {_gte: "${yesterdayStartTime}", _lte: "${yesterdayEndTime}"}, end_date: {_is_null: true}}, _set: {end_camp_marked_by: "system", end_date: "${today}"}) { | |
| affected_rows | |
| } |
|
1 participant
I have ensured that following
Pull Request Checklistis taken care of before sending this PRSummary by CodeRabbit
New Features
updateScholarshipIdmethod inBeneficiariesControllerto handle scholarship information updates via POST requests.CampControllerfor camp-related operations.ObservationsModulefor improved modular functionality.Enhancements
type.Updates
package.jsonfor enhanced performance and security.Bug Fixes
Refactor
PrepareCertificateHtmlCronto optimize certificate generation process.Middleware
IpMiddlewarefor enhanced IP user ID validation across routes.