Skip to content

add: entity notification - #150

Open
eric-pSAP wants to merge 23 commits into
mainfrom
notifEntity
Open

add: entity notification#150
eric-pSAP wants to merge 23 commits into
mainfrom
notifEntity

Conversation

@eric-pSAP

Copy link
Copy Markdown
Contributor

Add support for adding notifications onto entities.

@eric-pSAP
eric-pSAP marked this pull request as ready for review August 27, 2026 16:25

@hyperspace-pr-bot hyperspace-pr-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread lib/utils.js
Comment thread lib/utils.js Outdated
Comment thread cds-plugin.js Outdated
Comment thread lib/compile.js Outdated
Comment thread tests/unit/lib/entityUnitNotify.test.js Outdated
Comment thread lib/notificationTypes.js Outdated
@eric-pSAP
eric-pSAP marked this pull request as draft August 27, 2026 16:52
@eric-pSAP
eric-pSAP marked this pull request as ready for review August 28, 2026 11:56

@hyperspace-pr-bot hyperspace-pr-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread lib/utils.js Outdated
Comment thread lib/utils.js
@eric-pSAP
eric-pSAP marked this pull request as draft August 28, 2026 12:04
@eric-pSAP
eric-pSAP marked this pull request as ready for review August 28, 2026 12:07
hyperspace-pr-bot[bot]

This comment was marked as duplicate.

@eric-pSAP
eric-pSAP marked this pull request as draft August 28, 2026 12:14
@eric-pSAP
eric-pSAP marked this pull request as ready for review August 28, 2026 12:17
@hyperspace-pr-bot

Copy link
Copy Markdown
Contributor

Summary

The following content is AI-generated and provides a summary of the pull request:


Add Entity-Level @notifications Support

New Feature

✨ Introduces support for attaching notifications directly to CDS entities via a new @notifications annotation. This enables notifications to be triggered on standard CRUD operations (e.g., READ, CREATE) against an entity, with conditional filtering, recipient resolution, priority, and optional explicit parameter mapping — without requiring a dedicated event definition.

Changes

  • cds-plugin.js: Added a service.after("*") hook that reads @notifications entries from the request target entity, filters matching hooks by operation, evaluates optional where conditions via a database query, and dispatches notifications via buildNotificationFromEntity for each result row.

  • lib/utils.js: Added three new exported helpers:

    • resolveRecipients(recipients, data) — resolves recipient IDs from $self.field refs, plain refs, or literal strings, with deduplication across arrays.
    • resolveWhereXpr(where) — normalizes CDS where annotation shapes into a flat expression array.
    • resolveParamValue(value, data) — resolves a parameter value from a ref, CDS = path, or literal val.
    • buildNotificationFromEntity(hook, data) — builds the full notification payload from an entity hook definition and entity data.
  • lib/compile.js: Extended notificationTypesFromModel to also scan entity definitions for @notifications entries and register the referenced type keys as notification types (with default templates and delivery channels), skipping duplicates already registered via events.

  • lib/notificationTypes.js: Added a guard in createNotificationType that throws a clear error if the notification type has no template title fields (TemplateSensitive, TemplatePublic, or TemplateGrouped). Exported createNotificationType for external use.

  • tests/bookshop/srv/notifications.cds: Added a CatalogTest service with a Books entity projection annotated with two @notifications entries for testing conditional notification triggering and explicit parameter mapping.

  • tests/integration/entityNotification.test.js: New integration test suite covering startup registration, conditional READ triggering, priority resolution, auto-mapped and explicit properties.

  • tests/unit/lib/entityUnitNotify.test.js: New unit test suite covering resolveWhereXpr, resolveRecipients, buildNotificationFromEntity, and notificationTypesFromModel for entity notifications.

  • tests/unit/lib/notificationTypes.test.js: Added a unit test verifying that createNotificationType throws before any I/O when template title fields are missing.

  • tests/integration/bookshop.test.js: Added an integration test for the empty-template guard; minor test name casing fixes.

  • tests/bookshop/db/data/sap.capire.bookshop-Authors.csv: Fixed a typo ("Allen" → "Allan") in Edgar Allan Poe's name.


  • 🔄 Regenerate and Update Summary
  • ✏️ Insert as PR Description (deletes this comment)
  • 🗑️ Delete comment
PR Bot Information

Version: 1.29.54

  • Output Template: Default Template
  • Event Trigger: pull_request.ready_for_review
  • Summary Prompt: Default Prompt
  • Correlation ID: 6b376a00-a2da-11f1-939c-1a369b211b11
  • LLM: anthropic--claude-4.6-sonnet
  • File Content Strategy: Full file content

@hyperspace-pr-bot hyperspace-pr-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@eric-pSAP
eric-pSAP requested a review from stefanrudi August 28, 2026 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant