Ks 026 sep 26 email notification 1959#2235
Conversation
- Introduced `sendUserAddedEditorNotification` service to notify users when added as project editors. - Added new MJML email template `user-added-project-editor.mjml` for editor notification styling. - Integrated the service into `updateProjectById` to trigger notifications for added project editors. - Implemented email queuing with rate limiting in `NotificationService` to comply with sending limits.
…for project editor flow - Updated `sendUserAddedEditorNotification` log descriptions to reflect correct "project editor" role. - Adjusted MJML template to replace "project admin" with "project editor." - Ensured only users with the "Editor" role receive notifications in `updateProjectById`.
- Added `getCurrentProjectMembers` utility to fetch existing project members. - Updated `updateProjectById` controller to compare new and current members for targeted notifications. - Ensured notifications are triggered only for users newly added as project editors.
- Applied consistent indentation throughout `sendEmail` function. - Improved readability of mail option object initialization. fix(project): refine logic for sending notifications to added project members - Updated notification flow to account for both newly added and re-added members. - Refactored logic to calculate added members post-update for accurate notifications. - Ensured changes do not block the response while sending notifications asynchronously.
- Updated `user-added-project-editor.mjml` and `user-added-project-admin.mjml` to include `{{project_name}}` in headings for clarity.
… values on successful save - Clear validation errors when adding/removing team members - Update initialValuesRef after successful save to reset form state - Ensure save button properly enables/disables based on actual changes
solan117
requested review from
HarshP4585,
MuhammadKhalilzadeh,
gorkem-bwl and
gorkemcetin
WalkthroughAdds client-side per-field error clearing and initial-values sync; server diffs current project members to detect additions, sends editor-specific notifications, introduces a persistent rate-limited in-process email queue, adds project-member lookup util, new editor notification module and template, tweaks admin template, and updates .gitignore. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant API as ProjectController
participant U as ProjectUtils
participant N as NotificationService (queue)
participant E as EmailService
C->>API: Update project (members/roles)
API->>U: getCurrentProjectMembers(projectId, tenant)
U-->>API: currentMemberIds[]
API->>API: diff -> addedMembers[]
loop each addedMember
alt role == Editor (role_id = 3)
API->>N: enqueue sendEmailWithTemplate(editor template, data)
note right of N #EDF7FF: enqueue returns per-email Promise (fire-and-forget)
else
API->>API: no editor notification
end
end
rect rgba(230,245,255,0.6)
Note over N,E: processQueue() sequentially sends queued emails\nusing token-bucket rate limiting and persisted state
N->>E: sendEmailDirectly(recipient, subject, template)
E-->>N: success / error
N-->>API: resolve/reject per-item Promise (controller used fire-and-forget)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Servers/services/notificationService.ts (1)
155-172: Preserve PII sanitization when bubbling errorsInside
sendEmailDirectlywe sanitize the error message to scrub email addresses, but we immediately rethrow the originalerror. Any caller that logs or surfaces that rejection will leak the unsanitized email, defeating the sanitization step and reopening the compliance gap. Please rethrow the sanitized error (optionally attaching the original ascause) so downstream logs stay redacted.Apply this diff to propagate the sanitized error:
await logFailure({ eventType: "Create", description: `Failed to send email to ${maskEmail(recipientEmail)}`, functionName: "sendEmailWithTemplate", fileName: "NotificationService.ts", error: sanitized, }); - throw error; + (sanitized as any).cause = error; + throw sanitized;
🧹 Nitpick comments (1)
Servers/utils/project.utils.ts (1)
645-658: Rely on Sequelize model mapping to avoid theanycast.Line 657 casts to
any[], which we can eliminate by letting Sequelize hydrateProjectsMembersModelinstances the same way the rest of this file already does. This keeps the return type honest and avoids surprises ifuser_idcomes back as a string.export const getCurrentProjectMembers = async ( projectId: number, tenant: string, transaction?: Transaction ): Promise<number[]> => { const currentMembersResult = await sequelize.query( `SELECT user_id FROM "${tenant}".projects_members WHERE project_id = :project_id`, { replacements: { project_id: projectId }, - type: QueryTypes.SELECT, + mapToModel: true, + model: ProjectsMembersModel, transaction, } ); - return (currentMembersResult as any[]).map((m) => m.user_id); + return currentMembersResult.map((m) => m.user_id); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
Clients/src/presentation/pages/ProjectView/ProjectSettings/index.tsx(2 hunks)Servers/controllers/project.ctrl.ts(4 hunks)Servers/services/emailService.ts(1 hunks)Servers/services/notificationService.ts(3 hunks)Servers/services/userNotification/userAddedAdminNotification.ts(1 hunks)Servers/services/userNotification/userAddedEditorNotification.ts(1 hunks)Servers/templates/user-added-project-admin.mjml(1 hunks)Servers/templates/user-added-project-editor.mjml(1 hunks)Servers/utils/project.utils.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
Servers/utils/project.utils.ts (1)
Servers/database/db.ts (1)
sequelize(141-141)
Servers/services/userNotification/userAddedEditorNotification.ts (3)
Servers/utils/logger/logHelper.ts (3)
logProcessing(22-30)logSuccess(33-49)logFailure(51-68)Servers/utils/user.utils.ts (1)
getUserByIdQuery(137-144)Servers/services/notificationService.ts (1)
notificationService(177-177)
Servers/controllers/project.ctrl.ts (4)
Servers/utils/project.utils.ts (2)
getProjectByIdQuery(114-159)getCurrentProjectMembers(644-658)Servers/utils/user.utils.ts (1)
getUserByIdQuery(137-144)Servers/services/userNotification/userAddedEditorNotification.ts (1)
sendUserAddedEditorNotification(19-84)Servers/utils/logger/logHelper.ts (1)
logFailure(51-68)
🔇 Additional comments (4)
Clients/src/presentation/pages/ProjectView/ProjectSettings/index.tsx (1)
447-452: Clearing member-field errors on change is a nice touchResetting the error alongside the multi-select update keeps the validation UX tight.
Servers/templates/user-added-project-admin.mjml (1)
14-16: Copy update looks consistent with template data.The new header and sentence reuse the existing
project_name,actor_name, anduser_namevariables, so no additional data plumbing is required. Nicely aligned with the subject line.Servers/services/userNotification/userAddedAdminNotification.ts (1)
62-63: Template rename matches the new asset.The updated filename aligns with
Servers/templates/user-added-project-admin.mjml, so the notification will keep rendering without further changes.Servers/templates/user-added-project-editor.mjml (1)
14-22: Template wiring looks ready to ship.All required placeholders (
project_name,actor_name,user_name,project_url) are present, matching the data provided by the editor notification service, so rendering should work out of the box.
Docstrings generation was requested by @solan117. * #2235 (comment) The following files were modified: * `Servers/controllers/project.ctrl.ts`
|
Note Generated docstrings for this pull request at #2238 |
…er successful save - Introduce `newValues` to synchronize `initialValuesRef` and `setValues` for consistent state management.
|
@HarshP4585 and @Br0wnHammer , would you guys please do a test on this and get beck to me with your feedbacks? |
… sanitized errors - Assigned the original error as the `.cause` property of the sanitized error before re-throwing it for improved debugging context.
…iting - Added token bucket algorithm to handle rate limiting with burst capacity and compliance to the email send limits. - Introduced persistence for rate limiting state to maintain consistency across restarts. - Updated queue processing to incorporate exponential backoff on failures.
…essing actions - Introduced an `initialization` promise to enforce asynchronous initialization before processing the email queue or sending emails. - Adjusted token validation logic to handle edge cases with loaded rate limit state.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
Servers/services/notificationService.ts (4)
271-294: Correct logger metadata: wrong function name and file name casing.These logs originate from
sendEmailDirectlybut reportsendEmailWithTemplate; file name casing also differs from the actual file.logProcessing({ description: `Sending email with template: ${templateFileName}`, - functionName: "sendEmailWithTemplate", - fileName: "NotificationService.ts", + functionName: "sendEmailDirectly", + fileName: "notificationService.ts", }); @@ await logSuccess({ eventType: "Create", description: `Email sent successfully to ${maskEmail(recipientEmail)}`, - functionName: "sendEmailWithTemplate", - fileName: "NotificationService.ts", + functionName: "sendEmailDirectly", + fileName: "notificationService.ts", }); @@ await logFailure({ eventType: "Create", description: `Failed to send email to ${maskEmail(recipientEmail)}`, - functionName: "sendEmailWithTemplate", - fileName: "NotificationService.ts", + functionName: "sendEmailDirectly", + fileName: "notificationService.ts", error: sanitized, });Also applies to: 304-313
121-128: Persist state atomically to avoid partial/corrupted files.Use a temp file + rename to make writes atomic.
private async saveRateLimitState(): Promise<void> { try { - await fs.writeFile(rateLimitStateFile, JSON.stringify(this.rateLimitState), 'utf8'); + const tmp = `${rateLimitStateFile}.tmp`; + await fs.writeFile(tmp, JSON.stringify(this.rateLimitState), 'utf8'); + await fs.rename(tmp, rateLimitStateFile); } catch (error) { // Log but don't throw - persistence failure shouldn't break email sending console.warn('Failed to save rate limit state:', error); } }
297-313: Prefer native Error cause over manual mutation.Construct the error with
{ cause }instead of assigning later.- const sanitized = new Error( - String((error as any)?.message ?? error).replace( - /\b[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,}\b/gi, - "[redacted]" - ) - ); + const sanitized = new Error( + String((error as any)?.message ?? error).replace( + /\b[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,}\b/gi, + "[redacted]" + ), + { cause: error as unknown } + ); @@ - (sanitized as any).cause = error; - throw sanitized; + throw sanitized;
62-73: Operational note: per-process limiter and local JSON state.If you run multiple app instances (pods/workers), each will send up to ~1.6 rps, exceeding provider caps system-wide. Consider a centralized queue/limiter (Redis/Cloud task queue) or provider-level throttle, and cap in-memory queue size to avoid unbounded growth.
Also applies to: 121-128
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Servers/services/notificationService.ts(4 hunks)
🔇 Additional comments (2)
Servers/services/notificationService.ts (2)
58-77: Nice: prior rate-limit gaps are addressed.Initialization is awaited, 0 tokens are preserved on restore, and a min-gap/backoff is enforced before each send. This closes the burst-on-restart loopholes flagged earlier.
Also applies to: 94-103, 160-184, 201-216, 220-223
33-51: Good PII hygiene and path hardening.Email masking in logs and
path.basenameto constrain template reads are solid.Also applies to: 271-281
… during shutdown - Released processing lock but ensured the queue resumes if pending items are added during the winding-down phase.
…ency - Adjusted `templatesDir` to point to `Servers/templates` for streamlined directory structure and compatibility.
MuhammadKhalilzadeh
deleted the
ks-026-Sep-26-email-notification-1959
branch
3 participants
Describe your changes
feat(notifications): add user-added-as-editor notification flow
sendUserAddedEditorNotificationservice to notify users when added as project editors.user-added-project-editor.mjmlfor editor notification styling.updateProjectByIdto trigger notifications for added project editors.NotificationServiceto comply with sending limits.feat(project): refine logic for sending notifications to added project members
getCurrentProjectMembersutility to fetch existing project members.updateProjectByIdcontroller to compare new and current members for targeted notifications.For ProjectSettings/index.tsx
fix(project settings): clear field-specific errors and update initial values on successful save
Write your issue number after "Fixes "
Fixes #1959
Please ensure all items are checked off before requesting a review: