Skip to content

Switch from compiling Notification Types to Event Types#125

Open
eric-pSAP wants to merge 12 commits into
mainfrom
eventTypes
Open

Switch from compiling Notification Types to Event Types#125
eric-pSAP wants to merge 12 commits into
mainfrom
eventTypes

Conversation

@eric-pSAP
Copy link
Copy Markdown
Contributor

Plug-in currently builds and compiles from a user-written json file. This is slow and inefficient. Types also (automatically) will come from event types from cds files where events are annotated with @notification (json file remains backwards compatible).

Copy link
Copy Markdown
Contributor

@hyperspace-insights hyperspace-insights Bot left a comment

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 feature — deriving notification types from CDS model annotations rather than requiring a hand-authored JSON file. However, there are several substantive issues: a redundant cds.load during startup that should use the already-available cds.model, a potential TypeError in resolveEnum when the channel value is not a string, a silent data loss when the struct form of @notification is used instead of flat annotation keys, and unit tests that bypass cds.reflect() and therefore do not accurately validate the production code path. Please address these before merging.

PR Bot Information

Version: 1.20.43

  • File Content Strategy: Full file content
  • Event Trigger: pull_request.opened
  • Correlation ID: 01b9afab-28a3-4551-8db9-7abecd2e74cc
  • LLM: anthropic--claude-4.6-sonnet

Comment thread lib/compile.js Outdated
Comment thread lib/compile.js
Comment thread cds-plugin.js
Comment thread lib/build.js
Comment thread tests/unit/lib/compile.test.js
This was referenced May 21, 2026
@eric-pSAP eric-pSAP marked this pull request as ready for review May 27, 2026 11:54
@hyperspace-insights
Copy link
Copy Markdown
Contributor

Summary

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


Feat: Define Notification Types via CDS Event Annotations

New Features

✨ This pull request introduces a major enhancement allowing notification types to be defined directly within CDS models using @notification annotations on event definitions. This provides a more efficient and integrated developer experience compared to the previous method of using a static srv/notification-types.json file.

The key improvements include:

  • Annotation-Based Definitions: Define notification types, templates, and delivery channels directly in your CDS model.
  • Backward Compatibility: The existing srv/notification-types.json file is still supported, and its definitions are merged with those from CDS annotations.
  • Email Support via Annotations: Configure email delivery channels and templates using @notification.deliveryChannels and @notification.template.email.*.
  • i18n Integration: Use {i18n>key} syntax in annotation strings for internationalization.
  • Automatic Registration: Notification types are now automatically registered and synced with the notification service on application startup in hybrid/production environments, eliminating the need for a manual cds build or deployment for type changes.
  • Bug Fixes: Resolved an issue with enum references in @notification.deliveryChannels and improved error messages for type registration failures.

Changes

  • lib/compile.js: New file containing the core logic to compile @notification-annotated CDS events into the required notification type structure.
  • cds-plugin.js: Hooks into the CDS loaded event to automatically inject a recipients element into notification events. The served logic is updated to compile and merge types from both CDS annotations and the JSON file.
  • lib/build.js & lib/content-deployment.js: Updated the build and deployment tasks to incorporate the new annotation compilation logic.
  • README.md: Extensively rewritten to document the new annotation-based approach as the recommended method, providing detailed examples.
  • CHANGELOG.md: Updated with all new features and fixes introduced in this PR.
  • lib/notificationTypes.js: Improved error handling for failed API requests.
  • tests/: Added a new bookshop test project and comprehensive unit/integration tests for the new features, including compilation, i18n, and merging logic.

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

Version: 1.21.0

  • Event Trigger: pull_request.ready_for_review
  • Summary Prompt: Default Prompt
  • File Content Strategy: Full file content
  • Output Template: Default Template
  • LLM: gemini-2.5-pro
  • Correlation ID: 7cb893f4-b27b-445f-9c3a-087f81f6687d

Copy link
Copy Markdown
Contributor

@hyperspace-insights hyperspace-insights Bot left a comment

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 feature (CDS annotation-based notification type discovery) but has several correctness issues: a spurious warning in content-deployment.js when no types path is configured, incorrect environment-based branching in cds-plugin.js that conflates service kind with deployment environment, unnecessary model loading in production for non-REST setups, a hardcoded personal email in a test fixture, a typo in the README ("iori" → "Fiori"), and a unit test for content-deployment that doesn't mock the new compile module it depends on.

PR Bot Information

Version: 1.21.0

  • File Content Strategy: Full file content
  • LLM: anthropic--claude-4.6-sonnet
  • Event Trigger: pull_request.ready_for_review
  • Correlation ID: 7cb893f4-b27b-445f-9c3a-087f81f6687d

Comment thread lib/content-deployment.js Outdated
Comment thread cds-plugin.js
Comment thread cds-plugin.js Outdated
Comment thread README.md Outdated
Comment thread tests/bookshop/srv/cat-service.js Outdated
Comment thread tests/unit/lib/content-deployment.test.js
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