Ks 033 oct 03 email notification 1959#2296
Conversation
…change - Implemented `sendMemberRoleChangedEditorToAdminNotification` to notify users when their role changes from editor to admin. - Integrated email template `MEMBER_ROLE_CHANGED_EDITOR_TO_ADMIN` with dynamic data for notification personalization.
- Improved `getUserByIdQuery` to validate user existence and convert plain objects to `UserModel` instances. - Enhanced `updateUserById` and `updateUserRole` functions to notify admins when a user's role changes from Editor (role_id = 3) to Admin (role_id = 1). - Added detailed error logging for failed project retrieval and notification sending during role updates. - Updated `getUserProjects` to include tenant-specific queries with joined data for project members.
…sable module - Merged `sendUserAddedToProjectNotification`, `sendMemberRoleChangedEditorToAdminNotification`, and `sendProjectCreatedNotification` into a unified `projectNotifications.ts` module. - Centralized notification configurations and streamlined template handling for role-based and project events. - Removed deprecated individual notification handlers for improved maintainability and consistency.
…tifications module
WalkthroughCentralizes project-related email notifications into Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Admin as Admin (actor)
participant API as User Controller
participant DB as DB
participant Notif as projectNotifications
participant Mail as Email Service
Admin->>API: PATCH /users/:id (change role)
API->>DB: fetch old user
DB-->>API: oldUser (oldRoleId)
API->>DB: update user role
DB-->>API: updated user
alt oldRole = Editor && newRole = Admin
API->>DB: getUserProjects(userId, tenant)
DB-->>API: [projects...]
loop per project (fire-and-forget)
API-)Notif: sendMemberRoleChangedEditorToAdminNotification({projectId,projectName,actorId,userId})
Note right of Notif: Asynchronous, errors logged via logFailure
Notif->>DB: fetch actor & user
DB-->>Notif: actor/user
Notif->>Mail: sendEmailWithTemplate(...)
Mail-->>Notif: result
end
end
API-->>Admin: 200 OK
sequenceDiagram
autonumber
participant Ctrl as Project Controller
participant Notif as projectNotifications
participant DB as DB
participant Mail as Email Service
Ctrl-)Notif: sendProjectCreatedNotification({projectId, projectName, adminId})
Notif->>DB: fetch admin
DB-->>Notif: admin record
Notif->>Mail: sendEmailWithTemplate(project.created.admin,...)
Mail-->>Notif: result
Notif-->>Ctrl: resolve / throw
sequenceDiagram
autonumber
participant Ctrl as Membership Controller
participant Notif as projectNotifications
participant DB as DB
participant Mail as Email Service
Ctrl-)Notif: sendUserAddedToProjectNotification({projectId, projectName, adminId, userId, role})
Notif->>DB: fetch actor & user
DB-->>Notif: records
Notif->>Mail: sendEmailWithTemplate(templateByRole,...)
Mail-->>Notif: result
Notif-->>Ctrl: resolve / throw
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (4 warnings)
✅ Passed checks (1 passed)
✨ 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.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Servers/controllers/project.ctrl.ts(1 hunks)Servers/controllers/user.ctrl.ts(7 hunks)Servers/services/projectNotification/projectCreationNotification.ts(0 hunks)Servers/services/userNotification/projectNotifications.ts(1 hunks)Servers/services/userNotification/userAddedToProjectNotification.ts(0 hunks)Servers/utils/user.utils.ts(2 hunks)
💤 Files with no reviewable changes (2)
- Servers/services/userNotification/userAddedToProjectNotification.ts
- Servers/services/projectNotification/projectCreationNotification.ts
🧰 Additional context used
🧬 Code graph analysis (3)
Servers/controllers/user.ctrl.ts (4)
Servers/utils/user.utils.ts (1)
getUserProjects(416-428)Servers/utils/project.utils.ts (1)
getUserProjects(21-59)Servers/services/userNotification/projectNotifications.ts (1)
sendMemberRoleChangedEditorToAdminNotification(246-259)Servers/utils/logger/logHelper.ts (1)
logFailure(51-68)
Servers/utils/user.utils.ts (2)
Clients/src/domain/models/Common/user/user.model.ts (1)
UserModel(1-25)Servers/utils/project.utils.ts (1)
getUserProjects(21-59)
Servers/services/userNotification/projectNotifications.ts (4)
Servers/constants/emailTemplates.ts (1)
EMAIL_TEMPLATES(14-33)Servers/utils/logger/logHelper.ts (3)
logProcessing(22-30)logSuccess(33-49)logFailure(51-68)Servers/utils/user.utils.ts (1)
getUserByIdQuery(137-154)Servers/services/notificationService.ts (1)
notificationService(321-321)
…ateUserRole logic - Removed unnecessary debug log statement in `updateUserById`. - Added normalization for `newRoleId` to handle string inputs gracefully in `updateUserRole`.
…uery - Updated `getUserByIdQuery` to return `null` if the user is not found, instead of throwing an error.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Servers/utils/user.utils.ts (1)
137-154: Restore null return for missing users to preserve 404 handling.This change breaks existing error handling. Throwing when a user is absent forces every caller that previously checked for a falsy return into catch blocks that emit 500 responses instead of the intended 404. The past review comment on this function remains unaddressed.
Apply this diff to restore the original behavior:
-export const getUserByIdQuery = async (id: number): Promise<UserModel> => { +export const getUserByIdQuery = async (id: number): Promise<UserModel | null> => { const [userObj] = await sequelize.query<any>( "SELECT * FROM public.users WHERE id = :id", { replacements: { id }, type: QueryTypes.SELECT, } ); if (!userObj) { - throw new Error(`User not found with ID: ${id}`); + return null; } // Convert plain object to UserModel instance const user = new UserModel(); Object.assign(user, userObj); return user; };
🧹 Nitpick comments (1)
Servers/utils/user.utils.ts (1)
416-428: Consider consolidating duplicategetUserProjectsfunctions.Two
getUserProjectsfunctions exist with different signatures and logic:
Servers/utils/user.utils.ts(this file):(userId: number, tenant: string)Servers/utils/project.utils.ts:({ userId, role, transaction }, tenant)The
project.utils.tsversion includes role-based filtering and ownership checks that this version lacks. This naming collision creates confusion and maintenance burden.Consider:
- Renaming this function to
getUserProjectsByMembershipto clarify scope, or- Consolidating into a single function with optional parameters for role/transaction if both are needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Servers/utils/user.utils.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Servers/utils/user.utils.ts (2)
Clients/src/domain/models/Common/user/user.model.ts (1)
UserModel(1-25)Servers/utils/project.utils.ts (1)
getUserProjects(21-59)
🔇 Additional comments (1)
Servers/utils/user.utils.ts (1)
416-428: Ignore missing ownership concern; function scope matches usage.
user.utils.getUserProjectsis explicitly scoped to membership-only queries (controllers’ comments confirm this) and all invocations now supply thetenantparameter.Likely an incorrect or invalid review comment.
- Updated `getUserByIdQuery` to return a `UserModel` instance or `null` if the user is not found. - Introduced `getUserByIdOrThrow` for mandatory user retrieval, throwing an error if the user is not found. - Updated method signatures, examples, and comments to align with the new logic.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Servers/utils/user.utils.ts (1)
436-448: Handle project owners in getUserProjects
The query in Servers/utils/user.utils.ts (l. 436–448) only returns projects where the user is in projects_members. It must also include projects they own. Change to:
- LEFT JOIN "${tenant}".projects_members pm ON p.id = pm.project_id
- WHERE p.owner = :user_id OR pm.user_id = :user_id
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Servers/utils/user.utils.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Servers/utils/user.utils.ts (2)
Clients/src/domain/models/Common/user/user.model.ts (1)
UserModel(1-25)Servers/utils/project.utils.ts (1)
getUserProjects(21-59)
🔇 Additional comments (1)
Servers/utils/user.utils.ts (1)
152-174: LGTM! Clean separation of concerns.The
getUserByIdOrThrowwrapper provides a clear API for cases where the user must exist, eliminating the need for null checks at call sites. The documentation and implementation are both clear.
| export const getUserByIdQuery = async (id: number): Promise<UserModel> => { | ||
| const user = await sequelize.query("SELECT * FROM public.users WHERE id = :id", { | ||
| replacements: { id }, | ||
| mapToModel: true, | ||
| model: UserModel, | ||
| }); | ||
| return user[0]; | ||
| const users = await sequelize.query<UserModel>( | ||
| "SELECT * FROM public.users WHERE id = :id", | ||
| { | ||
| replacements: { id }, | ||
| model: UserModel, | ||
| mapToModel: true, // converts results into UserModel instances | ||
| } | ||
| ); | ||
|
|
||
| // users will be an array. Return first element or null if not found | ||
| return users[0]; | ||
| }; |
There was a problem hiding this comment.
Return null instead of undefined and update the return type.
The current implementation returns users[0], which evaluates to undefined when no user is found, but the past review comment specifically requested returning null to preserve 404 handling in callers. Additionally, the return type is still Promise<UserModel> instead of Promise<UserModel | null>, which creates a type mismatch.
Apply this diff to address the past review comment:
-export const getUserByIdQuery = async (id: number): Promise<UserModel> => {
+export const getUserByIdQuery = async (id: number): Promise<UserModel | null> => {
const users = await sequelize.query<UserModel>(
"SELECT * FROM public.users WHERE id = :id",
{
replacements: { id },
model: UserModel,
mapToModel: true, // converts results into UserModel instances
}
);
// users will be an array. Return first element or null if not found
- return users[0];
+ return users[0] || null;
};This ensures callers can safely check for null (e.g., if (!user)) and the type signature accurately reflects the function's behavior.
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getUserByIdQuery = async (id: number): Promise<UserModel> => { | |
| const user = await sequelize.query("SELECT * FROM public.users WHERE id = :id", { | |
| replacements: { id }, | |
| mapToModel: true, | |
| model: UserModel, | |
| }); | |
| return user[0]; | |
| const users = await sequelize.query<UserModel>( | |
| "SELECT * FROM public.users WHERE id = :id", | |
| { | |
| replacements: { id }, | |
| model: UserModel, | |
| mapToModel: true, // converts results into UserModel instances | |
| } | |
| ); | |
| // users will be an array. Return first element or null if not found | |
| return users[0]; | |
| }; | |
| export const getUserByIdQuery = async (id: number): Promise<UserModel | null> => { | |
| const users = await sequelize.query<UserModel>( | |
| "SELECT * FROM public.users WHERE id = :id", | |
| { | |
| replacements: { id }, | |
| model: UserModel, | |
| mapToModel: true, // converts results into UserModel instances | |
| } | |
| ); | |
| // users will be an array. Return first element or null if not found | |
| return users[0] || null; | |
| }; |
🤖 Prompt for AI Agents
In Servers/utils/user.utils.ts around lines 138 to 150, the function currently
returns users[0] which becomes undefined when no record is found and its
signature is Promise<UserModel>; change the return type to Promise<UserModel |
null> and ensure the function returns null instead of undefined when no user is
found (e.g., return users.length ? users[0] : null), keeping the existing
sequelize.query call and mapToModel behavior so callers can reliably check for
null.
# Conflicts: # Servers/controllers/project.ctrl.ts
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Servers/controllers/user.ctrl.ts (1)
687-727: Notification logic is correct, but consider variable naming.The role-change detection and per-project notification flow work correctly. The fire-and-forget pattern with comprehensive error logging is appropriate. However, the variable names
newRoleIdNumandoldRoleIdNumcould be clearer sinceroleId(notnewRoleId) is used here.Consider this small clarity improvement:
- // Convert to numbers explicitly for comparison - const oldRoleIdNum = Number(oldRoleId); - const newRoleIdNum = Number(roleId); - - if (newRoleIdNum === 1 && oldRoleIdNum === 3) { + // Check if role changed from Editor (3) to Admin (1) + if (Number(roleId) === 1 && Number(oldRoleId) === 3) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Servers/controllers/project.ctrl.ts(1 hunks)Servers/controllers/user.ctrl.ts(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Servers/controllers/project.ctrl.ts
🧰 Additional context used
🧬 Code graph analysis (1)
Servers/controllers/user.ctrl.ts (3)
Servers/utils/user.utils.ts (1)
getUserProjects(436-448)Servers/services/userNotification/projectNotifications.ts (1)
sendMemberRoleChangedEditorToAdminNotification(246-259)Servers/utils/logger/logHelper.ts (1)
logFailure(51-68)
🔇 Additional comments (7)
Servers/controllers/user.ctrl.ts (7)
46-47: LGTM!The new imports for
sendMemberRoleChangedEditorToAdminNotificationandlogFailureare correctly placed and align with the PR's goal of centralizing project notifications.
613-616: LGTM!The
roleIdnormalization correctly handles string-to-number conversion, ensuring type consistency for subsequent comparisons.
652-654: LGTM!Capturing
oldRoleIdbefore the user update is the correct approach for detecting role changes.
857-857: LGTM!The tenant context is correctly passed to
getUserProjects, ensuring proper multi-tenant isolation.
1003-1006: Normalization fixes the previous issue.The
newRoleIdnormalization correctly addresses the previous review comment about the short-circuit when the client sends a string. This ensures the role comparison at line 1078 works as intended.Based on past review comments.
1062-1063: LGTM!Capturing
oldRoleIdbefore updating the role is the correct approach for detecting role transitions.
1077-1110: LGTM!The role-change detection (Editor → Admin) and per-project notification logic are correctly implemented. The fire-and-forget pattern with comprehensive error logging is appropriate, and the normalization ensures the comparison at line 1078 works as expected.
|
@Aryanak47 and @swaleha456 , would you please provide your reviews here? |
|
|
||
| // Send notification for each project (fire-and-forget) | ||
| for (const project of userProjects) { | ||
| sendMemberRoleChangedEditorToAdminNotification({ |
There was a problem hiding this comment.
For functions like this, its better to name them like this:
sendMemberRoleChangedNotification, and then you have two props, oldRole, newRole.
Please modify it, so the logic is handled like that
|
@solan117 Please address my comments in a next pull request |
2 participants
Describe your changes
feat(notifications): add email notification for editor-to-admin role change
sendMemberRoleChangedEditorToAdminNotificationto notify users when their role changes from editor to admin.MEMBER_ROLE_CHANGED_EDITOR_TO_ADMINwith dynamic data for notification personalization.sendUserAddedToProjectNotification,sendMemberRoleChangedEditorToAdminNotification, andsendProjectCreatedNotificationinto a unifiedprojectNotifications.tsmodule.Write your issue number after "Fixes "
Fixes #1959
Please ensure all items are checked off before requesting a review: