add: entity notification - #150
Conversation
There was a problem hiding this comment.
The PR introduces a useful entity-level @notifications hook but has several correctness issues worth addressing: the hook.parameters mapping crashes or silently produces empty values when data is an array or when a value lacks a ref, the n.where.xpr access is unguarded and will throw if the CDS parser returns a different shape, the NotificationTypeKey prefix may not be applied consistently for entity-based notifications (unlike the event path), the model.definitions iteration style is inconsistent between the existing event loop and the new entity loop in compile.js, and the template validation in createNotificationType fires after an unnecessary CSRF network call. The test helper's makeModel also returns an array instead of the object-keyed map that the real CDS runtime provides, which masks the iteration mismatch in unit tests.
PR Bot Information
Version: 1.29.54
- File Content Strategy: Full file content
- Event Trigger:
pull_request.ready_for_review - LLM:
anthropic--claude-4.6-sonnet - Correlation ID:
f53a0c50-a233-11f1-98c1-cbc6c4dc054e
There was a problem hiding this comment.
The pull request introduces entity-level @notifications support alongside solid unit and integration test coverage. However, there are several correctness issues that need to be addressed: the missing getNotificationTypesKeyWithPrefix call on hook.type (which will cause local-mode notification lookups to fail silently), the auto-map branch using data instead of singleData when data is an array, and the previously flagged issues around the n.where expression guard, the hook.parameters ref handling, the compile.js iteration inconsistency, and the misplaced template validation. Please ensure all raised comments are reviewed before merging.
PR Bot Information
Version: 1.29.54
- File Content Strategy: Full file content
- Correlation ID:
81483f20-a2d7-11f1-9004-5a57c20afdb8 - Event Trigger:
pull_request.ready_for_review - LLM:
anthropic--claude-4.6-sonnet
SummaryThe following content is AI-generated and provides a summary of the pull request: Add Entity-Level
|
There was a problem hiding this comment.
Looking at the diff and the current file states, all the issues I previously identified have already been addressed in the current file versions (the diff shows the post-fix state). The previous review comments covered all the substantive bugs. Let me check for any remaining new issues not yet raised.
The current state of lib/utils.js shows singleData is already used in the auto-map branch (line 386: Object.entries(singleData)) and the explicit-parameters branch uses resolveParamValue(value, singleData). The cds-plugin.js already uses resolveWhereXpr. The lib/compile.js entity loop at line 78 still uses for (const def of model.definitions) consistently with the existing event loop. All the previously flagged issues appear to have been incorporated.
One new issue I notice: in cds-plugin.js, notifications.notify(notification) is still called with the raw output of buildNotificationFromEntity without applyValueLengthConstraints — this was flagged in my previous comments. Also, the NotificationTypeKey still doesn't use getNotificationTypesKeyWithPrefix. These were previously flagged and are still present in the diff.
Since those were already raised in my prior comments, I should not duplicate them. The code in the diff still shows those issues unresolved, but they are already covered by existing review comments.
All substantive issues have already been raised in previous review comments. The author should address the outstanding comments regarding missing getNotificationTypesKeyWithPrefix on NotificationTypeKey, missing applyValueLengthConstraints before notify, and the other previously flagged bugs before merging.
PR Bot Information
Version: 1.29.54
- Correlation ID:
6b376a00-a2da-11f1-939c-1a369b211b11 - LLM:
anthropic--claude-4.6-sonnet - Event Trigger:
pull_request.ready_for_review - File Content Strategy: Full file content
Add support for adding notifications onto entities.