Conversation
SummaryThe following content is AI-generated and provides a summary of the pull request: Add i18n Support for Notification TypesNew Features✨ This PR introduces internationalization (i18n) support for CDS-defined notification types, moving beyond hardcoded English strings. Notification type templates are now compiled from CDS model annotations and resolved against the full list of 44 ANS-supported languages, with automatic fallback to English when a locale-specific translation is missing. Key capabilities added:
Changes
PR Bot InformationVersion:
|
![hyperspace-insights[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR introduces i18n support across the notification pipeline with good test coverage, but has several correctness issues: the most critical is that cds.load() is called redundantly inside the "served" handler instead of using cds.model, meaning the "loaded" hook's recipients injection is bypassed for that model instance. Additionally, resolveEnum().toUpperCase() can throw on non-string channel values, and cds.reflect() in compile.js is inconsistent with how the rest of the codebase iterates definitions directly.
PR Bot Information
Version: 1.20.47
- File Content Strategy: Full file content
- Event Trigger:
pull_request.opened - Correlation ID:
685f3a58-da82-4729-88de-7f2eec06c33d - LLM:
anthropic--claude-4.6-sonnet
| if (!model) return [] | ||
| const types = [] | ||
|
|
||
| for (const def of Object.values(cds.reflect(model).definitions)) { |
There was a problem hiding this comment.
Logic Error: notificationTypesFromModel uses cds.reflect(model) on a plain object that may not be a real CDS linked model
In cds-plugin.js, the model is loaded with await cds.load(srvPath) which returns a linked/reflected model, so cds.reflect() there is fine. However, in unit tests (compile.test.js), the function is called with bare plain objects like { definitions: { ... } }. cds.reflect() on a plain object may throw or silently produce an empty/wrong definitions set, making real-world parity with the tests uncertain.
More importantly, the notificationTypesFromModel function called in cds-plugin.js at line 30 and in lib/build.js at line 21 receives model returned by cds.load() (already linked), but in compile.js line 18 it calls cds.reflect(model) which double-reflects it. cds.reflect() is idempotent for already-linked models, but when test helpers pass raw objects the .definitions lookup could return unexpected results.
Consider aligning the implementation to directly iterate model.definitions (as the "loaded" hook in cds-plugin.js already does at line 4), removing the cds.reflect() wrapper, since both call sites already provide a loaded/linked model:
for (const def of Object.values(model.definitions)) {| for (const def of Object.values(cds.reflect(model).definitions)) { | |
| for (const def of Object.values(model.definitions)) { |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| if (def['@notification.deliveryChannels']?.length) { | ||
| type.DeliveryChannels = def['@notification.deliveryChannels'].map(ch => { | ||
| if (!ch.channel) return null | ||
| const entry = { Type: resolveEnum(ch.channel).toUpperCase() } |
There was a problem hiding this comment.
Bug: resolveEnum(ch.channel).toUpperCase() will throw a TypeError if resolveEnum returns a non-string value
resolveEnum returns its input unchanged when it is not an object with "=". If ch.channel is null, a number, or an object without "=", calling .toUpperCase() on it will throw a TypeError at runtime.
The guard if (!ch.channel) return null on line 45 only catches falsy values. A truthy non-string (e.g. { channel: 42 } or an object missing "=") will still reach toUpperCase() and crash.
Consider adding a string check after resolving the enum value:
| const entry = { Type: resolveEnum(ch.channel).toUpperCase() } | |
| const resolved = resolveEnum(ch.channel) | |
| if (typeof resolved !== 'string') return null | |
| const entry = { Type: resolved.toUpperCase() } |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
|
|
||
| const typesPath = cds.env.requires?.notifications?.types | ||
| const srvPath = path.join(cds.root, cds.env.folders.srv) | ||
| const model = await cds.load(srvPath) |
There was a problem hiding this comment.
Bug: cds.load(srvPath) in the "served" handler loads the service model a second time, which is redundant and potentially inconsistent
By the time "served" fires, CDS has already fully loaded and linked the application model (accessible via cds.model). Calling cds.load(srvPath) again performs a fresh, separate load that bypasses any model extensions or modifications applied during the normal boot sequence (including the "loaded" hook registered earlier in this same file). This means the freshly-loaded model may not reflect the recipients elements that the "loaded" hook injects.
Instead, use the already-loaded and fully-resolved model:
| const model = await cds.load(srvPath) | |
| const model = cds.model |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
|
|
||
| test("Fall back to raw value when i18n key not found in any locale", () => { | ||
| const cds = require('@sap/cds') | ||
| cds.i18n = { labels: { at: () => undefined } } |
There was a problem hiding this comment.
Bug: cds.i18n is overwritten directly with assignment instead of Object.defineProperty, breaking the afterEach restore
At line 239, the test assigns cds.i18n = { ... } directly. If cds.i18n is a non-writable or non-configurable property, this silently fails (in non-strict mode) and the mock is never applied. More critically, the afterEach restores via Object.defineProperty(require('@sap/cds'), 'i18n', originalI18nDescriptor), but since the descriptor was captured before the direct assignment, the restore path may be inconsistent across tests.
Use Object.defineProperty consistently as the other tests in this file do:
| cds.i18n = { labels: { at: () => undefined } } | |
| Object.defineProperty(cds, 'i18n', { | |
| value: { labels: { at: () => undefined } }, | |
| configurable: true, | |
| writable: true | |
| }) |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| }) | ||
|
|
||
| test("Resolve {i18n>KEY} in subtitle field", () => { | ||
| Object.defineProperty(cds, 'i18n', { |
There was a problem hiding this comment.
Bug: The cds variable used in the last test ("Resolve {i18n>KEY} in subtitle field") is not locally required — it refers to whatever cds is in scope, which is the module-level require('@sap/cds') from line 1. However, the afterEach restores the descriptor captured in beforeEach. If this test runs after one that mutated cds.i18n via direct assignment (line 239), the descriptor captured in beforeEach may not match expectations.
More directly: the test at line 257-267 uses Object.defineProperty(cds, ...) where cds is the outer module scope binding. This is consistent, but a local const cds = require('@sap/cds') (like the other i18n tests) would make the intent explicit and avoid any confusion about which binding is being modified.
| Object.defineProperty(cds, 'i18n', { | |
| const cds = require('@sap/cds') | |
| Object.defineProperty(cds, 'i18n', { |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
1 participant
Adds i18n support beyond hard-coded English. Prereq: #125