feat(alert): implements handlebars templating for alert contents#1461
feat(alert): implements handlebars templating for alert contents#1461ygrishajev merged 2 commits intomainfrom
Conversation
WalkthroughThis update introduces dynamic templating for alert messages using Handlebars, adds HTML sanitization for email content, and enhances alert payloads with richer context. It restructures alert message variables, modifies configuration handling, updates test setups, and introduces new configuration seeder utilities for testing. Several dependencies and environment variables are also updated. Changes
Sequence Diagram(s)sequenceDiagram
participant Service as DeploymentBalanceAlertsService
participant Repo as AlertRepository
participant Message as AlertMessageService
participant Broker as MessageBroker
Service->>Repo: Fetch alert (prev)
Service->>Repo: Update alert status (next)
Repo-->>Service: Updated alert
Service->>Message: getMessage({vars: {alert: {prev, next}, data}})
Message-->>Service: AlertMessagePayload
Service->>Broker: onMessage(AlertMessagePayload)
sequenceDiagram
participant EmailSender as EmailSenderService
participant Sanitize as sanitize-html
participant Novu as novu.trigger
EmailSender->>Sanitize: sanitize(content, allow <a href>)
Sanitize-->>EmailSender: sanitizedContent
EmailSender->>Novu: trigger({subject, content: sanitizedContent})
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (18)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (17)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration 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: 3
🔭 Outside diff range comments (1)
apps/notifications/src/modules/alert/services/alert-message/alert-message.service.spec.ts (1)
11-24: 🛠️ Refactor suggestionRemove duplicate test case.
Both test cases now have identical logic and assertions after removing the summaryPrefix. The second test case should either be removed or modified to test a different scenario.
- it("should an alert payload without prefix", async () => { - const { service } = await setup(); - - expect( - service.getMessage({ - summary: "CPU usage: {{usage}}%", - description: "Server {{server}} has high CPU load.", - vars - }) - ).toEqual({ - summary: "CPU usage: 90%", - description: "Server node-1 has high CPU load." - }); - });Also applies to: 26-39
🧹 Nitpick comments (1)
apps/notifications/src/modules/alert/services/template/template.service.ts (1)
10-12: Consider adding Handlebars security configurationsWhile Handlebars has built-in protections, consider explicitly configuring security options to prevent potential template injection attacks, especially if templates can be user-provided.
Consider creating a pre-compiled template cache and adding runtime options:
private readonly templateCache = new Map<string, HandlebarsTemplateDelegate>(); interpolate(template: string, context: object): string { let compiled = this.templateCache.get(template); if (!compiled) { compiled = Handlebars.compile(template, { noEscape: false, // Ensure HTML escaping strict: true // Throw on missing properties }); this.templateCache.set(template, compiled); } return compiled(context); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (18)
apps/notifications/env/.env.test(1 hunks)apps/notifications/package.json(2 hunks)apps/notifications/src/modules/alert/config/env.config.ts(1 hunks)apps/notifications/src/modules/alert/config/index.ts(1 hunks)apps/notifications/src/modules/alert/services/alert-message/alert-message.service.spec.ts(1 hunks)apps/notifications/src/modules/alert/services/alert-message/alert-message.service.ts(1 hunks)apps/notifications/src/modules/alert/services/chain-message-alert/chain-message-alert.service.spec.ts(4 hunks)apps/notifications/src/modules/alert/services/chain-message-alert/chain-message-alert.service.ts(1 hunks)apps/notifications/src/modules/alert/services/deployment-alert/deployment-alert.service.spec.ts(4 hunks)apps/notifications/src/modules/alert/services/deployment-alert/deployment-alert.service.ts(5 hunks)apps/notifications/src/modules/alert/services/deployment-balance-alerts/deployment-balance-alerts.service.spec.ts(9 hunks)apps/notifications/src/modules/alert/services/deployment-balance-alerts/deployment-balance-alerts.service.ts(4 hunks)apps/notifications/src/modules/alert/services/template/template.service.spec.ts(0 hunks)apps/notifications/src/modules/alert/services/template/template.service.ts(1 hunks)apps/notifications/src/modules/notifications/services/email-sender/email-sender.service.ts(2 hunks)apps/notifications/test/functional/balance-alert.spec.ts(3 hunks)apps/notifications/test/functional/chain-message-alert.spec.ts(2 hunks)apps/notifications/test/seeders/alert-config.seeder.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/notifications/src/modules/alert/services/template/template.service.spec.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/notifications/test/seeders/alert-config.seeder.ts (3)
apps/notifications/src/modules/alert/config/env.config.ts (1)
AlertEnvConfig(9-9)apps/notifications/src/modules/alert/config/index.ts (2)
AlertConfig(9-9)NAMESPACE(7-7)apps/notifications/src/lib/namespaced/namespaced.ts (1)
namespaced(5-6)
apps/notifications/src/modules/alert/services/deployment-balance-alerts/deployment-balance-alerts.service.ts (6)
apps/api/src/core/services/config/config.service.ts (1)
ConfigService(9-22)apps/notifications/src/modules/alert/config/index.ts (1)
AlertConfig(9-9)apps/notifications/src/modules/alert/services/conditions-matcher/conditions-matcher.service.ts (1)
isMatching(22-25)packages/logging/src/servicies/logger/logger.service.ts (1)
error(107-109)apps/notifications/src/lib/rich-error/rich-error.ts (1)
RichError(1-37)apps/notifications/src/modules/alert/repositories/alert/alert.repository.ts (1)
DeploymentBalanceAlertOutput(24-24)
🪛 Biome (1.9.4)
apps/notifications/src/modules/alert/services/chain-message-alert/chain-message-alert.service.spec.ts
[error] 205-205: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: validate-notifications
🔇 Additional comments (28)
apps/notifications/package.json (2)
75-75: LGTM: sanitize-html dependency addition.The addition of sanitize-html is appropriate for securing email content as mentioned in the AI summary. The version 2.17.0 aligns with the HTML sanitization feature being implemented.
66-66:Details
✅ Verification successful
Verify the handlebars version for security vulnerabilities.
The handlebars version 4.7.8 should be checked for known security vulnerabilities, as template engines can be vectors for security issues.
🏁 Script executed:
#!/bin/bash # Description: Check for security advisories for handlebars version 4.7.8 # Check for security advisories using GitHub CLI gh api graphql -f query=' { securityVulnerabilities(first: 10, ecosystem: NPM, package: "handlebars") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }' # Check latest version curl -s https://registry.npmjs.org/handlebars | jq '.["dist-tags"].latest'Length of output: 2492
Handlebars v4.7.8 is secure and up to date
- Queried GitHub security advisories: all critical/high vulnerabilities (prototype pollution, RCE) were patched by v4.7.7.
- Verified npm registry: v4.7.8 is the current latest release.
- No outstanding security issues in 4.7.8—safe to approve this dependency addition.
apps/notifications/src/modules/alert/config/index.ts (1)
7-7: LGTM: Export NAMESPACE for better modularity.Exporting the NAMESPACE constant improves code organization and enables consistent usage across modules, particularly for the new seeder utilities mentioned in the AI summary.
apps/notifications/env/.env.test (1)
9-9: LGTM: CONSOLE_WEB_URL addition supports dynamic alert links.The new environment variable enables dynamic generation of console management links in alert messages, which aligns with the enhanced templating functionality being implemented.
apps/notifications/src/modules/alert/services/alert-message/alert-message.service.spec.ts (1)
18-18: LGTM: Removal of summaryPrefix aligns with service changes.The removal of the summaryPrefix property correctly reflects the changes in the AlertMessageService implementation where this property was removed.
Also applies to: 33-33
apps/notifications/test/functional/balance-alert.spec.ts (2)
53-53: LGTM: Template variable update reflects new payload structure.The change from
{{balance}}to{{data.balance}}correctly reflects the restructured alert message payloads where event data is now nested under thedatakey, as mentioned in the AI summary.Also applies to: 69-69
104-104: LGTM: Removal of [TRIGGERED] prefix is consistent.The removal of the
[TRIGGERED]prefix from the expected summary aligns with the changes in AlertMessageService where the summaryPrefix property was removed.apps/notifications/test/functional/chain-message-alert.spec.ts (1)
51-52: Template variable updates look good.The changes correctly update template placeholders to use the new
data.prefix, aligning with the restructured alert message payload that now includes nestedalertanddataobjects.Also applies to: 72-73
apps/notifications/src/modules/alert/config/env.config.ts (1)
4-6: Configuration additions are well-structured.The new environment variables are properly typed with appropriate defaults and validation. The
CONSOLE_WEB_URLas a required string andDEPLOYMENT_BALANCE_BLOCKS_THROTTLEwith coercion and default value of 10 are sensible additions.apps/notifications/src/modules/alert/services/deployment-alert/deployment-alert.service.spec.ts (1)
3-3: Test updates properly reflect dynamic message generation.The addition of
ConfigModule.forFeature(moduleConfig)and the change from specific string assertions toexpect.any(String)correctly accommodate the new dynamic templating system where alert messages are generated at runtime.Also applies to: 8-8, 52-54, 80-82, 196-196
apps/notifications/src/modules/alert/services/chain-message-alert/chain-message-alert.service.spec.ts (2)
50-57: Template variable structure changes are correct.The updated
varsstructure with nestedalert(containingprevandnextstates) anddataobjects properly reflects the enriched message payload that includes both alert state information and event data.Also applies to: 182-189, 194-201, 253-260
233-239: Mock implementation for updateById looks good.The mock properly returns an updated alert object, which aligns with the service logic that updates alerts before generating message payloads.
apps/notifications/src/modules/notifications/services/email-sender/email-sender.service.ts (1)
4-4: Excellent security enhancement with HTML sanitization.The addition of
sanitize-htmlwith a conservative allowlist (only<a>tags withhrefattributes) properly protects against XSS attacks while maintaining basic formatting capabilities for email content. The destructuring also improves code readability.Also applies to: 23-23, 30-38
apps/notifications/src/modules/alert/services/chain-message-alert/chain-message-alert.service.ts (1)
39-52: Good enhancement for template contextThe enriched message payload with
prevandnextalert states provides excellent context for Handlebars templates, enabling dynamic content based on alert state transitions.apps/notifications/src/modules/alert/services/alert-message/alert-message.service.ts (1)
5-26: Clean simplification aligns with Handlebars approachThe removal of
summaryPrefixand direct delegation toTemplateServiceis a good design choice. Prefixes and other formatting can now be handled within Handlebars templates themselves.apps/notifications/test/seeders/alert-config.seeder.ts (2)
14-14: Well-structured test data generationThe seeder correctly implements the namespacing pattern and provides good test data generation utilities.
8-12:Details
✅ Verification successful
Verify CONSOLE_WEB_URL format expectations
The code strips "https://" from the generated URL. Please verify this matches the expected format in the application code.
🏁 Script executed:
#!/bin/bash # Description: Check how CONSOLE_WEB_URL is used in the codebase to verify format expectations # Search for CONSOLE_WEB_URL usage rg -A 3 "CONSOLE_WEB_URL" # Look for any URL construction patterns with this config rg -A 3 "config.*CONSOLE_WEB_URL|CONSOLE_WEB_URL.*http"Length of output: 2226
CONSOLE_WEB_URL formatting is correct
The test seeder intentionally strips the
https://prefix so that in deployment-alert.service.ts the URL is reconstructed as`<a href="https://${baseUrl}/deployments/${dseq}">${baseUrl}</a>`This matches expectations—no changes required.
apps/notifications/src/modules/alert/services/deployment-balance-alerts/deployment-balance-alerts.service.spec.ts (2)
41-47: Consistent and proper test updatesThe mock implementation correctly simulates the repository's merge behavior, and all test cases have been properly updated to verify the new message structure with
prevandnextalert states.Also applies to: 92-98, 144-150, 179-185
266-266: Good addition of configuration moduleAdding the ConfigModule with the alert module configuration ensures tests run with proper configuration context.
apps/notifications/src/modules/alert/services/template/template.service.ts (1)
4-6: Useful helper for template conditionalsThe "eq" helper enables conditional logic in templates, which is essential for the dynamic alert messages mentioned in the PR objectives.
apps/notifications/src/modules/alert/services/deployment-balance-alerts/deployment-balance-alerts.service.ts (4)
2-2: LGTM: Proper dependency injection setup for configuration-driven alerts.The imports and constructor injection are correctly implemented to support the new configuration-driven approach with typed ConfigService.
Also applies to: 6-6, 9-9, 23-23
67-68: LGTM: Dynamic configuration retrieval improves maintainability.Replacing the hardcoded throttle constant with dynamic configuration retrieval from
ConfigServiceis a good practice that enhances flexibility and maintainability.
76-93: LGTM: Improved alert processing with structured payload and status-based messaging.The refactored logic properly:
- Only sends messages when alert status actually changes (line 78)
- Provides structured payload with previous and updated alert states
- Includes contextual data for template processing
This aligns well with the handlebars templating approach and reduces unnecessary message spam.
103-127: LGTM: Enhanced error handling with structured payload.The updated
suspendErroneousAlertmethod properly:
- Returns the updated alert for consistency
- Uses structured
varspayload format- Includes cause information for template processing
- Maintains proper error logging for non-deployment-closed errors
The payload structure is consistent with the main alert processing flow.
apps/notifications/src/modules/alert/services/deployment-alert/deployment-alert.service.ts (4)
3-3: LGTM: Proper ConfigService integration for dynamic configuration.The import and constructor injection of
ConfigService<AlertConfig>follows the established pattern and enables configuration-driven console URL management.Also applies to: 5-5, 67-70
158-160: LGTM: Well-structured Handlebars template generation.The
getTemplatemethod efficiently constructs conditional Handlebars templates using:
- Proper nested conditional logic with
{{#if}}blocks- The
eqhelper for status comparisons- Clear separation of suspended, triggered, and recovered states
The template structure supports the three distinct alert states effectively.
145-155: LGTM: Dynamic alert templates with contextual messaging.The summary and description templates properly:
- Use Handlebars syntax for dynamic content (
{{alert.next.params.dseq}})- Include contextual links to the deployment console
- Provide clear, actionable messages for each alert state
- Maintain consistency in template structure
The templates effectively guide users to take appropriate actions based on alert status.
189-191: LGTM: Simplified template for deployment closed alerts.The closed alert templates use straightforward Handlebars syntax without complex conditionals, which is appropriate for this single-state alert type. The console link integration provides users with direct access to deployment management.
...fications/src/modules/alert/services/chain-message-alert/chain-message-alert.service.spec.ts
Outdated
Show resolved
Hide resolved
.../notifications/src/modules/alert/services/chain-message-alert/chain-message-alert.service.ts
Outdated
Show resolved
Hide resolved
apps/notifications/src/modules/alert/services/deployment-alert/deployment-alert.service.ts
Show resolved
Hide resolved
ygrishajev
force-pushed
the
feature/alerts
branch
from
b0e9911 to
016673b
Compare
ygrishajev
force-pushed
the
feature/alerts
branch
from
016673b to
8737b37
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1461 +/- ##
==========================================
- Coverage 39.37% 38.92% -0.45%
==========================================
Files 854 852 -2
Lines 20357 20219 -138
Branches 3737 3688 -49
==========================================
- Hits 8016 7871 -145
+ Misses 12024 11624 -400
- Partials 317 724 +407
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
2 participants
refs #1452
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores